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

[Marco Gerards]

Fabian Greffrath <address@hidden> 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