[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 19:58
To: Miller, Shao
Cc: For discussion of Syslinux and tftp-hpa
Subject: Re: [syslinux] [PATCH 2/4] chain.c: gpt's index/private.index
mismatchfix, cosmetic iterator changes

... ... ...

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

What I meant was: There isn't a huge emphasis on free() right now since
a COMBOOT32's heap doesn't persist past the module's exit (if I recall
correctly).

For the sake of habit, I free() where I think it's warranted, but didn't
scan through and find every allocation in the rest of the code.  The
focus for those iterators was really getting something functional by a
deadline.  It would appear that you've completed that functionality in
some spots it was lacking. :)  At least the free()s that are there are
ready to go if you or someone else feels like adding them everywhere
else free() is warranted.

-----
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
-----
Well...  Lumping the labels _underneath_ means "I failed to allocate
'foo'.  Unwind everything that came before."  If you lump on top, code
that might not be working with 'foo' has to know that 'err_foo' is the
error path at that point in the code.  If you insert another chunk of
code in-between and add another error path, you have to change 'err_foo'
to 'err_bar'.  For example:

/* Allocate foo */

/* Not-yet-implemented */

/* Do some stuff */
if (some_error) goto err_foo

/* ... */

/* Not-yet-implemented error-out */

err_foo:
/* De-allocate foo */

If at some point /* Not-yet-implemented */ is replaced with code and
there's a new error path, you have to revisit /* Do some stuff */ and
maybe change 'err_foo' to some new label.

If we get an error allocating 'foo', it would seem that we don't need to
de-allocate 'foo', so the de-allocation might be better off _before_ the
error label. :)

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

To be honest, I found the iterator work to be looking too complex before
I had finished it.  I've just been waiting for the librarization to
happen before revisiting it.  It looks like you've found some
refinements to do before that time has arrived. :)

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

... ... ...

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

Thanks again Michal.  Again, there're definitely some good work that can
be done.  As the deadline approached, "peeking inside the internals" was
about all there was time for. :)  I don't like to do a bunch of work and
find out that's not the way to go, so I've been patiently waiting for
com32lib_disk to be merged before looking at chain.c again.  It seems
you're on top of it, which is great. :)

- Shao Miller