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

[Jim Meyering]

Phillip Susi wrote:

> On 3/29/2010 10:16 AM, Petr Uzel wrote:
>> This would reintroduce problems described in this thread:
>> http://lists.alioth.debian.org/pipermail/parted-devel/2010-January/003355.html
>
> I'm not sure I understand this problem.  Transient opens from
> udev/hal/whatever should only happen right after we change the partition
> table, so this should only ever be a problem if we try to change it,
> then immediately alter it again.
>
> It looks like Ubuntu has a patch to invoke udevadm settle.  Perhaps this
> is another way of solving that problem?  If this is a good solution then
> maybe it should be applied upstream?
>
>> 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()
>
> If we got to this point then linux_partition_is_busy() already checked
> and thought that the partition was not mounted.  If it does, then parted
> denies the request to remove the partition  before ever trying to sync
> the partition table.  If it actually turns out that it is busy, then
> perhaps a 1 second sleep followed by a retry might help, though again, I
> don't see why this would happen unless we sync the partition table twice
> in rapid succession without a call to udevadm settle.
>
>>> +				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?
>
> I suppose I could.  Looking at that it looks like I could also use
> LINUX_SPECIFIC(dev)->fd instead of building the name and calling open()
> myself.

If you can use an existing FD, then please do.
That would be far better.

If you must use open, then you must handle failure,
and must not call ioctl or close on a negative "fd".

Other than this bit of quoted code, I haven't reviewed
your change at all -- was waiting for the newer version
you said would be coming soon.  Did I miss it?

>>> +				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().
>
> I didn't bother checking since I initialize the size and geometry to
> zero, so if any of the open or ioctl calls fail, they will still be
> zero, which will not match the current start and length, so we throw the
> warning anyhow.
>