On Mon, Dec 05, 2011 at 06:09:40PM -0500, Phillip Susi wrote: > On 12/05/2011 01:32 PM, Petr Uzel wrote: > >> I merged your patch with mine. > > > >> How does this look? > > > > You did not try to compile it, did you? :) > > No, that was my first draft. How's this look?
Overall, it looks good to me. I especially like the simplification of _disk_sync_part_table(). I'm attaching an incremental patch fixing some (mostly stylistic) issues which I spotted. Do you have any feedback from the kernel developers regarding the GET_PARTITION ioctl() ? > > From 3d5047e5e78bc0b4957877d530d2392e7e268f5a Mon Sep 17 00:00:00 2001 > From: Phillip Susi <[email protected]> > Date: Mon, 5 Dec 2011 18:02:28 -0500 > Subject: [PATCH 3/3] Avoid the HDIO_GETGEO when possible > > We were using the long depreciated HDIO_GETGEO ioctl on the > partition to get its start sector. Use the new BLKPG_GET_PARTITION > ioctl instead. This allows for disks > 2TB and partitioned loop > devices, which don't support HDIO_GETGEO. As a fallback when > BLKPG_GET_PARTITION fails or is not availible, try getting the > values from sysfs, and finally use BLKGETSIZE64 and HDIO_GETGEO > as a last resort. > > This modifies and superceeds Petr Uzel's patch: > libparted: fix reading partition start sector from kernel > --- > libparted/arch/linux.c | 134 +++++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 111 insertions(+), 23 deletions(-) > > diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c > index 3799b9d..460d0e8 100644 > --- a/libparted/arch/linux.c > +++ b/libparted/arch/linux.c > @@ -2410,6 +2410,26 @@ _blkpg_remove_partition (PedDisk* disk, int n) > BLKPG_DEL_PARTITION); > } > > +#ifdef BLKPG_GET_PARTITION > +static int > + > +_blkpg_get_partition (PedPartition *part, long long *start, long long > *length) > +{ > + struct blkpg_partition linux_part; > + int ret; > + > + memset (&linux_part, 0, sizeof (linux_part)); > + linux_part.pno = part->num; > + if (_blkpg_part_command (part->disk->dev, &linux_part, > + BLKPG_GET_PARTITION)) > + { > + *start = linux_part.start; > + *length = linux_part.length; > + return 1; > + } else return 0; > +} > +#endif > + > /* Read the integer from /sys/block/DEV_BASE/ENTRY and set *VAL > to that value, where DEV_BASE is the last component of DEV->path. > Upon success, return true. Otherwise, return false. */ > @@ -2432,6 +2452,92 @@ _sysfs_int_entry_from_dev(PedDevice const* dev, const > char *entry, int *val) > return ok; > } > > +/* Read the unsigned long long from /sys/block/DEV_BASE/PART_BASE/ENTRY > + and set *VAL to that value, where DEV_BASE is the last component of path > to > + block device corresponding to PART and PART_BASE is the sysfs name of > PART. > + Upon success, return true. Otherwise, return false. */ > +static bool > +_sysfs_ull_entry_from_part(PedPartition const* part, const char *entry, > unsigned long long *val) > +{ > + char path[128]; > + char *part_name = linux_partition_get_path(part); > + if (!part_name) > + return false; > + > + int r = snprintf(path, sizeof(path), "/sys/block/%s/%s/%s", > + last_component(part->disk->dev->path), > + last_component(part_name), entry); > + free(part_name); > + if (r < 0 || r >= sizeof(path)) > + return false; > + > + FILE *fp = fopen(path, "r"); > + if (!fp) > + return false; > + > + bool ok = fscanf(fp, "%llu", val) == 1; > + fclose(fp); > + > + return ok; > +} > + > + > +/* Get the starting sector and length of a partition PART within a block > device > + * Use blkpg if availible, then check sysfs and then use HDIO_GETGEO and > BLKGETSIZE64 > + * ioctls as fallback. Upon success, return true. Otherwise, return false. */ > +static bool > +_kernel_get_partition_start_and_length(PedPartition *part, unsigned long > long *start, > + unsigned long long *length) > +{ > + int fd = -1; > + int ok; > + PED_ASSERT(part); > + PED_ASSERT(start); > + PED_ASSERT(length); > + > +#ifdef BLKPG_GET_PARTITION > + ok = _blkpg_get_partition (part, start, length); > + if (ok) > + return ok; > +#endif > + char *dev_name = linux_partition_get_path (part); > + if (!dev_name) > + return false; > + > + ok = _sysfs_ull_entry_from_part (part, "start", start); > + if (!ok) { > + struct hd_geometry geom; > + int fd = open (dev_name, O_RDONLY); > + if (fd != -1 && ioctl (fd, HDIO_GETGEO, &geom)) { > + *start = geom.start; > + ok = true; > + } > + } > + ok = _sysfs_ull_entry_from_part (part, "size", length); > + if (!ok) > + { > + if (fd == -1) > + fd = open (dev_name, O_RDONLY); > + if (fd != -1 && ioctl (fd, BLKGETSIZE64, length)) > + ok = true; > + } > + if (ok) > + *length /= part->disk->dev->sector_size; > + if (fd != -1) > + close (fd); > + > + if (!ok) > + ped_exception_throw ( > + PED_EXCEPTION_BUG, > + PED_EXCEPTION_CANCEL, > + _("Unable to determine the size and length of %s."), > + dev_name); > + free (dev_name); > + return ok; > +} > + > + > + > /* > * The number of partitions that a device can have depends on the kernel. > * If we don't find this value in /sys/block/DEV/ext_range, we will use our > own > @@ -2516,33 +2622,15 @@ _disk_sync_part_table (PedDisk* disk) > } > > for (i = 1; i <= lpn; i++) { > - const PedPartition *part = ped_disk_get_partition (disk, i); > + PedPartition *part = ped_disk_get_partition (disk, i); > if (part) { > if (!ok[i - 1] && errnums[i - 1] == EBUSY) { > - struct hd_geometry geom; > - unsigned long long length = 0; > + unsigned long long length; > + unsigned long long start; > /* get start and length of existing > partition */ > - char *dev_name = _device_get_part_path > (disk->dev, i); > - if (!dev_name) > - goto cleanup; > - int fd = open (dev_name, O_RDONLY); > - if (fd == -1 > - || ioctl (fd, HDIO_GETGEO, &geom) > - || ioctl (fd, BLKGETSIZE64, &length)) { > - ped_exception_throw ( > - > PED_EXCEPTION_BUG, > - > PED_EXCEPTION_CANCEL, > - _("Unable to determine the size and length of %s."), > - dev_name); > - if (fd != -1) > - close (fd); > - free (dev_name); > + if > (!_kernel_get_partition_start_and_length(part, &start, &length)) > goto cleanup; > - } > - free (dev_name); > - length /= disk->dev->sector_size; > - close (fd); > - if (geom.start == part->geom.start > + if (start == part->geom.start > && length == part->geom.length) > ok[i - 1] = 1; > /* If the new partition is unchanged and the > -- > 1.7.5.4 > Petr -- Petr Uzel IRC: ptr_uzl @ freenode
diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index df3cdeb..c1c30f0 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2430,21 +2430,19 @@ _blkpg_remove_partition (PedDisk* disk, int n)
#ifdef BLKPG_GET_PARTITION
static int
-
_blkpg_get_partition (PedPartition *part, long long *start, long long *length)
{
struct blkpg_partition linux_part;
- int ret;
memset (&linux_part, 0, sizeof (linux_part));
linux_part.pno = part->num;
if (_blkpg_part_command (part->disk->dev, &linux_part,
- BLKPG_GET_PARTITION))
- {
+ BLKPG_GET_PARTITION)) {
*start = linux_part.start;
*length = linux_part.length;
return 1;
- } else return 0;
+ } else
+ return 0;
}
#endif
@@ -2533,10 +2531,10 @@ _sysfs_ull_entry_from_part(PedPartition const* part, const char *entry, unsigned
/* Get the starting sector and length of a partition PART within a block device
- * Use blkpg if availible, then check sysfs and then use HDIO_GETGEO and BLKGETSIZE64
+ * Use blkpg if available, then check sysfs and then use HDIO_GETGEO and BLKGETSIZE64
* ioctls as fallback. Upon success, return true. Otherwise, return false. */
static bool
-_kernel_get_partition_start_and_length(PedPartition *part, unsigned long long *start,
+_kernel_get_partition_start_and_length(PedPartition const *part, unsigned long long *start,
unsigned long long *length)
{
int fd = -1;
@@ -2561,11 +2559,15 @@ _kernel_get_partition_start_and_length(PedPartition *part, unsigned long long *s
if (fd != -1 && ioctl (fd, HDIO_GETGEO, &geom)) {
*start = geom.start;
ok = true;
+ } else {
+ if (fd != -1)
+ close(fd);
+ return false;
}
}
+
ok = _sysfs_ull_entry_from_part (part, "size", length);
- if (!ok)
- {
+ if (!ok) {
if (fd == -1)
fd = open (dev_name, O_RDONLY);
if (fd != -1 && ioctl (fd, BLKGETSIZE64, length))
@@ -2587,7 +2589,6 @@ _kernel_get_partition_start_and_length(PedPartition *part, unsigned long long *s
}
-
/*
* The number of partitions that a device can have depends on the kernel.
* If we don't find this value in /sys/block/DEV/ext_range, we will use our own
pgps6bNP3Oa47.pgp
Description: PGP signature
