Re: test -e patch

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 honeypot@aybabtu.com.  Note: this address is only intended
for spam harvesters.  Writing to it will get you added to my black list.

Re: test -e patch

[Robert Millan]

On Wed, Jun 06, 2007 at 01:42:48AM +0200, adrian15 wrote:
> 
> 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.

Btw, you can also test it with grub-emu.

-- 
Robert Millan

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

Re: test -e patch

[adrian15]

>> > 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.
> 
> Btw, you can also test it with grub-emu.

What are the differences between grub-emu and the grub legacy's grub shell ?

An example of how to use it?

adrian15

Re: test -e patch

[adrian15]

> 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
I would like to have a floppy with the fs on it. See my other mail about
my floppy creation adventure.
> 
>>    > 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.
Ok. I understand then.
> 
>>> {
>>> 	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?
I have not tried a device node, but with a directory it fails.

>  Perhaps grub_file_stat would make more sense
You might be right but why then you (grub2 developers) have developed
the search command with the help of grub_file_open ??

Should the search command be reimplemented with grub_file_stat ?

>> What are the error polices ?
> 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.
We will have to wait for marco_g for telling us how it works, then.
> 
>>> 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 :-)
I've checked that it does not read from struct grub_arg_option options ,
strange thing.

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

I do not want a long option :)

adrian15

Re: test -e patch

[adrian15]

>> 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 ?
> 
> Or maybe it also reads from:
> static const struct grub_arg_option options ?

I've found it.

It reads from the register command and ALSO from options IF you tell it
to do so with:

   (void)mod;			/* To stop warning. */
   grub_register_command ("[", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE,
			 "[ EXPRESSION ]", "Evaluate an expression", options);
   grub_register_command ("test", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE,
			 "test EXPRESSION", "Evaluate an expression", options);


adrian15

Re: test -e patch

[Marco Gerards]

adrian15 <adrian15@raulete.net> 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;
>>
>>
>> }
>>
>>
>> \f
>> 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

Re: test -e patch

[Marco Gerards]

adrian15 <adrian15@raulete.net> writes:

>>> 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 ?
>>
>> Or maybe it also reads from:
>> static const struct grub_arg_option options ?
>
> I've found it.
>
> It reads from the register command and ALSO from options IF you tell it
> to do so with:
>
>   (void)mod;			/* To stop warning. */
>   grub_register_command ("[", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE,
> 			 "[ EXPRESSION ]", "Evaluate an expression", options);
>   grub_register_command ("test", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE,
> 			 "test EXPRESSION", "Evaluate an expression", options);

If you don't pass the options to this functions, the options won't be
parsed.  One fun thing you can try: options can be tab-completed in
GRUB 2 :-)

--
Marco

Re: test -e patch

[adrian15]

> adrian15 <adrian15@raulete.net> 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.  ;-) 

Do you mean you also have the '-e' option ?


> Please have a look at the wiki.  It has quite some information about
> GRUB 2.
Whenever possible I'll download some info from the wiki.

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

FILE
or
FILE exists ?

Or have you coded it yourself too?

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

Ok. I will try that every function propagates errors.

> 
>> > What are the error polices ?
> 
> Returning the err_t that grub_error returns when possible.
Ok.
>> > 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...

Ok. We will wait for your code.

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

Do you mean the -e options support
or
do you mean the -e options showing at help test support ?

>> > +static void
>> > +test_file_exists (const char *key)
> 
> Why not filename?

test_filename_exists
or
filename

?

>> >  {
>> > +
> 
> You accidently introduced a whiteline.

No whitelines after an initial {. I write down it.

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

In my code the only implemented option is '-e'. When there will be more
I could add more nested if with the other options, or maybe better we
will enjoy your improved code.


adrian15

Re: test -e patch

[Marco Gerards]

adrian15 <adrian15@raulete.net> writes:

>> adrian15 <adrian15@raulete.net> 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.  ;-)
>
> Do you mean you also have the '-e' option ?

I mean I have everything that is possible.  That includes `-e', but
also all other features.

Something noone looked at is "expr".

>> Please have a look at the wiki.  It has quite some information about
>> GRUB 2.
> Whenever possible I'll download some info from the wiki.
>
>>> > Should I write "Test if a file exists" instead of "test if a file
>>> > exists" or "FILE exists"?
>>
>> FILE
>
> FILE
> or
> FILE exists ?

FILE exists

> Or have you coded it yourself too?

Hm?

[...]

>> 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...
>
> Ok. We will wait for your code.

:-)

Sorry for the confusion :-/

>>> > 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.
>
> Do you mean the -e options support
> or
> do you mean the -e options showing at help test support ?

Well, the version for GNU/Linux doesn't show help text.  Perhaps it
cannot be implemented using the argument parser in a clean way.  I do
not remember.

>>> > +static void
>>> > +test_file_exists (const char *key)
>>
>> Why not filename?
>
> test_filename_exists
> or
> filename

I mean instead of key.

> ?
>
>>> >  {
>>> > +
>>
>> You accidently introduced a whiteline.
>
> No whitelines after an initial {. I write down it.

Well, usually adding whitespaces around code you didn't change is
wrong or dirty.

>>> > +  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.
>
> In my code the only implemented option is '-e'. When there will be more
> I could add more nested if with the other options, or maybe better we
> will enjoy your improved code.

Sure.  Just poke me a lot during the weekend on IRC ;-)

--
Marco

test -e patch

[adrian15]

[-- Attachment #1: Type: text/plain, Size: 3441 bytes --]

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

Could you please test it?

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 ?

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

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

What are the error polices ?

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


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

>     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;
> 
> 
> }
> 
> 
> \f
> 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 ?

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

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



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


[-- Attachment #2: test_e.patch --]
[-- Type: text/x-patch, Size: 1839 bytes --]

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

Re: gcs doubt #1

[adrian15]

>>> If you can provide a patch for sourcecode to improve or add comments
>>> in a sane way, I am willing to commit those patches.  Just please
>>> don't make the mistake to add useless comments.
>> An useful comment is one that:
>> 	-Describes a function algorithm
>> 	-Describes what a function does (as a black box)
>> 	-Boot of them ?
> 
> Yes, but I oppose to adding obvious comments.  But patches will be
> reviewed, so you will learn fast enough what we want :-).

You had to choose one of the three options :).
Well... it doesn't matter. I've understood that I have to send patches
and learn by trial-error.

adrian15

Re: test -e patch

[adrian15]

>>> adrian15 <adrian15@raulete.net> writes:
>>>
>>>>> Attached you will find the patch adding test -e support for grub2.
>> Do you mean you also have the '-e' option ?
> 
> I mean I have everything that is possible.  That includes `-e', but
> also all other features.
Ok.
> 
>>>>> +static void
>>>>> +test_file_exists (const char *key)
>>> Why not filename?
>> test_filename_exists
>> or
>> filename
> 
> I mean instead of key.

You are right. It was a not clean copy-paste from search command.

adrian15

Re: grub2 miscelanea questions (1/2)

[adrian15]

>> 3) Why any grub2.cfg file (I suppose it's the substitute for menu.lst)
>> is inside the grub2 source code?
> 
> What do you mean?

Usually a program should include examples on how to use their files and
document examples and so on... why isn't there any grub2.cfg ?

>> 4) Why any doc folder is inside the grub2 source code?
> 
> There is?  We need documentation...
:) I might work on this too... but as an unofficial documentation. I do
not like the rigid official documentations.
>  	
>> 5) grub2 lacks
>> ==================
>> 5.1) pager
>> -------------
>>
>> When I want to do a big ls the output does not stop screen by screen...
>> Is there any hidden pager that I do not know?
>> Maybe is not implemented yet?
> 
> set pager=1
> 
> It just isn't documented :-/

We should be able to prompt with help command (help variables perhaps?)
these kind of tricks or pseudocommands.

> 
>> 5.2) Chainload DEVICES
>> ------------------------
>>
>> How the hell do you boot windows?
>> Maybe from a file that has inside it the first 512 bytes from the
>> Windows partition?
> 
> chainload (hd0,1)+
> boot

I've checked that this works:
chainloader (hd0,1)+1
boot

>> 6) Compulsory Argument dilemma
>> ================================	
>> If I run help I can seen things like this:
>> 	
>> ls [-f|-l|-s] NAME
>>
>> and
>>
>> [ EXPRESSION ]
> 
> Yes?
> 
>> If [ ] means optional it should also mean optional in the case of the
>> test alias [ .!!!
> 
> What do you mean?

The [] has not a single meaning in this program.
	If it's written after a command in the help description it means: Optional.
	If it's used as a program it means that it runs the test command.

It might be confusing for some users I suppose.

Why did not use to new bash syntax? $((variable))

> 
>> 7) set help incomplete
>> ========================
>> Set help should say that if no argument is set then it shows all the
>> environment variables.
> 
> Why would it?  Only using set shows all env. variables.
Because it is a hidden feature.
If you do not how grub does work and type "help set"
it shows "Set an environment variable".

The user is unable to know that "Only using set shows all env.
variables" because it isn't written anywhere.

>> 9) help does not complain when no command help is available
>> =============================================================
> How it currently works is looking at the first characters.  For
> example try "help l".

I know... but if the first characters do not match with any command
beginning I think an error should be prompted. Don't you think so?

> 
>> 10) help --all
>> =================
>> Do we need this functionality in grub2?
> 
> Are there hidden commands for which this is important?  In that case,
> yes.

The hipotetical dd command. :)
> 
>> 12) ls and -h argument
>> ========================
>>
>> This is not an error but it curious. You can ask for the help of command
>> with this:
>> command -h
>> however
>> ls -h
>> works in a different way.
>>
>> I suppose we may have the same problem with '-u' in the future.
> 
> Ehm, I am not sure.  You mean `-h' is short for help and we should
> remove it?

I mean that if an user gets used to use the "-h" option for getting grub
commands help then when using ls, he will be confused a little.

adrian15

Re: grub2 miscelanea questions (2/2)

[adrian15]

>> 19) rescue and exit failure on qemu
>> ==============================
>> I think that booting from my computer does not give this error.
>> If I boot from QEMU and type:
>>
>> grub> rescue
>> and then :
>> grub rescue> exit
>>
>> it says: FATAL: INT18: BOOT FAILURE
> 
> Weird.  Can you debug this?

I've checked this again.
If I do this booting from my pc I get the grub2 floppy booting again.
If I do this from qemu the FATAL: INT18: BOOT FAILURE appears again.
I would bet that it is a qemu bios's lack of the 18 interruption.
>> 20) grub rescue help
>> ========================
>>
>> help : Types the commands and a minimal explanation: OK
>> help any_command : Types the commands and a minimal explanation: NOT OK
>> help not_a_command : Types the commands and a minimal explanation: NOT OK
> 
> huh?

You boot grub2 and you see
grub>
you type rescue and you see
grub rescue>
and now what it is funny that if you type the help command, an space,
and whatever you want to write it always shows all the commands help.

It may confuse some users as "Grub is not working".

> 
>> 21) grub rescue exit help
>> ===========================
>> When you are in grub rescue mode and you type help the exit line it's
>> the following one:
>>
>> exit	exit from GRUB
>>
>> Should it not prompt ?
>>
>> exit	exit from GRUB rescue
> 
> The exit command terminates GRUB and continues the boot process, IIRC.
You are right. Exit from GRUB is right, then.

> 
>> 22) copyright message missing
>> ================================
>> Why isn't there (when Grub is running) any message about the GRUB
>> Copyright and the GPL license and that you should have received and this
>> and that?
> 
> Why should there be such message?
This comes from GPL 2:

If the program is interactive, make it output a short notice like this
when it starts in an interactive mode:

     Gnomovision version 69, Copyright (C) year  name of author
     Gnomovision comes with ABSOLUTELY NO WARRANTY; for details type
`show w'.
     This is free software, and you are welcome to redistribute it
     under certain conditions; type `show c' for details.

>> Maybe we should implement an 'about' command and run it when booting :) ?
> Perhaps, but I don't see the added value.
See above about the GPL.
> 
>> 23) set read only variables
>> ==============================
>> unset prefix lets me "delete" the prefix variable.
>> unset root does not let me the "root" variable.
>>
>> I remember that bash has an option to set read only variables.
>> Maybe the root variable is a read only variable?
> 
> You can easily modify the code.  But I do not like making such
> variables read-only.

I did not want to make any variable read only. I only wanted to make you
know that the "root" variable was actually a read only variable because
there is no way to delete (unset) it. It always comes to live again.

Is it a normal thing?

adrian15