[PATCH] Refactor commands/search.c

[PATCH] Refactor commands/search.c

[Pavel Roskin]

ChangeLog:

	* commands/search.c (search_file): Merge into ...
	(search_fs): ... this.  On error, don't output what we were
	looking for.  Accept search type as argument.
	(grub_cmd_search): Pass search type to search_fs().
---

 commands/search.c |  190 ++++++++++++++++++++++-------------------------------
 1 files changed, 80 insertions(+), 110 deletions(-)

diff --git a/commands/search.c b/commands/search.c
index b29eefb..f45d8ab 100644
--- a/commands/search.c
+++ b/commands/search.c
@@ -47,124 +47,94 @@ enum options
  };
 
 static void
-search_fs (const char *key, const char *var, int no_floppy, int is_uuid)
-{
-  int count = 0;
-  auto int iterate_device (const char *name);
-
-  int iterate_device (const char *name)
-    {
-      grub_device_t dev;
-      int abort = 0;
-
-      /* Skip floppy drives when requested.  */
-      if (no_floppy &&
-	  name[0] == 'f' && name[1] == 'd' &&
-	  name[2] >= '0' && name[2] <= '9')
-	return 0;
-
-      dev = grub_device_open (name);
-      if (dev)
-	{
-	  grub_fs_t fs;
-	  int (*compare_fn) (const char *, const char *);
-	  grub_err_t (*quid_fn) (grub_device_t, char **);
-
-	  fs = grub_fs_probe (dev);
-	  compare_fn = is_uuid ? grub_strcasecmp : grub_strcmp;
-	  quid_fn = is_uuid ? fs->uuid : fs->label;
-
-	  if (fs && quid_fn)
-	    {
-	      char *quid;
-
-	      quid_fn (dev, &quid);
-	      if (grub_errno == GRUB_ERR_NONE && quid)
-		{
-		  if (compare_fn (quid, key) == 0)
-		    {
-		      /* Found!  */
-		      count++;
-		      if (var)
-			{
-			  grub_env_set (var, name);
-			  abort = 1;
-			}
-		      else
-			  grub_printf (" %s", name);
-		    }
-
-		  grub_free (quid);
-		}
-	    }
-
-	  grub_device_close (dev);
-	}
-
-      grub_errno = GRUB_ERR_NONE;
-      return abort;
-    }
-
-  grub_device_iterate (iterate_device);
-
-  if (count == 0)
-    grub_error (GRUB_ERR_FILE_NOT_FOUND, "no such device: %s", key);
-}
-
-static void
-search_file (const char *key, const char *var, int no_floppy)
+search_fs (const char *key, const char *var, int no_floppy, enum options type)
 {
   int count = 0;
   char *buf = 0;
-  auto int iterate_device (const char *name);
 
+  auto int iterate_device (const char *name);
   int iterate_device (const char *name)
-    {
-      grub_size_t len;
-      char *p;
-      grub_file_t file;
-      int abort = 0;
-
-      /* Skip floppy drives when requested.  */
-      if (no_floppy &&
-	  name[0] == 'f' && name[1] == 'd' &&
-	  name[2] >= '0' && name[2] <= '9')
-	return 0;
-
-      len = grub_strlen (name) + 2 + grub_strlen (key) + 1;
-      p = grub_realloc (buf, len);
-      if (! p)
-	return 1;
-
-      buf = p;
-      grub_sprintf (buf, "(%s)%s", name, key);
-
-      file = grub_file_open (buf);
-      if (file)
-	{
-	  /* Found!  */
-	  count++;
-	  if (var)
-	    {
-	      grub_env_set (var, name);
-	      abort = 1;
-	    }
-	  else
-	    grub_printf (" %s", name);
-
-	  grub_file_close (file);
-	}
-
-      grub_errno = GRUB_ERR_NONE;
-      return abort;
-    }
+  {
+    int found = 0;
+
+    /* Skip floppy drives when requested.  */
+    if (no_floppy &&
+	name[0] == 'f' && name[1] == 'd' &&
+	name[2] >= '0' && name[2] <= '9')
+      return 0;
+
+    if (type == SEARCH_FILE)
+      {
+	grub_size_t len;
+	char *p;
+	grub_file_t file;
+
+	len = grub_strlen (name) + 2 + grub_strlen (key) + 1;
+	p = grub_realloc (buf, len);
+	if (! p)
+	  return 1;
+
+	buf = p;
+	grub_sprintf (buf, "(%s)%s", name, key);
+
+	file = grub_file_open (buf);
+	if (file)
+	  {
+	    found = 1;
+	    grub_file_close (file);
+	  }
+      }
+    else
+      {
+	grub_device_t dev;
+	grub_fs_t fs;
+	int (*compare_fn) (const char *, const char *);
+	grub_err_t (*quid_fn) (grub_device_t, char **);
+	char *quid;
+
+	dev = grub_device_open (name);
+	if (dev)
+	  {
+	    fs = grub_fs_probe (dev);
+	    compare_fn =
+	      (type == SEARCH_FS_UUID) ? grub_strcasecmp : grub_strcmp;
+	    quid_fn = (type == SEARCH_FS_UUID) ? fs->uuid : fs->label;
+
+	    if (fs && quid_fn)
+	      {
+		quid_fn (dev, &quid);
+		if (grub_errno == GRUB_ERR_NONE && quid)
+		  {
+		    if (compare_fn (quid, key) == 0)
+		      found = 1;
+
+		    grub_free (quid);
+		  }
+	      }
+
+	    grub_device_close (dev);
+	  }
+      }
+
+    if (found)
+      {
+	count++;
+	if (var)
+	  grub_env_set (var, name);
+	else
+	  grub_printf (" %s", name);
+      }
+
+    grub_errno = GRUB_ERR_NONE;
+    return (found && var);
+  }
 
   grub_device_iterate (iterate_device);
 
   grub_free (buf);
 
   if (grub_errno == GRUB_ERR_NONE && count == 0)
-    grub_error (GRUB_ERR_FILE_NOT_FOUND, "no such file: %s", key);
+    grub_error (GRUB_ERR_FILE_NOT_FOUND, "no such device");
 }
 
 static grub_err_t
@@ -180,11 +150,11 @@ grub_cmd_search (grub_extcmd_t cmd, int argc, char **args)
     var = state[SEARCH_SET].arg ? state[SEARCH_SET].arg : "root";
 
   if (state[SEARCH_LABEL].set)
-    search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, 0);
+    search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, SEARCH_LABEL);
   else if (state[SEARCH_FS_UUID].set)
-    search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, 1);
+    search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, SEARCH_FS_UUID);
   else if (state[SEARCH_FILE].set)
-    search_file (args[0], var, state[SEARCH_NO_FLOPPY].set);
+    search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, SEARCH_FILE);
   else
     return grub_error (GRUB_ERR_INVALID_COMMAND, "unspecified search type");
 

Re: [PATCH] Refactor commands/search.c

[Arthur Marsh]

Pavel Roskin wrote, on 2009-07-07 06:43:
> ChangeLog:
> 
> 	* commands/search.c (search_file): Merge into ...
> 	(search_fs): ... this.  On error, don't output what we were
> 	looking for.  Accept search type as argument.
> 	(grub_cmd_search): Pass search type to search_fs().

I tried this on current SVN, and am now getting "device not found" errors.

My /boot/grub/device.map is:

(hd0)   /dev/hda
(hd1)   /dev/disk/by-id/scsi-SIBM_DCAS-34330W_F3T92021
#(hd2)   /dev/disk/by-id/usb-Verbatim_STORE_N_GO_078A18B40293-0:0
#(hd2)   /dev/disk/by-id/usb-Flash_Drive_AU_USB2.0_OGUN4WMN-0:0
(hd3) /dev/hdc

all 3 uncommented devices do exist.

For more details see:
https://savannah.gnu.org/bugs/?26834 search for UUID fails with 
dedicated /boot filesystem on disk larger than what BIOS sees

I'll try commenting out all but /dev/hda and re-running update-grub.

Regards,

Arthur.

Re: [PATCH] Refactor commands/search.c

[Arthur Marsh]

Arthur Marsh wrote, on 07/07/09 10:33:
> Pavel Roskin wrote, on 2009-07-07 06:43:
>> ChangeLog:
>>
>>     * commands/search.c (search_file): Merge into ...
>>     (search_fs): ... this.  On error, don't output what we were
>>     looking for.  Accept search type as argument.
>>     (grub_cmd_search): Pass search type to search_fs().
> 
> I tried this on current SVN, and am now getting "device not found" errors.
> 
> My /boot/grub/device.map is:
> 
> (hd0)   /dev/hda
> (hd1)   /dev/disk/by-id/scsi-SIBM_DCAS-34330W_F3T92021
> #(hd2)   /dev/disk/by-id/usb-Verbatim_STORE_N_GO_078A18B40293-0:0
> #(hd2)   /dev/disk/by-id/usb-Flash_Drive_AU_USB2.0_OGUN4WMN-0:0
> (hd3) /dev/hdc
> 
> all 3 uncommented devices do exist.
> 
> For more details see:
> https://savannah.gnu.org/bugs/?26834 search for UUID fails with 
> dedicated /boot filesystem on disk larger than what BIOS sees
> 
> I'll try commenting out all but /dev/hda and re-running update-grub.

when I try to boot grub2, I get two very quick

error: no such device

at the grub command prompt when I do ls I get:

(hd0) (hd0,1) (hd1) (hd2) (fd0)
error: no such device

It would be good to know *what* is leading to the "error: no such device".

Regards,

Arthur Marsh.

Re: [PATCH] Refactor commands/search.c

[Pavel Roskin]

On Tue, 2009-07-07 at 11:16 +0930, Arthur Marsh wrote:

> at the grub command prompt when I do ls I get:
> 
> (hd0) (hd0,1) (hd1) (hd2) (fd0)
> error: no such device
> 
> It would be good to know *what* is leading to the "error: no such device".

Me too.  There are two occurrences of "no such device" in the sources.
One is in the search command since you are using my patch.  The other is
in disk/efi/efidisk.c.  But it's not even compiled on the i386-pc
platform.

Could you please double check that it's the message that comes from the
"ls" command?

-- 
Regards,
Pavel Roskin

Re: [PATCH] Refactor commands/search.c

[Arthur Marsh]

Pavel Roskin wrote, on 07/07/09 11:43:
> On Tue, 2009-07-07 at 11:16 +0930, Arthur Marsh wrote:
> 
>> at the grub command prompt when I do ls I get:
>>
>> (hd0) (hd0,1) (hd1) (hd2) (fd0)
>> error: no such device
>>
>> It would be good to know *what* is leading to the "error: no such device".
> 
> Me too.  There are two occurrences of "no such device" in the sources.
> One is in the search command since you are using my patch.  The other is
> in disk/efi/efidisk.c.  But it's not even compiled on the i386-pc
> platform.
> 
> Could you please double check that it's the message that comes from the
> "ls" command?
> 

ok, checking my notes... "error: no such device" does not seem to come 
from the "ls" command. Sorry for any confusion.

when I did the "ls" command from real grub, I received:

error: no such disk

Arthur.

Re: [PATCH] Refactor commands/search.c

[Robert Millan]

On Mon, Jul 06, 2009 at 05:13:03PM -0400, Pavel Roskin wrote:
> ChangeLog:
> 
> 	* commands/search.c (search_file): Merge into ...
> 	(search_fs): ... this.  On error, don't output what we were
> 	looking for.  Accept search type as argument.
> 	(grub_cmd_search): Pass search type to search_fs().

Very nice work.  I've been wanting to do this for a while...

> +    if (type == SEARCH_FILE)
> +      {
> +	grub_size_t len;
> +	char *p;
> +	grub_file_t file;
> +
> +	len = grub_strlen (name) + 2 + grub_strlen (key) + 1;
> +	p = grub_realloc (buf, len);
> +	if (! p)
> +	  return 1;
> +
> +	buf = p;
> +	grub_sprintf (buf, "(%s)%s", name, key);
> +
> +	file = grub_file_open (buf);
> +	if (file)
> +	  {
> +	    found = 1;
> +	    grub_file_close (file);
> +	  }
> +      }
> +    else
> +      {

I'd suggest making this clearer by eithe explicitly checking for
SEARCH_XXX or otherwise put a comment after the else.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Refactor commands/search.c

[Pavel Roskin]

On Tue, 2009-07-07 at 20:42 +0200, Robert Millan wrote:

> I'd suggest making this clearer by eithe explicitly checking for
> SEARCH_XXX or otherwise put a comment after the else.

Done.  Committed.

-- 
Regards,
Pavel Roskin