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