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: Marco Gerards
Subject: Re: [PATCH] Bug fix for parser
Date: Sun, 13 Jan 2008 19:42:29 +0100
User-agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)

Bean <address@hidden> writes:

Hi,

> I write a patch for parser related bug, it fix the following problems:
>
> 1, major modification for parse.y, now it should work better. For example,
>
> if echo aa; echo bb; then echo cc; fi
>
> it works properly now.

Nice :-)

> 2, Check for undefined variable. for example, if AA is not defined,
>
> echo $AA
>
> caused problem in the old version. Now it shows blank line.

Ah, good :)

> 3, This following commands:
>
> function aa { echo bb; }
> aa
>
> Shows
>
> bb
> error: unknown command `aa'
>
> Now it is ok.

:-)

> 4, If an command return error in grub.cfg, the following content is
> not parsed, for example
>
> test A=B
> menuentry aa {
> set root=(hd0,1)
> chainloader +1
> }
>
> The menu is not parsed as test A=B set errno to 1.
>
> This bug can be quite tricky, for example, i remember someone report
> that update-grub has problem as the menu is not showed at all, but in
> fact, the problem is caused by the font command.

How did you deal with this?  It's hard to determine if errors should
mean the script is stopped or should continue.

> 5, echo module not included in command.lst
> yes, this is problem is very old, but nobody seems to be fixing it.

:-)

> -- 
> Bean
>
> 2007-12-31  Bean  <address@hidden>
>
>       * normal/execute.c (grub_script_exec_argument_to_string): Check for 
> undefined variable.
>       (grub_script_execute_cmdline): Reset grub_errno.
>
>       * normal/main.c (read_config_file): Reset grub_errno.

I am not sure if you do not introduce new problems this way...?

>       * normal/parse.y (script): Changed.
>       (script_1): New.
>       (delimiter): New.
>       (command): Changed.
>       (commands): Changed.
>       (commandblock): Changed.
>       (menuentry): Changed. 
>       (if): Changed.

Please describe what was changed.

>
>       * conf/common.rmk (pkgdata_MODULES): Add echo.mod.
>
>
> diff --git a/conf/common.rmk b/conf/common.rmk
> index 4773f7d..994d560 100644
> --- a/conf/common.rmk
> +++ b/conf/common.rmk
> @@ -208,7 +208,7 @@ lvm_mod_LDFLAGS = $(COMMON_LDFLAGS)
>  # Commands.
>  pkglib_MODULES += hello.mod boot.mod terminal.mod ls.mod     \
>       cmp.mod cat.mod help.mod font.mod search.mod            \
> -     loopback.mod configfile.mod                             \
> +     loopback.mod configfile.mod echo.mod                    \
>       terminfo.mod test.mod blocklist.mod hexdump.mod
>  
>  # For hello.mod.
> diff --git a/normal/execute.c b/normal/execute.c
> index 4462ddd..ab0897c 100644
> --- a/normal/execute.c
> +++ b/normal/execute.c
> @@ -49,7 +49,8 @@ grub_script_execute_argument_to_string (struct 
> grub_script_arg *arg)
>        if (argi->type == 1)
>       {
>         val = grub_env_get (argi->str);
> -       size += grub_strlen (val);
> +       if (val)
> +         size += grub_strlen (val);
>       }
>        else
>       size += grub_strlen (argi->str);
> @@ -67,7 +68,8 @@ grub_script_execute_argument_to_string (struct 
> grub_script_arg *arg)
>        if (argi->type == 1)
>       {
>         val = grub_env_get (argi->str);
> -       grub_strcat (chararg, val);
> +       if (val)
> +         grub_strcat (chararg, val);
>       }
>        else
>       grub_strcat (chararg, argi->str);
> @@ -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?

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

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

> +delimiter:   '\n'
> +             | ';'
> +             | delimiter '\n'
> +;

ok

>  newlines:    /* Empty */
>               | newlines '\n'
>  ;
> @@ -124,39 +134,29 @@ grubcmd:        GRUB_PARSER_TOKEN_NAME arguments
>  ;
>  
>  /* A single command.  */
> -command:     grubcmd         { $$ = $1; }
> -             | if            { $$ = $1; }
> -             | function      { $$ = 0;  }
> -             | menuentry     { $$ = $1; }
> +command:     grubcmd delimiter { $$ = $1; }
> +             | if delimiter  { $$ = $1; }
> +             | commandblock delimiter { $$ = $1; }
> +             | error delimiter
> +               {
> +                 $$ = 0;
> +                 yyerror (state, "Incorrect command");
> +                 state->err = 1;
> +                 yyerrok;
> +               }
>  ;

Ok.

>  /* A block of commands.  */
> -commands:    command '\n'
> -               { 
> -                 $$ = grub_script_add_cmd (state, 0, $1);
> -               }
> -             | command
> -               { 
> +commands:    command
> +               {
>                   $$ = grub_script_add_cmd (state, 0, $1);
>                 }
> -             | command ';' commands
> -               { 
> -                 struct grub_script_cmdblock *cmd;
> -                 cmd = (struct grub_script_cmdblock *) $3;
> -                 $$ = grub_script_add_cmd (state, cmd, $1);
> -               }
> -             | command '\n' newlines commands
> -               { 
> +             | command commands
> +               {
>                   struct grub_script_cmdblock *cmd;
> -                 cmd = (struct grub_script_cmdblock *) $4;
> +                 cmd = (struct grub_script_cmdblock *) $2;
>                   $$ = grub_script_add_cmd (state, cmd, $1);
>                 }
> -             | error
> -               {
> -                 yyerror (state, "Incorrect command");
> -                 state->err = 1;
> -                 yyerrok;
> -               }
>  ;

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.

>                 }
>                  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?

>                 {
>                   char *menu_entry;
>                   menu_entry = grub_script_lexer_record_stop 
> (state->lexerstate);
> +                 grub_script_lexer_deref (state->lexerstate);
>                   $$ = grub_script_create_cmdmenu (state, $2, menu_entry, 0);
>                 }
>  ;
> @@ -218,14 +224,14 @@ if_statement:   "if" { grub_script_lexer_ref 
> (state->lexerstate); }
>  ;
>  
>  /* The if statement.  */
> -if:           if_statement grubcmd ';' "then" commands "fi"
> +if:           if_statement commands "then" newlines commands "fi"
>                 {
>                   $$ = grub_script_create_cmdif (state, $2, $5, 0);
>                   grub_script_lexer_deref (state->lexerstate);
>                 }
> -              | if_statement grubcmd ';' "then" commands "else" commands  
> "fi"
> +              | if_statement commands "then" newlines commands "else" 
> newlines commands  "fi"
>                 {
> -                 $$ = grub_script_create_cmdif (state, $2, $5, $7);
> +                 $$ = grub_script_create_cmdif (state, $2, $5, $8);
>                   grub_script_lexer_deref (state->lexerstate);
>                 }
>  ;
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel





reply via email to

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