Re: [RFC memory leak?] Minor memory leak fix
- From
Fredrik Gustafsson <iveqy@iveqy.com>
- Date
- Mar 11, 2014, 12:40 UTC
- Message-ID
- <20140311124009.GI14327@paksenarrion.iveqy.com>
- In-Reply-To
- <CACsJy8Bp6rpe4hLg0eMBCjPjL5uDwXj-7ZVmZtUZvR26unAuoQ@mail.gmail.com>
On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:
Show 6 quoted lines
> On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote: > > Strbuf needs to be released even if it's locally declared. > > "path" is declared static. So yes it's a leak but the leak is minimum. > Your patch would make more sense if "static" is gone and it's leaked > after every write_archive_entry call.
That's one of the reasons of the RFC. I know Junio thinks that minor things shouldn't be fixed by themselfes because it takes up review bandwidth, so it's better to fix them once you touch that part of the code anyway. (At least that's how I've understood him).
This leak is at about 4.1 kB so it's not huge.
Show 6 quoted lines
> > + if (ATTR_TRUE(check[0].value)) {
> > + to_ret = 0;
> > + goto cleanup;
> > + }
>
> to_ret is already 0 so I think "goto cleanup;" is enough.Agree, fixed in next iteration.
Show 17 quoted lines
> > err = write_entry(args, sha1, path.buf, path.len, mode);
> > - if (err)
> > - return err;
> > - return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> > + if (err) {
> > + to_ret = err;
> > + goto cleanup;
> > + }
> > + to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> > + goto cleanup;
>
> Maybe if (err) to_ret = ...; else to_ret = ...; so we only need one
> "goto cleanup" statement. Going even further:
>
> to_ret = write_entry(...);
> if (!to_ret) to_ret = (S_ISDIR(...));
> goto cleanup;Agree, fixed in next iteration.
-- Med vänlig hälsning Fredrik Gustafsson tel: 0733-608274 e-post: iveqy@iveqy.com