Re: Scripting support (PATCH)

[Marco Gerards]

Hollis Blanchard <address@hidden> writes:

> I don't have many comments, since I'm not familiar with lexing and
> parsing. Just a couple small comments:
>
> On Oct 29, 2005, at 4:41 PM, Marco Gerards wrote:
>>
>> `if' accepts any command.  I showed the example of using the `['
>> command.  It can be made similar to the `[' command of coreutils.
>> This command sets the environment variable `RESULT' to either 0 or 1.
>> After execution `if' checks `RESULT' and either executes the `if' or
>> the `else' branch.  It would also be possible to use a return value
>> instead of `RESULT', but that was too much trouble for me at first.
>> But of course this can be changed if someone can convince me of that.
>
> You want it to be "$RESULT" instead of "$?" ?

You are right.

> I really don't like that each command has to explicitly set RESULT. As
> you note, it would be better if the return code from the command were
> automatically placed into the status environment variable.

Most command return grub_err_t.  The only commands that matter for us
are commands like `['.  Would you propose every commands returns an
int and that on function return grub_errno is checked?

>> +#ifdef GRUB_UTIL
>> +void
>> +grub_lsb_init (void)
>> +{
>> +  grub_register_command ("[", grub_cmd_lsb, GRUB_COMMAND_FLAG_CMDLINE,
>> +                     "[ EXPRESSION ]", "Evaluate an expression", 0);
>> +}
>> +
>> +void
>> +grub_lsb_fini (void)
>> +{
>> +  grub_unregister_command ("[");
>> +}
>> +#else /* ! GRUB_UTIL */
>> +GRUB_MOD_INIT
>> +{
>> +  (void)mod;                        /* To stop warning. */
>> +  grub_register_command ("[", grub_cmd_lsb, GRUB_COMMAND_FLAG_CMDLINE,
>> +                     "[ EXPRESSION ]", "Evaluate an expression", 0);
>> +}
>> +
>> +GRUB_MOD_FINI
>> +{
>> +  grub_unregister_command ("[");
>> +}
>> +#endif /* ! GRUB_UTIL */
>
> We *really* need to redefine GRUB_MOD_INIT/FINI to remove all this
> duplicated code. I guess I will add that to my list.

Cool :-)

>> +/* A part of an argument.  */
>> +struct grub_script_arg
>> +{
>> +  /* If this is 0, STR is a string.  If it is one, STR is a variable
>> +     name.  */
>> +  int type;
>
> This should probably be an enum.

Yes, good catch.

> Since this code won't break any existing behavior (simple "ls (hd,0)/"
> will still work, right?), I guess it can be committed as soon as
> serious issues like the memory leak have been fixed.

That is right.  All normal code will continue to function, otherwise
it would be a bug or have been discussed on the list.  So far there is
nothing that would break the current behavior except multiple lines
(which I should fix before committing).

--
Marco