From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1JEl5f-0000W6-MI for mharc-grub-devel@gnu.org; Tue, 15 Jan 2008 07:40:27 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JEl5e-0000Vo-6s for grub-devel@gnu.org; Tue, 15 Jan 2008 07:40:26 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JEl5d-0000VD-M6 for grub-devel@gnu.org; Tue, 15 Jan 2008 07:40:25 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JEl5d-0000VA-JD for grub-devel@gnu.org; Tue, 15 Jan 2008 07:40:25 -0500 Received: from smtp-vbr11.xs4all.nl ([194.109.24.31]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JEl5d-0003lC-2m for grub-devel@gnu.org; Tue, 15 Jan 2008 07:40:25 -0500 Received: from localhost.localdomain (249-174.surfsnel.dsl.internl.net [145.99.174.249]) by smtp-vbr11.xs4all.nl (8.13.8/8.13.8) with ESMTP id m0FCeNRL096616 for ; Tue, 15 Jan 2008 13:40:24 +0100 (CET) (envelope-from mgerards@xs4all.nl) From: Marco Gerards To: The development of GRUB 2 References: <87ir1xftyi.fsf@xs4all.nl> <8763xv4a7t.fsf@xs4all.nl> Mail-Copies-To: mgerards@xs4all.nl Date: Tue, 15 Jan 2008 13:41:15 +0100 In-Reply-To: (bean123ch@gmail.com's message of "Tue, 15 Jan 2008 20:30:45 +0800") Message-ID: <87abn71ct0.fsf@xs4all.nl> User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Virus-Scanned: by XS4ALL Virus Scanner X-detected-kernel: by monty-python.gnu.org: FreeBSD 4.6-4.9 Subject: Re: [PATCH] Bug fix for parser X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Jan 2008 12:40:26 -0000 Bean 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