Re: [PATCH] bug fix for scripting (update 2)

[Marco Gerards]

Bean <address@hidden> writes:

> This patch fixes the following bugs:
>
> 1. Token parser
>
> echo aa"bb"cc
> old: aa"bb"
> new:  aa"bb"cc
>
> set BB=1
> echo aa$BB
> old: aa 1
> new: aa1
>
> set BB=1
> echo ${BB}aa
> old: 1 aa
> new: 1aa
>
> set AA=1
> set BB=$AA
> echo $BB
> old: empty
> new: 1
>
> AA is not defined
> echo $AA
> old: trash
> new: empty
>
> 2. function
>
> function aa {
> echo $AA
> }
> old: infinite loop
> new: ok
>
> function bb {
>   echo Hello
> }
> old: Unknown command `echo'
> new: ok
>
> BTW, the echo module needs to be enabled manually in conf/common.rmk.
>
> -- 
> Bean
> -- 
> Bean

The header of the changelog entry is missing.

>       * kern/parser.c (state_transitions): Exit GRUB_PARSER_STATE_VARNAME
>       state when "/" is encountered.
>
>       * normal/execute.c (grub_script_argument_to_string): Output empty
>       string when variable does not exist.
>
>       * normal/execute.c (grub_script_execute_cmdline): Reset error
>       number.
>
>       * normal/lexer.c (grub_script_yylex2): Return arg structure when
>       mixed text is detected.

Can you leave this part out of the patch for now?  See the text below.

>       * normal/parser.y : token GRUB_PARSER_TOKEN_VAR returns arg
>       instead of text.
>
>
> 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  25 Jun 2007 11:43:57 -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);

ok

>       }
>        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);

ok

>       }
>        else
>       grub_strcat (chararg, argi->str);
> @@ -100,6 +102,9 @@
>    grubcmd = grub_command_find (cmdline->cmdname);
>    if (! grubcmd)
>      {
> +      /* Reset error number */
> +      grub_errno = 0;
> +

Please use GRUB_ERR_NONE.  I know this isn't done consistently, but
this is what I prefer.

Why is this change needed and whih problem does it fix?

>        /* It's not a GRUB command, try all functions.  */
>        func = grub_script_function_find (cmdline->cmdname);
>        if (! func)
> 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    25 Jun 2007 11:43:58 -0000
> @@ -161,6 +161,8 @@
>    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;
> @@ -196,13 +198,14 @@
>       }
>      }
>  
> +back:
>    newstate = grub_parser_cmdline_state (state->state, *state->script, &use);
>  
>    /* Check if it is a text.  */
>    if (check_textstate (newstate))
>      {
>        /* In case the string is not quoted, this can be a one char
> -      length symbol.  */
> +         length symbol.  */
>        if (newstate == GRUB_PARSER_STATE_TEXT)
>       {
>         switch (*state->script)
> @@ -213,7 +216,7 @@
>                 newstate = grub_parser_cmdline_state (state->state,
>                                                       *state->script, &use);
>                 if (! (state->state == GRUB_PARSER_STATE_TEXT
> -                      && *state->script == ' '))
> +                         && *state->script == ' '))

Why do you change indention here?

>                   {
>                     grub_dprintf ("scripting", "token=` '\n");
>                     return ' ';
> @@ -260,8 +263,6 @@
>         if (newstate == GRUB_PARSER_STATE_TEXT
>             && state->state != GRUB_PARSER_STATE_ESC)
>           {
> -           int breakout = 0;
> -
>             switch (use)
>               {
>               case ' ':
> @@ -269,13 +270,13 @@
>               case '}':
>               case ';':
>               case '\n':
> -               breakout = 1;
> +               done = 1;
>               }
> -           if (breakout)
> +           if (done)
>               break;
> -           *(bp++) = use;
>           }
> -       else if (use)
> +
> +       if (use)
>           *(bp++) = use;
>  
>         state->state = newstate;
> @@ -285,27 +286,39 @@
>        /* A string of text was read in.  */
>        *bp = '\0';
>        grub_dprintf ("scripting", "token=`%s'\n", buffer);
> -      yylval->string = buffer;
>  
> -      /* Detect some special tokens.  */
> -      if (! grub_strcmp (buffer, "while"))
> -     return GRUB_PARSER_TOKEN_WHILE;
> -      else if (! grub_strcmp (buffer, "if"))
> -     return GRUB_PARSER_TOKEN_IF;
> -      else if (! grub_strcmp (buffer, "function"))
> -     return GRUB_PARSER_TOKEN_FUNCTION;
> -      else if (! grub_strcmp (buffer, "menuentry"))
> -     return GRUB_PARSER_TOKEN_MENUENTRY;
> -      else if (! grub_strcmp (buffer, "@"))
> -     return GRUB_PARSER_TOKEN_MENUENTRY;
> -      else if (! grub_strcmp (buffer, "else"))
> -     return GRUB_PARSER_TOKEN_ELSE;
> -      else if (! grub_strcmp (buffer, "then"))
> -     return GRUB_PARSER_TOKEN_THEN;
> -      else if (! grub_strcmp (buffer, "fi"))
> -     return GRUB_PARSER_TOKEN_FI;
> -      else
> -     return GRUB_PARSER_TOKEN_NAME;
> +      if (!*state->script)
> +     done = 1;
> +
> +      if ((done) && (!arg))
> +     {
> +       yylval->string = buffer;
> +
> +       /* Detect some special tokens.  */
> +       if (! grub_strcmp (buffer, "while"))
> +         return GRUB_PARSER_TOKEN_WHILE;
> +       else if (! grub_strcmp (buffer, "if"))
> +         return GRUB_PARSER_TOKEN_IF;
> +       else if (! grub_strcmp (buffer, "function"))
> +         return GRUB_PARSER_TOKEN_FUNCTION;
> +       else if (! grub_strcmp (buffer, "menuentry"))
> +         return GRUB_PARSER_TOKEN_MENUENTRY;
> +       else if (! grub_strcmp (buffer, "@"))
> +         return GRUB_PARSER_TOKEN_MENUENTRY;
> +       else if (! grub_strcmp (buffer, "else"))
> +         return GRUB_PARSER_TOKEN_ELSE;
> +       else if (! grub_strcmp (buffer, "then"))
> +         return GRUB_PARSER_TOKEN_THEN;
> +       else if (! grub_strcmp (buffer, "fi"))
> +         return GRUB_PARSER_TOKEN_FI;
> +       else
> +         return GRUB_PARSER_TOKEN_NAME;
> +     }
> +
> +      if (bp != buffer)
> +     arg =
> +       grub_script_arg_add (parsestate, arg, GRUB_SCRIPT_ARG_TYPE_STR,
> +                            buffer);

You are still doing parsing in the lexer.  I don't like that...  If
you make a separate patch that excludes this, be can apply that first
before we work on this specific problem.  I don't want to hold back
applying the fixes for these other bugs because of this.

>      }
>    else if (newstate == GRUB_PARSER_STATE_VAR
>          || newstate == GRUB_PARSER_STATE_QVAR)
> @@ -341,19 +354,33 @@
>       }
>  
>        *bp = '\0';
> -      state->state = newstate;
> -      yylval->string = buffer;
> -      grub_dprintf ("scripting", "vartoken=`%s'\n", buffer);
> +      if (newstate == GRUB_PARSER_STATE_VARNAME)
> +        state->state = GRUB_PARSER_STATE_TEXT;
>  
> -      return GRUB_PARSER_TOKEN_VAR;
> +      if (bp != buffer)
> +     arg = grub_script_arg_add (parsestate, arg, GRUB_SCRIPT_ARG_TYPE_VAR,
> +                                buffer);
>      }
>    else
>      {
>        /* There is either text or a variable name.  In the case you
> -      arrive here there is a serious problem with the lexer.  */
> +         arrive here there is a serious problem with the lexer.  */
>        grub_error (GRUB_ERR_BAD_ARGUMENT, "Internal error\n");
>        return 0;
>      }
> +
> +  if ((done) || (!*state->script))
> +    {
> +      if (arg)
> +     {
> +       yylval->arg = arg;
> +       return GRUB_PARSER_TOKEN_VAR;
> +     }
> +      else
> +     return ' ';
> +    }
> +
> +  goto back;
>  }
>  
>  void
> 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   25 Jun 2007 11:43:58 -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;
>  %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;
>                 }
>               | 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     25 Jun 2007 11:43:58 -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},
>  
>    { GRUB_PARSER_STATE_QVAR, GRUB_PARSER_STATE_QVARNAME2, '{', 0},
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel

--
Marco