[parted-devel] [PATCH 0/3 v2] Support for partitions on loop devices
Petr Uzel
petr.uzel at suse.cz
Fri Sep 30 06:37:17 UTC 2011
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 <petr.uzel at suse.cz>
> 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 <petr.uzel at suse.cz>
> 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 <petr.uzel at suse.cz>
> 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 <petr.uzel at suse.cz>
> 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 <petr.uzel at suse.cz>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20110930/6609d8a8/attachment.pgp>
More information about the parted-devel
mailing list