Re: [PATCH] Bug fix for parser

[Marco Gerards <mgerards@xs4all.nl>]

Bean <bean123ch@gmail.com> 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  <bean123ch@gmail.com>
>
> 	* 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
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel