grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Bug fix for parser


From: Bean
Subject: Re: [PATCH] Bug fix for parser
Date: Mon, 14 Jan 2008 03:18:03 +0800

On Jan 14, 2008 2:42 AM, Marco Gerards <address@hidden> 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.


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

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

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

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.

-- 
Bean




reply via email to

[Prev in Thread] Current Thread [Next in Thread]