Re: [PATCH 3/5] Avoid the HDIO_GETGEO when possible

[Jim Meyering]

Phillip Susi wrote:
> 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

Thanks.
This looks good, too, but there were some syntactic problems:
  - added lines going beyond column 80
  - compilation failed --enable-gcc-warnings due to shadowed "fd" decl

I've fixed those with the following incremental:

However, this is enough of an improvement that it deserves a NEWS entry.
Would you care to write that?  Also, since it is fixing a bug, I'll bet
you can at least outline a test case that fails without your patch yet
passes with it.  Would you please do that, or better still, write a
test suite addition?

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index 1bd7274..39372bb 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2454,7 +2454,8 @@ _sysfs_int_entry_from_dev(PedDevice const* dev, const 
char *entry, int *val)
    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)
+_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);
@@ -2480,10 +2481,12 @@ _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 available, then check sysfs and then use HDIO_GETGEO and 
BLKGETSIZE64
- * ioctls as fallback. Upon success, return true. Otherwise, return false. */
+   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 const *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;
@@ -2507,13 +2510,13 @@ _kernel_get_partition_start_and_length(PedPartition 
const *part, unsigned long l
         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)) {
+                int dev_fd = open (dev_name, O_RDONLY);
+                if (dev_fd != -1 && ioctl (dev_fd, HDIO_GETGEO, &geom)) {
                         *start = geom.start;
                         ok = true;
                 } else {
-                        if (fd != -1)
-                                close(fd);
+                        if (dev_fd != -1)
+                                close(dev_fd);
                         free (dev_name);
                         return false;
                 }
@@ -2631,7 +2634,8 @@ _disk_sync_part_table (PedDisk* disk)
                                 unsigned long long length;
                                 unsigned long long start;
                                 /* get start and length of existing partition 
*/
-                                if 
(!_kernel_get_partition_start_and_length(part, &start, &length))
+                                if 
(!_kernel_get_partition_start_and_length(part,
+                                                                &start, 
&length))
                                         goto cleanup;
                                 if (start == part->geom.start
                                    && length == part->geom.length)