Re: test -e patch

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:

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

[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

[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