From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1HvEM4-000752-7X for mharc-grub-devel@gnu.org; Mon, 04 Jun 2007 11:20:24 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1HvEM2-00074n-Eq for grub-devel@gnu.org; Mon, 04 Jun 2007 11:20:22 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1HvEM1-00074b-Tp for grub-devel@gnu.org; Mon, 04 Jun 2007 11:20:22 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1HvEM1-00074Y-Nk for grub-devel@gnu.org; Mon, 04 Jun 2007 11:20:21 -0400 Received: from smtp-vbr9.xs4all.nl ([194.109.24.29]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1HvEM0-0006DL-Us for grub-devel@gnu.org; Mon, 04 Jun 2007 11:20:21 -0400 Received: from localhost.localdomain (249-174.surfsnel.dsl.internl.net [145.99.174.249]) by smtp-vbr9.xs4all.nl (8.13.8/8.13.8) with ESMTP id l54FKGuP039987 for ; Mon, 4 Jun 2007 17:20:18 +0200 (CEST) (envelope-from mgerards@xs4all.nl) From: Marco Gerards To: The development of GRUB 2 References: <4665F4F8.2040305@raulete.net> Mail-Copies-To: mgerards@xs4all.nl Date: Mon, 04 Jun 2007 17:26:16 +0200 In-Reply-To: <4665F4F8.2040305@raulete.net> (adrian15@raulete.net's message of "Wed, 06 Jun 2007 01:42:48 +0200") Message-ID: <87fy577nc7.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: FreeBSD 4.6-4.9 Subject: Re: test -e patch 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: Mon, 04 Jun 2007 15:20:23 -0000 adrian15 writes: > Attached you will find the patch adding test -e support for grub2. > > This is my first patch. I have compiled it without no errors. Urgh... I thought/hoped I told you I had a test.c rewrite sitting on my harddisk? Or did I tell Robert to poke me until next weekend so I will work on it? It includes everything you expect from test.c, expect the cleanup and testing. ;-) > However as long as the grub2.tar.gz that Marco gave me did not have any > documentation about how to create a floppy (or at least I did not manage > to find it) I have not tested it. Please have a look at the wiki. It has quite some information about GRUB 2. > I have applied the GNU Coding Standards Style on the patch although the > copy-pasted code that you will find in the mail body has not the GCS > applied. > > Here there are some doubts: > > > static const struct grub_arg_option options[] = >> { >> {"file", 'e', 0, "test if a file exists", 0, 0}, >> {0, 0, 0, 0, 0, 0} >> }; > > Is this correct? What's that last line for? Is it compulsory ? It means that this is the last option. There is no other way to tell if this is the last option without this. > Should I write "Test if a file exists" instead of "test if a file > exists" or "FILE exists"? FILE >> >> static void >> test_file_exists (const char *key) > > Do you prefer another name like: file_exists_test ? > >> { >> grub_file_t file; >> >> file = grub_file_open (key); >> if (file) >> { >> grub_printf (" %s", key); > Should I prompt the existent file or not? > Should I prompt %s exists maybe ? > Should I prompt an \n also? >> grub_file_close (file); >> grub_errno = GRUB_ERR_NONE; >> } >> else >> { >> grub_error (GRUB_ERR_FILE_NOT_FOUND, "File does not exists."); > > How the hell should I treat grub errors. Maybe the test_file_exists has > to return a static grub_err_t and then from grub_cmd_test I should call > it like this: return (test_file_exists (args[0])) so that the error > propagates ? Right. Otherwise the error will be lost. > What are the error polices ? Returning the err_t that grub_error returns when possible. >> } >> } >> >> >> static grub_err_t >> grub_cmd_test (struct grub_arg_list *state __attribute__ ((unused)), int argc, >> char **args) >> >> { >> >> char *eq; >> char *eqis; >> >> if (state[0].set) > > I suppose this refers to the first line of static const struct > grub_arg_option options and thus as -e is the first option and if its > set we should run the correspondent function. Yes. > One question. Should we put the test-e function into a separate file or > not ? No, the problem is that the design of test.c (which is just a placeholder) is wrong. It needs a proper parser for the arguments and a way to deal with this... >> test_file_exists (args[0]); >> else >> { >> /* XXX: No fancy expression evaluation yet. */ >> if (argc == 0) >> return 0; >> eq = grub_strdup (args[0]); >> eqis = grub_strchr (eq, '='); >> if (! eqis) >> return 0; >> >> *eqis = '\0'; >> eqis++; >> /* Check an expression in the form `A=B'. */ >> if (grub_strcmp (eq, eqis)) >> grub_error (GRUB_ERR_TEST_FAILURE, "false"); >> grub_free (eq); >> } >> return grub_errno; >> >> >> } >> >> >> >> GRUB_MOD_INIT(test) >> { >> (void)mod; /* To stop warning. */ >> grub_register_command ("[", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE, >> "[ EXPRESSION ]", "Evaluate an expression", 0); >> grub_register_command ("test", grub_cmd_test, GRUB_COMMAND_FLAG_CMDLINE, >> "test EXPRESSION", "Evaluate an expression", 0); >> } > > I understand this register commands. I suppose this information is read > from the help command or it isn't ? Yes, the help command uses this information. > Or maybe it also reads from: > static const struct grub_arg_option options ? Help only does when you use "help test". So it can use both. > The question is if the user will see the -e, -f or other options when > querying the test command help or not ? They should. But I am not sure if the final version will support this. Especially because of the nested syntax of the test arguments. >> >> GRUB_MOD_FINI(test) >> { >> grub_unregister_command ("["); >> grub_unregister_command ("test"); >> } > > That's all for my first compiled patch. > I hope that Marco_g is happy :). > But you know Marco_g I will continue my non-sense grub legacy > development too. ;) :-)) > adrian15 > > diff -urN grub2_2007_05_31_original/commands/test.c grub2_2007_05_31/commands/test.c > --- grub2_2007_05_31_original/commands/test.c 2005-11-13 16:47:08.000000000 +0100 > +++ grub2_2007_05_31/commands/test.c 2007-06-01 12:13:11.000000000 +0200 > @@ -24,32 +24,64 @@ > #include > #include > #include > +#include > + > +static const struct grub_arg_option options[] = > + { > + {"file", 'e', 0, "Test if a file exists", 0, 0}, > + {0, 0, 0, 0, 0, 0} > + }; > + > +static void > +test_file_exists (const char *key) Why not filename? > +{ > + grub_file_t file; > + > + file = grub_file_open (key); > + if (file) > + { > + grub_printf (" %s", key); > + grub_file_close (file); > + grub_errno = GRUB_ERR_NONE; > + } > + else > + { > + grub_error (GRUB_ERR_FILE_NOT_FOUND, "File does not exists."); > + } > +} > + > > static grub_err_t > grub_cmd_test (struct grub_arg_list *state __attribute__ ((unused)), int argc, > char **args) > > { > + You accidently introduced a whiteline. > char *eq; > char *eqis; > > - /* XXX: No fancy expression evaluation yet. */ > + if (state[0].set) > + test_file_exists (args[0]); > + else > + { This means that this check is run for any other expression. This is quite error sensitive. > + /* XXX: No fancy expression evaluation yet. */ > > - if (argc == 0) > - return 0; > + if (argc == 0) > + return 0; > + > + eq = grub_strdup (args[0]); > + eqis = grub_strchr (eq, '='); > + if (! eqis) > + return 0; > + > + *eqis = '\0'; > + eqis++; > + /* Check an expression in the form `A=B'. */ > + if (grub_strcmp (eq, eqis)) > + grub_error (GRUB_ERR_TEST_FAILURE, "false"); > + grub_free (eq); > + } > > - eq = grub_strdup (args[0]); > - eqis = grub_strchr (eq, '='); > - if (! eqis) > - return 0; > - > - *eqis = '\0'; > - eqis++; > - /* Check an expression in the form `A=B'. */ > - if (grub_strcmp (eq, eqis)) > - grub_error (GRUB_ERR_TEST_FAILURE, "false"); > - grub_free (eq); > - > return grub_errno; > } -- Marco