[PATCH] Bug fix for parser

[PATCH] Bug fix for parser

[Bean]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

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.

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.

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.

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

-- 
Bean

[-- Attachment #2: grub2-parser-new2.diff --]
[-- Type: application/octet-stream, Size: 6236 bytes --]

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.

	* normal/parse.y (script): Changed.
	(script_1): New.
	(delimiter): New.
	(command): Changed.
	(commands): Changed.
	(commandblock): Changed.
	(menuentry): Changed. 
	(if): 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;
+
       /* 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;
+
       /* 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; }
+;
+
+delimiter:	'\n'
+		| ';'
+		| delimiter '\n'
+;
+
 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;
+		  }
 ;
 
 /* 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;
-		  }
 ;
 
 /* 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);
 		  }
                 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 '}'
 		  {
 		    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);
 		  }
 ;

Re: [PATCH] Bug fix for parser

[Marco Gerards]

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

Re: [PATCH] Bug fix for parser

[Bean]

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.


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

Re: [PATCH] Bug fix for parser

[Marco Gerards]

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

Re: [PATCH] Bug fix for parser

[Bean]

> > 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 problem is that it's not always an error when grub_errno is set,
for example, the test command use it to report whether or not two
strings equals.

> >> I do not like the name "script_1".
> >
> > what do you suggest ?
>
> Hopefully someone else knows a better name? :-)

how about script_init for the old script, and script here.

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

yes, the grub_script_lexer_record_start is in menuentry.

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

in the old implementation,

commandblock:	'{'
		  {
		    grub_script_lexer_ref (state->lexerstate);
		  }
                newlines commands '}'
                  {
		    grub_script_lexer_deref (state->lexerstate);
                    grub_script_lexer_record_start (state->lexerstate);
		    $$ = $4;
		  }
;

menuentry:	"menuentry" argument newlines commandblock
		  {
		    char *menu_entry;
		    menu_entry = grub_script_lexer_record_stop (state->lexerstate);
		    $$ = grub_script_create_cmdmenu (state, $2, menu_entry, 0);
		  }

it calls grub_script_lexer_record_start in commandblock, and
grub_script_lexer_record_stop in menuentry, but now it's both inside
menuentry. i think it's better this way. besides, if you use
commandblock alone in old implemenation, it will cause problem becuase
there is no matching grub_script_lexer_record_stop.

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

I didn't include the fix on the lexer, for example. ${AA}${BB} should
expand  to one token instead of two. I think it's better to use a
separate patch.

-- 
Bean

Re: [PATCH] Bug fix for parser

[Marco Gerards]

Bean <bean123ch@gmail.com> writes:

>> > 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 problem is that it's not always an error when grub_errno is set,
> for example, the test command use it to report whether or not two
> strings equals.

Agreed.

>> >> I do not like the name "script_1".
>> >
>> > what do you suggest ?
>>
>> Hopefully someone else knows a better name? :-)
>
> how about script_init for the old script, and script here.

Fine for me.  Besides that, we can always change this and in the
meanwhile these names will do.

>> >> >  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?
>
> yes, the grub_script_lexer_record_start is in menuentry.

Good :-)

>> >> 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.
>
> in the old implementation,
>
> commandblock:	'{'
> 		  {
> 		    grub_script_lexer_ref (state->lexerstate);
> 		  }
>                 newlines commands '}'
>                   {
> 		    grub_script_lexer_deref (state->lexerstate);
>                     grub_script_lexer_record_start (state->lexerstate);
> 		    $$ = $4;
> 		  }
> ;
>
> menuentry:	"menuentry" argument newlines commandblock
> 		  {
> 		    char *menu_entry;
> 		    menu_entry = grub_script_lexer_record_stop (state->lexerstate);
> 		    $$ = grub_script_create_cmdmenu (state, $2, menu_entry, 0);
> 		  }
>
> it calls grub_script_lexer_record_start in commandblock, and
> grub_script_lexer_record_stop in menuentry, but now it's both inside
> menuentry. i think it's better this way. besides, if you use
> commandblock alone in old implemenation, it will cause problem becuase
> there is no matching grub_script_lexer_record_stop.

For now your approach is ok.  But perhaps I will reintroduce this
properly some day.  We'll see :-)

>> Btw, does this patch incorporate the previous patch you sent in
>> regarding scripting?
>
> I didn't include the fix on the lexer, for example. ${AA}${BB} should
> expand  to one token instead of two. I think it's better to use a
> separate patch.

Right.  I am completely unhappy with the lexer as it is now... :-/
The main reason for writing it manually is that flex depends on all
kinds of stuff I do not want in GRUB.  Like reading files, etc.

Thanks a lot for fixing bugs related to scripting.  Lately I am rather
busy and I was the only person who worked on it.  More testing, better
error handling, etc is appriciated.

I still have an unfinished "test" command on my HD.

--
Marco

Re: [PATCH] Bug fix for parser

[Bean]

On Jan 15, 2008 8:41 PM, Marco Gerards <mgerards@xs4all.nl> wrote:
> Right.  I am completely unhappy with the lexer as it is now... :-/
> The main reason for writing it manually is that flex depends on all
> kinds of stuff I do not want in GRUB.  Like reading files, etc.
>
> Thanks a lot for fixing bugs related to scripting.  Lately I am rather
> busy and I was the only person who worked on it.  More testing, better
> error handling, etc is appriciated.
>
> I still have an unfinished "test" command on my HD.

thanks, if there is no problem, i would commit this now.

-- 
Bean

Re: [PATCH] Bug fix for parser

[Marco Gerards]

Bean <bean123ch@gmail.com> writes:

> On Jan 15, 2008 8:41 PM, Marco Gerards <mgerards@xs4all.nl> wrote:
>> Right.  I am completely unhappy with the lexer as it is now... :-/
>> The main reason for writing it manually is that flex depends on all
>> kinds of stuff I do not want in GRUB.  Like reading files, etc.
>>
>> Thanks a lot for fixing bugs related to scripting.  Lately I am rather
>> busy and I was the only person who worked on it.  More testing, better
>> error handling, etc is appriciated.
>>
>> I still have an unfinished "test" command on my HD.
>
> thanks, if there is no problem, i would commit this now.

If you mean your patch, that's fine for me.  Please do not forget to
rename script_1.  Are there any other issues I brought up?

--
Marco

Re: [PATCH] Bug fix for parser

[Bean]

On Jan 15, 2008 10:33 PM, Marco Gerards <mgerards@xs4all.nl> wrote:
> Bean <bean123ch@gmail.com> writes:
>
>
> > On Jan 15, 2008 8:41 PM, Marco Gerards <mgerards@xs4all.nl> wrote:
> >> Right.  I am completely unhappy with the lexer as it is now... :-/
> >> The main reason for writing it manually is that flex depends on all
> >> kinds of stuff I do not want in GRUB.  Like reading files, etc.
> >>
> >> Thanks a lot for fixing bugs related to scripting.  Lately I am rather
> >> busy and I was the only person who worked on it.  More testing, better
> >> error handling, etc is appriciated.
> >>
> >> I still have an unfinished "test" command on my HD.
> >
> > thanks, if there is no problem, i would commit this now.
>
> If you mean your patch, that's fine for me.  Please do not forget to
> rename script_1.  Are there any other issues I brought up?

ok, commited.

-- 
Bean

Re: [PATCH] Bug fix for parser

[Marco Gerards]

Bean <bean123ch@gmail.com> writes:

[...]

>> > thanks, if there is no problem, i would commit this now.
>>
>> If you mean your patch, that's fine for me.  Please do not forget to
>> rename script_1.  Are there any other issues I brought up?
>
> ok, commited.

Thanks! :-)

--
Marco