[syslinux] chain.c32 (and partiter) updates v2

[Shao Miller]

On 11/5/2012 19:32, Michal Soltys wrote:
> This is a bit updated set of chain.c32 changes that simplifies a few things
> (and in partiter part), fixes few minor issues and adds a few new features.
>
> Details are in the following commits, below is the summary and pull details at
> the end.
>

Still reviewing! :)

On 11/8/2012 03:45, Michal Soltys wrote:
> On 08.11.2012 02:44, Shao Miller wrote:
>> Personally, I'd rather not, but anonymous members of this sort are in
>> C11... At least it removes the reserved identifier usage. :)
>>
>> Did you spot anonymous members being used elsewhere in the code-base, or
>> will this be the first such usage that you're aware of?
>>
>
> They were previously present in chain.c, when iterators were still part
> of it (that is, before my split patches were merged). I looked back at
> it and thought that perhaps I overdid earlier changes. For reference:
>
> git show 4c3e2f0e:com32/modules/chain.c
> around line 470

On 11/8/2012 03:51, Michal Soltys wrote:
> On 08.11.2012 02:52, Shao Miller wrote:
>> On 11/7/2012 20:44, Shao Miller wrote:
>>> Personally, I'd rather not, but anonymous members of this sort are in
>>> C11... At least it removes the reserved identifier usage. :)
>>
>> What is the benefit? Less typing?
>>
>
> In essence, yes. And with this approach it's as it was originally in
> your code. For the record, I'm fine with either approach.
>

The union member was named 'private' at that time, if I'm not mistaken. 
  It's 2012, but I still prefer C89, for portability habits. :)  Would 
it be a huge deal to 'git checkout -b feedback && git rebase -i HEAD~23' 
and drop this?  It might be, if it causes you to have to deal with 
conflicts in the subsequent commits.

On 11/8/2012 04:05, Michal Soltys wrote:
> On 08.11.2012 03:34, Shao Miller wrote:
>>
>> This member could still be useful if debugging/dumping. Was this change
>> prompted by GCC warnings?
>>
>> - Shao Miller
>>
>
> Good point, I actually readded in two tiny patches after #23. Warnings
> were not the reason here, just wasn't used anywhere in chain module
> (though if it's moved outside chain, then such field is certainly useful).

Ok.

On 11/8/2012 04:15, Michal Soltys wrote:
> On 08.11.2012 03:31, Shao Miller wrote:
>> On 11/5/2012 19:32, Michal Soltys wrote:
>>> + int bebr_index0; /* index of (0-3) of base ext. part., -1 of not
>>> present in MBR */
>>
>> "-1 if not present..."
>> ^- ?
>>
>> - Shao Miller
>>
>
> The initial value of this filed is (before these patches as well) set to
> -1, until the actual extended partition is found. This field is used in
> dos_next_mbr() and one-shot setup prep_base_ebr(). The choice of -1 and
>  >=0 values was iirc to match index0 behavior.
>
> Can be adjusted to 0 and 1 - 4 if it looks more natural (with rename to
> bebr_index).

-1 is fine by me. :)  I was actually asking about "-1 of not present..." 
versus "-1 if not present..."  (A typographical question.)

On 11/8/2012 04:49, Michal Soltys wrote:
>> On 11/5/2012 19:32, Michal Soltys wrote:
>>>   #ifndef _COM32_CHAIN_CHAIN_H
>>>   #define _COM32_CHAIN_CHAIN_H
>
> Good catch and no problems, one sed and it's fixed =)
>
> OTOH, what about using #pragma once ? Seems much cleaner and anything
> that could be used for compiling syslinux supports it (I think). From
> other projects, I think systemd has recently switched from #ifdefs pairs
> to #pragma
>

I'd be inclined towards C89, once more.  It doesn't have '#pragma once', 
as far as I know.  But COM32_CHAIN_CHAIN_H_ works!

On 11/8/2012 10:32, Michal Soltys wrote:
> On 2012-11-08 02:34, Shao Miller wrote:
>>
>> An alternative to this would be a final 'else' that bails with an error.
>>   While unlikely, it _could_ be useful for chain.c32 to understand APM,
>> some day.  That'll be even more useful once chain.c32 stuff is further
>> librarized for use with future multiple FS support.  If APM support were
>> introduced, this recent hunk would change back the way it was.
>
> Allright, will go back to earlier version.

I hope it's not a big chore as a rebase.  Else, it can be re-introduced 
if and when that day for APM support comes.