From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.33) id 1CCdUZ-0006SK-Ou for mharc-grub-devel@gnu.org; Wed, 29 Sep 2004 08:23:31 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.33) id 1CCdUX-0006S3-RF for grub-devel@gnu.org; Wed, 29 Sep 2004 08:23:29 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.33) id 1CCdUX-0006Rr-8X for grub-devel@gnu.org; Wed, 29 Sep 2004 08:23:29 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.33) id 1CCdUX-0006Ro-4l for grub-devel@gnu.org; Wed, 29 Sep 2004 08:23:29 -0400 Received: from [145.74.66.11] (helo=mail-cn.han.nl) by monty-python.gnu.org with esmtp (Exim 4.34) id 1CCdO0-00046D-Ct for grub-devel@gnu.org; Wed, 29 Sep 2004 08:16:44 -0400 Received: from localhost (charlie.han.nl [145.74.66.9]) by mail-cn.han.nl (Postfix) with ESMTP id 98788816A for ; Wed, 29 Sep 2004 14:16:43 +0200 (CEST) Received: from mail-cn.han.nl ([145.74.66.11]) by localhost (charlie.han.nl [145.74.66.9]) (amavisd-new, port 10024) with ESMTP id 20085-05 for ; Wed, 29 Sep 2004 14:16:40 +0200 (CEST) Received: from mail1.han.nl (mail1.han.nl [145.74.103.11]) by mail-cn.han.nl (Postfix) with ESMTP id AE3D783C8 for ; Wed, 29 Sep 2004 14:16:40 +0200 (CEST) Received: from marco.marco-g.com (mgerards.xs4all.nl [82.92.27.129]) by mail1.han.nl (Postfix) with ESMTP id 5BC63C045 for ; Wed, 29 Sep 2004 14:16:40 +0200 (CEST) Mail-Copies-To: metgerards@student.han.nl To: grub-devel@gnu.org References: <20040928190544.GA24852@artax.karlin.mff.cuni.cz> From: Marco Gerards Date: Wed, 29 Sep 2004 12:16:50 +0000 In-Reply-To: <20040928190544.GA24852@artax.karlin.mff.cuni.cz> (Tomas Ebenlendr's message of "Tue, 28 Sep 2004 21:05:44 +0200") Message-ID: <87fz51mfrh.fsf@marco.marco-g.com> User-Agent: Gnus/5.1006 (Gnus v5.10.6) Emacs/21.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Virus-Scanned: by amavisd-new@vscan-cn.han.nl Subject: Re: Automagic command loading 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: Wed, 29 Sep 2004 12:23:30 -0000 Tomas Ebenlendr writes: > Here is new patch. The commands list is stored in elf section > .uinfo.norm_cmds. The module searching mechanism is in autocmd.mod, > so you switch on autoloading by inserting this module. Interface > is designed such that there may be other modules autoregistering > commands. > > The contents of .uinfo.norm_cmds of all modules in $prefix gets > cached on insert of autocmd.mod, because reading of all modules takes > about 5 seconds on my bochs under Linux, 300Mhz Pentium. The contents > is recached by command cache_autocommands or when $prefix does not > match cached directory. This will be used on very rare ocasions. Does that mean that GRUB hangs for 5 seconds on boot when autocmd is used? I seriously doubt if that would be acceptable... So is there any way to speed this up, I think no one want to wait 5 seconds when GRUB is started... Here are my comments on your patch. A lot of comments are still related to the GCS. But I also have a lot of questions on how things work. Perhaps I can comment better on certain parts of the patch after you explained this. > conf/i386-pc.rmk (pkgdata_MODULES): New module autocmd.mod Please end the sentence with a ".". > gencmdlist.sh: New file. Greps NORMAL_COMMAND in sources and generates > list of commands (c source). Two spaces between sentences. But just "New file." would be enough already. Explaining what something does should be done in comments in the file. > incude/grub/normal.h (NORMAL_COMMAND): New macro. Used for marking > commands to appear in special elf section. in a special elf section > normal/command.c (grub_register_command_autoloader): New function. You forgot the '*' here. > (grub_unregister_command_autoloader): New function. > (grub_command_find): Execute autoloaders when command not found. was not found > - char pathname[grub_strlen (dirname) + grub_strlen (filename) + 1]; > + char pathname[grub_strlen (dirname) + grub_strlen (filename) + 2]; Please document this change in the changelog entry. > - grub_register_command ("hello", grub_cmd_hello, GRUB_COMMAND_FLAG_BOTH, > + grub_register_command (NORMAL_COMMAND("hello"), grub_cmd_hello, GRUB_COMMAND_FLAG_BOTH, > "hello", "Say hello", 0); Please make sure lines can be fully shown on 80x25 terminals. > +/* The command autoloader description. */ > +struct grub_command_autoloader What is is used for? How? > +{ > + /* The callback function. */ > + grub_dl_t (*func) (const char * command); const char *command What does this callback do? > +void EXPORT_FUNC(grub_register_command_autoloader) ( > + grub_dl_t (*hook) (const char * cmd)); > +void EXPORT_FUNC(grub_unregister_command_autoloader) ( > + grub_dl_t (*hook) (const char * cmd)); Can you explain this as well? > +#define MEMFAILED \ > + ({\ > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "Out of memory");\ > + goto failed;\ > + }) > + > +#define READFAILED \ > + ({\ > + grub_error (GRUB_ERR_FILE_READ_ERROR, "Error reading file");\ > + goto failed;\ > + }) Personally I don't like this and this is not required. grub_malloc sets the right error string and sets grub_errno. The same is true for any generic function. So you should just have "goto failed" where you have either MEMFAILED or READFAILED. If any of the functions you use don't do the right thing, it should be fixed there. > +/* Reads value of user information in file (elf - module). > + Don't forget to free() result. */ Isn't that something that is obvious? IMHO such things should not be documented. Or do you mean a malloc'ed buffer should be returned? In That case I would just say that. > +grub_err_t > +grub_dl_read_file_uinfo (const char *filename, const char * secname, char ** value, int * len) In the case of pointers, please do not add those extra spaces. Write: grub_dl_read_file_uinfo (const char *filename, const char *secname, char **value, int *len) > + section = grub_malloc (grub_strlen (secname) + 8); Where does this 8 come from? > + readbytes = grub_file_read (file,(char *) &e, sizeof (e)); A space after the comma. > + if ((readbytes < (signed) sizeof (e)) || (e.e_ehsize < readbytes) || (e.e_shentsize < (signed) sizeof (*s))) This line is too long... > + readbytes = grub_file_read (file,(char *) s, wantbytes); A space after the comma. > + if (0) > + { > +failed: > + ret = grub_errno; > + } This looks like something you used when debugging? Can you please clean this up? > +char * dirname = 0; Can you remove the space and make this static? > +module_commands_t modules_list = 0; Same here. > + module_commands_t p, q; module_commands_t p; module_commands_t q; > + > + for (p = modules_list; p; p=q) p = q > +static grub_err_t > +cache_uinfo_files (struct grub_arg_list *state __attribute__ ((unused)), > + int argc __attribute__ ((unused)), > + char **args __attribute__ ((unused))) The function name does not make clear to me that it is a command. I just recognised it as such because of the arguments. > + { > + char * pathname = 0; Space... > + if ((suffixidx <= 0) || (grub_strcmp (filename + suffixidx,".mod") != 0)) A space before ".mod". > + > + if (0) > + { > +ret: > + grub_free (secbuf); > + } Same comment as before. Is this debug code? > + fs = grub_fs_probe (dev); > + path = grub_strchr (dirname, ')') + 1; > + > + if (! path || ! fs || (path[0] != '/')) > + goto fail; > + > + (fs->dir) (dev, path, cache_module); Please don't do it like this. Just use grub_fs_dir. > + /* Test if module has in .uinfo.norm_cmds written the command name. */ > + s = p->commands; > + while (s - p->commands < p->commands_len) > + { > + if (grub_strcmp (s, command) == 0) > + break; > + s += grub_strlen (s) + 1; > + } What does it mean if .uinfo.norm_cmds is not found? > + if (s >= p->commands + p->commands_len) > + return 0; > + > + /* Test that module is not loaded. */ > + suffixidx = grub_strlen (p->module) - 4; Why "- 4"? > + /* Check if prefix is cached. */ > + path = grub_env_get ("prefix"); Isn't there a grub_get_prefix command? > + (dirname[len] && ((dirname[len] != '/') || dirname[len+1]))) len + 1 > + cache_uinfo_files (0, 0, 0); What is this? > + /* Try .mod. */ > + len = grub_strlen (command); > + modname = grub_malloc (len + 5); Where is this memory freed? > +void > +grub_register_command_autoloader (grub_dl_t (*hook) (const char * > +cmd)) So if I understand it correctly you made this generic. You can register an autoloader that is responsible for looking up modules. In that case it is easy to replace or could be not present at all. Can you have multiple autoloaders? Can you elaborate on the design of the autoloader? So explain us how an autoloader works, how it is used, the interfaces, etc? > +{ > + grub_command_autoloader_t func, *p; Please don't declare multiple variable on one line. > +grub_unregister_command_autoloader (grub_dl_t (*hook) (const char * > +cmd)) I am not happy with the prefix. You can better use the "grub_autoloader_" prefix. Not only here but everywhere where this is logical. > + grub_command_autoloader_t *p, q; Same here. Two questions related to the cache: - Where is it cached? - How will the cache be depleted when autocmd is unloaded? Thanks, Marco