Re: [PATCH] Add option to grub-probe to accept system devices as arguments

Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Please have a look at the patch I provided in my last mail, too. This is a 
different approach which requires no further command line option but *detects* 
whether the given argument is a path or a device.

Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Fabian Greffrath schrieb:
> Please have a look at the patch I provided in my last mail, too. This is 
> a different approach which requires no further command line option but 
> *detects* whether the given argument is a path or a device.

Ping! ;-)

Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Tue, Feb 12, 2008 at 02:17:11PM +0100, Fabian Greffrath wrote:
> Please have a look at the patch I provided in my last mail, too. This is a 
> different approach which requires no further command line option but 
> *detects* whether the given argument is a path or a device.

I find that too unreliable.  How do you reproduce current behaviour with
the command "grub-probe -t fs /dev/sda1" ?

I'd prefer exposed user options than hidden behaviour like this one.

-- 
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] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> I find that too unreliable.  How do you reproduce current behaviour with
> the command "grub-probe -t fs /dev/sda1" ?

I don't see the problem. Why should this be unreliable?

If the device does not exist, grub-probe will throw a "cannot stat /dev/sda1" 
error with and without the patch. If the device does exist, grub-probe will 
throw a "cannot find device for /dev/sda1" error without the patch and will 
return "ext2" with the patch applied.

See the following examples (I have /dev/hda1, but not /dev/sda1):

	Patched grub-probe:
	# grub-probe -t fs /dev/sda1
	grub-probe: error: cannot stat /dev/sda1.
	# grub-probe -t fs /dev/hda1
	ext2
	
	Current grub-probe:
	# grub-probe -t fs /dev/sda1
	grub-probe: error: Cannot stat `/dev/sda1'
	# grub-probe -t fs /dev/hda1
	grub-probe: error: cannot find a device for /dev/hda1.

> I'd prefer exposed user options than hidden behaviour like this one.

I still don't see the problem. The program will behave absolutely predictable: 
If the argument is a block device it will print the device's file system. If the 
argument is a valid path, it will prints the corresponding device's file system. 
If the argument is none of both, it will throw an error.

Cheers,
Fabian

Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Wed, Feb 13, 2008 at 05:51:53PM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >I find that too unreliable.  How do you reproduce current behaviour with
> >the command "grub-probe -t fs /dev/sda1" ?
> 
> I don't see the problem. Why should this be unreliable?
> 
> If the device does not exist, grub-probe will throw a "cannot stat 
> /dev/sda1" error with and without the patch. If the device does exist, 
> grub-probe will throw a "cannot find device for /dev/sda1" error without 
> the patch and will return "ext2" with the patch applied.
> 
> See the following examples (I have /dev/hda1, but not /dev/sda1):
> 
> 	Patched grub-probe:
> 	# grub-probe -t fs /dev/sda1
> 	grub-probe: error: cannot stat /dev/sda1.
> 	# grub-probe -t fs /dev/hda1
> 	ext2
> 	
> 	Current grub-probe:
> 	# grub-probe -t fs /dev/sda1
> 	grub-probe: error: Cannot stat `/dev/sda1'
> 	# grub-probe -t fs /dev/hda1
> 	grub-probe: error: cannot find a device for /dev/hda1.

The last one sounds like a bug.

> >I'd prefer exposed user options than hidden behaviour like this one.
> 
> I still don't see the problem. The program will behave absolutely 
> predictable: If the argument is a block device it will print the device's 
> file system. If the argument is a valid path, it will prints the 
> corresponding device's file system. If the argument is none of both, it 
> will throw an error.

Yes, but does the user know that?  Which interface is easier to figure out
for the user, one in which different actions require different parameters,
or one where the same parameters act in completely different way depending
on the file?

-- 
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] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> The last one sounds like a bug.

Yes, it's from the current *unpatched* grub-probe.

> Yes, but does the user know that? 

We could tell the user via 'grub-probe --help' that he may pass either a path or 
a block device as an argument.

> Which interface is easier to figure out
> for the user, one in which different actions require different parameters,
> or one where the same parameters act in completely different way depending
> on the file?

There are no different actions! Whether you give a valid path or a block device 
as an argument, the program behaves exactly the same. It does even behave 
exactly as before (i.e. unpatched) if given a path. Accepting devices as 
argument is simply an *added* feature.

Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Thu, Feb 14, 2008 at 09:33:48AM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >The last one sounds like a bug.
> 
> Yes, it's from the current *unpatched* grub-probe.

I mean that it should find the disk that contains /dev/hda1 device _node_.

> >Which interface is easier to figure out
> >for the user, one in which different actions require different parameters,
> >or one where the same parameters act in completely different way depending
> >on the file?
> 
> There are no different actions! Whether you give a valid path or a block 
> device as an argument, the program behaves exactly the same. It does even 
> behave exactly as before (i.e. unpatched) if given a path. Accepting 
> devices as argument is simply an *added* feature.

Consider the following:

$ cat > /boot/grub/device.map << EOF
(hd0)	/dev/device1
(hd1)	/dev/device2
EOF
$ mount /dev/device1 /mnt
$ touch /mnt/file
$ grub-probe -t drive /mnt/file
???
$ ln -s nowhere /mnt/symlink
$ grub-probe -t drive /mnt/symlink
???
$ mkfifo /mnt/fifo
$ grub-probe -t drive /mnt/fifo
???
$ mknod /mnt/device2
$ grub-probe -t drive /mnt/device2
???

Is the same output supposed to be printed in all "???" ?  If not, what is in
each one?

-- 
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] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> I mean that it should find the disk that contains /dev/hda1 device _node_.

Maybe, but I do not believe this has anything to do with my patch.

> Is the same output supposed to be printed in all "???" ?  If not, what is in
> each one?

In all of these cases the 'if (S_ISBLK (st.st_mode))' test introduced by my 
patch will fail and thus grub-probe's behaviour will be exactly as before.

Re: [PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Now that the FSF has received my copyright assignment paper back, I'd like to 
become productive on this issue again. From what we've discussed so far I assume 
that you prefer
- the introduction of a new command line option
- passing two parameters to probe(), a path and a device, of which one is NULL
- not exporting the IF_BLKDEV test into a new function as long as strdup() of 
the returned string cannot be avoided.

Cheers,
Fabian

[NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

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

Hi Robert,

I have created another patch which introduces some of your suggestions, e.g. 
probe() is called with two arguments and grub_util_check_block_device() does not 
  strdup() the returned string. However, grub-probe crashes with a double free 
error message if run with the -d option. I am somehow stuck in this situation, 
can you please help me?

I promise I'll provide a changelog as soon as there is a chance that my patch 
will get applied. ;)

Cheers,
Fabian
-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

[-- Attachment #2: grub-probe-20080222-1.patch --]
[-- Type: text/x-diff, Size: 4277 bytes --]

diff -Naru grub2-1.96+20080219~/include/grub/util/getroot.h grub2-1.96+20080219/include/grub/util/getroot.h
--- grub2-1.96+20080219~/include/grub/util/getroot.h	2008-01-12 16:11:56.000000000 +0100
+++ grub2-1.96+20080219/include/grub/util/getroot.h	2008-02-22 14:40:41.000000000 +0100
@@ -29,5 +29,6 @@
 char *grub_get_prefix (const char *dir);
 int grub_util_get_dev_abstraction (const char *os_dev);
 char *grub_util_get_grub_dev (const char *os_dev);
+char *grub_util_check_block_device (const char *blk_dev);
 
 #endif /* ! GRUB_UTIL_GETROOT_HEADER */
diff -Naru grub2-1.96+20080219~/util/getroot.c grub2-1.96+20080219/util/getroot.c
--- grub2-1.96+20080219~/util/getroot.c	2008-02-12 23:45:58.000000000 +0100
+++ grub2-1.96+20080219/util/getroot.c	2008-02-22 14:45:52.000000000 +0100
@@ -332,3 +332,17 @@
 
   return grub_dev;
 }
+
+char *
+grub_util_check_block_device (const char *blk_dev)
+{
+  struct stat st;
+
+  if (stat (blk_dev, &st) < 0)
+    grub_util_error ("Cannot stat `%s'", blk_dev);
+
+  if (S_ISBLK (st.st_mode))
+    return (blk_dev);
+  else
+    return 0;
+}
diff -Naru grub2-1.96+20080219~/util/grub-probe.c grub2-1.96+20080219/util/grub-probe.c
--- grub2-1.96+20080219~/util/grub-probe.c	2008-02-09 11:49:29.000000000 +0100
+++ grub2-1.96+20080219/util/grub-probe.c	2008-02-22 14:56:28.000000000 +0100
@@ -50,6 +50,7 @@
 };
 
 int print = PRINT_FS;
+static unsigned int argument_is_device = 0;
 
 void
 grub_putchar (int c)
@@ -100,16 +101,22 @@
 }
 
 static void
-probe (const char *path)
+probe (const char *path, const char *device_name)
 {
-  char *device_name;
   char *drive_name = NULL;
   char *grub_path = NULL;
   char *filebuf_via_grub = NULL, *filebuf_via_sys = NULL;
   int abstraction_type;
   grub_device_t dev = NULL;
   
-  device_name = grub_guess_root_device (path);
+  if (path == NULL)
+    {
+      if ( ! grub_util_check_block_device (device_name))
+        grub_util_error ("%s is not a block device.\n", path);
+    }
+  else
+    device_name = grub_guess_root_device (path);
+
   if (! device_name)
     grub_util_error ("cannot find a device for %s.\n", path);
 
@@ -226,6 +233,7 @@
 
 static struct option options[] =
   {
+    {"device", no_argument, 0, 'd'},
     {"device-map", required_argument, 0, 'm'},
     {"target", required_argument, 0, 't'},
     {"help", no_argument, 0, 'h'},
@@ -242,10 +250,11 @@
 	     "Try ``grub-probe --help'' for more information.\n");
   else
     printf ("\
-Usage: grub-probe [OPTION]... PATH\n\
+Usage: grub-probe [OPTION]... [PATH|DEVICE]\n\
 \n\
-Probe device information for a given path.\n\
+Probe device information for a given path (or device, if the -d option is given).\n\
 \n\
+  -d, --device              given argument is a system device, not a path\n\
   -m, --device-map=FILE     use FILE as the device map [default=%s]\n\
   -t, --target=(fs|drive|device|partmap|abstraction)\n\
                             print filesystem module, GRUB drive, system device, partition map module or abstraction module [default=fs]\n\
@@ -264,20 +273,24 @@
 main (int argc, char *argv[])
 {
   char *dev_map = 0;
-  char *path;
+  char *argument;
   
   progname = "grub-probe";
   
   /* Check for options.  */
   while (1)
     {
-      int c = getopt_long (argc, argv, "m:t:hVv", options, 0);
+      int c = getopt_long (argc, argv, "dm:t:hVv", options, 0);
       
       if (c == -1)
 	break;
       else
 	switch (c)
 	  {
+	  case 'd':
+	    argument_is_device = 1;
+	    break;
+
 	  case 'm':
 	    if (dev_map)
 	      free (dev_map);
@@ -321,10 +334,10 @@
   if (verbosity > 1)
     grub_env_set ("debug", "all");
 
-  /* Obtain PATH.  */
+  /* Obtain ARGUMENT.  */
   if (optind >= argc)
     {
-      fprintf (stderr, "No path is specified.\n");
+      fprintf (stderr, "No path or device is specified.\n");
       usage (1);
     }
 
@@ -334,7 +347,7 @@
       usage (1);
     }
 
-  path = argv[optind];
+  argument = argv[optind];
   
   /* Initialize the emulated biosdisk driver.  */
   grub_util_biosdisk_init (dev_map ? : DEFAULT_DEVICE_MAP);
@@ -343,7 +356,10 @@
   grub_init_all ();
 
   /* Do it.  */
-  probe (path);
+  if (argument_is_device)
+    probe (NULL,argument);
+  else
+    probe (argument,NULL);
   
   /* Free resources.  */
   grub_fini_all ();

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

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

Well, the solution seems to be easier than I expected. Is the attached patch 
acceptable?

Have a nice weekend,
Fabian

-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de



[-- Attachment #2: grub-probe-20080222-2.patch --]
[-- Type: text/x-diff, Size: 4404 bytes --]

diff -Naru grub2-1.96+20080219~/include/grub/util/getroot.h grub2-1.96+20080219/include/grub/util/getroot.h
--- grub2-1.96+20080219~/include/grub/util/getroot.h	2008-01-12 16:11:56.000000000 +0100
+++ grub2-1.96+20080219/include/grub/util/getroot.h	2008-02-22 14:40:41.000000000 +0100
@@ -29,5 +29,6 @@
 char *grub_get_prefix (const char *dir);
 int grub_util_get_dev_abstraction (const char *os_dev);
 char *grub_util_get_grub_dev (const char *os_dev);
+char *grub_util_check_block_device (const char *blk_dev);
 
 #endif /* ! GRUB_UTIL_GETROOT_HEADER */
diff -Naru grub2-1.96+20080219~/util/getroot.c grub2-1.96+20080219/util/getroot.c
--- grub2-1.96+20080219~/util/getroot.c	2008-02-12 23:45:58.000000000 +0100
+++ grub2-1.96+20080219/util/getroot.c	2008-02-22 14:45:52.000000000 +0100
@@ -332,3 +332,17 @@
 
   return grub_dev;
 }
+
+char *
+grub_util_check_block_device (const char *blk_dev)
+{
+  struct stat st;
+
+  if (stat (blk_dev, &st) < 0)
+    grub_util_error ("Cannot stat `%s'", blk_dev);
+
+  if (S_ISBLK (st.st_mode))
+    return (blk_dev);
+  else
+    return 0;
+}
diff -Naru grub2-1.96+20080219~/util/grub-probe.c grub2-1.96+20080219/util/grub-probe.c
--- grub2-1.96+20080219~/util/grub-probe.c	2008-02-09 11:49:29.000000000 +0100
+++ grub2-1.96+20080219/util/grub-probe.c	2008-02-22 16:00:01.000000000 +0100
@@ -50,6 +50,7 @@
 };
 
 int print = PRINT_FS;
+static unsigned int argument_is_device = 0;
 
 void
 grub_putchar (int c)
@@ -100,16 +101,22 @@
 }
 
 static void
-probe (const char *path)
+probe (const char *path, char *device_name)
 {
-  char *device_name;
   char *drive_name = NULL;
   char *grub_path = NULL;
   char *filebuf_via_grub = NULL, *filebuf_via_sys = NULL;
   int abstraction_type;
   grub_device_t dev = NULL;
   
-  device_name = grub_guess_root_device (path);
+  if (path == NULL)
+    {
+      if ( ! grub_util_check_block_device (device_name))
+        grub_util_error ("%s is not a block device.\n", device_name);
+    }
+  else
+    device_name = grub_guess_root_device (path);
+
   if (! device_name)
     grub_util_error ("cannot find a device for %s.\n", path);
 
@@ -220,12 +227,12 @@
   free (grub_path);
   free (filebuf_via_grub);
   free (filebuf_via_sys);
-  free (device_name);
   free (drive_name);
 }
 
 static struct option options[] =
   {
+    {"device", no_argument, 0, 'd'},
     {"device-map", required_argument, 0, 'm'},
     {"target", required_argument, 0, 't'},
     {"help", no_argument, 0, 'h'},
@@ -242,10 +249,11 @@
 	     "Try ``grub-probe --help'' for more information.\n");
   else
     printf ("\
-Usage: grub-probe [OPTION]... PATH\n\
+Usage: grub-probe [OPTION]... [PATH|DEVICE]\n\
 \n\
-Probe device information for a given path.\n\
+Probe device information for a given path (or device, if the -d option is given).\n\
 \n\
+  -d, --device              given argument is a system device, not a path\n\
   -m, --device-map=FILE     use FILE as the device map [default=%s]\n\
   -t, --target=(fs|drive|device|partmap|abstraction)\n\
                             print filesystem module, GRUB drive, system device, partition map module or abstraction module [default=fs]\n\
@@ -264,20 +272,24 @@
 main (int argc, char *argv[])
 {
   char *dev_map = 0;
-  char *path;
+  char *argument;
   
   progname = "grub-probe";
   
   /* Check for options.  */
   while (1)
     {
-      int c = getopt_long (argc, argv, "m:t:hVv", options, 0);
+      int c = getopt_long (argc, argv, "dm:t:hVv", options, 0);
       
       if (c == -1)
 	break;
       else
 	switch (c)
 	  {
+	  case 'd':
+	    argument_is_device = 1;
+	    break;
+
 	  case 'm':
 	    if (dev_map)
 	      free (dev_map);
@@ -321,10 +333,10 @@
   if (verbosity > 1)
     grub_env_set ("debug", "all");
 
-  /* Obtain PATH.  */
+  /* Obtain ARGUMENT.  */
   if (optind >= argc)
     {
-      fprintf (stderr, "No path is specified.\n");
+      fprintf (stderr, "No path or device is specified.\n");
       usage (1);
     }
 
@@ -334,7 +346,7 @@
       usage (1);
     }
 
-  path = argv[optind];
+  argument = argv[optind];
   
   /* Initialize the emulated biosdisk driver.  */
   grub_util_biosdisk_init (dev_map ? : DEFAULT_DEVICE_MAP);
@@ -343,7 +355,10 @@
   grub_init_all ();
 
   /* Do it.  */
-  probe (path);
+  if (argument_is_device)
+    probe (NULL,argument);
+  else
+    probe (argument,NULL);
   
   /* Free resources.  */
   grub_fini_all ();

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

Sorry for the long delay, I've been very busy (and unfortunately I'll continue
to be :-().

On Fri, Feb 22, 2008 at 04:36:35PM +0100, Fabian Greffrath wrote:
> Well, the solution seems to be easier than I expected. Is the attached 
> patch acceptable?

It looks good to me.  Just a pair of minor details (simply for consistency with
code style in grub):

> +  if (path == NULL)
> +    {
> +      if ( ! grub_util_check_block_device (device_name))

could you remove the space in "( !" ?

> +  if (argument_is_device)
> +    probe (NULL,argument);
> +  else
> +    probe (argument,NULL);

please add a space after the comma.

If you fix those, include a ChangeLog entry and noone else objects, I'll commit
it.  Feel free to ping me with a direct mail if I stale for too long.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Fri, Feb 22, 2008 at 03:32:02PM +0100, Fabian Greffrath wrote:
>  However, grub-probe crashes with a double free error message if run with 
> the -d option. I am somehow stuck in this situation, can you please help me?

Is this solved now?  If not, you could send a gdb backtrace to give us an idea
on what's wrong.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

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

Hello!

Robert Millan schrieb:
> It looks good to me.  Just a pair of minor details (simply for consistency with
> code style in grub):
> [...]

done.

> If you fix those, include a ChangeLog entry and noone else objects, I'll commit
> it.  Feel free to ping me with a direct mail if I stale for too long.

You are CCed. ;)

Robert Millan schrieb:
 > Is this solved now?  If not, you could send a gdb backtrace to give us an idea
 > on what's wrong.

Yes, it's fixed in the second patch that you reviewed (and in the one that's 
attached, too).

Cheers,
Fabian

-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: grub-probe-20080222-3.diff --]
[-- Type: text/x-patch; name="grub-probe-20080222-3.diff", Size: 5227 bytes --]

2008-02-25  Fabian Greffrath  <greffrath@leat.rub.de>

	* include/grub/util/getroot.h (grub_util_check_block_device): Export new
	function.
	* util/getroot.c (grub_util_check_block_device): New function that
	returns the given argument if it is a block device and returns NULL else.
	* util/grub-probe.c (argument_is_device): New variable.
	(probe): Promote device_name from a variable to an argument. Receive
	device_name from grub_util_check_block_device() if path is NULL and from
	grub_guess_root_device() else. Do not free() device_name anymore.
	(options): Introduce new parameter '-d, --device'. 
	(main): Add description of the new parameter to the help screen.
	Rename path variable to argument. Set argument_is_device if the '-d'
	option is given. Pass argument to probe() depending on
	argument_is_device.


diff -Naru grub2-1.96+20080219~/include/grub/util/getroot.h grub2-1.96+20080219/include/grub/util/getroot.h
--- grub2-1.96+20080219~/include/grub/util/getroot.h	2008-01-12 16:11:56.000000000 +0100
+++ grub2-1.96+20080219/include/grub/util/getroot.h	2008-02-22 14:40:41.000000000 +0100
@@ -29,5 +29,6 @@
 char *grub_get_prefix (const char *dir);
 int grub_util_get_dev_abstraction (const char *os_dev);
 char *grub_util_get_grub_dev (const char *os_dev);
+char *grub_util_check_block_device (const char *blk_dev);
 
 #endif /* ! GRUB_UTIL_GETROOT_HEADER */
diff -Naru grub2-1.96+20080219~/util/getroot.c grub2-1.96+20080219/util/getroot.c
--- grub2-1.96+20080219~/util/getroot.c	2008-02-12 23:45:58.000000000 +0100
+++ grub2-1.96+20080219/util/getroot.c	2008-02-22 14:45:52.000000000 +0100
@@ -332,3 +332,17 @@
 
   return grub_dev;
 }
+
+char *
+grub_util_check_block_device (const char *blk_dev)
+{
+  struct stat st;
+
+  if (stat (blk_dev, &st) < 0)
+    grub_util_error ("Cannot stat `%s'", blk_dev);
+
+  if (S_ISBLK (st.st_mode))
+    return (blk_dev);
+  else
+    return 0;
+}
diff -Naru grub2-1.96+20080219~/util/grub-probe.c grub2-1.96+20080219/util/grub-probe.c
--- grub2-1.96+20080219~/util/grub-probe.c	2008-02-09 11:49:29.000000000 +0100
+++ grub2-1.96+20080219/util/grub-probe.c	2008-02-22 16:00:01.000000000 +0100
@@ -50,6 +50,7 @@
 };
 
 int print = PRINT_FS;
+static unsigned int argument_is_device = 0;
 
 void
 grub_putchar (int c)
@@ -100,16 +101,22 @@
 }
 
 static void
-probe (const char *path)
+probe (const char *path, char *device_name)
 {
-  char *device_name;
   char *drive_name = NULL;
   char *grub_path = NULL;
   char *filebuf_via_grub = NULL, *filebuf_via_sys = NULL;
   int abstraction_type;
   grub_device_t dev = NULL;
   
-  device_name = grub_guess_root_device (path);
+  if (path == NULL)
+    {
+      if (! grub_util_check_block_device (device_name))
+        grub_util_error ("%s is not a block device.\n", device_name);
+    }
+  else
+    device_name = grub_guess_root_device (path);
+
   if (! device_name)
     grub_util_error ("cannot find a device for %s.\n", path);
 
@@ -220,12 +227,12 @@
   free (grub_path);
   free (filebuf_via_grub);
   free (filebuf_via_sys);
-  free (device_name);
   free (drive_name);
 }
 
 static struct option options[] =
   {
+    {"device", no_argument, 0, 'd'},
     {"device-map", required_argument, 0, 'm'},
     {"target", required_argument, 0, 't'},
     {"help", no_argument, 0, 'h'},
@@ -242,10 +249,11 @@
 	     "Try ``grub-probe --help'' for more information.\n");
   else
     printf ("\
-Usage: grub-probe [OPTION]... PATH\n\
+Usage: grub-probe [OPTION]... [PATH|DEVICE]\n\
 \n\
-Probe device information for a given path.\n\
+Probe device information for a given path (or device, if the -d option is given).\n\
 \n\
+  -d, --device              given argument is a system device, not a path\n\
   -m, --device-map=FILE     use FILE as the device map [default=%s]\n\
   -t, --target=(fs|drive|device|partmap|abstraction)\n\
                             print filesystem module, GRUB drive, system device, partition map module or abstraction module [default=fs]\n\
@@ -264,20 +272,24 @@
 main (int argc, char *argv[])
 {
   char *dev_map = 0;
-  char *path;
+  char *argument;
   
   progname = "grub-probe";
   
   /* Check for options.  */
   while (1)
     {
-      int c = getopt_long (argc, argv, "m:t:hVv", options, 0);
+      int c = getopt_long (argc, argv, "dm:t:hVv", options, 0);
       
       if (c == -1)
 	break;
       else
 	switch (c)
 	  {
+	  case 'd':
+	    argument_is_device = 1;
+	    break;
+
 	  case 'm':
 	    if (dev_map)
 	      free (dev_map);
@@ -321,10 +333,10 @@
   if (verbosity > 1)
     grub_env_set ("debug", "all");
 
-  /* Obtain PATH.  */
+  /* Obtain ARGUMENT.  */
   if (optind >= argc)
     {
-      fprintf (stderr, "No path is specified.\n");
+      fprintf (stderr, "No path or device is specified.\n");
       usage (1);
     }
 
@@ -334,7 +346,7 @@
       usage (1);
     }
 
-  path = argv[optind];
+  argument = argv[optind];
   
   /* Initialize the emulated biosdisk driver.  */
   grub_util_biosdisk_init (dev_map ? : DEFAULT_DEVICE_MAP);
@@ -343,7 +355,10 @@
   grub_init_all ();
 
   /* Do it.  */
-  probe (path);
+  if (argument_is_device)
+    probe (NULL, argument);
+  else
+    probe (argument, NULL);
   
   /* Free resources.  */
   grub_fini_all ();

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Mon, Feb 25, 2008 at 10:49:01AM +0100, Fabian Greffrath wrote:
> >If you fix those, include a ChangeLog entry and noone else objects, I'll 
> >commit
> >it.  Feel free to ping me with a direct mail if I stale for too long.
> 
> You are CCed. ;)
> 
> Robert Millan schrieb:
> > Is this solved now?  If not, you could send a gdb backtrace to give us an 
> idea
> > on what's wrong.
> 
> Yes, it's fixed in the second patch that you reviewed (and in the one 
> that's attached, too).

Ok.  Does anybody object to Fabian's patch?  I'd like to commit it.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> Ok.  Does anybody object to Fabian's patch?  I'd like to commit it.

Silence is consense. ;)


-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Thu, Feb 28, 2008 at 08:26:49AM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >Ok.  Does anybody object to Fabian's patch?  I'd like to commit it.
> 
> Silence is consense. ;)

Committed.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> Committed.

Great, thanks!

Now let's get support for os-prober output parsing.


-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Thu, Feb 28, 2008 at 11:28:13AM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >Committed.
> 
> Great, thanks!
> 
> Now let's get support for os-prober output parsing.

Ok.  Is there any mail for that waiting for input?  I lose track of things.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

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

Robert Millan schrieb:
> Ok.  Is there any mail for that waiting for input?  I lose track of things.

No problem, please find the script attached.

[-- Attachment #2: 30_os-prober --]
[-- Type: text/plain, Size: 2793 bytes --]

#! /bin/sh -e

# update-grub helper script.
# Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
#
# GRUB is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# GRUB is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with GRUB.  If not, see <http://www.gnu.org/licenses/>.

if [ -x "`which os-prober 2>/dev/null`" ] ; then
  OSPROBED="`os-prober | tr ' ' '^' | paste -s -d ' '`"
fi

if [ -n "${OSPROBED}" ] ; then
  for OS in ${OSPROBED} ; do
    DEVICE="`echo ${OS} | cut -d ':' -f 1`"
    LONGNAME="`echo ${OS} | cut -d ':' -f 2 | tr '^' ' '`"
    LABEL="`echo ${OS} | cut -d ':' -f 3 | tr '^' ' '`"
    BOOT="`echo ${OS} | cut -d ':' -f 4`"

    if [ -z "${LONGNAME}" ] ; then
      LONGNAME="${LABEL}"
    fi

    echo "Found ${LONGNAME} on ${DEVICE}" >&2
    
    TESTDEVICE="`grub-probe --target=device $0`"
    if [ -n "`grub-probe --target=drive --device ${TESTDEVICE} 2>/dev/null`" ] ; then
      case "${BOOT}" in
        chain)
          CHAINROOT="`grub-probe --target=drive --device ${DEVICE}`"

          cat << EOF
menuentry "${LONGNAME} (on ${DEVICE})" {
	set root=${CHAINROOT}
	chainloader +1
}
EOF
        ;;
        linux)
          if [ -x "`which linux-boot-prober 2>/dev/null`" ] ; then
            LINUXPROBED="`linux-boot-prober ${DEVICE} | tr ' ' '^' | paste -s -d ' '`"
          fi

          if [ -n "${LINUXPROBED}" ] ; then
            for LINUX in ${LINUXPROBED} ; do
              LROOT="`echo ${LINUX} | cut -d ':' -f 1`"
              LBOOT="`echo ${LINUX} | cut -d ':' -f 2`"
              LLABEL="`echo ${LINUX} | cut -d ':' -f 3 | tr '^' ' '`"
              LKERNEL="`echo ${LINUX} | cut -d ':' -f 4`"
              LINITRD="`echo ${LINUX} | cut -d ':' -f 5`"
              LPARAMS="`echo ${LINUX} | cut -d ':' -f 6- | tr '^' ' '`"

              LINUXROOT="`grub-probe --target=drive --device ${LBOOT}`"

              if [ -z "${LLABEL}" ] ; then
                LLABEL="${LONGNAME}"
              fi

              cat << EOF
menuentry "${LLABEL} (on ${DEVICE})" {
	set root=${LINUXROOT}
	linux ${LKERNEL} ${LPARAMS}
EOF
              if [ -n "${LINITRD}" ] ; then
                cat << EOF
	initrd ${LINITRD}
EOF
              fi
              cat << EOF
}
EOF
            done
          fi
        ;;
        hurd)
          # not yet...
        ;;
        *)
        ;;
      esac
    fi
  done
fi

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Thu, Feb 28, 2008 at 12:46:37PM +0100, Fabian Greffrath wrote:
> 
> if [ -x "`which os-prober 2>/dev/null`" ] ; then

Better do this like it's done in other scripts (check grub-install).  Not sure
why, I guess it's more portable that way...

>           if [ -x "`which linux-boot-prober 2>/dev/null`" ] ; then

same here.

>         hurd)
>           # not yet...
>         ;;

For this one, I think it'd be much better to rework 10_hurd so that code is
shared, rather than duplicating the whole blurb.

Do you plan to do that later?

Btw, what about *BSD?  Does os-prober detect those as "chain" or ... ?

>         *)
>         ;;

Maybe it's better to echo something here, otherwise it gives the impression
OS FooBar was just detected and properly added.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> Better do this like it's done in other scripts (check grub-install).  Not sure
> why, I guess it's more portable that way...

You mean
   if [ "x$(FOO)" = "x" ] ...
?

> For this one, I think it'd be much better to rework 10_hurd so that code is
> shared, rather than duplicating the whole blurb.

Yes, we could source out the complete linux/hurd kernel-adding blurbs 
into an external shell library that is shared with 10_* and 30_os-prober.

However, please don't forget these are two different things. 10_* look 
on / and /boot, while os-prober looks on all *other* partitions.

> Do you plan to do that later?

Not a concrete plan yet, However, we should first get thinks *working* 
before we care about cosmetics, IMHO. ;)

> Btw, what about *BSD?  Does os-prober detect those as "chain" or ... ?

I asked myself the same question before. I seems like os-prober does 
not support any of the BSD flavours, which sounds strange since it 
even discovers Solaris on i386.

Should be a bug against os-prober.

> Maybe it's better to echo something here, otherwise it gives the impression
> OS FooBar was just detected and properly added.

Definitely. I'll add a short warning message.

Furthermore, I am going to modify the script to immediately 'exit 0' 
if it turns out it misses the tools required to update the boot menu 
(i.e. os-prober, linux-boot-prober, grub-probe (>= 20080228)). These 
tests are spread all over the script at the moment.

Best Regards,
Fabian

PS: Shouldn't 30_os-prober be an upstream feature of os-prober?!

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> Better do this like it's done in other scripts (check grub-install).  Not sure
> why, I guess it's more portable that way...

Ah, now I know what you ment. However, I cannot really see the 
advantage in

  grub_probe=${sbindir}/`echo grub-probe | sed ${transform}`

with ${transform}="s,x,x,".

I rather reads like WTF to me... ;)


BTW, in another mail from 4 Feb you asked my if the
"echo ... | cut ..." mess in my script could be simplified with the
"echo ... | read ..." feature. I am sorry, but the answer is "no".

This is because read will run in a separate process and cannot set the 
variables in the parent shell, see 
<http://www.unixguide.net/unix/bash/E4.shtml>.

Cheers,
Fabian
-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Fri, Feb 29, 2008 at 02:46:17PM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >Better do this like it's done in other scripts (check grub-install).  Not 
> >sure
> >why, I guess it's more portable that way...
> 
> Ah, now I know what you ment. However, I cannot really see the 
> advantage in
> 
>  grub_probe=${sbindir}/`echo grub-probe | sed ${transform}`
> 
> with ${transform}="s,x,x,".
> 
> I rather reads like WTF to me... ;)

Maybe Okuji knows... ;-)

> BTW, in another mail from 4 Feb you asked my if the
> "echo ... | cut ..." mess in my script could be simplified with the
> "echo ... | read ..." feature. I am sorry, but the answer is "no".
> 
> This is because read will run in a separate process and cannot set the 
> variables in the parent shell, see 
> <http://www.unixguide.net/unix/bash/E4.shtml>.

Ok no big deal.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Robert Millan]

On Fri, Feb 29, 2008 at 09:33:13AM +0100, Fabian Greffrath wrote:
> >For this one, I think it'd be much better to rework 10_hurd so that code is
> >shared, rather than duplicating the whole blurb.
> 
> Yes, we could source out the complete linux/hurd kernel-adding blurbs 
> into an external shell library that is shared with 10_* and 30_os-prober.
> 
> However, please don't forget these are two different things. 10_* look 
> on / and /boot, while os-prober looks on all *other* partitions.

For 10_linux it isn't that important since the boot parameters are so small,
it's 10_hurd that has more reusable stuff (that big blurb).

> >Do you plan to do that later?
> 
> Not a concrete plan yet, However, we should first get thinks *working* 
> before we care about cosmetics, IMHO. ;)

In that case maybe it's better to leave Hurd support for later, than
duplicating the whole thing.

> >Maybe it's better to echo something here, otherwise it gives the impression
> >OS FooBar was just detected and properly added.
> 
> Definitely. I'll add a short warning message.
> 
> Furthermore, I am going to modify the script to immediately 'exit 0' 
> if it turns out it misses the tools required to update the boot menu 
> (i.e. os-prober, linux-boot-prober, grub-probe (>= 20080228)). These 
> tests are spread all over the script at the moment.

Ok.

> PS: Shouldn't 30_os-prober be an upstream feature of os-prober?!

Not sure about that.  My initial intention was that in general one could plug
in scripts from other system components, but often the code in them depends on
many update-grub interfaces which makes it likely to break if something is
changed.

For now only very simple add-ons exist (memtest86, invaders...).  But this
one will probably end up being more intermangled with other GRUB parts (see
that we're already discussing how to reduce code duplication with 10_hurd).

I'd suggest putting it in GRUB, at least for now, if that is fine with you.

-- 
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: [NEW PATCH] Add option to grub-probe to accept system devices as arguments

[Fabian Greffrath]

Robert Millan schrieb:
> For 10_linux it isn't that important since the boot parameters are so small,
> it's 10_hurd that has more reusable stuff (that big blurb).

This is true. Furthermore I still haven't found out how to get 
variables like "host-port", "device-port", "task-create", etc..

> In that case maybe it's better to leave Hurd support for later, than
> duplicating the whole thing.

Allright, so at the moment the script handles chainloader and linux.

> For now only very simple add-ons exist (memtest86, invaders...).  

Hehe, I know invaders quite well. ;)

> I'd suggest putting it in GRUB, at least for now, if that is fine with you.

Yes, sure!



-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

[NEW PATCH] os-prober output parser

[Fabian Greffrath]

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

I have improved the script a little bit; in particular I moved the 
tests for the required software to the beginning of the script and let 
it gracefully 'exit 0' if anything is missing.

Let me comment on these tests:
- The first test (lines 19-22) is to check if grub-probe is recent 
enough to support the '--device' option. I have changed this to grep 
for it in the '--help' output instead of test-probing a given device, 
since you never know if a device.map allready exists (imaging booting 
from knoppix).
- I have changed the second test (24-27) to not utilize the 'which' 
utility anymore. It's in the debianutils package and I don't know if 
there's a chance that this is installed on non-Debian systems. On the 
other hand, we are utilizing os-prober which is Debian-only at the 
moment...
- The third test (30-33) is redundant, strictly speaking. The script 
will never enter the for/do loop if the os-prober output is empty. 
However, I found it reasonably to 'exit 0' before even attempting to 
enter the loop, YMMV. If it doesn't suit you, it can be removed safely.

Furthermore I have removed the 'hurd' stub. For the time beeing, HURD 
kernels remain unsupported and throw a warning message just like 
macos* and all other !linux and !chain operating systems.

Best Regards,
Fabian

-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

[-- Attachment #2: 30_os-prober --]
[-- Type: text/plain, Size: 2725 bytes --]

#! /bin/sh -e

# update-grub helper script.
# Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
#
# GRUB is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# GRUB is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with GRUB.  If not, see <http://www.gnu.org/licenses/>.

if [ -z "`grub-probe --help 2> /dev/null | grep '\-d,\ \-\-device'`" ] ; then
  # grub-probe does not support '--device' option yet (introduced in 1.96+20080228)
  exit 0
fi

if [ -x /usr/bin/os-prober -a -x /usr/bin/linux-boot-prober ] ; then : ; else
  # os-prober/linux-boot-prober not installed
  exit 0
fi

OSPROBED="`os-prober 2> /dev/null | tr ' ' '^' | paste -s -d ' '`"
if [ -z "${OSPROBED}" ] ; then
  # empty os-prober output, nothing doing
  exit 0
fi

for OS in ${OSPROBED} ; do
  DEVICE="`echo ${OS} | cut -d ':' -f 1`"
  LONGNAME="`echo ${OS} | cut -d ':' -f 2 | tr '^' ' '`"
  LABEL="`echo ${OS} | cut -d ':' -f 3 | tr '^' ' '`"
  BOOT="`echo ${OS} | cut -d ':' -f 4`"

  if [ -z "${LONGNAME}" ] ; then
    LONGNAME="${LABEL}"
  fi

  echo "Found ${LONGNAME} on ${DEVICE}" >&2

  case ${BOOT} in
    chain)
      CHAINROOT="`grub-probe --target=drive --device ${DEVICE} 2> /dev/null`"

      cat << EOF
menuentry "${LONGNAME} (on ${DEVICE})" {
	set root=${CHAINROOT}
	chainloader +1
}
EOF
    ;;
    linux)
      LINUXPROBED="`linux-boot-prober ${DEVICE} 2> /dev/null | tr ' ' '^' | paste -s -d ' '`"
 
      for LINUX in ${LINUXPROBED} ; do
        LROOT="`echo ${LINUX} | cut -d ':' -f 1`"
        LBOOT="`echo ${LINUX} | cut -d ':' -f 2`"
        LLABEL="`echo ${LINUX} | cut -d ':' -f 3 | tr '^' ' '`"
        LKERNEL="`echo ${LINUX} | cut -d ':' -f 4`"
        LINITRD="`echo ${LINUX} | cut -d ':' -f 5`"
        LPARAMS="`echo ${LINUX} | cut -d ':' -f 6- | tr '^' ' '`"

        LINUXROOT="`grub-probe --target=drive --device ${LBOOT} 2> /dev/null`"

        if [ -z "${LLABEL}" ] ; then
          LLABEL="${LONGNAME}"
        fi

        cat << EOF
menuentry "${LLABEL} (on ${DEVICE})" {
	set root=${LINUXROOT}
	linux ${LKERNEL} ${LPARAMS}
EOF
        if [ -n "${LINITRD}" ] ; then
          cat << EOF
	initrd ${LINITRD}
EOF
        fi
        cat << EOF
}
EOF
      done
    ;;
    hurd|*)
      echo "  ${LONGNAME} is not yet supported by update-grub." >&2
    ;;
  esac
done

Re: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

Fabian Greffrath schrieb:
> I have improved the script a little bit; 

Robert, have you allready had a look at it? :)

-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] os-prober output parser

[Robert Millan]

Sorry for the delay in repliing.  Been very busy :-(

I tried the script and have the following comments:

When run in the console it makes Linux spit a truckload of information about
filesystem probing.  Is there any way we could avoid this?

How do you prevent it from detecting your own partition?  I didn't see any
provisions for this.

On Wed, Mar 05, 2008 at 01:27:56PM +0100, Fabian Greffrath wrote:
> ???#! /bin/sh -e

Whops.  This tag (utf-8 stuff?) breaks the Linux shebang parser :-)

> if [ -x /usr/bin/os-prober -a -x /usr/bin/linux-boot-prober ] ; then : ; else

Can we avoid hardcoding their absolute path?

-- 
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: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

Robert Millan schrieb:
> Sorry for the delay in repliing.  Been very busy :-(

No problem. ;)

> When run in the console it makes Linux spit a truckload of information about
> filesystem probing.  Is there any way we could avoid this?

I don't know why this happens since I allready divert stderr to 
/dev/null in the os-prober call. Maybe this is a bug in os-prober itself.

> How do you prevent it from detecting your own partition?  I didn't see any
> provisions for this.

I don't have to. That's the way os-prober works.

> Whops.  This tag (utf-8 stuff?) breaks the Linux shebang parser :-)

Yes, sorry. I am forced to store the file on a Windows partition in 
order to send it as an email attachment. Don't ask me about UTF8 and 
Windows. ;)

> Can we avoid hardcoding their absolute path?

Yes, via the 'which' command - as has been implemented in previous 
versions of this script. But back then you had objections against it...



-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] os-prober output parser

[Robert Millan]

On Wed, Mar 19, 2008 at 04:18:42PM +0100, Fabian Greffrath wrote:
> >When run in the console it makes Linux spit a truckload of information 
> >about
> >filesystem probing.  Is there any way we could avoid this?
> 
> I don't know why this happens since I allready divert stderr to 
> /dev/null in the os-prober call. Maybe this is a bug in os-prober itself.

They're printed directly by Linux.  No idea how to stop that, but I don't think
this problem is a showstopper anyway (they're always printed to /dev/console).

> >How do you prevent it from detecting your own partition?  I didn't see any
> >provisions for this.
> 
> I don't have to. That's the way os-prober works.

Ok.

> >Whops.  This tag (utf-8 stuff?) breaks the Linux shebang parser :-)
> 
> Yes, sorry. I am forced to store the file on a Windows partition in 
> order to send it as an email attachment. Don't ask me about UTF8 and 
> Windows. ;)

Argh.. :-)

> >Can we avoid hardcoding their absolute path?
> 
> Yes, via the 'which' command - as has been implemented in previous 
> versions of this script. But back then you had objections against it...

I prefer which myself, but IIRC Okuji said this isn't as much portable.  The
alternative is to do it like grub-install:

  set $grub_setup dummy
  if test -f "$1"; then
      :
  else
      echo "$1: Not found." 1>&2
      exit 1
  fi

which is very ugly if you ask me.  Okuji, do you have any comments on this?

-- 
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: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

Robert Millan schrieb:
> which is very ugly if you ask me.  Okuji, do you have any comments on this?

Apart from this issue, is the script allright?


-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] os-prober output parser

[Robert Millan]

On Thu, Mar 20, 2008 at 11:02:35AM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >which is very ugly if you ask me.  Okuji, do you have any comments on this?
> 
> Apart from this issue, is the script allright?

Also, you can remove this check now:

if [ -z "`grub-probe --help 2> /dev/null | grep '\-d,\ \-\-device'`" ] ; then
  # grub-probe does not support '--device' option yet (introduced in 1.96+20080228)
  exit 0
fi

and please include a ChangeLog entry.  As far as I'm concerned, there's nothing
else missing.

-- 
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: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

Robert Millan schrieb:
> and please include a ChangeLog entry.  As far as I'm concerned, there's nothing
> else missing.

Fine! However, should the script utilize the 'which' command or not?

-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

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

Fabian Greffrath schrieb:
> Fine! However, should the script utilize the 'which' command or not?

Allright, since I haven't heard any further complaints regarding the 
usage of the 'which' command, here's my proposed script. Please remove 
the UTF-8 crap from the shebang line if it is still there.


2008-04-07  Fabian Greffrath  <fabian.greffrath@web.de>

	* util\grub.d\30_os-prober: New script. Use `os-prober' and
	`linux-boot-prober' (if installed) to detect other operating
	systems which are installed on the computer and add them to
	the boot menu.


-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

[-- Attachment #2: 30_os-prober --]
[-- Type: text/plain, Size: 2570 bytes --]

#! /bin/sh -e

# update-grub helper script.
# Copyright (C) 2006,2007,2008  Free Software Foundation, Inc.
#
# GRUB is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# GRUB is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with GRUB.  If not, see <http://www.gnu.org/licenses/>.

if [ -z "`which os-prober 2> /dev/null`" -o -z "`which linux-boot-prober 2> /dev/null`" ] ; then
  # missing os-prober and/or linux-boot-prober
  exit 0
fi

OSPROBED="`os-prober 2> /dev/null | tr ' ' '^' | paste -s -d ' '`"
if [ -z "${OSPROBED}" ] ; then
  # empty os-prober output, nothing doing
  exit 0
fi

for OS in ${OSPROBED} ; do
  DEVICE="`echo ${OS} | cut -d ':' -f 1`"
  LONGNAME="`echo ${OS} | cut -d ':' -f 2 | tr '^' ' '`"
  LABEL="`echo ${OS} | cut -d ':' -f 3 | tr '^' ' '`"
  BOOT="`echo ${OS} | cut -d ':' -f 4`"

  if [ -z "${LONGNAME}" ] ; then
    LONGNAME="${LABEL}"
  fi

  echo "Found ${LONGNAME} on ${DEVICE}" >&2

  case ${BOOT} in
    chain)
      CHAINROOT="`grub-probe --target=drive --device ${DEVICE} 2> /dev/null`"

      cat << EOF
menuentry "${LONGNAME} (on ${DEVICE})" {
	set root=${CHAINROOT}
	chainloader +1
}
EOF
    ;;
    linux)
      LINUXPROBED="`linux-boot-prober ${DEVICE} 2> /dev/null | tr ' ' '^' | paste -s -d ' '`"
 
      for LINUX in ${LINUXPROBED} ; do
        LROOT="`echo ${LINUX} | cut -d ':' -f 1`"
        LBOOT="`echo ${LINUX} | cut -d ':' -f 2`"
        LLABEL="`echo ${LINUX} | cut -d ':' -f 3 | tr '^' ' '`"
        LKERNEL="`echo ${LINUX} | cut -d ':' -f 4`"
        LINITRD="`echo ${LINUX} | cut -d ':' -f 5`"
        LPARAMS="`echo ${LINUX} | cut -d ':' -f 6- | tr '^' ' '`"

        LINUXROOT="`grub-probe --target=drive --device ${LBOOT} 2> /dev/null`"

        if [ -z "${LLABEL}" ] ; then
          LLABEL="${LONGNAME}"
        fi

        cat << EOF
menuentry "${LLABEL} (on ${DEVICE})" {
	set root=${LINUXROOT}
	linux ${LKERNEL} ${LPARAMS}
EOF
        if [ -n "${LINITRD}" ] ; then
          cat << EOF
	initrd ${LINITRD}
EOF
        fi
        cat << EOF
}
EOF
      done
    ;;
    hurd|*)
      echo "  ${LONGNAME} is not yet supported by update-grub." >&2
    ;;
  esac
done

Re: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

Hi Robert,

I am curious why this didn't make it into the last Debian upload?

Cheers,
Fabian

Fabian Greffrath schrieb:
> Fabian Greffrath schrieb:
>> Fine! However, should the script utilize the 'which' command or not?
> 
> Allright, since I haven't heard any further complaints regarding the 
> usage of the 'which' command, here's my proposed script. Please remove 
> the UTF-8 crap from the shebang line if it is still there.
> 
> 
> 2008-04-07  Fabian Greffrath  <fabian.greffrath@web.de>
> 
>     * util\grub.d\30_os-prober: New script. Use `os-prober' and
>     `linux-boot-prober' (if installed) to detect other operating
>     systems which are installed on the computer and add them to
>     the boot menu.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de

Re: [NEW PATCH] os-prober output parser

[Robert Millan]

On Wed, Apr 09, 2008 at 10:32:47AM +0200, Fabian Greffrath wrote:
> Hi Robert,
> 
> I am curious why this didn't make it into the last Debian upload?

My policy for Debian uploads is not to diverge from upstream source unless I
can't avoid it.

I'll look at your last patch when I have time; which I currently don't.

Or maybe someone else can take care of it.

-- 
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: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

Robert Millan schrieb:
> I'll look at your last patch when I have time; which I currently don't.

Allright, thanks for the reply!

Re: [NEW PATCH] os-prober output parser

[Robert Millan]

On Mon, Apr 07, 2008 at 10:39:46AM +0200, Fabian Greffrath wrote:
> 
> Allright, since I haven't heard any further complaints regarding the 
> usage of the 'which' command, here's my proposed script. Please remove 
> the UTF-8 crap from the shebang line if it is still there.
> 
> 
> 2008-04-07  Fabian Greffrath  <fabian.greffrath@web.de>
> 
> 	* util\grub.d\30_os-prober: New script. Use `os-prober' and
> 	`linux-boot-prober' (if installed) to detect other operating
> 	systems which are installed on the computer and add them to
> 	the boot menu.

You forgot adding it to the build system.  Fixed that for you (hey, it's the
least I could do after making you wait aeons for this).

Thanks

-- 
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: [NEW PATCH] os-prober output parser

[Fabian Greffrath]

I allready appended this to the Debian bug report, but for the record:

Please make the grub-pc package Recommend: os-prober.

Cheers,
Fabian


-- 
Dipl.-Phys. Fabian Greffrath

Ruhr-Universität Bochum
Lehrstuhl für Energieanlagen und Energieprozesstechnik (LEAT)
Universitätsstr. 150, IB 3/134
D-44780 Bochum

Telefon: +49 (0)234 / 32-26334
Fax:     +49 (0)234 / 32-14227
E-Mail:  greffrath@leat.ruhr-uni-bochum.de