Re: [PATCH parted 1/5] dasd: Cleanup anchor handling

[Jim Meyering]

Hans de Goede wrote:
> On 11/04/2009 10:32 AM, Jim Meyering wrote:
>> Hans de Goede wrote:
>>> The current dasd label code keeps an fdasd anchor struct in the
>>> DasdDiskSpecific struct, and fills this during dasd_read. However this
>>> anchor does not get updated with any future mods, until dasd_write,
>>> at which points it gets completely re-initialized.
>>
>> Thanks for these patches.
>> My first pass at reviewing them spotted only a few nits:
>>
>> - it requires a test (outline is fine, as before)
>>
>
> Not sure how helpful that would be as running such a test requires doing
> so on an s390 virtual machine.
>
> Anyways outline:
> -before do ped_disk_fresh_new(dasd_device, ped_disk_type_get("dasd") would 
> always
>  fail, as the ioctl in dasd_alloc would fail with a BADFD errno
> -after this works
>
> -before doing ped_disk_duplicate() on a PedDisk with a dasd DiskType, would
>  fail triggering an assert (no partition_duplicate implementation), now it
>  works.

Thanks.
This is helpful indeed, even if I don't take the time
to write a test script to automate it.

>> - any fix like that deserves an entry in NEWS;
>>      Please add an entry for your new alignment-related functions, while
>>      you're at it.
>>
>
> Will do.
>
>> - the new index, "int i" (in 2/5) should be declared "unsigned int i".
>>      Also, please move the declaration so it is right before the first use:
>>      i.e., declare it on the line right before the for-loop.
>>      In general, please move declarations you add or touch "down"
>>      to be as close to first use as possible.
>
> I duplicated the new way the loop is done from the loop in dasd_write,
> and i is a signed int there (in dasd_write) too.

Please do not treat existing code as a model to be followed.
You can tell from the irregular formatting that presentation
was never a priority here.

> The same for the declaration,
> also note that declaring variables after statements is not allowed by
> ANSI-C (89), it is by ISO C99.

Ha ;-)
I know very well the differences between C89 and C99 and have made this
policy change in numerous projects for years, starting with coreutils.
There has been no fall-out at all, since compilers that don't accept
decl-after-stmt are not useful on any valid porting target I know of.

Parted has had instances of decl-after-stmt for a couple years now,
and as far as I know, no one has complained.

> In general I'm a favor of declaring all the variables at the top coding style
> (and this keeps the code compilable by any ANSI-C compiler), but if the coding
> style for parted is to declare variables close to their first use, I'll adapt.

IMHO, the idea that all-decls-at-the-top is "good" or even "acceptable"
these days must be eradicated.  There are many reasons to move them
"down" and afaik, none other than "that's what I'm used to" for
doing otherwise.

Thanks for adapting.  I hope you find that it is worthwhile.
If you'd like detailed rationale, let me know.