[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: automagic module loading - normal commands
From: |
Marco Gerards |
Subject: |
Re: automagic module loading - normal commands |
Date: |
Wed, 22 Sep 2004 21:51:45 +0000 |
User-agent: |
Gnus/5.1006 (Gnus v5.10.6) Emacs/21.3 (gnu/linux) |
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