Add option to grub-probe to accept system devices as arguments
PATCH 1/1 Add option to grub-probe to accept system devices as arguments
The following patch adds to grub-probe the ability to accept system devices as arguments and e.g. convert between system devices and GRUB drives.
This patch is improved over the one from my previous posting and has some minor issues fixed. Please use this version instead. It is all my work.
2008-02-11 Fabian Greffrath <[email protected]>
* util/grub-probe.c: Add new parameter '-d, --device'. If this is set, grub-probe expects the given argument to be a block device. All of the '--target' parameters work with this option as well. If the '--device' parameter is not set, grub-probe will work as before.
* util/getroot.c (grub_util_check_block_device): New public function that returns the given argument if it is a block device and returns NULL else.
* util/getroot.h (grub_util_check_block_device): Export function.
include/grub/util/getroot.h +1 -0
@@ -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 */
util/getroot.c +14 -0
@@ -327,3 +327,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 strdup (blk_dev); +else +return 0 ; +}
util/grub-probe.c +21 -5
@@ -50,6 +50,7 @@ };int print =PRINT_FS ; +static unsigned int argument_is_device =0 ;void grub_putchar (int c)
@@ -84,9 +85,18 @@int abstraction_type;grub_device_t dev =NULL ; - device_name =grub_guess_root_device (path); +if (argument_is_device) + device_name =grub_util_check_block_device (path); +else + device_name =grub_guess_root_device (path); +if (! device_name) -grub_util_error ("cannot find a device for %s.\n" , path); + { +if (argument_is_device) +grub_util_error ("%s is not a block device.\n" , path); +else +grub_util_error ("cannot find a device for %s.\n" , path); + }if (print ==PRINT_DEVICE ) {
@@ -201,6 +211,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' },
@@ -217,10 +228,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.\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\
@@ -246,13 +258,17 @@/* 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);
Quoted 7 lines · Fabian Greffrath
The following patch adds to grub-probe the ability to accept system devices as
arguments and e.g. convert between system devices and GRUB drives.This patch is improved over the one from my previous posting and has some minor
issues fixed. Please use this version instead. It is all my work.
Hi Fabian,
Some comments on this one.
Quoted 6 lines
???2008-02-11 Fabian Greffrath <[email protected]>
*
util/grub-probe.c: Add new parameter '-d, --device'. If this is set, grub-probe expects the given argument to be a block device. All of the '--target' parameters work with this option as well. If the '--device' parameter is not set, grub-probe will work as before.
You need to list every function or variable separately. E.g:
* util/grub-probe.c (argument_is_device): New variable. (probe): Blah blah.
(main): Etc.
If you use the -p option to diff it'll be easier to check it just by reading the patch.
util/getroot.c line 343
Quoted 13 lines
util/getroot.c +13
-0+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 strdup (blk_dev); +else +return 0 ; +}
Is there really a need to strdup() it?
Quoted 12 lines
diff -Naru
grub2-1.96+20080203~/util/grub-probe.cgrub2-1.96+20080203/util/grub-probe.cutil/grub-probe.c +1
-0@@ -50,6 +50,7 @@ };int print =PRINT_FS ; +static unsigned int argument_is_device =0 ;void grub_putchar (int c)
util/grub-probe.c line 91
Quoted 8 lines
util/grub-probe.c +4
-1@@ -84,9 +85,18 @@int abstraction_type;grub_device_t dev =NULL ; - device_name =grub_guess_root_device (path); +if (argument_is_device) + device_name =grub_util_check_block_device (path); +else + device_name =grub_guess_root_device (path);
I find it confusing that you change the meaning of the path' variable without renaming it. Also, the variable that describes what path' is (argument_is_device) is passed separately.
What would you think of a scheme where both are passed as strings and either can be NULL? E.g.
probe (char *path, char *device_name)
{
if (path == NULL)
{
if (grub_util_check_block_device (device_name) == -1)
return -1;
}
else
device_name = grub_guess_root_device (path);
}util/grub-probe.c line 233
Quoted 5 lines
util/grub-probe.c +2
-2-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.\n\
I suspect advertising it here might lead users to think they can pass a device to it, without caring about ...
util/grub-probe.c line 235
Quoted 1 line
util/grub-probe.c +1
-0+ -d, --device given argument is a system device, not a path\n\
... this option.
Signature · 5 lines
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 /.)
Hi Robert,
Quoted 3 lines
You need to list every function or variable separately. [...]
If you use the -p option to diff it'll be easier to check it just by reading the patch.
OK, thanks, I didn't know this. Next time... ;)
Quoted 1 line
Is there really a need to
strdup()it?
Yep. If you return the pointer (i.e. to `path') you'll get memory corruption as soon as you try to free (device_name).
Quoted 3 lines
I find it confusing that you change the meaning of the
path' variable without renaming it. Also, the variable that describes whatpath' is (argument_is_device) is passed separately.
Allright. Maybe the path' variable should be renamed to argument'.
Quoted 2 lines
What would you think of a scheme where both are passed as strings and either can be NULL?
Well, I prefer the idea to have the argument stored in one string and have a separate boolean switch that tells if the string contains a device name or a path. But I am always open to suggestions. ;)
Quoted 2 lines
I suspect advertising it here might lead users to think they can pass a device to it, without caring about [...] this option.
Allright, this should be changed to something like this:
"Probe device information for a given path (or device, if the -d option is given)."
Thanks!
Cheers,
Fabian
Quoted 4 lines · Fabian Greffrath
Quoted 1 line
Is there really a need to
strdup()it?Yep. If you return the pointer (i.e. to `path') you'll get memory corruption as soon as you try to free (
device_name).
You put that function in a separate file, which indicates it is meant to be a general-purpose function, but its spec is constrained by the code in grub-probe (i.e. you strdup() not because it is needed for the purpose of this function, but because grub-probe already calls free()).
In case of a general-purpose function, it doesn't make much sense to strdup it. Consider what would a caller have to do in case it wasn't using free() already:
ret = check (device);
if (! ret)
fatal ("%s is bad", device);
free (ret);
instead of just:
if (! check (device))
fatal ("%s is bad", device);
Quoted 6 lines
Quoted 4 lines
I find it confusing that you change the meaning of the `path' variable without
renaming it. Also, the variable that describes what `path' is (argument_is_device) is passed separately.Allright. Maybe the
path' variable should be renamed toargument'.
That works too ... but please put them together (i.e. both as arguments to probe()).
Quoted 7 lines
Quoted 3 lines
I suspect advertising it here might lead users to think they can pass a device
to it, without caring about [...] this option.Allright, this should be changed to something like this:
"Probe device information for a given path (or device, if the -d option is given)."
Ok.
Signature · 5 lines
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 /.)
Quoted 4 lines
You put that function in a separate file, which indicates it is meant to be a general-purpose function, but its spec is constrained by the code in grub-probe (i.e. you
strdup()not because it is needed for the purpose of this function, but because grub-probe already callsfree()).
That's right. Consequently there are two alternatives left:
- grub_util_check_block_device() returns the pointer instead of strdup() and the call to free() in grub-probe is conditional upon if argument_is_device is true or false; which I consider an ugly hack.
- I keep getroot.{c,h} untouched and include these four lines in grub-probe; which is a pity because I consider the function quite usefull, though still not perfect (see above).
In the context of the second alternative, the attached (draft!) patch leads to the following results:
$ mount /dev/hda1 on / type ext3 (rw,errors=remount-ro) /dev/hda6 on /home type ext3 (rw) $ su
# grub-probe No path or device is specified. Try ``grub-probe --help'' for more information. # grub-probe /home ext2 # grub-probe /homer grub-probe: error: cannot stat /homer.
# grub-probe /dev/hda1 ext2 # grub-probe --target=drive No path or device is specified. Try ``grub-probe --help'' for more information. # grub-probe --target=drive /home (hd0,6) # grub-probe --target=drive /homer grub-probe: error: cannot stat /homer.
# grub-probe --target=drive /dev/hda1 (hd0,1)
How do you like that?
Cheers,
Fabian
util/grub-probe.c +24 -19
@@ -1,4 +1,4 @@ -/* grub-probe.c - probe device information for a given path */ +/* grub-probe.c - probe device information for a given path or device */ /* * GRUB -- GRand Unified Bootloader * Copyright (C) 2005,2006,2007,2008 Free Software Foundation, Inc.
@@ -100,7 +100,7 @@ probe_partmap (grub_disk_t disk) }static void -probe (const char *path) +probe (const char *argument) {char *device_name;char *drive_name =NULL ;
@@ -108,10 +108,18 @@ probe (const char *path)char *filebuf_via_grub =NULL , *filebuf_via_sys =NULL ;int abstraction_type;grub_device_t dev =NULL ; - - device_name =grub_guess_root_device (path); +struct stat st; + +if (stat (argument, &st) <0 ) +grub_util_error ("cannot stat %s.\n" , argument); + +if (S_ISBLK (st.st_mode)) + device_name =strdup (argument); +else + device_name =grub_guess_root_device (argument); +if (! device_name) -grub_util_error ("cannot find a device for %s.\n" , path); +grub_util_error ("cannot find a device for %s.\n" , argument);if (print ==PRINT_DEVICE ) {
@@ -178,21 +186,18 @@ probe (const char *path)if (print ==PRINT_FS ) { -struct stat st;grub_fs_t fs; -stat (path, &st); -if (st.st_mode ==S_IFREG ) {/* Regular file. Verify that we can read it properly. */ grub_file_t file; -grub_util_info ("reading %s via OS facilities" , path); - filebuf_via_sys =grub_util_read_image (path); +grub_util_info ("reading %s via OS facilities" , argument); + filebuf_via_sys =grub_util_read_image (argument); -grub_util_info ("reading %s via GRUB facilities" , path); -asprintf (&grub_path,"(%s)%s" , drive_name, path); +grub_util_info ("reading %s via GRUB facilities" , argument); +asprintf (&grub_path,"(%s)%s" , drive_name, argument); file =grub_file_open (grub_path); filebuf_via_grub =xmalloc (file->size);grub_file_read (file, filebuf_via_grub, file->size);
@@ -242,9 +247,9 @@ usage (int status) "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.\n\ \n\ -m, --device-map=FILE use FILE as the device map [default=%s]\n\ -t, --target=(fs|drive|device|partmap|abstraction)\n\
@@ -264,7 +269,7 @@ int main (int argc, char *argv[]) { char *dev_map = 0; -char *path; +char *argument; progname ="grub-probe" ;
@@ -321,10 +326,10 @@ main (int argc, char *argv[])if (verbosity >1 )grub_env_set ("debug" ,"all" ); -/* Obtain PATH. */ +/* Obtain PATH or DEVICE. */ if (optind >= argc) { -fprintf (stderr,"No path is specified.\n" ); +fprintf (stderr,"No path or device is specified.\n" );usage (1 ); }
@@ -334,7 +339,7 @@ main (int argc, char *argv[])usage (1 ); } - path = argv[optind]; + argument = argv[optind];/* Initialize the emulated biosdisk driver. */ grub_util_biosdisk_init (dev_map ? :DEFAULT_DEVICE_MAP );
@@ -343,7 +348,7 @@ main (int argc, char *argv[])grub_init_all ();/* Do it. */ -probe (path); +probe (argument);/* Free resources. */ grub_fini_all ();
Quoted 13 lines · Fabian Greffrath
Quoted 4 lines
You put that function in a separate file, which indicates it is meant to be a general-purpose function, but its spec is constrained by the code in grub-probe (i.e. you
strdup()not because it is needed for the purpose of this function, but because grub-probe already callsfree()).That's right. Consequently there are two alternatives left:
-
grub_util_check_block_device()returns the pointer instead ofstrdup()and the
call tofree()in grub-probe is conditional upon ifargument_is_deviceis true
or false; which I consider an ugly hack.
Quoted 4 lines
- I keep getroot.{c,h} untouched and include these four lines in grub-probe; which is a pity because I consider the function quite usefull, though still not
perfect (see above).
That's why I proposed to pass both path and device, and let either of them be NULL. When you do this:
- You can determine whether device' is provided (hence you can skip converting from path') simply with "if (device)".
- You can make the free() part independant of any other indications by
issuing:
if (device)
free (device);
if (path)
free (path);Signature · 5 lines
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 /.)
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.
Quoted 1 line · 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.
Ping! ;-)
Quoted 3 lines · 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.
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.
Signature · 5 lines
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 /.)
Quoted 2 lines · Robert Millan
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.
Quoted 1 line
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
Quoted 19 lines · Fabian Greffrath
Quoted 2 lines · Robert Millan
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
Quoted 5 lines
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.
Quoted 7 lines
Quoted 1 line
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?
Signature · 5 lines
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 /.)
Quoted 1 line · Robert Millan
The last one sounds like a bug.
Yes, it's from the current *unpatched* grub-probe.
Quoted 1 line
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.
Quoted 4 lines
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.
Quoted 4 lines · Fabian Greffrath
Quoted 1 line · Robert Millan
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_.
Quoted 9 lines
Quoted 4 lines
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?
Signature · 5 lines
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 /.)
Quoted 1 line · Robert Millan
I mean that it should find the disk that contains
/dev/hda1device _node_.
Maybe, but I do not believe this has anything to do with my patch.
Quoted 2 lines
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.
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
PATCH 1/1 Add option to grub-probe to accept system devices as arguments
In reply to Robert MillanHi 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
Signature · 10 lines
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: [email protected]
include/grub/util/getroot.h +1 -0
@@ -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 */
util/getroot.c +14 -0
@@ -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 ; +}
util/grub-probe.c +27 -11
@@ -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 ();
Well, the solution seems to be easier than I expected. Is the attached patch acceptable?
Have a nice weekend,
Fabian
Signature · 10 lines
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: [email protected]
include/grub/util/getroot.h +1 -0
@@ -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 */
util/getroot.c +14 -0
@@ -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 ; +}
util/grub-probe.c +27 -12
@@ -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 ();
Sorry for the long delay, I've been very busy (and unfortunately I'll continue to be :-().
Quoted 2 lines · Fabian Greffrath
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):
util/grub-probe.c line 114
Quoted 3 lines
util/grub-probe.c +3
-0+if (path ==NULL ) + { +if ( !grub_util_check_block_device (device_name))
could you remove the space in "( !" ?
util/grub-probe.c line 361
Quoted 4 lines
util/grub-probe.c +4
-0+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.
Signature · 5 lines
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 /.)
Hello!
Quoted 3 lines · Robert Millan
It looks good to me. Just a pair of minor details (simply for consistency with code style in grub):
[...]
done.
Quoted 2 lines
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. ;)
Quoted 2 lines · Robert Millan
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
Signature · 10 lines
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: [email protected]
2008-02-25 Fabian Greffrath <[email protected]>
* 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.
include/grub/util/getroot.h +1 -0
@@ -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 */
util/getroot.c +14 -0
@@ -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 ; +}
util/grub-probe.c +27 -12
@@ -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 ();
Quoted 6 lines · Fabian Greffrath
Quoted 3 lines
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. ;)
Quoted 7 lines
Quoted 1 line · Robert Millan
Is this solved now? If not, you could send a gdb backtrace to give us an
idea
Quoted 1 line
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.
Signature · 5 lines
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 /.)
Quoted 1 line · Robert Millan
Ok. Does anybody object to Fabian's patch? I'd like to commit it.
Silence is consense. ;)
Signature · 10 lines
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: [email protected]
Quoted 4 lines · Fabian Greffrath
Quoted 1 line · Robert Millan
Ok. Does anybody object to Fabian's patch? I'd like to commit it.
Silence is consense. ;)
Committed.
Signature · 5 lines
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 /.)
Quoted 1 line · Robert Millan
Committed.
Great, thanks!
Now let's get support for os-prober output parsing.
Signature · 10 lines
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: [email protected]
Quoted 6 lines · Fabian Greffrath
Quoted 1 line · Robert Millan
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.
Signature · 5 lines
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 /.)
Quoted 1 line · Robert Millan
Ok. Is there any mail for that waiting for input? I lose track of things.
No problem, please find the script attached.
#! /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
Quoted 2 lines · Fabian Greffrath
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...
Quoted 1 line
if [ -x "
which linux-boot-prober 2>/dev/null" ] ; then
same here.
Quoted 3 lines
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 ... ?
Quoted 2 lines
*)
;;Maybe it's better to echo something here, otherwise it gives the impression OS FooBar was just detected and properly added.
Signature · 5 lines
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 /.)
Quoted 2 lines · Robert Millan
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" ] ...
?
Quoted 2 lines
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.
Quoted 1 line
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. ;)
Quoted 1 line
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.
Quoted 2 lines
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?!
Quoted 8 lines · Fabian Greffrath
Quoted 2 lines
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).
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 1 line
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.
Signature · 5 lines
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 /.)
Quoted 2 lines · Robert Millan
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..
Quoted 2 lines
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.
Quoted 1 line
For now only very simple add-ons exist (memtest86, invaders...).
Hehe, I know invaders quite well. ;)
Quoted 1 line
I'd suggest putting it in GRUB, at least for now, if that is fine with you.
Yes, sure!
Signature · 10 lines
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: [email protected]
PATCH 1/1 os-prober output parser
In reply to Robert MillanI 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
Signature · 10 lines
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: [email protected]
#! /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
Quoted 1 line · Fabian Greffrath
I have improved the script a little bit;
Robert, have you allready had a look at it? :)
Signature · 10 lines
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: [email protected]
Re: [NEW PATCH] os-prober output parser
In reply to Fabian GreffrathSorry 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.
Quoted 1 line · Fabian Greffrath
???#!
/bin/sh-e
Whops. This tag (utf-8 stuff?) breaks the Linux shebang parser :-)
Quoted 1 line
if [ -x
/usr/bin/os-prober-a -x/usr/bin/linux-boot-prober] ; then : ; else
Can we avoid hardcoding their absolute path?
Signature · 5 lines
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
Quoted 1 line · Robert Millan
Sorry for the delay in repliing. Been very busy :-(
No problem. ;)
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 1 line
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. ;)
Quoted 1 line
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...
Signature · 10 lines
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: [email protected]
Re: [NEW PATCH] os-prober output parser
Quoted 6 lines · Fabian Greffrath
Quoted 3 lines
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/nullin 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).
Quoted 4 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 1 line
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.. :-)
Quoted 4 lines
Quoted 1 line
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 1fi
which is very ugly if you ask me. Okuji, do you have any comments on this?
Signature · 5 lines
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
Quoted 1 line · Robert Millan
which is very ugly if you ask me. Okuji, do you have any comments on this?
Apart from this issue, is the script allright?
Signature · 10 lines
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: [email protected]
Re: [NEW PATCH] os-prober output parser
Quoted 4 lines · Fabian Greffrath
Quoted 1 line · Robert Millan
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.
Signature · 5 lines
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
Quoted 2 lines · Robert Millan
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?
Signature · 10 lines
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: [email protected]
Re: [NEW PATCH] os-prober output parser
Quoted 1 line · Fabian Greffrath
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 <[email protected]>
* 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.
Signature · 10 lines
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: [email protected]
#! /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
Hi Robert,
I am curious why this didn't make it into the last Debian upload?
Cheers,
Fabian
Quoted 21 lines · Fabian Greffrath
Quoted 1 line · Fabian Greffrath
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 <[email protected]>
* util\grub.d\30_os-prober: New script. Use
os-prober' andlinux-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
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Signature · 10 lines
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: [email protected]
Re: [NEW PATCH] os-prober output parser
Quoted 3 lines · Fabian Greffrath
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.
Signature · 5 lines
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
Quoted 1 line · Robert Millan
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
In reply to Fabian GreffrathQuoted 6 lines · Fabian Greffrath
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.
Quoted 6 lines
2008-04-07 Fabian Greffrath <[email protected]>
* util\grub.d\30_os-prober: New script. Use
os-prober' andlinux-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
Signature · 5 lines
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
I allready appended this to the Debian bug report, but for the record:
Please make the grub-pc package Recommend: os-prober.
Cheers,
Fabian
Signature · 10 lines
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: [email protected]
Quoted 2 lines · Robert Millan
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
Signature · 10 lines
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: [email protected]
Quoted 5 lines · Fabian Greffrath
Quoted 3 lines · Robert Millan
Better do this like it's done in other scripts (check grub-install). Not sure
why, I guess it's more portable that way...
Quoted 8 lines
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... ;-)
Quoted 7 lines
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.
Signature · 5 lines
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 /.)
Quoted 2 lines · Fabian Greffrath
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.
Signature · 5 lines
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 /.)