[parted-devel] [PATCH] Properly sync partitions with operating system

[Jim Meyering]

Joel Granados Moreno wrote:
> diff --git a/include/parted/disk.h b/include/parted/disk.h
> index dfcf39e..9e50716 100644
> --- a/include/parted/disk.h
> +++ b/include/parted/disk.h
> @@ -210,6 +210,7 @@ struct _PedDiskOps {
>          /* other */
>          int (*alloc_metadata) (PedDisk* disk);
>          int (*get_max_primary_partition_count) (const PedDisk* disk);
> +        int (*get_max_partition_count) (const PedDisk* disk);
>  };
>
>  struct _PedDiskType {
> @@ -322,7 +323,7 @@ extern PedPartition* ped_disk_extended_partition (const PedDisk* disk);
>  /* internal functions */
>  extern PedDisk* _ped_disk_alloc (const PedDevice* dev, const PedDiskType* type);
>  extern void _ped_disk_free (PedDisk* disk);
> -
> +extern int ped_disk_get_max_partition_num(const PedDisk* disk);

Thanks for working on this.

If the only values ever returned are non-negative (as it seems they are),
then it'd be far more readable to make the return type "unsigned int".
Otherwise, I have to wonder if some of these functions may return a
negative value, and write code in each caller to handle that.

>  /** @} */
>
> diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> index 4a8c9e7..6fd0c75 100644
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -279,6 +279,9 @@ struct blkdev_ioctl_param {
>                  || (M) == SCSI_CDROM_MAJOR                              \
>                  || ((M) >= SCSI_DISK1_MAJOR && (M) <= SCSI_DISK7_MAJOR))
>
> +/* Maximum number of partitions supported by linux. */
> +#define MAX_NUM_PARTS		64
> +
>  static char* _device_get_part_path (PedDevice* dev, int num);
>  static int _partition_is_mounted_by_path (const char* path);
>
> @@ -2261,45 +2264,92 @@ _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 int
> +_device_get_partition_range(PedDevice* dev)
> +{
> +	int		range;
> +	char	path[128];
> +	FILE*	fp;
> +	bool	ok;
> +
> +	if(snprintf(path, sizeof(path), "/sys/block/%s/range",
> +				basename(dev->path)) < 0)

Unless you're sure that dev->path will never be too long, you
should also ensure that the snprintf return value is < sizeof path.
Otherwise, fopen will operate on a truncated string which might even
name an existing file.  Sure, it's not likely, but...

> +		return MAX_NUM_PARTS;
> +
> +	fp = fopen(path, "r");
> +	if(!fp)
> +		return MAX_NUM_PARTS;
> +
> +	ok = fscanf(fp, "%d", &range) == 1;
> +	fclose(fp);
> +	return ok ? range : MAX_NUM_PARTS;
> +}
> +
> +/*
> + * We sync the partition table in two step process:
> + * 1. We remove all the partitions from the kernel's tables.  The partitions
> + *    will not be removed if the ioctl call fails.
> + * 2. We add all the partitions that we hold in disk.
> + *
> + * To achieve this two step process we must calculate the minimum number of
> + * maximum possible partitions between what linux supports and what the label
> + * type supports. EX:
> + *
> + * number=MIN(max_parts_supported_in_linux,max_parts_supported_in_msdos_tables)
> + *
> + * For now we will hard code the maximum number of partitions for linux to
> + * MAX_NUM_PARTS. Might want to search for a way to include this from somewhere.

Thanks for adding comments.

>  static int
>  _disk_sync_part_table (PedDisk* disk)
>  {
> -        int largest_partnum = ped_disk_get_last_partition_num (disk);
> -        if (largest_partnum <= 0)
> -          return 1;
> +	PED_ASSERT(disk != NULL, return 0);
> +	PED_ASSERT(disk->dev != NULL, return 0);


It's not at all clear (from reading only these diffs) that you've made
only minimal changes to this function.  The trouble is that you've
changed indentation and formatting.  So please adjust this patch *not*
to make those formatting changes and you'll see that it's much smaller --
and more readable.

> -        int     last = 16;
> -        int*    rets = ped_malloc(sizeof(int) * last);
> -        int*    errnums = ped_malloc(sizeof(int) * last);
> -        int     ret = 1;
> -        int     i;
> +	/* lpn = largest partition number. */
> +	int lpn = PED_MIN(_device_get_partition_range(disk->dev),
> +				ped_disk_get_max_partition_num(disk));
>
> -        for (i = 1; i <= last; i++) {
> -                rets[i - 1] = _blkpg_remove_partition (disk, i);
> -                errnums[i - 1] = errno;
> -        }
> +	/* Its not possible to support largest_partnum <= 0. */
> +	if(lpn <= 0)
> +		return 0;

> -        for (i = 1; i <= last; i++) {
> -                const PedPartition *part;
> +	int*	rets = ped_malloc(sizeof(int) * lpn);
> +	int*	errnums = ped_malloc(sizeof(int) * lpn);
> +	int		ret = 1;
> +	int		i;
>
> -                part = ped_disk_get_partition (disk, i);
> -                if (part) {
> -                        /* busy... so we won't (can't!) disturb ;)  Prolly
> -                         * doesn't matter anyway, because users shouldn't be
> -                         * changing mounted partitions anyway...
> -                         */
> -                        if (!rets[i - 1] && errnums[i - 1] == EBUSY)
> -                                        continue;
> -
> -                        /* add the (possibly modified or new) partition */
> -                        if (!_blkpg_add_partition (disk, part))
> -                                ret = 0;
> -                }
> -        }
> +	for(i = 1; i <= lpn; i++) {
> +		rets[i - 1] = _blkpg_remove_partition (disk, i);
> +		errnums[i - 1] = errno;
> +	}
> +
> +	for(i = 1; i <= lpn; i++) {
> +		const PedPartition *part;
> +
> +		part = ped_disk_get_partition (disk, i);
> +		if (part) {
> +			/* busy... so we won't (can't!) disturb ;)  Prolly
> +			 * doesn't matter anyway, because users shouldn't be
> +			 * changing mounted partitions anyway...
> +			 */
> +			if (!rets[i - 1] && errnums[i - 1] == EBUSY)
> +					continue;
> +
> +			/* add the (possibly modified or new) partition */
> +			if (!_blkpg_add_partition (disk, part))
> +				ret = 0;
> +		}
> +	}
>
> -        free (rets);
> -        free (errnums);
> -        return ret;
> +	free (rets);
> +	free (errnums);
> +	return ret;
>  }

The rest looks ok.