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

[Hans de Goede]

Hi,

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.

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

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.

Regards,

Hans