Re: [PATCH] Bug fix for parser

[Marco Gerards <mgerards@xs4all.nl>]

Bean <bean123ch@gmail.com> writes:

> On Jan 14, 2008 2:42 AM, Marco Gerards <mgerards@xs4all.nl> wrote:
>> > @@ -99,6 +101,9 @@ grub_script_execute_cmdline (struct grub_script_cmd *cmd)
>> >    grubcmd = grub_command_find (cmdline->cmdname);
>> >    if (! grubcmd)
>> >      {
>> > +      /* Ignore errors.  */
>> > +      grub_errno = GRUB_ERR_NONE;
>> > +
>>
>> What are the implications of this?
>
> here is the function :
>   /* Lookup the command.  */
>   grubcmd = grub_command_find (cmdline->cmdname);
>   if (! grubcmd)
>     {
>       /* Ignore errors.  */
> +     grub_errno = GRUB_ERR_NONE;
>
>       /* It's not a GRUB command, try all functions.  */
>       func = grub_script_function_find (cmdline->cmdname);
>       if (! func)
> 	{
>
> if the command is not found, grub_errno would be set, this will effect
> the later grub_script_function_find when it try to load the module
> from disk.

Ah, ok.  Anyways, it looks fine to me in this context.

>> >        /* It's not a GRUB command, try all functions.  */
>> >        func = grub_script_function_find (cmdline->cmdname);
>> >        if (! func)
>> > diff --git a/normal/main.c b/normal/main.c
>> > index ccea447..32e649c 100644
>> > --- a/normal/main.c
>> > +++ b/normal/main.c
>> > @@ -261,6 +261,9 @@ read_config_file (const char *config, int nested)
>> >        /* Execute the command(s).  */
>> >        grub_script_execute (parsed_script);
>> >
>> > +      /* Ignore errors.  */
>> > +      grub_errno = GRUB_ERR_NONE;
>>
>> Same for this.  Errors are not shown this way, for example.
>
> if grub_error is set, it will effect the parser, caused menu not to be
> showed, etc.

Right, but do you want to see a menu if not everything is correctly executed?

>>
>> >        /* The parsed script was executed, throw it away.  */
>> >        grub_script_free (parsed_script);
>> >      }
>> > diff --git a/normal/parser.y b/normal/parser.y
>> > index 19cd1bd..8771773 100644
>> > --- a/normal/parser.y
>> > +++ b/normal/parser.y
>> > @@ -43,7 +43,7 @@
>> >  %token GRUB_PARSER_TOKEN_FI          "fi"
>> >  %token GRUB_PARSER_TOKEN_NAME
>> >  %token GRUB_PARSER_TOKEN_VAR
>> > -%type <cmd> script grubcmd command commands commandblock menuentry if
>> > +%type <cmd> script script_1 grubcmd command commands commandblock menuentry if
>> >  %type <arglist> arguments;
>> >  %type <arg> argument;
>> >  %type <string> "if" "while" "function" "else" "then" "fi"
>> > @@ -55,12 +55,22 @@
>> >
>> >  %%
>> >  /* It should be possible to do this in a clean way...  */
>> > -script:              { state->err = 0} newlines commands
>> > +script:              { state->err = 0} script_1
>> >                 {
>> > -                 state->parsed = $3;
>> > +                 state->parsed = $2;
>> >                 }
>> >  ;
>> >
>> > +script_1:    commands { $$ = $1; }
>> > +             | function '\n' { $$ = 0; }
>> > +             | menuentry '\n' { $$ = $1; }
>> > +;
>>
>> I do not like the name "script_1".
>
> what do you suggest ?

Hopefully someone else knows a better name? :-)

>>
>> Looks fine to me at first sight.
>>
>> >  /* A function.  Carefully save the memory that is allocated.  Don't
>> > @@ -194,7 +194,6 @@ function: "function" GRUB_PARSER_TOKEN_NAME
>> >  commandblock:        '{'
>> >                 {
>> >                   grub_script_lexer_ref (state->lexerstate);
>> > -                    grub_script_lexer_record_start (state->lexerstate);
>>
>> Ehm?  Can you explain this?  If I am not mistaken, this will result in
>> a memory leak.
>
> I split commandblock from menuentry, now it's a standalone statement.

Ah ok, so there is a grub_script_lexer_record_start elsewhere that
forfills the same role?

> commandblock:	'{'
> 		  {
> 		    grub_script_lexer_ref (state->lexerstate);
> 		  }
>                 newlines commands '}'
>                   {
> 		    grub_script_lexer_deref (state->lexerstate);
> 		    $$ = $4;
> 		  }
>
>
>>
>> >                 }
>> >                  newlines commands '}'
>> >                    {
>> > @@ -204,10 +203,17 @@ commandblock:   '{'
>> >  ;
>> >
>> >  /* A menu entry.  Carefully save the memory that is allocated.  */
>> > -menuentry:   "menuentry" argument newlines commandblock
>> > +menuentry:   "menuentry" argument
>> > +               {
>> > +                 grub_script_lexer_ref (state->lexerstate);
>> > +               } newlines '{'
>> > +               {
>> > +                 grub_script_lexer_record_start (state->lexerstate);
>> > +               } newlines commands '}'
>>
>>
>> What was the problem here?
>
> I don't like the idea of deref in another statement, and now we can
> use {} separately.

Of deref?  Can you explain?  The idea of commandblock was that this
structure might show up more often.  But I have no objections for now
if a bug is fixed this way.

Btw, does this patch incorporate the previous patch you sent in
regarding scripting?

--
Marco