[parted-devel] [PATCH 2/2] libparted: improve support for partitions on loopback devices

[Jim Meyering]

Petr Uzel wrote:
> Since kernel 2.6.26, 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.

Thanks for doing this.

> Signed-off-by: Petr Uzel <petr.uzel at suse.cz>

Please omit Signed-off-by lines from the log in the future.
When you're the author, that's just redundant.
To do that just for parted, run this in your parted clone:

    git config format.signoff false


> +** New features
> +
> +  parted has improved support for partitionable loopback devices

Thanks for adding a NEWS entry.
Would you please also add a test to exercise this feature?

> diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> index adaf89a..2c0619a 100644
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -2419,32 +2419,74 @@ _blkpg_remove_partition (PedDisk* disk, int n)
>                                      BLKPG_DEL_PARTITION);
>  }
>
> -/*
> - * 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)

Please add a brief comment for each of these new functions, e.g.,

/* 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.  */

...which (along with that "bool ok") suggests that we can make the
return type be "bool" rather than "int":

> +static int
> +_sysfs_int_entry_from_dev(PedDevice *dev, const char *entry, int *val)
>  {
> -        int         range, r;
> +        int         r;
>          char        path[128];
>          FILE*       fp;
>          bool        ok;
>
> -        r = snprintf(path, sizeof(path), "/sys/block/%s/range",
> -                     last_component(dev->path));
> +        r = snprintf(path, sizeof(path), "/sys/block/%s/%s",
> +                     last_component(dev->path), entry);
>          if (r < 0 || r >= sizeof(path))
> -                return MAX_NUM_PARTS;
> +                return 0;

s/0/false/

>
>          fp = fopen(path, "r");
>          if (!fp)
> -                return MAX_NUM_PARTS;
> +                return 0;

s/0/false/

> -        ok = fscanf(fp, "%d", &range) == 1;
> +        ok = fscanf(fp, "%d", val) == 1;
>          fclose(fp);
>
> -        /* (range <= 0) is none sense.*/
> +        return ok;
> +}
> +
> +static unsigned int
> +_loop_get_partition_range(PedDevice* dev)
> +{
> +        int         max_part;
> +        FILE*       fp;

Please move this declaration down to the first use:

> +        bool        ok = 0;
> +
> +        /* max_part module param is exported since kernel 3.0 */
> +        fp = fopen("/sys/module/loop/parameters/max_part", "r");

           FILE *fp = fopen...

> +        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)

This parameter seems like it should be "const", along with the two
others, above.  If possible, please adjust:

   _device_get_partition_range(PedDevice const* dev)

> +{
> +        int         range;
> +        bool        ok;

Please move both of these declarations down:

> +
> +        /* loop handling is special */
> +        if (dev->type == PED_DEVICE_LOOP)
> +                return _loop_get_partition_range(dev);
> +
> +        ok = _sysfs_int_entry_from_dev(dev, "range", &range);

           int range;
           bool ok = _sysfs_int_entry_from_dev(dev, "range", &range);

>          return ok && range > 0 ? range : MAX_NUM_PARTS;