Re: status update for grub 2 developments?

[Marco Gerards <mgerards@xs4all.nl>]

Bean <bean123@126.com> writes:

> On Sun, Jun 24, 2007 at 12:22:18PM +0800, Bean wrote:
>> Some bugs I found on scripting.
>> 
>> 1. token parser
>> 
>> echo aa"bb"
>> aabb
>> (correct)
>> 
>> echo aa"bb"cc
>> aabb
>> (cc is lost)
>> 
>> echo aa$prefix
>> aa (hd0,1)/boot/grub
>> (should be one token)
>> 
>> echo $prefix/grub.cfg
>> (hd0,1)/boot/grub /grub.cfg
>> (should be one token)
>> 
>> The problem here is that when a variable is mixed with text, the token breaks.
>> I think this is also the reason why set doesn't work with variable.

Yes, this is a problem in the parser.

>> set AA=1
>> set BB=$AA
>> 
>> will expand to
>> 
>> set AA=1
>> set BB= 1
>> 
>> Thereforce the value of BB is empty instead of 1.
>> 
>
> I fix the token parser problem, changes include:
>
> 1. Inside grub_script_yylex2, mixed text is detected, and transformed into
> arg structure instead of text.

Can you please explain how this works?  This sounds like enforcing
parser policies on the lexer.  I don't like this, because in that case
the structure would be lost.  Actually, this can and should be fixed
in the parser or properly in the lexer.

If you want to solve this in the lexer, the lexer shouldn't release a
token when a "" is encountered.

The current lexer really sucks.  Does someone know of a good lexer
generator other than flex?  Flex depends on libc :-(.  That's the
reason we have this damn ugly lexer now...

> 2. Fix a small bug that cause trash output when an undefine variable is
> referenced.
>
> 3. Close varible definition when / is encountered.
>
> With this patch, the following command works properly:
>
> echo aa"BB"cc
> aaBBcc
>
> set AA=1
> set BB=$AA
> echo $BB
> 1
>
> echo $prefix/grub.cfg
> (hd0,1)/boot/grub/grub.cfg
>
> echo ${prefix}/grub.cfg
> (hd0,1)/boot/grub/grub.cfg
>
> set AA=1
> echo aa"$AA"bb
> aa1bb
>
> echo aa${?}bb
> aa0bb

Great :-)

> -- 
> Bean <bean123@126.com>
>
> 2007-06-24  Bean  <bean123@126.com>
>
> 	* kern/parser.c (state_transitions): Add new case.

Which case?

btw, please use "diff -up" ;-)

> 	* normal/execute.c (grub_script_argument_to_string):
> 	Output empty string when variable does not exist.
>
> 	* normal/lexer.c (grub_script_yylex2): Handle mixed text
> 	properly.
>
> 	* normal/parser.y : token GRUB_PARSER_TOKEN_VAR returns arg
> 	instead of text.
>
>
> Index: kern/parser.c
> ===================================================================
> RCS file: /sources/grub/grub2/kern/parser.c,v
> retrieving revision 1.2
> diff -u -r1.2 parser.c
> --- kern/parser.c	23 Nov 2005 03:36:24 -0000	1.2
> +++ kern/parser.c	24 Jun 2007 11:23:48 -0000
> @@ -44,6 +44,7 @@
>    { GRUB_PARSER_STATE_VAR, GRUB_PARSER_STATE_VARNAME2, '{', 0},
>    { GRUB_PARSER_STATE_VAR, GRUB_PARSER_STATE_VARNAME, 0, 1},
>    { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, ' ', 1},
> +  { GRUB_PARSER_STATE_VARNAME, GRUB_PARSER_STATE_TEXT, '/', 1},
>    { GRUB_PARSER_STATE_VARNAME2, GRUB_PARSER_STATE_TEXT, '}', 0},

Which bug does this solve?
  
>    { GRUB_PARSER_STATE_QVAR, GRUB_PARSER_STATE_QVARNAME2, '{', 0},
> Index: normal/execute.c
> ===================================================================
> RCS file: /sources/grub/grub2/normal/execute.c,v
> retrieving revision 1.4
> diff -u -r1.4 execute.c
> --- normal/execute.c	28 May 2006 21:58:34 -0000	1.4
> +++ normal/execute.c	24 Jun 2007 11:23:48 -0000
> @@ -50,7 +50,8 @@
>        if (argi->type == 1)
>  	{
>  	  val = grub_env_get (argi->str);
> -	  size += grub_strlen (val);
> +	  if (val)
> +	    size += grub_strlen (val);

Good one :-)

>  	}
>        else
>  	size += grub_strlen (argi->str);
> @@ -68,7 +69,8 @@
>        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);
> Index: normal/lexer.c
> ===================================================================
> RCS file: /sources/grub/grub2/normal/lexer.c,v
> retrieving revision 1.6
> diff -u -r1.6 lexer.c
> --- normal/lexer.c	4 Jun 2006 15:56:55 -0000	1.6
> +++ normal/lexer.c	24 Jun 2007 11:23:48 -0000
> @@ -50,7 +50,7 @@
>    struct grub_lexer_param *param;
>  
>    param = grub_malloc (sizeof (*param));
> -  if (! param)
> +  if (!param)
>      return 0;

Please don't do this...
  
>    param->state = GRUB_PARSER_STATE_TEXT;
> @@ -100,7 +100,7 @@
>        if (state->recording[--state->recordpos] != '}')
>  	{
>  	  grub_printf ("Internal error while parsing menu entry");
> -	  for (;;); /* XXX */
> +	  for (;;);		/* XXX */

Can you leave this out the patch?

>  	}
>        state->recording[state->recordpos] = '\0';
>      }
> @@ -118,7 +118,7 @@
>        char *old = state->recording;
>        state->recordlen += 100;
>        state->recording = grub_realloc (state->recording, state->recordlen);
> -      if (! state->recording)
> +      if (!state->recording)

Same here.

>  	{
>  	  grub_free (old);
>  	  state->record = 0;
> @@ -137,10 +137,10 @@
>  }
>  
>  int
> -grub_script_yylex2 (YYSTYPE *yylval, struct grub_parser_param *parsestate);
> +grub_script_yylex2 (YYSTYPE * yylval, struct grub_parser_param *parsestate);

Can you leave this out?
  
>  int
> -grub_script_yylex (YYSTYPE *yylval, struct grub_parser_param *parsestate)
> +grub_script_yylex (YYSTYPE * yylval, struct grub_parser_param *parsestate)

Same here.

>  {
>    int r = -1;
>  
> @@ -154,31 +154,32 @@
>  }
>  
>  int
> -grub_script_yylex2 (YYSTYPE *yylval, struct grub_parser_param *parsestate)
> +grub_script_yylex2 (YYSTYPE * yylval, struct grub_parser_param *parsestate)
>  {
>    grub_parser_state_t newstate;
>    char use;
>    char *buffer;
>    char *bp;
>    struct grub_lexer_param *state = parsestate->lexerstate;
> +  struct grub_script_arg *arg = 0;
> +  int done = 0;
>  
>    if (state->done)
>      return 0;
>  
> -  if (! *state->script)
> +  if (!*state->script)

Please leave this out.

>      {
>        /* Check if more tokens are requested by the parser.  */
>        if ((state->refs
> -	   || state->state == GRUB_PARSER_STATE_ESC)
> -	  && state->getline)
> +	   || state->state == GRUB_PARSER_STATE_ESC) && state->getline)
>  	{
> -	  while (!state->script || ! grub_strlen (state->script))
> +	  while (!state->script || !grub_strlen (state->script))
>  	    {
>  	      grub_free (state->newscript);
>  	      state->newscript = 0;
>  	      state->getline (&state->newscript);
>  	      state->script = state->newscript;
> -	      if (! state->script)
> +	      if (!state->script)
>  		return 0;
>  	    }
>  	  grub_dprintf ("scripting", "token=`\\n'\n");
> @@ -196,163 +197,192 @@
>  	}
>      }
>  
> -  newstate = grub_parser_cmdline_state (state->state, *state->script, &use);
> -
> -  /* Check if it is a text.  */
> -  if (check_textstate (newstate))
> +  while (1)

Can you please split up the code after this, so the changes unrelated
to the argument parsing are still there.  I am not sure what to do
with the argument parsing part yet.  While I do certainly like the
other changes.  This code is hard to review if both changes are
interleaved.

> Index: normal/parser.y
> ===================================================================
> RCS file: /sources/grub/grub2/normal/parser.y,v
> retrieving revision 1.6
> diff -u -r1.6 parser.y
> --- normal/parser.y	4 Jun 2006 15:56:55 -0000	1.6
> +++ normal/parser.y	24 Jun 2007 11:23:48 -0000
> @@ -46,9 +46,9 @@
>  %token GRUB_PARSER_TOKEN_VAR
>  %type <cmd> script grubcmd command commands commandblock menuentry if
>  %type <arglist> arguments;
> -%type <arg> argument;
> +%type <arg> argument GRUB_PARSER_TOKEN_VAR;

I still have my doubts on this.  Actually, I will try to look into the
parser myself to see what needs to be done precisely.

>  %type <string> "if" "while" "function" "else" "then" "fi"
> -%type <string> text GRUB_PARSER_TOKEN_NAME GRUB_PARSER_TOKEN_VAR
> +%type <string> text GRUB_PARSER_TOKEN_NAME
>  
>  %pure-parser
>  %lex-param { struct grub_parser_param *state };
> @@ -86,7 +86,7 @@
>     for example: `foo${bar}baz'.  */
>  argument:	GRUB_PARSER_TOKEN_VAR
>  		  {
> -		    $$ = grub_script_arg_add (state, 0, GRUB_SCRIPT_ARG_TYPE_VAR, $1);
> +		    $$ = $1;
>  		  }

Same here, of course.

--
Marco