Re: test -e patch

[Marco Gerards]

adrian15 <address@hidden> writes:

> Attached you will find the patch adding test -e support for grub2.
>
> This is my first patch. I have compiled it without no errors.

Urgh... I thought/hoped I told you I had a test.c rewrite sitting on
my harddisk?  Or did I tell Robert to poke me until next weekend so I
will work on it?  It includes everything you expect from test.c,
expect the cleanup and testing. ;-)

> 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) I have not tested it.

Please have a look at the wiki.  It has quite some information about
GRUB 2.

> I have applied the GNU Coding Standards Style on the patch although the
> copy-pasted code that you will find in the mail body has not the GCS
> applied.
>
> Here there are some doubts:
>
>    > 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 means that this is the last option.  There is no other way to tell
if this is the last option without this.

> Should I write "Test if a file exists" instead of "test if a file
> exists" or "FILE exists"?

FILE


>>
>> static void
>> test_file_exists (const char *key)
>
> Do you prefer another name like: file_exists_test ?
>
>> {
>>      grub_file_t file;
>>
>>       file = grub_file_open (key);
>>       if (file)
>>      {
>>        grub_printf (" %s", key);
> Should I prompt the existent file or not?
> Should I prompt %s exists maybe ?
> Should I prompt an \n also?
>>        grub_file_close (file);
>>        grub_errno = GRUB_ERR_NONE;
>>      }
>>       else
>>      {
>>        grub_error (GRUB_ERR_FILE_NOT_FOUND, "File does not exists.");
>
> 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 ?

Right.  Otherwise the error will be lost.

> What are the error polices ?

Returning the err_t that grub_error returns when possible.

>>      }
>> }
>>
>>
>> static grub_err_t
>> grub_cmd_test (struct grub_arg_list *state __attribute__ ((unused)), int 
>> argc,
>>             char **args)
>>
>> {
>>
>>   char *eq;
>>   char *eqis;
>>
>>   if (state[0].set)
>
> I suppose this refers to the first line of static const struct
> grub_arg_option options and thus as -e is the first option and if its
> set we should run the correspondent function.

Yes.

> One question. Should we put the test-e function into a separate file or
> not ?

No, the problem is that the design of test.c (which is just a
placeholder) is wrong.  It needs a proper parser for the arguments and
a way to deal with this...

>>     test_file_exists (args[0]);
>>   else
>>   {
>>     /* XXX: No fancy expression evaluation yet.  */
>>       if (argc == 0)
>>       return 0;
>>       eq = grub_strdup (args[0]);
>>     eqis = grub_strchr (eq, '=');
>>     if (! eqis)
>>       return 0;
>>
>>     *eqis = '\0';
>>     eqis++;
>>     /* Check an expression in the form `A=B'.  */
>>     if (grub_strcmp (eq, eqis))
>>       grub_error (GRUB_ERR_TEST_FAILURE, "false");
>>     grub_free (eq);
>>   }
>>     return grub_errno;
>>
>>
>> }
>>
>>
>> 
>> 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, the help command uses this information.

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

Help only does when you use "help test".  So it can use both.

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

They should.  But I am not sure if the final version will support
this.  Especially because of the nested syntax of the test arguments.

>>
>> GRUB_MOD_FINI(test)
>> {
>>   grub_unregister_command ("[");
>>   grub_unregister_command ("test");
>> }
>
> That's all for my first compiled patch.
> I hope that Marco_g is happy :).
> But you know Marco_g I will continue my non-sense grub legacy
> development too. ;)

:-))

> adrian15
>
> 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},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +static void
> +test_file_exists (const char *key)

Why not filename?

> +{
> +  grub_file_t file;
> +
> +  file = grub_file_open (key);
> +  if (file)
> +    {
> +      grub_printf (" %s", key);
> +      grub_file_close (file);
> +      grub_errno = GRUB_ERR_NONE;
> +    }
> +  else
> +    {
> +      grub_error (GRUB_ERR_FILE_NOT_FOUND, "File does not exists.");
> +    }
> +}
> +
>  
>  static grub_err_t
>  grub_cmd_test (struct grub_arg_list *state __attribute__ ((unused)), int 
> argc,
>              char **args)
>  
>  {
> +

You accidently introduced a whiteline.

>    char *eq;
>    char *eqis;
>  
> -  /* XXX: No fancy expression evaluation yet.  */
> +  if (state[0].set)
> +    test_file_exists (args[0]);
> +  else
> +  {

This means that this check is run for any other expression.  This is
quite error sensitive.

> +    /* XXX: No fancy expression evaluation yet.  */
>    
> -  if (argc == 0)
> -    return 0;
> +    if (argc == 0)
> +      return 0;
> +  
> +    eq = grub_strdup (args[0]);
> +    eqis = grub_strchr (eq, '=');
> +    if (! eqis)
> +      return 0;
> +
> +    *eqis = '\0';
> +    eqis++;
> +    /* Check an expression in the form `A=B'.  */
> +    if (grub_strcmp (eq, eqis))
> +      grub_error (GRUB_ERR_TEST_FAILURE, "false");
> +    grub_free (eq);
> +  }
>    
> -  eq = grub_strdup (args[0]);
> -  eqis = grub_strchr (eq, '=');
> -  if (! eqis)
> -    return 0;
> -
> -  *eqis = '\0';
> -  eqis++;
> -  /* Check an expression in the form `A=B'.  */
> -  if (grub_strcmp (eq, eqis))
> -    grub_error (GRUB_ERR_TEST_FAILURE, "false");
> -  grub_free (eq);
> -
>    return grub_errno;
>  }


--
Marco