[PATCH] search -d|--disk

[PATCH] search -d|--disk

[Robert Millan]

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


Hi,

This patch adds an option to the search command to restrict the search of
a file to a given disk.  It will then probe for the file in each of its
partitions, but not in other disks.

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

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

2009-06-22  Robert Millan  <rmh.grub@aybabtu.com>

	* commands/search.c: Include `<grub/disk.h>' and
	`<grub/partition.h>'.
	(struct grub_arg_option options): Add "disk" option.
	(enum options): Add `SEARCH_DISK' key.
	(search_file): Add a new parameter, `restrict_to', which when
	set will restrict the search to a given disk.
	(grub_cmd_search): Recognize -d|--disk option, and pass it as
	parameter to search_file().

Index: commands/search.c
===================================================================
--- commands/search.c	(revision 2362)
+++ commands/search.c	(working copy)
@@ -23,6 +23,8 @@
 #include <grub/err.h>
 #include <grub/dl.h>
 #include <grub/device.h>
+#include <grub/disk.h>
+#include <grub/partition.h>
 #include <grub/file.h>
 #include <grub/env.h>
 #include <grub/extcmd.h>
@@ -34,6 +36,7 @@ static const struct grub_arg_option opti
     {"fs-uuid",		'u', 0, "search devices by a filesystem UUID", 0, 0},
     {"set",		's', GRUB_ARG_OPTION_OPTIONAL, "set a variable to the first device found", "VAR", ARG_TYPE_STRING},
     {"no-floppy",	'n', 0, "do not probe any floppy drive", 0, 0},
+    {"disk",		'd', 0, "only search a file in this disk and its partitions", "VAR", ARG_TYPE_DEVICE},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -44,6 +47,7 @@ enum options
     SEARCH_FS_UUID,
     SEARCH_SET,
     SEARCH_NO_FLOPPY,
+    SEARCH_DISK,
  };
 
 static void
@@ -110,7 +114,7 @@ search_fs (const char *key, const char *
 }
 
 static void
-search_file (const char *key, const char *var, int no_floppy)
+search_file (const char *key, const char *var, int no_floppy, grub_disk_t restrict_to)
 {
   int count = 0;
   char *buf = 0;
@@ -157,7 +161,37 @@ search_file (const char *key, const char
       return abort;
     }
 
-  grub_device_iterate (iterate_device);
+  if (restrict_to)
+    {
+      unsigned int partcount = 0, i;
+      grub_size_t len;
+      char *partname;
+      int ret;
+
+      auto int hook (struct grub_disk *disk, const grub_partition_t partition);
+      int hook (struct grub_disk *disk __attribute__ ((unused)),
+		const grub_partition_t partition __attribute__ ((unused)))
+	{
+	  partcount++;
+	  return 0;
+	}
+
+      grub_partition_iterate (restrict_to, hook);
+
+      if (! iterate_device (restrict_to->name))
+	for (i = 1; i <= partcount; i++)
+	  {
+	    len = grub_strlen (restrict_to->name);
+	    partname = grub_malloc (len + 4);
+	    grub_sprintf (partname, "%s,%u", restrict_to->name, i);
+	    ret = iterate_device (partname);
+	    grub_free (partname);
+	    if (ret)
+	      break;
+	  }
+    }
+  else
+    grub_device_iterate (iterate_device);
 
   grub_free (buf);
 
@@ -170,6 +204,7 @@ grub_cmd_search (grub_extcmd_t cmd, int 
 {
   struct grub_arg_list *state = cmd->state;
   const char *var = 0;
+  grub_disk_t disk = NULL;
 
   if (argc == 0)
     return grub_error (GRUB_ERR_INVALID_COMMAND, "no argument specified");
@@ -177,12 +212,20 @@ grub_cmd_search (grub_extcmd_t cmd, int 
   if (state[SEARCH_SET].set)
     var = state[SEARCH_SET].arg ? state[SEARCH_SET].arg : "root";
 
+  if (state[SEARCH_DISK].set)
+    {
+      if (state[SEARCH_DISK].arg)
+	disk = grub_disk_open (state[SEARCH_DISK].arg);
+      else
+	return grub_error (GRUB_ERR_INVALID_COMMAND, "unspecified disk");
+    }
+
   if (state[SEARCH_LABEL].set)
     search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, 0);
   else if (state[SEARCH_FS_UUID].set)
     search_fs (args[0], var, state[SEARCH_NO_FLOPPY].set, 1);
   else if (state[SEARCH_FILE].set)
-    search_file (args[0], var, state[SEARCH_NO_FLOPPY].set);
+    search_file (args[0], var, state[SEARCH_NO_FLOPPY].set, disk);
   else
     return grub_error (GRUB_ERR_INVALID_COMMAND, "unspecified search type");
 
@@ -196,7 +239,7 @@ GRUB_MOD_INIT(search)
   cmd =
     grub_register_extcmd ("search", grub_cmd_search,
 			  GRUB_COMMAND_FLAG_BOTH,
-			  "search [-f|-l|-u|-s|-n] NAME",
+			  "search [-f|-l|-u|-s|-n|-d] NAME",
 			  "Search devices by file, filesystem label or filesystem UUID."
 			  " If --set is specified, the first device found is"
 			  " set to a variable. If no variable name is"

Re: [PATCH] search -d|--disk

[Pavel Roskin]

On Tue, 2009-06-23 at 01:48 +0200, Robert Millan wrote:

> This patch adds an option to the search command to restrict the search of
> a file to a given disk.  It will then probe for the file in each of its
> partitions, but not in other disks.

First of all, it would be great is all proposals to add more code come
with a possible use case.  We are approaching the point when grub with
all modules would become too big to some boot partitions.

"VAR" should be "DISK".

"in this disk" should be "on this disk" (disclaimer: English is not my
native language).

"and its partitions" seems excessive to me.  If the disk has partitions,
of course that where we would look.

The implementation is not working:

sh:grub> search -d foo -l /
 hd0,3

I hope that it would be possible to specify a partition for the disk, so
that we easily can look e.g. for a partition with label "/" on a disk
with a partition containing "/boot/grub/grub.cfg".

-- 
Regards,
Pavel Roskin

Re: [PATCH] search -d|--disk

[Robert Millan]

On Mon, Jun 22, 2009 at 09:51:35PM -0400, Pavel Roskin wrote:
> On Tue, 2009-06-23 at 01:48 +0200, Robert Millan wrote:
> 
> > This patch adds an option to the search command to restrict the search of
> > a file to a given disk.  It will then probe for the file in each of its
> > partitions, but not in other disks.
> 
> First of all, it would be great is all proposals to add more code come
> with a possible use case.

QEMU has a feature in which you can specify the boot drive from command
line (-boot parameter).  After i386-qemu port is merged, I plan to add
some code to read this from CMOS and export it to some variable.

When on GRUB, it is up to the user to decide what "boot this drive" means.
An interesting option is to search for a specific file in the disk we're told,
and then act upon it (e.g. configfile /grub.cfg, multiboot /grub.elf,
linux /vmlinuz, whatever).

> We are approaching the point when grub with
> all modules would become too big to some boot partitions.

Yes.  Note that search.mod is not included in i386-pc's core.img, though.

> "VAR" should be "DISK".
> 
> "in this disk" should be "on this disk" (disclaimer: English is not my
> native language).
> 
> "and its partitions" seems excessive to me.  If the disk has partitions,
> of course that where we would look.

Ok.

> The implementation is not working:
> 
> sh:grub> search -d foo -l /
>  hd0,3
> 
> I hope that it would be possible to specify a partition for the disk, so
> that we easily can look e.g. for a partition with label "/" on a disk
> with a partition containing "/boot/grub/grub.cfg".

I implemented it only for files.  It's trivial to do it for labels/uuids
too, but it's annoying because doing so results in code duplication.

In general, there's a lot of code duplication in search.mod.  I recently
tried to diminish that by merging search_label() with search_fs_uuid() but
it's still far from ideal.

Any suggestions on what to do about this?

-- 
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] search -d|--disk

[Pavel Roskin]

On Tue, 2009-06-23 at 13:50 +0200, Robert Millan wrote: 
> On Mon, Jun 22, 2009 at 09:51:35PM -0400, Pavel Roskin wrote:
> > On Tue, 2009-06-23 at 01:48 +0200, Robert Millan wrote:
> > 
> > > This patch adds an option to the search command to restrict the search of
> > > a file to a given disk.  It will then probe for the file in each of its
> > > partitions, but not in other disks.
> > 
> > First of all, it would be great is all proposals to add more code come
> > with a possible use case.
> 
> QEMU has a feature in which you can specify the boot drive from command
> line (-boot parameter).  After i386-qemu port is merged, I plan to add
> some code to read this from CMOS and export it to some variable.
> 
> When on GRUB, it is up to the user to decide what "boot this drive" means.
> An interesting option is to search for a specific file in the disk we're told,
> and then act upon it (e.g. configfile /grub.cfg, multiboot /grub.elf,
> linux /vmlinuz, whatever).

I think the convention is to load the first sector of the disk.  I don't
think it's necessary to invent anything extra.  Maybe we could pass
environment variables using some qemu facility.

> > We are approaching the point when grub with
> > all modules would become too big to some boot partitions.
> 
> Yes.  Note that search.mod is not included in i386-pc's core.img, though.

I know.  But we are already at the point where the full GRUB install
cannot coexist with yaboot on a standard 1Mb boot partition on PowerMAC.

> > "VAR" should be "DISK".
> > 
> > "in this disk" should be "on this disk" (disclaimer: English is not my
> > native language).
> > 
> > "and its partitions" seems excessive to me.  If the disk has partitions,
> > of course that where we would look.
> 
> Ok.
> 
> > The implementation is not working:
> > 
> > sh:grub> search -d foo -l /
> >  hd0,3
> > 
> > I hope that it would be possible to specify a partition for the disk, so
> > that we easily can look e.g. for a partition with label "/" on a disk
> > with a partition containing "/boot/grub/grub.cfg".
> 
> I implemented it only for files.  It's trivial to do it for labels/uuids
> too, but it's annoying because doing so results in code duplication.

Yes, it's annoying, but partial implementations are annoying to the end users.

Perhaps it should be possible to implement disk filtering using the
mechanism used to skip floppies.

> In general, there's a lot of code duplication in search.mod.  I recently
> tried to diminish that by merging search_label() with search_fs_uuid() but
> it's still far from ideal.
> 
> Any suggestions on what to do about this?

For the boot disk selection, it's better to follow BIOS conventions.  As
for the advanced scripting, it can be done in lua.  

-- 
Regards,
Pavel Roskin

Re: [PATCH] search -d|--disk

[Robert Millan]

On Tue, Jun 23, 2009 at 05:17:22PM -0400, Pavel Roskin wrote:
> > 
> > QEMU has a feature in which you can specify the boot drive from command
> > line (-boot parameter).  After i386-qemu port is merged, I plan to add
> > some code to read this from CMOS and export it to some variable.
> > 
> > When on GRUB, it is up to the user to decide what "boot this drive" means.
> > An interesting option is to search for a specific file in the disk we're told,
> > and then act upon it (e.g. configfile /grub.cfg, multiboot /grub.elf,
> > linux /vmlinuz, whatever).
> 
> I think the convention is to load the first sector of the disk.

That's the BIOS convention, which is useful when there's a BIOS.  Otherwise
there isn't much you can do with 512 bytes of code.

> Maybe we could pass
> environment variables using some qemu facility.

Yes, qemu exports -boot parameter to CMOS, just like memory size.  GRUB can
read it from there.

> > I implemented it only for files.  It's trivial to do it for labels/uuids
> > too, but it's annoying because doing so results in code duplication.
> 
> Yes, it's annoying, but partial implementations are annoying to the end users.
> 
> Perhaps it should be possible to implement disk filtering using the
> mechanism used to skip floppies.

I'll see what I can do.

-- 
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] search -d|--disk

[Arthur Marsh]

Robert Millan wrote, on 24/06/09 07:25:
> On Tue, Jun 23, 2009 at 05:17:22PM -0400, Pavel Roskin wrote:
>>> QEMU has a feature in which you can specify the boot drive from command
>>> line (-boot parameter).  After i386-qemu port is merged, I plan to add
>>> some code to read this from CMOS and export it to some variable.
>>>
>>> When on GRUB, it is up to the user to decide what "boot this drive" means.
>>> An interesting option is to search for a specific file in the disk we're told,
>>> and then act upon it (e.g. configfile /grub.cfg, multiboot /grub.elf,
>>> linux /vmlinuz, whatever).
>> I think the convention is to load the first sector of the disk.
> 
> That's the BIOS convention, which is useful when there's a BIOS.  Otherwise
> there isn't much you can do with 512 bytes of code.
> 
>> Maybe we could pass
>> environment variables using some qemu facility.
> 
> Yes, qemu exports -boot parameter to CMOS, just like memory size.  GRUB can
> read it from there.
> 
>>> I implemented it only for files.  It's trivial to do it for labels/uuids
>>> too, but it's annoying because doing so results in code duplication.
>> Yes, it's annoying, but partial implementations are annoying to the end users.
>>
>> Perhaps it should be possible to implement disk filtering using the
>> mechanism used to skip floppies.
> 
> I'll see what I can do.
> 

Please cc: me when this patch is merged - I'd like to get to the bottom 
of why:

search --no-floppy --fs-uuid --set bfdeb6d6-0b77-4beb-a63d-bdc3e455b8ea

fails. (See https://savannah.gnu.org/bugs/?26834 )

Being able to only search the correct disk (and produce meaningful 
debugging output) would help find the source of this problem which 
prevents automatic booting with GRUB2.

Regards,

Arthur.

Re: [PATCH] search -d|--disk

[Robert Millan]

On Wed, Jun 24, 2009 at 12:33:52PM +0930, Arthur Marsh wrote:
>
> Please cc: me when this patch is merged - I'd like to get to the bottom  
> of why:
>
> search --no-floppy --fs-uuid --set bfdeb6d6-0b77-4beb-a63d-bdc3e455b8ea
>
> fails. (See https://savannah.gnu.org/bugs/?26834 )
>
> Being able to only search the correct disk (and produce meaningful  
> debugging output) would help find the source of this problem which  
> prevents automatic booting with GRUB2.

Can't you apply it by hand to do that test?

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