[PATCH RFC] Simplifying linux_find_partition()

[PATCH RFC] Simplifying linux_find_partition()

[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.

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.


diff --git a/util/biosdisk.c b/util/biosdisk.c
index fcf01a4..7079a97 100644
--- a/util/biosdisk.c
+++ b/util/biosdisk.c
@@ -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++)
     {


-- 
Regards,
Pavel Roskin

Re: [PATCH RFC] Simplifying linux_find_partition()

[Robert Millan]

On Sun, May 11, 2008 at 08:34:33PM -0400, Pavel Roskin wrote:
> 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.

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?

-- 
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 /.)

Re: [PATCH RFC] Simplifying linux_find_partition()

[Pavel Roskin]

On Mon, 2008-05-12 at 17:32 +0200, Robert Millan wrote:

> > 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.

> 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.

-- 
Regards,
Pavel Roskin

Re: [PATCH RFC] Simplifying linux_find_partition()

[Robert Millan]

On Thu, May 15, 2008 at 06:10:56PM -0400, Pavel Roskin wrote:
> On Mon, 2008-05-12 at 17:32 +0200, Robert Millan wrote:
> 
> > > 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.
> 
> > 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.

Fine with me.  Will you check that in soon?

-- 
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 /.)

Re: [PATCH RFC] Simplifying linux_find_partition()

[Pavel Roskin]

On Wed, 2008-05-28 at 15:46 +0200, Robert Millan wrote:

> Fine with me.  Will you check that in soon?

Applied.  Sorry for delay.

-- 
Regards,
Pavel Roskin