On Thu, Sep 29, 2011 at 04:42:56PM +0200, Jim Meyering wrote: > Petr Uzel wrote: > > This is the second version of patch series to improve support > > for partitionable loop devices in parted. > > > > Changes since v1: > > > > o Added test to exercise the feature. > > o Style fixes suggested by Jim. > > > [SNIP]
Hi Jim, thanks a lot for the review. The level of perfectionism with which you review the patches and suggest improvements is amazing! > Thanks. > I've made some small changes, first to logs, then to the code. > Here are the diffs, followed by a complete patch. > Note that two of your three patches did not apply cleanly, > and for one, I had to omit a space-changing hunk because > the line it modified must have been added in your other series, > which I have not yet applied. Yes, it was based on the series I posted before. > > The changes are mostly stylistic. > For comments something called "the active voice" is generally preferred. > From HACKING, > > When writing prose (documentation, comments, log entries), use an > active voice, not a passive one. I.e., say "print the frobnozzle", > not "the frobnozzle will be printed". OK. > > Thanks a lot for the new test. > I've adjusted it not to discard the results of grep, and > especially not stderr. If things go wrong, stderr will often > provide details. Sure, that makes perfect sense. > > I removed the "rm -f backing_file" statement, > since everything in the containing directory is > removed as part of normal clean-up at the end of each test. > > I noticed that you use the "udevadm" command. > It would be good to add a function in init.cfg that > makes the test skip when it's not available or when > it does not support the --timeout option. OK, I will submit another patch to check for this. > In fact, this entire test fails on pre-linux-3.0 systems. > (it's failing on my Fedora 15 2.6.40.4-5.fc15.x86_64 desktop) > It'd be nice to make it skip rather than fail in that case, but I > don't mind if these proposed test-related changes are in a separate > patch. That might be better regardless,... Hm, it works fine on my 2.6.37.6. What does it fail on on your system? > > If you ACK what I've included below, I'll push these three commits. ACKed. Thanks! > > --------------------------------------------------------------- > > > --- k 2011-09-29 16:22:38.841650281 +0200 > +++ k-new 2011-09-29 16:22:22.882216118 +0200 > @@ -1,11 +1,11 @@ > -From b98d3dab38095738abed76986547f0f4403f7caf Mon Sep 17 00:00:00 2001 > +From 3e35b6559ad1ebfbad47e8edb2b002d7c07c984c Mon Sep 17 00:00:00 2001 > From: Petr Uzel <[email protected]> > Date: Thu, 29 Sep 2011 15:45:23 +0200 > Subject: [PATCH 1/3] libparted: differentiate between plain files and loop > devices > > -Stop using PED_DEVICE_FILE for loopback devices since loopback > -is something other than plain files. > +Stop using PED_DEVICE_FILE for loopback devices; > +loopback are significantly different from plain files. > * include/parted/device.h (PedDeviceType): Add PED_DEVICE_LOOP. > * libparted/arch/linux.c (_device_probe_type): Detect loopback device. > * parted/parted.c (do_print): Add "loopback" to list of transports. > @@ -71,15 +71,14 @@ index 32c2fcc..51ecdaf 100644 > 1.7.7.rc0.362.g5a14 > > > -From 6f80711605f36bddaf07266601ba33ddd655ada3 Mon Sep 17 00:00:00 2001 > +From 1eb0cc30d9a2f6a2a6c1f7ec0e4003b1081c5738 Mon Sep 17 00:00:00 2001 > From: Petr Uzel <[email protected]> > Date: Thu, 29 Sep 2011 15:14:23 +0200 > Subject: [PATCH 2/3] libparted: improve support for partitions on loopback > devices > > -Since kernel 2.6.26, kernel allows partitions on loopback devices. > +Since linux-2.6.26, the kernel allows partitions on loopback devices. > Implement support for this feature in parted. > - > * libparted/arch/linux.c (_sysfs_int_entry_from_dev): New function. > (_loop_get_partition_range): New function. > (_device_get_partition_range): Add special handling for loop devices. > > > diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c > index 50b3426..b1c12b5 100644 > --- a/libparted/arch/linux.c > +++ b/libparted/arch/linux.c > @@ -2428,47 +2428,39 @@ _blkpg_remove_partition (PedDisk* disk, int n) > BLKPG_DEL_PARTITION); > } > > -/* > - * Read the integer from /sys/block/DEV_BASE/ENTRY and set *val > - * to that value, where DEV_BASE is the last component of > - * DEV->path. Upon success, return true. Otherwise, return false. > - */ > +/* Read the integer from /sys/block/DEV_BASE/ENTRY and set *VAL > + to that value, where DEV_BASE is the last component of DEV->path. > + Upon success, return true. Otherwise, return false. */ > static bool > _sysfs_int_entry_from_dev(PedDevice const* dev, const char *entry, int *val) > { > - int r; > char path[128]; > - FILE* fp; > - bool ok; > - > - r = snprintf(path, sizeof(path), "/sys/block/%s/%s", > - last_component(dev->path), entry); > + int r = snprintf(path, sizeof(path), "/sys/block/%s/%s", > + last_component(dev->path), entry); > if (r < 0 || r >= sizeof(path)) > return false; > > - fp = fopen(path, "r"); > + FILE *fp = fopen(path, "r"); > if (!fp) > return false; > > - ok = fscanf(fp, "%d", val) == 1; > + bool ok = fscanf(fp, "%d", val) == 1; > fclose(fp); > > return ok; > } > > -/* > - * Returns maximum number of partitions that the loopback device can hold. > - * First checks if the loop module exports max_part parameter (since kernel > - * 3.0), if it does not succeed, it falls back to checking ext_range, which > - * seems to have (for some reason) different semantics compared to other > - * devices; specifically, ext_range <= 1 means that the loopback device does > - * not support partitions. > - */ > +/* Return the maximum number of partitions that the loopback device can hold. > + First, check the loop-module-exported max_part parameter (since > linux-3.0). > + If that is not available, fall back to checking ext_range, which seems to > + have (for some reason) different semantics compared to other devices; > + specifically, ext_range <= 1 means that the loopback device does > + not support partitions. */ > static unsigned int > _loop_get_partition_range(PedDevice const* dev) > { > int max_part; > - bool ok = 0; > + bool ok = false; > > /* max_part module param is exported since kernel 3.0 */ > FILE *fp = fopen("/sys/module/loop/parameters/max_part", "r"); > diff --git a/tests/t8001-loop-blkpg.sh b/tests/t8001-loop-blkpg.sh > index 4373d7c..a521309 100755 > --- a/tests/t8001-loop-blkpg.sh > +++ b/tests/t8001-loop-blkpg.sh > @@ -23,12 +23,11 @@ require_root_ > cleanup_fn_() > { > test -n "$loopdev" && losetup -d "$loopdev" > - rm -f backing_file > } > > # If the loop module is loaded, unload it first > -if lsmod | grep '^loop[[:space:]]' >/dev/null 2>&1; then > - rmmod loop || fail=1 > +if lsmod | grep '^loop[[:space:]]'; then > + rmmod loop || fail=1 > fi > > # Insert loop module with max_part > 1 > @@ -47,19 +46,19 @@ compare err /dev/null || fail=1 # expect no output > > # Create a partition > parted -s "$loopdev" mkpart primary 1M 2M > err 2>&1 || fail=1 > -compare err /dev/null || fail=1 # expect no output > +compare /dev/null err || fail=1 # expect no output > udevadm settle --timeout=5 || fail=1 > > # Verify that the partition appeared in /proc/partitions > entry=`basename "$loopdev"p1` > -grep "$entry" /proc/partitions >/dev/null 2>&1 || fail=1 > +grep "$entry" /proc/partitions || fail=1 > > # Remove the partition > parted -s "$loopdev" rm 1 > err 2>&1 || fail=1 > -compare err /dev/null || fail=1 # expect no output > +compare /dev/null err || fail=1 # expect no output > udevadm settle --timeout=5 || fail=1 > > # Verify that the partition got removed from /proc/partitions > -grep "$entry" /proc/partitions >/dev/null 2>&1 && fail=1 > +grep "$entry" /proc/partitions && fail=1 > > Exit $fail > > > Here is the complete series. > Please review it. I'll push this if/when you ACK. > > -------------------------------------------- > > From 3e35b6559ad1ebfbad47e8edb2b002d7c07c984c Mon Sep 17 00:00:00 2001 > From: Petr Uzel <[email protected]> > Date: Thu, 29 Sep 2011 15:45:23 +0200 > Subject: [PATCH 1/3] libparted: differentiate between plain files and loop > devices > > Stop using PED_DEVICE_FILE for loopback devices; > loopback are significantly different from plain files. > * include/parted/device.h (PedDeviceType): Add PED_DEVICE_LOOP. > * libparted/arch/linux.c (_device_probe_type): Detect loopback device. > * parted/parted.c (do_print): Add "loopback" to list of transports. > --- > include/parted/device.h | 3 ++- > libparted/arch/linux.c | 7 ++++++- > parted/parted.c | 2 +- > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/include/parted/device.h b/include/parted/device.h > index 0634465..b94765c 100644 > --- a/include/parted/device.h > +++ b/include/parted/device.h > @@ -48,7 +48,8 @@ typedef enum { > PED_DEVICE_SDMMC = 14, > PED_DEVICE_VIRTBLK = 15, > PED_DEVICE_AOE = 16, > - PED_DEVICE_MD = 17 > + PED_DEVICE_MD = 17, > + PED_DEVICE_LOOP = 18 > } PedDeviceType; > > typedef struct _PedDevice PedDevice; > diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c > index 3792b19..5452f30 100644 > --- a/libparted/arch/linux.c > +++ b/libparted/arch/linux.c > @@ -594,7 +594,7 @@ _device_probe_type (PedDevice* dev) > } else if (_is_virtblk_major(dev_major)) { > dev->type = PED_DEVICE_VIRTBLK; > } else if (dev_major == LOOP_MAJOR) { > - dev->type = PED_DEVICE_FILE; > + dev->type = PED_DEVICE_LOOP; > } else if (dev_major == MD_MAJOR) { > dev->type = PED_DEVICE_MD; > } else { > @@ -1385,6 +1385,11 @@ linux_new (const char* path) > goto error_free_arch_specific; > break; > > + case PED_DEVICE_LOOP: > + if (!init_generic (dev, _("Loopback device"))) > + goto error_free_arch_specific; > + break; > + > case PED_DEVICE_DM: > { > char* type; > diff --git a/parted/parted.c b/parted/parted.c > index 32c2fcc..51ecdaf 100644 > --- a/parted/parted.c > +++ b/parted/parted.c > @@ -853,7 +853,7 @@ _print_disk_info (const PedDevice *dev, const PedDisk > *disk) > "cpqarray", "file", "ataraid", > "i2o", > "ubd", "dasd", "viodasd", "sx8", > "dm", > "xvd", "sd/mmc", "virtblk", "aoe", > - "md"}; > + "md", "loopback"}; > > char* start = ped_unit_format (dev, 0); > PedUnit default_unit = ped_unit_get_default (); > -- > 1.7.7.rc0.362.g5a14 > > > From 1eb0cc30d9a2f6a2a6c1f7ec0e4003b1081c5738 Mon Sep 17 00:00:00 2001 > From: Petr Uzel <[email protected]> > Date: Thu, 29 Sep 2011 15:14:23 +0200 > Subject: [PATCH 2/3] libparted: improve support for partitions on loopback > devices > > Since linux-2.6.26, the kernel allows partitions on loopback devices. > Implement support for this feature in parted. > * libparted/arch/linux.c (_sysfs_int_entry_from_dev): New function. > (_loop_get_partition_range): New function. > (_device_get_partition_range): Add special handling for loop devices. > * NEWS: Mention this change. > --- > NEWS | 4 ++ > libparted/arch/linux.c | 77 +++++++++++++++++++++++++++++++++++++---------- > 2 files changed, 64 insertions(+), 17 deletions(-) > > diff --git a/NEWS b/NEWS > index 6c55ec9..293dad8 100644 > --- a/NEWS > +++ b/NEWS > @@ -2,6 +2,10 @@ GNU parted NEWS -*- > outline -*- > > * Noteworthy changes in release ?.? (????-??-??) [?] > > +** New features > + > + parted has improved support for partitionable loopback devices > + > ** Bug fixes > > libparted: no longer aborts (failed assertion) due to a nilfs2_probe bug > diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c > index 5452f30..b1c12b5 100644 > --- a/libparted/arch/linux.c > +++ b/libparted/arch/linux.c > @@ -2428,32 +2428,75 @@ _blkpg_remove_partition (PedDisk* disk, int n) > BLKPG_DEL_PARTITION); > } > > +/* Read the integer from /sys/block/DEV_BASE/ENTRY and set *VAL > + to that value, where DEV_BASE is the last component of DEV->path. > + Upon success, return true. Otherwise, return false. */ > +static bool > +_sysfs_int_entry_from_dev(PedDevice const* dev, const char *entry, int *val) > +{ > + char path[128]; > + int r = snprintf(path, sizeof(path), "/sys/block/%s/%s", > + last_component(dev->path), entry); > + if (r < 0 || r >= sizeof(path)) > + return false; > + > + FILE *fp = fopen(path, "r"); > + if (!fp) > + return false; > + > + bool ok = fscanf(fp, "%d", val) == 1; > + fclose(fp); > + > + return ok; > +} > + > +/* Return the maximum number of partitions that the loopback device can hold. > + First, check the loop-module-exported max_part parameter (since > linux-3.0). > + If that is not available, fall back to checking ext_range, which seems to > + have (for some reason) different semantics compared to other devices; > + specifically, ext_range <= 1 means that the loopback device does > + not support partitions. */ > +static unsigned int > +_loop_get_partition_range(PedDevice const* dev) > +{ > + int max_part; > + bool ok = false; > + > + /* max_part module param is exported since kernel 3.0 */ > + FILE *fp = fopen("/sys/module/loop/parameters/max_part", "r"); > + if (fp) { > + ok = fscanf(fp, "%d", &max_part) == 1; > + fclose(fp); > + } > + > + if (ok) > + return max_part > 0 ? max_part : 0; > + > + /* > + * max_part is not exported - check ext_range; > + * device supports partitions if ext_range > 1 > + */ > + int range; > + ok = _sysfs_int_entry_from_dev(dev, "range", &range); > + > + return ok && range > 1 ? range : 0; > +} > + > /* > * The number of partitions that a device can have depends on the kernel. > * If we don't find this value in /sys/block/DEV/range, we will use our own > * value. > */ > static unsigned int > -_device_get_partition_range(PedDevice* dev) > +_device_get_partition_range(PedDevice const* dev) > { > - int range, r; > - char path[128]; > - FILE* fp; > - bool ok; > - > - r = snprintf(path, sizeof(path), "/sys/block/%s/range", > - last_component(dev->path)); > - if (r < 0 || r >= sizeof(path)) > - return MAX_NUM_PARTS; > + /* loop handling is special */ > + if (dev->type == PED_DEVICE_LOOP) > + return _loop_get_partition_range(dev); > > - fp = fopen(path, "r"); > - if (!fp) > - return MAX_NUM_PARTS; > - > - ok = fscanf(fp, "%d", &range) == 1; > - fclose(fp); > + int range; > + bool ok = _sysfs_int_entry_from_dev(dev, "range", &range); > > - /* (range <= 0) is none sense.*/ > return ok && range > 0 ? range : MAX_NUM_PARTS; > } > > -- > 1.7.7.rc0.362.g5a14 > > > From d079fe7d4a70cc9b30651a560f3dfe749eeb3cff Mon Sep 17 00:00:00 2001 > From: Petr Uzel <[email protected]> > Date: Thu, 29 Sep 2011 15:14:24 +0200 > Subject: [PATCH 3/3] tests: add test for partitionable loop devices > > * tests/t8001-loop-blkpg.sh: New file. > * tests/Makefile.am: Add test. > --- > tests/Makefile.am | 1 + > tests/t8001-loop-blkpg.sh | 64 > +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 0 deletions(-) > create mode 100755 tests/t8001-loop-blkpg.sh > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index e721f88..903ca64 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -43,6 +43,7 @@ TESTS = \ > t6000-dm.sh \ > t7000-scripting.sh \ > t8000-loop.sh \ > + t8001-loop-blkpg.sh \ > t9010-big-sector.sh \ > t9020-alignment.sh \ > t9021-maxima.sh \ > diff --git a/tests/t8001-loop-blkpg.sh b/tests/t8001-loop-blkpg.sh > new file mode 100755 > index 0000000..a521309 > --- /dev/null > +++ b/tests/t8001-loop-blkpg.sh > @@ -0,0 +1,64 @@ > +#!/bin/sh > +# Test support for partitions on loop devices > + > +# Copyright (C) 2008-2011 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > + > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > + > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +. "${srcdir=.}/init.sh"; path_prepend_ ../parted > + > +require_root_ > + > +cleanup_fn_() > +{ > + test -n "$loopdev" && losetup -d "$loopdev" > +} > + > +# If the loop module is loaded, unload it first > +if lsmod | grep '^loop[[:space:]]'; then > + rmmod loop || fail=1 > +fi > + > +# Insert loop module with max_part > 1 > +modprobe loop max_part=7 || fail=1 > + > +# Create backing file > +dd if=/dev/zero of=backing_file bs=1M count=4 >/dev/null 2>&1 || fail=1 > + > +# Set up loop device on top of backing file > +loopdev=`losetup -f --show backing_file` > +test -z "$loopdev" && fail=1 > + > +# Expect this to succeed > +parted -s "$loopdev" mklabel msdos > err 2>&1 || fail=1 > +compare err /dev/null || fail=1 # expect no output > + > +# Create a partition > +parted -s "$loopdev" mkpart primary 1M 2M > err 2>&1 || fail=1 > +compare /dev/null err || fail=1 # expect no output > +udevadm settle --timeout=5 || fail=1 > + > +# Verify that the partition appeared in /proc/partitions > +entry=`basename "$loopdev"p1` > +grep "$entry" /proc/partitions || fail=1 > + > +# Remove the partition > +parted -s "$loopdev" rm 1 > err 2>&1 || fail=1 > +compare /dev/null err || fail=1 # expect no output > +udevadm settle --timeout=5 || fail=1 > + > +# Verify that the partition got removed from /proc/partitions > +grep "$entry" /proc/partitions && fail=1 > + > +Exit $fail > -- > 1.7.7.rc0.362.g5a14 Petr -- Petr Uzel IRC: ptr_uzl @ freenode
pgpuaH5bg1dc0.pgp
Description: PGP signature
_______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/parted-devel
