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

[Michal Soltys]

On 10-07-27 15:58, Miller, Shao wrote:
>
> 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.
>

I don't really know that :) I haven't peeked inside syslinux that far 
yet. But my general approach to [de]allocation is to either do all of 
it, or not do it at all. If we do some, but skip the rest because it's 
not really important in the end and/or a bit inconvenient along the way 
- then it's just wrong, imho.

> 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).
>

Yea, I know what you mean. I think.

  - alloc some "a"
  - bail on error to a:
  - alloc some "b"
  - bail on error to b:
...
  - something
    return [eventually]
...
b:
  - dealloc b
a:
  - dealloc a
  - return with error

get_first_partition() follows it to the letter +/-, similary 
next_mbr_part (besides that unreachable call). There's nothing wrong 
though with e.g. shifting "dealloc b" under a:, as long as we're 
properly guarded (or better, if it's simple enough to be bundled into 
one logical function, etc.)

(( theoretizing, I really prefer variable+single jump approach - even if 
for the sole reason that unwinding "linearly" is not always valid ))

But in ebr iterator, we already have everything possible allocated at 
the very beginning of the function, which generally comes down (in this 
case) to

err_insane:
err_ebr:
out_finished:
	free everything and leave

Regarding earlier unreachable function - if the iterator was expanded 
one or the other way, that function could be as well not enough, or too 
much - so it would need adjusting either way.

Anyway, what about doing the following:

err_something1:
err_something2:
...etc.
	part->free(part);
	return NULL;

Where free() (similary to next()) would be specfic to the iterator or 
generic. Well, sort-of virtual destructor defacto. It wouldn't expose 
internals, it can be naturally called from other functions, documentary 
value of different labels is kept (and comments can add more details).

Of course - I'm fine with either approach. It's a small detail after all.

Anyway, I'll prep the patches (I think 2 - one for iterator and 
allocation related things, the other for actual functionality). I have 
some extra ideas too. Feel free to adjust the patches afterwards.

> 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.
>

Well, it's true - I agree it's not pretty. But analogous thing is done 
in find_by_label or when handover area is created - peek inside 
interator's internals to check if it's perhaps a gpt one. Or when mbr 
iterator knows how to allocate and initalize ebr iterator. Well, 
regarding free_iter - mentioned above iterator-dependent free() should 
do the thing elegantly.

Maybe some simple function/variable reporting iterator type should be 
added ?