Skip to contentSkip to conversation

Simplifying linux_find_partition()

RFC PATCH In review Pavel Roskin 2 participants 5 messages May 12, 2008 – May 30, 2008

lore
Share & tools
lore

Pavel Roskin

author
#0 lore raw

RFC PATCH 1/1 Simplifying linux_find_partition()

Hello!

linux_find_partition() is used to make it possible to access partitions using the native OS devices for the partitions, rather than the device for the whole drive.

As new devices are supported by GRUB, linux_find_partition() should be updated to support them, but this is not always done. The reason is that the problem is not obvious and doesn't prevent the basic functionality.

This patch simplifies the code and makes it handle all devices supported by Linux, perhaps even all devices that will be supported.

I'm assuming that linux_find_partition() is only called for whole drives, not for partitions, so the code doesn't need to strip anything from the device name. We only need to strip "disc" from devfs names.

Also, I checked devices.txt from Linux, and I see a simple pattern there. If the device ends in a number, the partitions are made by adding "p" and the number. Otherwise, only the number is added.

ChangeLog:

* util/biosdisk.c (linux_find_partition): Don't try to handle anything but whole drives. Simplify logic. Add "p%d" after numbers and "%d" after other characters.

util/biosdisk.c +5 -29
index fcf01a4..7079a97 100644
@@ -226,40 +226,16 @@ linux_find_partition (char *dev, unsigned long sector)
       p = real_dev + len - 4;
       format = "part%d";
     }
-  else if ((strncmp (real_dev + 5, "hd", 2) == 0
-	    || strncmp (real_dev + 5, "vd", 2) == 0
-	    || strncmp (real_dev + 5, "sd", 2) == 0)
-	   && real_dev[7] >= 'a' && real_dev[7] <= 'z')
+  else if (real_dev[len - 1] >= '0' && real_dev[len - 1] <= '9')
     {
-      p = real_dev + 8;
-      format = "%d";
-    }
-  else if (strncmp (real_dev + 5, "rd/c", 4) == 0)	/* dac960 */
-    {
-      p = strchr (real_dev + 9, 'd');
-      if (! p)
-	return 0;
-
-      p++;
-      while (*p && isdigit (*p))
-	p++;
-
+      p = real_dev + len;
       format = "p%d";
     }
-  else if (strncmp (real_dev + 5, "cciss/c", sizeof("cciss/c")-1) == 0)
+  else
     {
-      p = strchr (real_dev + 5 + sizeof("cciss/c")-1, 'd');
-      if (! p)
-	return 0;
-
-      p++;
-      while (*p && isdigit (*p))
-	p++;
-
-      format = "p%d";
+      p = real_dev + len;
+      format = "%d";
     }
-  else
-    return 0;
 
   for (i = 1; i < 10000; i++)
     {
Signature · 2 lines
Regards,
Pavel Roskin
#1 lore raw
Quoted 14 lines · Pavel Roskin

Hello!

linux_find_partition() is used to make it possible to access partitions using the native OS devices for the partitions, rather than the device for the whole drive.

As new devices are supported by GRUB, linux_find_partition() should be updated to support them, but this is not always done. The reason is that the problem is not obvious and doesn't prevent the basic functionality.

This patch simplifies the code and makes it handle all devices supported by Linux, perhaps even all devices that will be supported.

Quoted 7 lines

I'm assuming that linux_find_partition() is only called for whole drives, not for partitions, so the code doesn't need to strip anything from the device name. We only need to strip "disc" from devfs names.

Also, I checked devices.txt from Linux, and I see a simple pattern there. If the device ends in a number, the partitions are made by adding "p" and the number. Otherwise, only the number is added.

Good catch! But please make sure it's never called for partitions.

Also, perhaps an even simpler logic could be:

  if (real_dev[len - 1] >= '0' && real_dev[len - 1] <= '9')
    real_dev[len++] = 'p';

then you can treat real_dev as "%dp%d" form unconditionally. Would that work?

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)
author
#2 lore raw
Quoted 5 lines · Robert Millan
Quoted 3 lines

Also, I checked devices.txt from Linux, and I see a simple pattern there. If the device ends in a number, the partitions are made by adding "p" and the number. Otherwise, only the number is added.

Good catch! But please make sure it's never called for partitions.

As far as I understand, the code already assumes that. linux_find_partition() is only called if disk->partition is not NULL. Besides, open_device() is passed the disk information as grub_disk_t, which should not be used for partitions.

Quoted 8 lines

Also, perhaps an even simpler logic could be:

  if (real_dev[len - 1] >= '0' && real_dev[len - 1] <= '9')
    real_dev[len++] = 'p';

then you can treat real_dev as "%dp%d" form unconditionally. Would that
work?

It would work, but I prefer not give "len" a conditional meaning, where it's the length of the original device name for devfs devices, but the length of the partition name if "p" is appended. Conditional meanings can cause bugs.

Signature · 2 lines
Regards,
Pavel Roskin
#3 lore raw
Quoted 18 lines · Pavel Roskin
Quoted 5 lines · Robert Millan
Quoted 3 lines

Also, I checked devices.txt from Linux, and I see a simple pattern there. If the device ends in a number, the partitions are made by adding "p" and the number. Otherwise, only the number is added.

Good catch! But please make sure it's never called for partitions.

As far as I understand, the code already assumes that. linux_find_partition() is only called if disk->partition is not NULL. Besides, open_device() is passed the disk information as grub_disk_t, which should not be used for partitions.

Quoted 5 lines

Also, perhaps an even simpler logic could be:

  if (real_dev[len - 1] >= '0' && real_dev[len - 1] <= '9')
    real_dev[len++] = 'p';
Quoted 8 lines
Quoted 3 lines

then you can treat real_dev as "%dp%d" form unconditionally. Would that
work?

It would work, but I prefer not give "len" a conditional meaning, where it's the length of the original device name for devfs devices, but the length of the partition name if "p" is appended. Conditional meanings can cause bugs.

Fine with me. Will you check that in soon?

Signature · 5 lines
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)
author Applied
#4 lore raw
Quoted 1 line · Robert Millan

Fine with me. Will you check that in soon?

Applied. Sorry for delay.

Signature · 2 lines
Regards,
Pavel Roskin