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: Tue, 15 Jan 2008 13:41:15 +0100
User-agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)

Bean <address@hidden> 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





reply via email to

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