[PATCH] register dummy drive

[PATCH] register dummy drive

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]


There's no reason grub-probe should fail if it can't resolve drive, when we
just asked for -t fs, -t fs_uuid or -t partmap.

This patch solves the problem by spliting device/drive map[] entry registration
into a separate function, and using that from grub-probe.c to register a dummy
drive that will last during the current execution.

It's diffed relative to my previous patch for device function names (but if
we need to change something in that one, not a problem for me to readapt it,
of course).

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

[-- Attachment #2: register_dummy_drive.diff --]
[-- Type: text/x-diff, Size: 4790 bytes --]

2008-06-02  Robert Millan  <rmh@aybabtu.com>

	* include/grub/util/biosdisk.h (grub_util_biosdisk_register): New
	function declaration.
	* util/biosdisk.c (open_device): Remove first grub_util_info() call
	(too verbose).
	(grub_util_biosdisk_register): New function.
	(read_device_map): Use grub_util_biosdisk_register() for registration
	of drive/device entries rather than doing it ourselves.
	* util/grub-probe.c (probe): If we weren't asked to -t drive, allow
	grub_util_get_grub_dev() to fail by registering a dummy drive entry,
	so that other options like -t fs, -t fs_uuid or -t partmap will work
	regardless.

diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../tmp.old/include/grub/util/biosdisk.h ./include/grub/util/biosdisk.h
--- ../tmp.old/include/grub/util/biosdisk.h	2007-07-22 01:32:25.000000000 +0200
+++ ./include/grub/util/biosdisk.h	2008-06-02 15:33:49.000000000 +0200
@@ -23,5 +23,6 @@
 void grub_util_biosdisk_init (const char *dev_map);
 void grub_util_biosdisk_fini (void);
 char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
+int grub_util_biosdisk_register (char *device, char *drive);
 
 #endif /* ! GRUB_BIOSDISK_MACHINE_UTIL_HEADER */
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../tmp.old/util/biosdisk.c ./util/biosdisk.c
--- ../tmp.old/util/biosdisk.c	2008-06-02 15:41:31.000000000 +0200
+++ ./util/biosdisk.c	2008-06-02 15:38:13.000000000 +0200
@@ -288,7 +288,6 @@ open_device (const grub_disk_t disk, gru
       is_partition = linux_find_partition (dev, disk->partition->start);
     
     /* Open the partition.  */
-    grub_util_info ("opening the device `%s'", dev);
     fd = open (dev, flags);
     if (fd < 0)
       {
@@ -464,6 +463,20 @@ static struct grub_disk_dev grub_util_bi
     .next = 0
   };
 
+/* Registers a device/drive map.  */
+int
+grub_util_biosdisk_register (char *device, char *drive)
+{
+  int index;
+  /* Find a free slot.  */
+  index = find_grub_drive (NULL);
+  if (index < 0)
+    return -1;
+  map[index].device = device;
+  map[index].drive = drive;
+  return 0;
+}
+
 static void
 read_device_map (const char *dev_map)
 {
@@ -486,7 +499,7 @@ read_device_map (const char *dev_map)
     {
       char *p = buf;
       char *e;
-      int drive;
+      char *device, *drive;
       
       lineno++;
       
@@ -502,19 +515,14 @@ read_device_map (const char *dev_map)
 	show_error ("No open parenthesis found");
 
       p++;
-      /* Find a free slot.  */
-      drive = find_grub_drive (NULL);
-      if (drive < 0)
-	show_error ("Map table size exceeded");
-
       e = p;
       p = strchr (p, ')');
       if (! p)
 	show_error ("No close parenthesis found");
 
-      map[drive].drive = xmalloc (p - e + sizeof ('\0'));
-      strncpy (map[drive].drive, e, p - e + sizeof ('\0'));
-      map[drive].drive[p - e] = '\0';
+      drive = xmalloc (p - e + sizeof ('\0'));
+      strncpy (drive, e, p - e + sizeof ('\0'));
+      drive[p - e] = '\0';
 
       p++;
       /* Skip leading spaces.  */
@@ -540,12 +548,13 @@ read_device_map (const char *dev_map)
       /* On Linux, the devfs uses symbolic links horribly, and that
 	 confuses the interface very much, so use realpath to expand
 	 symbolic links.  */
-      map[drive].device = xmalloc (PATH_MAX);
-      if (! realpath (p, map[drive].device))
+      device = xmalloc (PATH_MAX);
+      if (! realpath (p, device))
 	grub_util_error ("Cannot get the real path of `%s'", p);
 #else
-      map[drive].device = xstrdup (p);
+      device = xstrdup (p);
 #endif
+      grub_util_biosdisk_register (device, drive);
     }
 
   fclose (fp);
diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../tmp.old/util/grub-probe.c ./util/grub-probe.c
--- ../tmp.old/util/grub-probe.c	2008-05-30 13:07:10.000000000 +0200
+++ ./util/grub-probe.c	2008-06-02 15:40:52.000000000 +0200
@@ -153,15 +153,25 @@ probe (const char *path, char *device_na
     }
 
   drive_name = grub_util_get_grub_dev (device_name);
-  if (! drive_name)
-    grub_util_error ("Cannot find a GRUB drive for %s.  Check your device.map.\n", device_name);
   
   if (print == PRINT_DRIVE)
     {
+      if (! drive_name)
+	grub_util_error ("Cannot find a GRUB drive for %s.  Check your device.map.\n", device_name);
+
       printf ("(%s)\n", drive_name);
       goto end;
     }
 
+  /* For the purpose of checks that follow, a dummy drive name will do.  */
+  if (! drive_name)
+    {
+      drive_name = "dummy-grub-drive";
+      grub_util_info ("Cannot find GRUB drive for %s.  Registering \"(%s)\t%s\".",
+		      device_name, drive_name, device_name);
+      grub_util_biosdisk_register (xstrdup (device_name), xstrdup (drive_name));
+    }
+
   grub_util_info ("opening %s", drive_name);
   dev = grub_device_open (drive_name);
   if (! dev)

Re: [PATCH] register dummy drive

[Pavel Roskin]

On Mon, 2008-06-02 at 15:50 +0200, Robert Millan wrote:
> There's no reason grub-probe should fail if it can't resolve drive, when we
> just asked for -t fs, -t fs_uuid or -t partmap.
> 
> This patch solves the problem by spliting device/drive map[] entry registration
> into a separate function, and using that from grub-probe.c to register a dummy
> drive that will last during the current execution.

Cannot hostfs be used by default for all userspace utilities?

-- 
Regards,
Pavel Roskin

Re: [PATCH] register dummy drive

[Robert Millan]

Uhm.. however, you still need device-specific knowledge to find the full device
containing your partition (needed for -t partmap).

I'm not sure if there's a point to continue in this direction.  Should we
just exclude partmap from this?

On Mon, Jun 02, 2008 at 03:50:14PM +0200, Robert Millan wrote:
> 
> There's no reason grub-probe should fail if it can't resolve drive, when we
> just asked for -t fs, -t fs_uuid or -t partmap.
> 
> This patch solves the problem by spliting device/drive map[] entry registration
> into a separate function, and using that from grub-probe.c to register a dummy
> drive that will last during the current execution.
> 
> It's diffed relative to my previous patch for device function names (but if
> we need to change something in that one, not a problem for me to readapt it,
> of course).
> 
> -- 
> 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 /.)

> 2008-06-02  Robert Millan  <rmh@aybabtu.com>
> 
> 	* include/grub/util/biosdisk.h (grub_util_biosdisk_register): New
> 	function declaration.
> 	* util/biosdisk.c (open_device): Remove first grub_util_info() call
> 	(too verbose).
> 	(grub_util_biosdisk_register): New function.
> 	(read_device_map): Use grub_util_biosdisk_register() for registration
> 	of drive/device entries rather than doing it ourselves.
> 	* util/grub-probe.c (probe): If we weren't asked to -t drive, allow
> 	grub_util_get_grub_dev() to fail by registering a dummy drive entry,
> 	so that other options like -t fs, -t fs_uuid or -t partmap will work
> 	regardless.
> 
> diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../tmp.old/include/grub/util/biosdisk.h ./include/grub/util/biosdisk.h
> --- ../tmp.old/include/grub/util/biosdisk.h	2007-07-22 01:32:25.000000000 +0200
> +++ ./include/grub/util/biosdisk.h	2008-06-02 15:33:49.000000000 +0200
> @@ -23,5 +23,6 @@
>  void grub_util_biosdisk_init (const char *dev_map);
>  void grub_util_biosdisk_fini (void);
>  char *grub_util_biosdisk_get_grub_dev (const char *os_dev);
> +int grub_util_biosdisk_register (char *device, char *drive);
>  
>  #endif /* ! GRUB_BIOSDISK_MACHINE_UTIL_HEADER */
> diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../tmp.old/util/biosdisk.c ./util/biosdisk.c
> --- ../tmp.old/util/biosdisk.c	2008-06-02 15:41:31.000000000 +0200
> +++ ./util/biosdisk.c	2008-06-02 15:38:13.000000000 +0200
> @@ -288,7 +288,6 @@ open_device (const grub_disk_t disk, gru
>        is_partition = linux_find_partition (dev, disk->partition->start);
>      
>      /* Open the partition.  */
> -    grub_util_info ("opening the device `%s'", dev);
>      fd = open (dev, flags);
>      if (fd < 0)
>        {
> @@ -464,6 +463,20 @@ static struct grub_disk_dev grub_util_bi
>      .next = 0
>    };
>  
> +/* Registers a device/drive map.  */
> +int
> +grub_util_biosdisk_register (char *device, char *drive)
> +{
> +  int index;
> +  /* Find a free slot.  */
> +  index = find_grub_drive (NULL);
> +  if (index < 0)
> +    return -1;
> +  map[index].device = device;
> +  map[index].drive = drive;
> +  return 0;
> +}
> +
>  static void
>  read_device_map (const char *dev_map)
>  {
> @@ -486,7 +499,7 @@ read_device_map (const char *dev_map)
>      {
>        char *p = buf;
>        char *e;
> -      int drive;
> +      char *device, *drive;
>        
>        lineno++;
>        
> @@ -502,19 +515,14 @@ read_device_map (const char *dev_map)
>  	show_error ("No open parenthesis found");
>  
>        p++;
> -      /* Find a free slot.  */
> -      drive = find_grub_drive (NULL);
> -      if (drive < 0)
> -	show_error ("Map table size exceeded");
> -
>        e = p;
>        p = strchr (p, ')');
>        if (! p)
>  	show_error ("No close parenthesis found");
>  
> -      map[drive].drive = xmalloc (p - e + sizeof ('\0'));
> -      strncpy (map[drive].drive, e, p - e + sizeof ('\0'));
> -      map[drive].drive[p - e] = '\0';
> +      drive = xmalloc (p - e + sizeof ('\0'));
> +      strncpy (drive, e, p - e + sizeof ('\0'));
> +      drive[p - e] = '\0';
>  
>        p++;
>        /* Skip leading spaces.  */
> @@ -540,12 +548,13 @@ read_device_map (const char *dev_map)
>        /* On Linux, the devfs uses symbolic links horribly, and that
>  	 confuses the interface very much, so use realpath to expand
>  	 symbolic links.  */
> -      map[drive].device = xmalloc (PATH_MAX);
> -      if (! realpath (p, map[drive].device))
> +      device = xmalloc (PATH_MAX);
> +      if (! realpath (p, device))
>  	grub_util_error ("Cannot get the real path of `%s'", p);
>  #else
> -      map[drive].device = xstrdup (p);
> +      device = xstrdup (p);
>  #endif
> +      grub_util_biosdisk_register (device, drive);
>      }
>  
>    fclose (fp);
> diff -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../tmp.old/util/grub-probe.c ./util/grub-probe.c
> --- ../tmp.old/util/grub-probe.c	2008-05-30 13:07:10.000000000 +0200
> +++ ./util/grub-probe.c	2008-06-02 15:40:52.000000000 +0200
> @@ -153,15 +153,25 @@ probe (const char *path, char *device_na
>      }
>  
>    drive_name = grub_util_get_grub_dev (device_name);
> -  if (! drive_name)
> -    grub_util_error ("Cannot find a GRUB drive for %s.  Check your device.map.\n", device_name);
>    
>    if (print == PRINT_DRIVE)
>      {
> +      if (! drive_name)
> +	grub_util_error ("Cannot find a GRUB drive for %s.  Check your device.map.\n", device_name);
> +
>        printf ("(%s)\n", drive_name);
>        goto end;
>      }
>  
> +  /* For the purpose of checks that follow, a dummy drive name will do.  */
> +  if (! drive_name)
> +    {
> +      drive_name = "dummy-grub-drive";
> +      grub_util_info ("Cannot find GRUB drive for %s.  Registering \"(%s)\t%s\".",
> +		      device_name, drive_name, device_name);
> +      grub_util_biosdisk_register (xstrdup (device_name), xstrdup (drive_name));
> +    }
> +
>    grub_util_info ("opening %s", drive_name);
>    dev = grub_device_open (drive_name);
>    if (! dev)


-- 
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] register dummy drive

[Robert Millan]

On Mon, Jun 02, 2008 at 10:11:50AM -0400, Pavel Roskin wrote:
> On Mon, 2008-06-02 at 15:50 +0200, Robert Millan wrote:
> > There's no reason grub-probe should fail if it can't resolve drive, when we
> > just asked for -t fs, -t fs_uuid or -t partmap.
> > 
> > This patch solves the problem by spliting device/drive map[] entry registration
> > into a separate function, and using that from grub-probe.c to register a dummy
> > drive that will last during the current execution.
> 
> Cannot hostfs be used by default for all userspace utilities?

Part of what makes grub-probe interesting is that it shares a lot of code with
the freestanding GRUB you will run later, so when it is used during
grub-install & update-grub, it is very useful to catch possible problems.  I
think hostfs would defeat that purpose.

-- 
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] register dummy drive

[Pavel Roskin]

On Tue, 2008-06-03 at 23:12 +0200, Robert Millan wrote:
> On Mon, Jun 02, 2008 at 10:11:50AM -0400, Pavel Roskin wrote:
> > On Mon, 2008-06-02 at 15:50 +0200, Robert Millan wrote:
> > > There's no reason grub-probe should fail if it can't resolve drive, when we
> > > just asked for -t fs, -t fs_uuid or -t partmap.
> > > 
> > > This patch solves the problem by spliting device/drive map[] entry registration
> > > into a separate function, and using that from grub-probe.c to register a dummy
> > > drive that will last during the current execution.
> > 
> > Cannot hostfs be used by default for all userspace utilities?
> 
> Part of what makes grub-probe interesting is that it shares a lot of code with
> the freestanding GRUB you will run later, so when it is used during
> grub-install & update-grub, it is very useful to catch possible problems.  I
> think hostfs would defeat that purpose.

Well, then we probably don't want to ignore any errors.  When do you
have the situation that the drive cannot be resolved?

My concern is that introducing a dummy object for non-testing purposes
could indicate bad code quality.

-- 
Regards,
Pavel Roskin

Re: [PATCH] register dummy drive

[Robert Millan]

On Tue, Jun 03, 2008 at 06:03:51PM -0400, Pavel Roskin wrote:
> > > > 
> > > > This patch solves the problem by spliting device/drive map[] entry registration
> > > > into a separate function, and using that from grub-probe.c to register a dummy
> > > > drive that will last during the current execution.
> > 
> > Part of what makes grub-probe interesting is that it shares a lot of code with
> > the freestanding GRUB you will run later, so when it is used during
> > grub-install & update-grub, it is very useful to catch possible problems.  I
> > think hostfs would defeat that purpose.
> 
> Well, then we probably don't want to ignore any errors.  When do you
> have the situation that the drive cannot be resolved?

For example, when the next version of Linux decides to rename all devices (it
tends to do that often) and suddenly none of them match any device.map entry.

The thing is, for what we're trying to do (-t fs, -t fs_uuid, -t partmap) we
don't care how will GRUB identify those devices when it's running on the BIOS,
so just any string that uniquely identifies them will be enough so we can
run our filesystem / partmap probes.

> My concern is that introducing a dummy object for non-testing purposes
> could indicate bad code quality.

It got a bit stuck anyway; see my other mail about -t partmap not working
with this method.

-- 
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] register dummy drive

[Pavel Roskin]

On Wed, 2008-06-04 at 14:07 +0200, Robert Millan wrote:
> On Tue, Jun 03, 2008 at 06:03:51PM -0400, Pavel Roskin wrote:
> > > > > 
> > > > > This patch solves the problem by spliting device/drive map[] entry registration
> > > > > into a separate function, and using that from grub-probe.c to register a dummy
> > > > > drive that will last during the current execution.
> > > 
> > > Part of what makes grub-probe interesting is that it shares a lot of code with
> > > the freestanding GRUB you will run later, so when it is used during
> > > grub-install & update-grub, it is very useful to catch possible problems.  I
> > > think hostfs would defeat that purpose.
> > 
> > Well, then we probably don't want to ignore any errors.  When do you
> > have the situation that the drive cannot be resolved?
> 
> For example, when the next version of Linux decides to rename all devices (it
> tends to do that often) and suddenly none of them match any device.map entry.
> 
> The thing is, for what we're trying to do (-t fs, -t fs_uuid, -t partmap) we
> don't care how will GRUB identify those devices when it's running on the BIOS,
> so just any string that uniquely identifies them will be enough so we can
> run our filesystem / partmap probes.

OK, then the "dummy drive" is not really dummy.  Those in device.map are
dummy :-)

I'm basically fine with anything that gets rid on device.map at least
for single-drive installs.

-- 
Regards,
Pavel Roskin