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 <[email protected]> > 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; _______________________________________________ parted-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/parted-devel
