[parted-devel] Bug: Removal of BLKPG causes regression of ability to manipulate disks with other partitions in use
Jim Meyering
jim at meyering.net
Tue Mar 30 07:58:05 UTC 2010
Phillip Susi wrote:
> Attaching the current patch applied in Ubuntu and commenting:
>
> On Mon, 2010-03-29 at 13:04 -0400, Phillip Susi wrote:
>> On 3/29/2010 12:44 PM, Jim Meyering wrote:
>> > If you can use an existing FD, then please do.
>> > That would be far better.
>>
>> Will do.
>
> It seems that parted currently only opens an fd on the raw disk, not the
> partitions. It looks like the BLKPG ioctls were originally intended to
> allow requests to read the partition tables in use on the raw device,
> but these have not yet been implemented, so the HD_GETGEO ioctl is
> required on the partition device, requiring the local open(). While it
> would be nice to be able to reuse an existing fd, it does not appear
> possible at this time, so Ubuntu is moving forward with this patch for
> the lucid release. Please review and comment or apply if you find it
> acceptable.
Thanks for the new patch.
I'll make the request once again:
Please give details of precisely how you test this (I assume you do).
Best is to give a script like one of those in tests/, but a
sequence of parted commands would be fine, too.
This will require root permissions and the use of the scsi_debug module,
so you may find it easiest to base a test script on one of the
existing 5 that do that:
tests/t3000-resize-fs.sh
tests/t3200-type-change.sh
tests/t9010-big-sector.sh
tests/t9020-alignment.sh
tests/t9030-align-check.sh
> Again, error-check-BLKPG.patch is to be applied on top of
> put-back-BLKPG.patch I posted earlier, which reverses the changes from
> parted 1.8 to parted 2.2.
Once you've addressed the comments below,
please re-post both patches (rebased to be against head of master)
in "git format-patch" format, so that I can apply them with a simple
"git am FILE" each. If you're not very familiar with git, and these
guidelines for submitting patches, here's what will soon be adapted
to serve as parted's contribution guidelines:
http://git.sv.gnu.org/cgit/coreutils.git/tree/HACKING
That means you should choose a "summary line" (length <= 72) to be the
first line of your commit log, followed by a blank line and then
discussion, justification and details on lines 3..N.
> From: Phillip Susi <psusi at cfl.rr.com>
> Description: Improve BLKPG error checking
> In the event that some partitions are in use, they can not be modified in the
> running kernel using 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.
> Forwarded: http://lists.alioth.debian.org/pipermail/parted-devel/2010-March/003518.html
> Last-Update: 2010-03-29
>
> Index: b/libparted/arch/linux.c
> ===================================================================
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -2417,7 +2417,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 cannot because 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
> @@ -2457,12 +2459,30 @@
> 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;
> + if (!rets[i - 1] && errnums[i - 1] == EBUSY) {
> + struct hd_geometry geom;
> + unsigned long long length;
> + int fd;
Please move fd "down" to the open call.
int fd = open ...
> + char *dev_name;
Combine declaration and initialization. Move this decl "down".
> +
> + memset (&geom, 0, sizeof (geom));
> + length = 0;
Combine initialization and declaration. More concise/readable.
unsigned long long length = 0;
> + /* get start and length of existing partition */
> + dev_name = _device_get_part_path (disk->dev, i);
Don't segfault when dev_name is NULL.
> + fd = open (dev_name, O_RDONLY);
You must free dev_name here, to avoid a leak.
You forgot to skip the ioctl/close calls upon open failure.
> + ioctl (fd, HDIO_GETGEO, &geom);
> + ioctl (fd, BLKGETSIZE64, &length);
> + length /= disk->dev->sector_size;
> + close (fd);
> + if (geom.start == part->geom.start &&
> + length == part->geom.length)
> + rets[i - 1] = 1;
> + /* if the new partition is unchanged and the exiting
> + one was not removed because it was in use, then
> + reset the error flag and skip adding it
> + since it is already there */
> + continue;
> + }
>
> /* add the (possibly modified or new) partition */
> if (!_blkpg_add_partition (disk, part))
> @@ -2470,6 +2490,24 @@
> }
> }
>
> + char parts[100];
> + parts[0] = 0;
> + /* now warn about any errors */
> + for (i = 1; i <= lpn; i++)
> + if (!rets[i - 1] && errnums[i - 1] != ENXIO)
> + sprintf (parts + strlen (parts), "%i, ", i);
> + if (parts[0]) {
> + parts[strlen (parts) - 2] = 0;
> + ped_exception_throw (
> + PED_EXCEPTION_WARNING,
> + PED_EXCEPTION_IGNORE,
> + _("WARNING: partition(s) %s on %s could not be modified, probably "
> + "because it/they is/are in use. As a result, the old partition(s) "
> + "will remain in use until after reboot. You should reboot "
> + "now before making further changes."),
> + parts, disk->dev->path);
> + }
> +
> free (rets);
> free (errnums);
> return ret;
More information about the parted-devel
mailing list