[syslinux] [PATCH 2/4] chain.c: gpt's index/private.index mismatchfix, cosmetic iterator changes

[Michal Soltys]

On 27.07.2010 02:03, Miller, Shao wrote:
> -----Original Message-----
> From: syslinux-bounces at zytor.com [mailto:syslinux-bounces at zytor.com] On
> Behalf Of Michal Soltys
> Sent: Monday, July 26, 2010 18:51
> To: hpa at zytor.com
> Cc: syslinux at zytor.com
> Subject: [syslinux] [PATCH 2/4] chain.c: gpt's index/private.index
> mismatchfix, cosmetic iterator changes
>
>
> 2) free(ebr_part); in mbr iterator was not reachable. ebr iterator
> takes care of freeing itself and its parent.
>
> -----
> This is a coding style picked up from Michael Brown of iPXE.  The code
> might be unreachable, but it follows a logical path for "unwinding" what
> we've done.  For example, if some part of the code is changed and a new
> exit path comes into existence, the fall-through does not need
> consideration and the free() suddenly has a good use.  I'd suggest
> leaving it in...  Are you having an unreachable code path warning/error?
> -----
> I believe that's the same thing here.  Notice how things that need
> unwinding come just before the error label?  I find that it also helps
> to document our state.  I'd suggest leaving it in.
>

Warnings no, don't have them. It just somewhat looked awkward - past any 
label it's either error or nothing more to iterate. In both cases, we 
have to free all the stuff. I changed it a bit more in patch 3/4 - where 
freeing is done by generic free_iter() function, instead of some free() 
here, some free() there, some free() in main.

Maybe leave lables for reference, make their names consistent across all 
functions ? Then it would look like:


err_insane:
err_ebr:
out_finished:
	free_iter(ebr_part);
....
err_alloc:
out_finished:
	free_iter(mbr_part);

etc.



OT: With more complex functions, I tend to do something like:

int retval = -1;
/* variables, code, on conditions - set retval and jump to single out 
label */
	retval = 0; //all was good
out:
/* do epilogue / cleanup based on retval */
return retval;

It avoids code with many labels at the end falling from one to the 
other, with cherrypicked frees and co. between them.