Re: test -e patch

[Robert Millan]

On Wed, Jun 06, 2007 at 01:42:48AM +0200, adrian15 wrote:
> Attached you will find the patch adding test -e support for grub2.
> 
> This is my first patch. I have compiled it without no errors.
> 
> However as long as the grub2.tar.gz that Marco gave me did not have any
> documentation about how to create a floppy (or at least I did not manage
> to find it)

This should work:

  cat boot.img core.img | dd of=foo.img seek=0 conv=notrunc

>    > static const struct grub_arg_option options[] =
> >  {
> >    {"file", 'e', 0, "test if a file exists", 0, 0},
> >    {0, 0, 0, 0, 0, 0}
> >  };
> 
> Is this correct? What's that last line for? Is it compulsory ?

It's common practice for arrays to make them null-terminated to be able to
determine their limits without making assumptions about their size.  Doing it
with structs is akin to null-terminating a char array.

> >{
> >     grub_file_t file;
> >
> >      file = grub_file_open (key);

What happens if file is a device node, or a directory?  Does grub_file_open
work with these?  Perhaps grub_file_stat would make more sense (after all,
if you want to implement -f, -H, -d, etc we'll need to stat it).

> How the hell should I treat grub errors. Maybe the test_file_exists has
> to return a static grub_err_t and then from grub_cmd_test I should call
> it like this: return (test_file_exists (args[0])) so that the error
> propagates ?
> 
> What are the error polices ?

I'm not sure what the policy is, but this seems to not be in sync with the
cpuid module I wrote a while ago.

IMHO, an error would be if existance of the file cannot be determined.  I don't
think non-existance should be considered an error too.  However, existing code
in the test plugin already does this, which puzzles me.

Can someone compare them and tell which is the right one?

> >GRUB_MOD_INIT(test)
> >{
> >  (void)mod;                 /* To stop warning. */
> >  grub_register_command ("[", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE,
> >                      "[ EXPRESSION ]", "Evaluate an expression", 0);
> >  grub_register_command ("test", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE,
> >                      "test EXPRESSION", "Evaluate an expression", 0);
> >}
> 
> I understand this register commands. I suppose this information is read
> from the help command or it isn't ?

Yes.  Also with "test --help".

> Or maybe it also reads from:
> static const struct grub_arg_option options ?

That too.

> The question is if the user will see the -e, -f or other options when
> querying the test command help or not ?

Yes, based on options[].  But you should really test that and not blindly
believe me :-)

> diff -urN grub2_2007_05_31_original/commands/test.c 
> grub2_2007_05_31/commands/test.c
> --- grub2_2007_05_31_original/commands/test.c 2005-11-13 16:47:08.000000000 
> +0100
> +++ grub2_2007_05_31/commands/test.c  2007-06-01 12:13:11.000000000 +0200
> @@ -24,32 +24,64 @@
>  #include <grub/misc.h>
>  #include <grub/mm.h>
>  #include <grub/env.h>
> +#include <grub/file.h>
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    {"file", 'e', 0, "Test if a file exists", 0, 0},

Do we want a long option here?  bash/coreutils don't have any.

-- 
Robert Millan

My spam trap is address@hidden  Note: this address is only intended
for spam harvesters.  Writing to it will get you added to my black list.