Re: Automagic command loading

[Tomas Ebenlendr]

> 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...
> 

5 seconds on *very* slow machine. Probably caused by opening files by
full path. (I should figure out what is the slowest operation.)

> > +/* The command autoloader description.  */
> > +struct grub_command_autoloader
> 
> What is is used for?  How?
> 

Ok, i will add more comments. (This is used for storing a list of
command autoloader functions. (The module autocmd implements one such
function.))

> > +{
> > +  /* The callback function.  */
> > +  grub_dl_t (*func) (const char * command);
> 
> const char *command
> 
> What does this callback do?

O sorry, cut and pasted comment. This is the autoloading function.

> 
> > +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?
>
O.k. I'l add more comments in patch.
When command does not exist, the 'hook' is called. There can be
registered more hooks, they are executed in order they were registered.
Ouch, the return value of the hook is bad. Meaning should be: false -
command was not autoloaded (try next hook). True - command autoloaded.

> > +/* 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.
>

Yes malloc'ed buffer is returned by following function. I'll chacnge the
comment.

> In the case of pointers, please do not add those extra spaces.

That is my bad custom. I'll try not to do that.

> > +  section = grub_malloc (grub_strlen (secname) + 8);
> 
> Where does this 8 come from?
> 

".uinfo." string is prepended.

> > +  if (0)
> > +    {
> > +failed:
> > +      ret = grub_errno;
> > +    }
> 
> This looks like something you used when debugging?  Can you please
> clean this up?
> 

This construction I use when I want to say do following statement only
if 'goto failed' was used, then continue with common code. Should I
duplicate the common code (about 5 lines of destruction), or should I
jump backwards (rename failed to ret, and add after return:
failed: ret = grub_errno; goto ret;)?

> > +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.
> 

It is used both ways. (I call it manually with (0, 0, 0) atguments.)

> > +  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.
> 

Oh, I was copying the 'ls' command code. I didn't know this function.

> 
> What does it mean if .uinfo.norm_cmds is not found?
> 
The same as when empty: No command is provided by the module.

> > +      if (s >= p->commands + p->commands_len)
> > +   return 0;
> > +
> > +      /* Test that module is not loaded.  */
> > +      suffixidx = grub_strlen (p->module) - 4;
> 
> Why "- 4"?
> 

".mod"
I should probably use sizeof(".mod").

> > +  /* Check if prefix is cached.  */
> > +  path = grub_env_get ("prefix");
> 
> Isn't there a grub_get_prefix command?
>

Maybe. Again, this code I saw in dl.c and so I copied it.

> > +    cache_uinfo_files (0, 0, 0);
> 
> What is this?
> 

I just call the function that is also normal mode command. I don't like
function wrappers here.


> > +  /* Try <commandname>.mod.  */
> > +  len = grub_strlen (command);
> > +  modname = grub_malloc (len + 5);
> 
> Where is this memory freed?
> 

Bug. Thanks.

> > +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?
> 

Yes they are called in order they were registered.

> Can you elaborate on the design of the autoloader?  So explain us how
> an autoloader works, how it is used, the interfaces, etc?
> 

Ok. I'll write it somewhere. (My disk is broken now, so It will take
some time.)

> Two questions related to the cache:
> 
> - Where is it cached?
> 

In autocmd.mod, there is connected list of pairs module name (with
suffix), .uinfo.norm_cmds content.

> - How will the cache be depleted when autocmd is unloaded?
>
Cache is cleared (and dealocated) on unloading, and also on recaching.
(But when recaching, it is immadiately allocated again and filled in.)

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.74586052925