Re: Automagic command loading

[Marco Gerards]

Tomas Ebenlendr <address@hidden> 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 <commandname>.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