Re: [PATCH 06/26] get_mail_commit_oid(): avoid resource leak

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Junio & Stefan,


On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Stefan Beller <sbeller@xxxxxxxxxx> writes:
> 
> >> -       if (get_oid_hex(x, commit_id) < 0)
> >> -               return -1;
> >> +       if (!ret && get_oid_hex(x, commit_id) < 0)
> >> +               ret = -1;
> >>
> >
> > In similar cases of fixing mem leaks, we'd put a label here
> > and make excessive use of goto, instead of setting ret to -1.
> > As "ret" and the commands are short, this is visually just as appealing.
> 
> I wouldn't call that visually appealing.  Fixing resource leaks is
> good, and only because this function is short enough and the funny
> way to skip over various operation need to last for just a few
> instructions, it is tolerable.  If the function were shorter, then
> we may have used
> 
> 	if (!strbuf_getline_lf() &&
> 	    skip_prefix() &&
> 	    !get_oid_hex())
> 		ret = 0; /* happy */
> 	else
> 		ret = -1;
> 	release resources here;
>         return ret;

I did almost what you suggested here, but I avoided the explicit ret = 0,
as it is initialized that way.

> and that would have been OK.  If longer, as you said, jumping to a
> label at the end of function to release the acquired resource would
> be a lot more maintainable way than either of the above alternatives
> that are only usable for short functions.
> 
> The patch as posted makes the function fail to return -1 when it
> finds problems, by the way.  It needs to return "ret" not the
> hardcoded "0" at the end.

Oops.

Ciao,
Dscho



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]