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

[Robert Millan]

On Mon, Feb 11, 2008 at 09:53:15AM +0100, Fabian Greffrath wrote:
> 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.

> ???2008-02-11  Fabian Greffrath  <address@hidden>
> 
>       * 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.

> +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?

> diff -Naru grub2-1.96+20080203~/util/grub-probe.c 
> grub2-1.96+20080203/util/grub-probe.c
> --- grub2-1.96+20080203~/util/grub-probe.c    2008-01-25 23:33:57.000000000 
> +0100
> +++ grub2-1.96+20080203/util/grub-probe.c     2008-02-08 12:50:13.000000000 
> +0100
> @@ -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);

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);
}

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

> +  -d, --device              given argument is a system device, not a path\n\

... this option.

-- 
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 /.)