Re: Bug#461442: detection of other OSes in update-grub

Re: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

Hi!

From:     Marco Gerards
> Thanks for this patch, however you didn't really describe the problem
> you are solving.  I am not sure if this patch replaces your patch.
> This thread was initiated with a follow up to a mail Robert sent
> off-list.  So can you please describe what you are doing and why,
> otherwise this is simply a piece of code to me that I have to review
> ;-)
>
> So please explain what this is, I cannot accept patches if I do not
> know why I should accept them.

Sorry, I wasn't aware of the fact that somebody might not know which 
context this patch is associated with. ;)

I have both Debian sid and Windows XP installed on my computer at work. 
Each time I install a new kernel in Debian the update-grub script is run 
and the new kernel is added to the boot menu. Fine so far, but I was 
annoyed by the fact that the update-grub script is not able to detect 
Windows on the other partition and add it to the boot menu as well 
(please see Debian bugs #461442 [1] and #462218 [2]).

However, there is the os-prober package [3] in Debian. This program 
test-mounts all possible partitions on your computer and checks for 
installled operating systems on these. It prints out a line for each OS 
it can find containing the corrensponding partition, the name, a short 
name and a boot keyword as a colon-separated string, e.g. 
"/dev/sda1:Windows NT/2000/XP:WinNT:chain". Based on this information it 
should be possible to add further operating systems to the boot menu via 
a script in /etc/grub.d that interprets the output of os-prober. This is 
what the second of the attached files from my previous email does.

You will notice that os-prober prints the operating system's partition 
as a system device, whereas GRUB has its own nomenclature for disc 
partitions. The first of the attached files from my previous email is a 
patch, that adds the ability to grub-probe to convert between systems 
devices and grub drives (but not vice versa ATM).

Hope this provides some clarity... ;)

> Is this code only yours or did you use code from other places?

No, it's only my code. Of course there is a chance to find similar code 
in Debian-Installer or parts of GRUB, but this is only mine.

> Huh, you are mixing C code with a shellscript?
>   

Hey, of course not!

> What does this code below do and why does it appear to be in this .c
> file?

I have attached two files to my last mail (as explained before), which 
were mistakenly formatted in Thunderbird to appear inline as only one 
file. I believe it is obvious where the diff ends and where the shell 
scripts begins.

From:     Robert Millan
> Uhm I'm not sure if that's a good thing or may be overkill.  What does everyone
> else think about this?
>   

I do not believe this is an overkill. Do you want grub-probe to print 
"i-dont-check-my-arguments" (grub-probe --target=device --device 
i-dont-check-my-arguments) ?
Even worse, if you select another target than 'device' and if you don't 
check if the given parameter is *really* a device name, you will pass 
the faulty parameter over to the other functions that are called in 
probe () and fail with some "cannot stat ..." message . In my opinion 
this should not happen. Instead, grub-probe should fail gracefully with 
a dedicated error messge in this situation as proposed by my patch.

cheers,
Fabian

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=461442
[2] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=462218
[3] http://packages.debian.org/sid/os-prober

-- 
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: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Wed, Feb 06, 2008 at 11:12:44AM +0100, Fabian Greffrath wrote:
> I do not believe this is an overkill. Do you want grub-probe to print 
> "i-dont-check-my-arguments" (grub-probe --target=device --device 
> i-dont-check-my-arguments) ?
> Even worse, if you select another target than 'device' and if you don't 
> check if the given parameter is *really* a device name, you will pass 
> the faulty parameter over to the other functions that are called in 
> probe () and fail with some "cannot stat ..." message . In my opinion 
> this should not happen. Instead, grub-probe should fail gracefully with 
> a dedicated error messge in this situation as proposed by my patch.

Ok, I don't mind that much.

-- 
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: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

Robert Millan schrieb:
> Sure, we can fix grub-probe.  This was already needed for something else
> (but I forgot what ;-)).
>   

Nevertheless it will be very very convenient to have this in grub-probe.

> Can you propose a CLI by which grub-probe would be told to process devices
> rather than mount points (in grub-devel)?  The hard part here is not the code,
> but coming up with a consistent interface (the code is rather trivial, since
> the conversion operation is completely isolated).
Sure, CCing grub-devel.

We should stay with the '-t drive' option to print the GRUB drive. If no 
further option is given, the next item on the command line is expected 
to be a path - just like before.
Then, another option, e.g. '-o', should be introduced to specify the 
origin of the said item if it is different from "path", e.g.:

$ grub-probe
Usage: grub-probe [OPTION]... [ORIGIN]

Probe device information for a given path or, if the -o option is given, 
for a given GRUB drive or system device.

  -m, --device-map=FILE     use FILE as the device map 
[default=/boot/grub/device.map]
  -t, --target=(fs|drive|device|partmap|abstraction)
                            print filesystem module, GRUB drive, system 
device, partition map module or abstraction module [default=fs]
  -o, --origin=(path|drive|device)
                            expect to read from path, GRUB drive or 
system device [default=path]
  -h, --help                display this message and exit
  -V, --version             print version information and exit
  -v, --verbose             print verbose messages

This way it will be possible to even convert back and forth between GRUB 
drives and system devices.
The conversion that is needed to translate os-prober output to GRUB menu 
input will read:
GRUB_DEVICE="`grub-probe -t drive -o device ${PARTITION}`".

Hope you like this proposal (of course, "origin" is subject to change; 
it's just the first word that came to my mind that might be fitting)!

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: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Tue, Jan 22, 2008 at 02:14:53PM +0100, Fabian Greffrath wrote:
> 
> We should stay with the '-t drive' option to print the GRUB drive. If no 
> further option is given, the next item on the command line is expected 
> to be a path - just like before.
> Then, another option, e.g. '-o', should be introduced to specify the 
> origin of the said item if it is different from "path", e.g.:
> 
> $ grub-probe
> Usage: grub-probe [OPTION]... [ORIGIN]
> 
> Probe device information for a given path or, if the -o option is given, 
> for a given GRUB drive or system device.
> 
>  -m, --device-map=FILE     use FILE as the device map 
> [default=/boot/grub/device.map]
>  -t, --target=(fs|drive|device|partmap|abstraction)
>                            print filesystem module, GRUB drive, system 
> device, partition map module or abstraction module [default=fs]
>  -o, --origin=(path|drive|device)
>                            expect to read from path, GRUB drive or 
> system device [default=path]
>  -h, --help                display this message and exit
>  -V, --version             print version information and exit
>  -v, --verbose             print verbose messages
> 
> This way it will be possible to even convert back and forth between GRUB 
> drives and system devices.
> The conversion that is needed to translate os-prober output to GRUB menu 
> input will read:
> GRUB_DEVICE="`grub-probe -t drive -o device ${PARTITION}`".
> 
> Hope you like this proposal (of course, "origin" is subject to change; 
> it's just the first word that came to my mind that might be fitting)!

This implies that we'll have to support 6 different kinds of conversions
between path, drive and device, some of which might even be impossible.

Besides, perhaps it'd be cleaner to split this conversion to a separate
tool, so that grub-probe only operates on devices, and the other tool
converts paths to devices (but NOT necessarily devices to paths!).

What does everyone think about 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: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

>
> This implies that we'll have to support 6 different kinds of conversions
> between path, drive and device, some of which might even be impossible.
>
> Besides, perhaps it'd be cleaner to split this conversion to a separate
> tool, so that grub-probe only operates on devices, and the other tool
> converts paths to devices (but NOT necessarily devices to paths!).
>
> What does everyone think about this?
>   

Yes, it is really necessary!

How about this proposal:

$ grub-convert
Usage: grub-convert [-d|-g] [DEVICE|DRIVE]

Convert back and forth between Linux devices and GRUB drives.

-d, --device=DEVICE   convert DEVICE into a GRUB-drive [e.g. /dev/hda1 
-> (hd0,1)]
-g, --drive=DRIVE   convert DRIVE into a Linux device [e.g. (hd0,1) -> 
/dev/hda1]
-h, --help                display this message and exit
-V, --version             print version information and exit

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: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Wed, Jan 30, 2008 at 11:41:59AM +0100, Fabian Greffrath wrote:
> >
> >This implies that we'll have to support 6 different kinds of conversions
> >between path, drive and device, some of which might even be impossible.
> >
> >Besides, perhaps it'd be cleaner to split this conversion to a separate
> >tool, so that grub-probe only operates on devices, and the other tool
> >converts paths to devices (but NOT necessarily devices to paths!).

Actually, this can't work now.  After I added a sanity check (sorry, this
was necessary) to verify file reading work properly, everything is more
intermangled.

I'm not sure what would be the right approach.  Perhaps a flag in grub-probe
that means "please skip grub_guess_root_device, argument is a device, not a
path!" ?  This will let the sanity check know when it can't tell which file
you want to read.

Or perhaps something more radical that splits everything in different layers
(of exposed user commands) ?  But that could clutter the user with so many
commands.

Suggestions?

> How about this proposal:
> 
> $ grub-convert
> Usage: grub-convert [-d|-g] [DEVICE|DRIVE]

-- 
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: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

Robert Millan schrieb:
> Suggestions?

For the time being I could duplicate the convert() function from 
Debian's grub-installer to convert the device names.

However, I believe that there are plenty of cases in which you could use 
a generic device name conversion tool...

-- 
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: Bug#461442: detection of other OSes in update-grub

[Otavio Salvador]

Fabian Greffrath <greffrath@leat.rub.de> writes:

> Robert Millan schrieb:
>> Suggestions?
>
> For the time being I could duplicate the convert() function from
> Debian's grub-installer to convert the device names.

It could be nice so it could be tested on Debian while it's not
accepted upstream.

> However, I believe that there are plenty of cases in which you could
> use a generic device name conversion tool...

Sure but at least we can try it now and keep working in upstream to
remove this change.

-- 
        O T A V I O    S A L V A D O R
---------------------------------------------
 E-mail: otavio@debian.org      UIN: 5906116
 GNU/Linux User: 239058     GPG ID: 49A5F855
 Home Page: http://otavio.ossystems.com.br
---------------------------------------------
"Microsoft sells you Windows ... Linux gives
 you the whole house."

Re: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Thu, Jan 31, 2008 at 09:31:57AM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >Suggestions?
> 
> For the time being I could duplicate the convert() function from 
> Debian's grub-installer to convert the device names.
> 
> However, I believe that there are plenty of cases in which you could use 
> a generic device name conversion tool...

Sorry, I got confused.  The generic linux-device / grub-drive conversion tool
is no problem.  I think it's fine to add it.

The problem is if you need grub-probe to work with an input different than
a filesystem path.

If you want the former, feel free to send a patch to grub-devel.  For the
latter, look at my questions and send some feedback.

Or, if nobody wants to send feedback, I guess I'll have to propose something
myself.  But I prefer if this is discussed (given that we have time).

-- 
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: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

Robert Millan schrieb:
> Sorry, I got confused.  The generic linux-device / grub-drive conversion tool
> is no problem.  I think it's fine to add it.
>   

Fine!

> The problem is if you need grub-probe to work with an input different than
> a filesystem path.
>   

I won't need grub-probe at all as soon as the aforementioned tool exists.
(My initial idea was to integrate the functionality into grub-probe, but 
a separate tool is fine as well.)

> If you want the former, feel free to send a patch to grub-devel.

Hey, I'm allready proud of my little shell script... ;)

>   For the
> latter, look at my questions and send some feedback.
>   

Sorry, Robert, but which questions?

-- 
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: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Thu, Jan 31, 2008 at 03:26:24PM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >Sorry, I got confused.  The generic linux-device / grub-drive conversion 
> >tool
> >is no problem.  I think it's fine to add it.
> >  
> 
> Fine!
> 
> >The problem is if you need grub-probe to work with an input different than
> >a filesystem path.
> >  
> 
> I won't need grub-probe at all as soon as the aforementioned tool exists.
> (My initial idea was to integrate the functionality into grub-probe, but 
> a separate tool is fine as well.)
> 
> >If you want the former, feel free to send a patch to grub-devel.
> 
> Hey, I'm allready proud of my little shell script... ;)
> 
> >  For the
> >latter, look at my questions and send some feedback.
> >  
> 
> Sorry, Robert, but which questions?

It is described earlier in this thread, but it doesn't matter.  If
adding a drive/device conversion utility is enough for you, please
can you send a patch for it?

You could even merge it with the os-prober one and send it altogether,
if that's more practical for you.

Thank 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: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

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

Hi there,

Robert Millan schrieb:
> If you want the former, feel free to send a patch to grub-devel.  For the
> latter, look at my questions and send some feedback.

please find attached a patch that adds a new parameter '--device, -d' to 
grub-probe. If this parameter 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.
Furthermore I had to add a new public function to getroot.c that returns 
the given argument if it is a block device and returns NULL else. This 
was necessary, because else you could force grub-probe to print 'foobar' 
if run as 'grub-probe --target=device --device foobar'.

I consider this (i.e. adding a new option to grub-probe) a better 
solution than writing a separate tool only for this purpose.

The second file that I attached is the 30_os-prober script making use of 
the new grub-probe feature. Please note that the 'case ... in' part is 
only run if 'grub-probe --device' actually returns something usefull.

I am looking forward to your feedback.

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.diff --]
[-- Type: text/x-patch, Size: 3302 bytes --]

diff -urNad grub2-1.95+20080201~/include/grub/util/getroot.h grub2-1.95+20080201/include/grub/util/getroot.h
--- grub2-1.95+20080201~/include/grub/util/getroot.h	2008-01-12 16:11:56.000000000 +0100
+++ grub2-1.95+20080201/include/grub/util/getroot.h	2008-02-03 18:14:11.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 -urNad grub2-1.95+20080201~/util/getroot.c grub2-1.95+20080201/util/getroot.c
--- grub2-1.95+20080201~/util/getroot.c	2008-01-12 16:11:56.000000000 +0100
+++ grub2-1.95+20080201/util/getroot.c	2008-02-03 21:56:29.000000000 +0100
@@ -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;
+}
diff -urNad grub2-1.95+20080201~/util/grub-probe.c grub2-1.95+20080201/util/grub-probe.c
--- grub2-1.95+20080201~/util/grub-probe.c	2008-01-25 23:33:57.000000000 +0100
+++ grub2-1.95+20080201/util/grub-probe.c	2008-02-03 19:30:50.000000000 +0100
@@ -50,6 +50,7 @@
 };
 
 int print = PRINT_FS;
+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);

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

#! /bin/sh -e

# update-grub helper script.
# <insert copyright and license blurb here>

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: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Mon, Feb 04, 2008 at 09:43:58AM +0100, Fabian Greffrath wrote:
> the given argument if it is a block device and returns NULL else. This 
> was necessary, because else you could force grub-probe to print 'foobar' 
> if run as 'grub-probe --target=device --device foobar'.

Why is that a problem?

> --- grub2-1.95+20080201~/util/getroot.c	2008-01-12 16:11:56.000000000 +0100
> +++ grub2-1.95+20080201/util/getroot.c	2008-02-03 21:56:29.000000000 +0100
> @@ -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);

Missing space here. ^

> +  else
> +    return 0;
> +}

I think this function could be called from the other part of this file which
performs similar checks (if this functionality is to be kept, that is).

> diff -urNad grub2-1.95+20080201~/util/grub-probe.c grub2-1.95+20080201/util/grub-probe.c
> --- grub2-1.95+20080201~/util/grub-probe.c	2008-01-25 23:33:57.000000000 +0100
> +++ grub2-1.95+20080201/util/grub-probe.c	2008-02-03 19:30:50.000000000 +0100
> @@ -50,6 +50,7 @@
>  };
>  
>  int print = PRINT_FS;
> +unsigned int argument_is_device = 0;

I know that the call to probe() is not supposed to be reentrant, but I'd
prefer not to break reentrancy if it can be easily avoided;  it is possible
that probe() needs to recurse onto itself in the future (because of RAID/LVM).

> #! /bin/sh -e
> 
> # update-grub helper script.
> # <insert copyright and license blurb here>

Please remember to fix that in later versions of the patch ;-)

>         linux)
>           if [ -x "`which linux-boot-prober 2>/dev/null`" ] ; then
>             LINUXPROBED="`linux-boot-prober ${DEVICE} | tr ' ' '|' | paste -s -d ' '`"
>           fi

Is it possible to share code with 10_linux.in here?

>           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 '|' ' '`"

Maybe this can be simplified with "echo something | read a b c d" feature?

-- 
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: Bug#461442: detection of other OSes in update-grub

[Fabian Greffrath]

Robert Millan schrieb:
> Why is that a problem?
>   

Because foobar is not a block device, but grub-probe claims that it will 
print a device if '--target=device' is given.

> I think this function could be called from the other part of this file which
> performs similar checks (if this functionality is to be kept, that is).
>   

Yes, I am fine with this.

> I know that the call to probe() is not supposed to be reentrant, but I'd
> prefer not to break reentrancy if it can be easily avoided;  it is possible
> that probe() needs to recurse onto itself in the future (because of RAID/LVM).
>   

OK, but should I keep it uninitialized?

> Please remember to fix that in later versions of the patch ;-)
>   

Sure, my copyright assignment paper is on it's way...

> Is it possible to share code with 10_linux.in here?
>   

Only if os-prober is installed. But then, os-prober does not check for 
kernels on / and /target.

> Maybe this can be simplified with "echo something | read a b c d" feature?
>   

For cosmetic reasons, yes. ;)

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: Bug#461442: detection of other OSes in update-grub

[Robert Millan]

On Mon, Feb 04, 2008 at 03:47:26PM +0100, Fabian Greffrath wrote:
> Robert Millan schrieb:
> >Why is that a problem?
> >  
> 
> Because foobar is not a block device, but grub-probe claims that it will 
> print a device if '--target=device' is given.

Uhm I'm not sure if that's a good thing or may be overkill.  What does everyone
else think about this?

> >I know that the call to probe() is not supposed to be reentrant, but I'd
> >prefer not to break reentrancy if it can be easily avoided;  it is possible
> >that probe() needs to recurse onto itself in the future (because of 
> >RAID/LVM).
> >  
> 
> OK, but should I keep it uninitialized?

Depends on what you want to do ;-)

> >Is it possible to share code with 10_linux.in here?
> >  
> 
> Only if os-prober is installed. But then, os-prober does not check for 
> kernels on / and /target.

What I mean is that 10_linux.in already has code that probes for stuff like
Linux images and initrd files, and it might be a good idea to use the same
logic.

Or does os-prober provide any additional feature/knowledge about 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: Bug#461442: detection of other OSes in update-grub

[Marco Gerards]

Fabian Greffrath <greffrath@leat.rub.de> writes:

Hi!

> Robert Millan schrieb:
>> If you want the former, feel free to send a patch to grub-devel.  For the
>> latter, look at my questions and send some feedback.
>
> please find attached a patch that adds a new parameter '--device, -d'
> to grub-probe. If this parameter 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.
> Furthermore I had to add a new public function to getroot.c that
> returns the given argument if it is a block device and returns NULL
> else. This was necessary, because else you could force grub-probe to
> print 'foobar' if run as 'grub-probe --target=device --device foobar'.

Thanks for this patch, however you didn't really describe the problem
you are solving.  I am not sure if this patch replaces your patch.
This thread was initiated with a follow up to a mail Robert sent
off-list.  So can you please describe what you are doing and why,
otherwise this is simply a piece of code to me that I have to review
;-)

So please explain what this is, I cannot accept patches if I do not
know why I should accept them.

> I consider this (i.e. adding a new option to grub-probe) a better
> solution than writing a separate tool only for this purpose.
>
> The second file that I attached is the 30_os-prober script making use
> of the new grub-probe feature. Please note that the 'case ... in' part
> is only run if 'grub-probe --device' actually returns something
> usefull.
>
> I am looking forward to your feedback.

Here it is ;-)

First of all, a ChangeLog entry is missing.

Is this code only yours or did you use code from other places?

> diff -urNad grub2-1.95+20080201~/include/grub/util/getroot.h grub2-1.95+20080201/include/grub/util/getroot.h
> --- grub2-1.95+20080201~/include/grub/util/getroot.h	2008-01-12 16:11:56.000000000 +0100
> +++ grub2-1.95+20080201/include/grub/util/getroot.h	2008-02-03 18:14:11.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 -urNad grub2-1.95+20080201~/util/getroot.c grub2-1.95+20080201/util/getroot.c
> --- grub2-1.95+20080201~/util/getroot.c	2008-01-12 16:11:56.000000000 +0100
> +++ grub2-1.95+20080201/util/getroot.c	2008-02-03 21:56:29.000000000 +0100
> @@ -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);

strdup (blk_dev);

> +  else
> +    return 0;
> +}
> diff -urNad grub2-1.95+20080201~/util/grub-probe.c grub2-1.95+20080201/util/grub-probe.c
> --- grub2-1.95+20080201~/util/grub-probe.c	2008-01-25 23:33:57.000000000 +0100
> +++ grub2-1.95+20080201/util/grub-probe.c	2008-02-03 19:30:50.000000000 +0100
> @@ -50,6 +50,7 @@
>  };
>  
>  int print = PRINT_FS;
> +unsigned int argument_is_device = 0;

Shouldn't this be static?

>  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);
> #! /bin/sh -e


Huh, you are mixing C code with a shellscript?

What does this code below do and why does it appear to be in this .c
file?

> # update-grub helper script.
> # <insert copyright and license blurb here>
>
> 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