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

[Miller, Shao]

-----Original Message-----
From: Michal Soltys [mailto:soltys at ziu.info] 
Sent: Tuesday, July 27, 2010 04:51
To: For discussion of Syslinux and tftp-hpa
Cc: Miller, Shao; hpa at zytor.com
Subject: Re: [syslinux] [PATCH 2/4] chain.c: gpt's index/private.index
mismatchfix, cosmetic iterator changes

... ... ...

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.
-----End of Original Message-----

Ok, I think it's just a difference in preference, then.  I think H.
Peter has even expressed that it's not all that important to free() near
a module-exit condition in these COMBOOT32 modules at this time. :)  I
might have misunderstood him, however.

In the original code:
-----
    return part;

err_insane:

    free(part->block);
    part->block = NULL;
err_ebr:

out_finished:
    free(part->private.ebr.parent->block);
    free(part->private.ebr.parent);
    free(part->block);
    free(part);
    return NULL;
-----
Essentially what I'm trying to do is say "this is what it means to clean
up after an error" with the fall-through labels for erroring-out.
You'll see that the error labels are lumped together underneath whatever
their subject is.  The 'out' is, like you said, "nothing more to
iterate," which is a condition we reach either due to an error, or due
to an end condition.  That's why 'out_finished' is reached by the error
unwinding.  The 'out_finished' is lumped together above its clean-up
subject(s).

I think that you are suggesting that the code be rebalanced regarding
the trade-off between "documenting what constitutes clean-up for what
condition" versus "we know what needs cleaning up in any case across
these functions, so do it in one place."  That's ok, but now your
'free_iter()' includes an explicit test for (and therefore has knowledge
about) EBR iterators, rather than that knowledge being packed inside the
EBR iteration function.  If we had more different kinds of iterators,
free_iter() would start to fill up with more cases to consider.  We
aren't likely to have more cases soon, so I really don't mind agreeing
to the rebalancing you propose. :)  That was my rationale.

- Shao Miller