Re: [PATCH parted 1/7] libparted: add ped_device_get_xxx_alignment() functions

[Hans de Goede]

Hi,

On 10/29/2009 05:38 PM, Jim Meyering wrote:
> Hans de Goede wrote:
> > Add ped_device_get_minimum_alignment() and ped_device_get_optimum_alignment()
> > functions to libparted.
> >
> > Note this is a resent of my previous patchset with a number of typos corrected:
> > aligment ->  alignment
> > minimal_alignment ->  minimum_aligment
> > optimal_alignment ->  optimum_aligment
>
> Hi Hans,
>
> Thanks for fixing that.
> However, while the result does compile on rawhide, it fails to compile on F12:
>
>    In file included from arch/linux.c:22:
>    ./arch/linux.h:43: error: expected specifier-qualifier-list before 
> 'blkid_topology'
>    arch/linux.c: In function 'linux_get_minimum_alignment':
>    arch/linux.c:2526: error: 'blkid_topology' undeclared (first use in this 
> function)
>    ...
>    arch/linux.c:2539: error: expected ';' before 'tp'

Ah,

Then the HAVE_BLKID_BLKID_H gets defined there while it should not, I did not 
touch
that part of the code (I never touch autofoo, too scary). HAVE_BLKID_BLKID_H 
should
only be defined if the libblkid is new enough (so has topology info).

> Also, in the future, please insert notes like the one above after
> the "---" line in git format-patch output.  Otherwise it ends
> up in the git commit log and I might have to (or forget to) amend it out.

Will do.

> Also, please limit the commit log line length to 72 characters,
> since the git log is used to automatically generate a ChangeLog file
> and that procedure deliberately does not wrap long lines.

Will try to keep this mind.

> That latter is why we prefer at least some mention of what files
> and functions (in standard ChangeLog notation) are affected by
> each patch -- see other log entries for examples.

/me goes check other log entries.

I see I'll use this commit msg style from now on, if I forget please
let me know, I will probably need some training on this :)

Actually I've just found 2 small bugs in my blkid use, I'll do 2
new patches for that and use the proper changelog style.

> Finally, any significant change like this needs at least a
> trace of testing.  Preferably a new script in tests/ that exercises
> any new code.  That makes it trivial to ensure that in the covered
> code paths there are no new leaks or obvious-to-valgrind problems.
> If you don't have time to write the test, give me a few hints
> and I'll do that.

Thanks for volunteering for this!

Ok, what I've done for testing is:
sudo modprobe scsi_debug physblk_exp=3 lowest_aligned=7 num_parts=4

And then you get a new /dev/sdx (so you need to find the highest available 
/dev/sdX somehow, there is a slight race here but I think we can live with that)

Then create a parted device with path=/dev/sdx, and then:

get_minimum_alignment() should return:
an alignment with offset 7 and grain_size 8, and

get_optimum_alignment() should return:
an alignment with offset 7 and grain_size 64

get_minimal_aligned_constraint should return:
an constraint with a
start alignment with offset 7 and grain_size 8 and
an end aligment with offset 6 and grain_size 8

get_optimal_aligned_constraint should return:
an constraint with a
start alignment with offset 7 and grain_size 64 and
an end aligment with offset 6 and grain_size 64


For PedDisk.get_partition_alignment, you can create a disk
with an msdos label and then it should return an alignment
with offset 0, grain_size 1 (duplicate of ped_alignment_any)

Or an pc98 label and then it should return an alignment with
offset 0, grain_size cylinder_size

> I see that with this patch series on rawhide, nearly every single test
> that is run via "make check" (as non-root) now fails.
> Did "make check" pass for you?

Erm, I'm a big fan of testcases really I am, but I'm not used
to having them so I always forget to run them. This is probably caused
by 1 of the 2 bugs I mentioned above, When creating a PedDevice from
a regular file, the arch_specific->probe field does not get initialized
and then in ped_device_destroy(), free() gets called on a random pointer
causing a segfault (I only tested with real devices sofar).

Yip, with this fix in my tree (will send patches shortly) "make check"
works fine.

Regards,

Hans