[parted-devel] Bug: Removal of BLKPG causes regression of ability to manipulate disks with other partitions in use

[Petr Uzel]

Hi all,

On Fri, Mar 26, 2010 at 11:19:37AM -0400, Phillip Susi wrote:
> Attached are the two patches I have come up with. They are self
> described. I have already been asked by Colin Watson to clean it up a
> bit so the coding style conforms ot the surrounding code, which I will
> be doing tomorrow. Please let me know what you think about them.
[...]
> From: Phillip Susi <psusi at cfl.rr.com>
> Description: In the event that some partitions are in use,
>   they can not be modified in the running kernel usign BLKPG.
>   Warn the user that this is the case and advise them to reboot,
>   just like we do when BLKRRPART fails for the same reason, unless
>   the partition in question is unchanged.
> Index: parted-2.2/libparted/arch/linux.c
> ===================================================================
> --- parted-2.2.orig/libparted/arch/linux.c	2010-03-25 20:19:29.407917597 -0400
> +++ parted-2.2/libparted/arch/linux.c	2010-03-25 20:22:08.487917004 -0400
> @@ -2411,7 +2411,9 @@
>   * Sync the partition table in two step process:
>   * 1. Remove all of the partitions from the kernel's tables, but do not attempt
>   *    removal of any partition for which the corresponding ioctl call fails.
> - * 2. Add all the partitions that we hold in disk.
> + * 2. Add all the partitions that we hold in disk, throwing a warning
> + *    if we can not beacuse step 1 failed to remove it and it is not being
> + *    added back with the same start and length.
>   *
>   * To achieve this two step process we must calculate the minimum number of
>   * maximum possible partitions between what linux supports and what the label
> @@ -2444,24 +2446,46 @@
>          int i;
>  
>          for (i = 1; i <= lpn; i++) {
> -                rets[i - 1] = _blkpg_remove_partition (disk, i);
> -                errnums[i - 1] = errno;
> +		  rets[i - 1] = _blkpg_remove_partition (disk, i);
> +		  errnums[i - 1] = errno;

This would reintroduce problems described in this thread:
http://lists.alioth.debian.org/pipermail/parted-devel/2010-January/003355.html

I'm not sure the solution is possible, but I'd suggest to do something
like the following:
- if the BLKPG_DEL_PART ioctl returns EBUSY, then
  - check whether the partition is busy (linux_partition_is_busy() )
  - if it is not busy, then wait for some specific time and retry the
    BLKPG_DEL_PART ioctl()

The idea behind this is not to fail if something (like HAL) opens the
device for a short period of time, but do not unnecessarily wait if
the partition is mounted.

>          }
>  
>          for (i = 1; i <= lpn; i++) {
> -                const PedPartition *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;
> -                }
> +		  const PedPartition *part = ped_disk_get_partition (disk, i);
> +		  if (part) {
> +			if (!rets[i - 1] && errnums[i - 1] == EBUSY)
> +			  {
> +				struct hd_geometry geom;
> +				unsigned long long length;
> +				int fd;
> +				char *dev_name;
> +				
> +				memset( &geom, 0, sizeof( geom ) );
> +				length = 0;
> +				/* get start and length of existing partition */
> +				dev_name = _device_get_part_path (disk->dev, i);
> +				fd = open( dev_name, O_RDONLY );
> +				ioctl( fd, HDIO_GETGEO, &geom );
> +				ioctl( fd, BLKGETSIZE64, &length );
> +				length /= disk->dev->sector_size;

Why not use _device_get_length() here?

> +				close( fd );

I think there should be some checks of return values - if it
is not possible to open the device for whatever reason, or issue the
ioctl()s, then the code should jump directly to ped_exception_throw().

> +				if( geom.start != part->geom.start ||
> +					length != part->geom.length )
> +				  ped_exception_throw(
> +					PED_EXCEPTION_WARNING,
> +					PED_EXCEPTION_IGNORE,
> +					_("WARNING: partition %d on %s could not be modified, probably "
> +					  "because it is in use.  As a result, the old partition "
> +					  "will remain in use until after reboot. You should reboot "
> +					  "now before making further changes."),
> +					i, disk->dev->path);
> +				continue;
> +			  }
> +			
> +			/* add the (possibly modified or new) partition */
> +			if (!_blkpg_add_partition (disk, part))
> +			  ret = 0;
> +		  }
>          }
>  
>          free (rets);


Petr

--
Petr Uzel, openSUSE Boosters Team
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/20100329/8cac98b1/attachment.pgp>