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 <[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.
> +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 /.)
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
