Re: automagic module loading - normal commands

[Marco Gerards]

Tomas Ebenlendr <address@hidden> writes:

> I want to do better my autocmd patch. Now it reads file autocmd.lst in
> root directory. (this can be changed via variable 'autocmd'). I have
> following questions:
>     - Do we want to have special file like autocmd.lst? Or
>       to 'probe' all modules: read a module, and read from some special
>       elf section supported commands? Or both? (Or other way to decide
>       which module to load?)

I think a autocmd.lst file is not that nice.  In that case you would
have to add every module manually to it.

Here are some comments.  I did not read the patch in detail so I
missed a lot I wanted to ask you about, I guess.

Did you make sure commands that did not get autoloaded won't we
automatically removed?  Or doesn't that ever happen?

>     - Do we want to have automatic module loading hardcoded? Or should
>       there be a generic interface, so some other module will drive
>       automatic module loading?

I don't understand what you mean.

>         Or from *.c (by preprocessing and greping, such as symbols
>         are from *.h)
>         Or else way?

Perhaps by grepping for register_command or so?  But as I said IMHO
autocmd.lst is not the right thing to do.

> I can implement any solution. Just feel free and say your opinion about
> this. (And other things that you like/dislike on the patch.)

Cool!

> Here is current version: (warning patch may be not aplicable, see date of
> files, to know version of grub).

Ok.

> +static grub_autocmd_list_t * grub_autocmd_list = 0;

Can you remove the space for grub_autocmd_list?

> +  char *cmdname = 0, *modulename = 0, *pos, *end;

Can you split this up?

> +       /* Not goto failed. Preserve old setting here.  */

After 'failed.' two spaces.  We use spacing as used in the USE (AFAIK).

> +grub_autocmd_set (struct grub_env_var * env)

The space before env.

> +  char * prefix = grub_env_get("prefix");

char *prefix = grub_env_get ("prefix");

> +
> +  p = grub_strchr (env->value, '/');

Is this to look up filenames / devices?  In that case see the patch I
applied today and the discussion about slashes in device names.   In
that case you should do the same to make sure it works on all machines.

> +    return grub_autocmd_read(env->value, grub_autocmd_count < 0);

Missing space.

> +  char * fname;

char *fname;

> +  grub_register_variable_hook("autocmd", 0, grub_autocmd_set);

> +    grub_env_set("autocmd",fname);

Missing space.

> +  grub_register_variable_hook("autocmd", 0, 0);

...

> +  grub_autocmd_read(0, 0);

...

Thanks,
Marco