[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH parted 1/7] libparted: add ped_device_get_xxx_alignment() fun
Hans de Goede
Re: [PATCH parted 1/7] libparted: add ped_device_get_xxx_alignment() functions
Fri, 30 Oct 2009 11:34:20 +0100
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:220.127.116.11pre) Gecko/20090922 Fedora/3.0-3.9.b4.fc12 Thunderbird/3.0b4
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
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
arch/linux.c: In function 'linux_get_minimum_alignment':
arch/linux.c:2526: error: 'blkid_topology' undeclared (first use in this
arch/linux.c:2539: error: expected ';' before 'tp'
Then the HAVE_BLKID_BLKID_H gets defined there while it should not, I did not
that part of the code (I never touch autofoo, too scary). HAVE_BLKID_BLKID_H
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.
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"
Re: [PATCH parted 1/7] libparted: add ped_device_get_xxx_alignment() functions, Jim Meyering, 2009/10/29
- Re: [PATCH parted 1/7] libparted: add ped_device_get_xxx_alignment() functions,
Hans de Goede <=
- [PATCH parted 1/7] libparted: add ped_device_get_xxx_alignment() functions, Hans de Goede, 2009/10/30
- [PATCH parted 2/7] linux: save blkid topology in arch_specific data for later use, Hans de Goede, 2009/10/30
- [PATCH parted 3/7] linux: add get_xxx_alignment() functions, Hans de Goede, 2009/10/30
- [PATCH parted 4/7] ped_device_get_constraint: return constraint without alignment, Hans de Goede, 2009/10/30
- [PATCH parted 5/7] libparted: add ped_device_get_xxx_aligned_constraint() functions, Hans de Goede, 2009/10/30
- [PATCH parted 6/7] libparted: add ped_disk_get_partition_alignment() function, Hans de Goede, 2009/10/30
- [PATCH parted 7/7] Implement get_partition_alignment DiskOp for all labels which need it, Hans de Goede, 2009/10/30