Re: Idea: elimination of the normal mode (revised version)

[Bean]

On Tue, Jul 22, 2008 at 1:27 AM, Marco Gerards <address@hidden> wrote:
> Hi,
>
> Bean <address@hidden> writes:
>
>> On Mon, Jul 21, 2008 at 4:02 AM, Marco Gerards <address@hidden> wrote:
>>> Hi,
>>>
>>> Bean <address@hidden> writes:
>>>
>>>> First of all, we can still keep rescue and normal command. But instead
>>>> of depending on normal.mod, normal command depends on module arg,
>>>> which is an option parser. Also, these two type of commands are of the
>>>> same command set. In fact, module arg is implemented as a pre parser,
>>>> which goes through the list of arguments and extract the options. In
>>>> the case of rescue command, the pre parser field is null, which means
>>>> it wants to parse options itself.
>>>
>>> pre parser?
>>
>> Maybe not the best word, the idea is that normal command is just like
>> rescue command, except that it goes through an extra parser that
>> convert options to grub_arg_list.
>
> Sorry, but I still don't understand this.  Do you propose:
> svn move normal/arg.c kern/arg.c
>
> ?
>
> If you propose a stripped down argument parser, how do arguments
> parsers differ amongst each other?
>

Actually, no need to move normal/arg.c to kernel, it can be a
standalone module. Normal command depend on it, while rescue command
don't.

there is only one argument parser, it's job is to scan the argument
for options like -f, --file and convert them to grub_arg_list, so that
the command handler don't have to parse option itself.

>
> Seems ok to me.
>
> Although what if we want additional handlers.  Like image readers or
> so?  We do not need to centralize this.  What about this:
>
> handler.c:
>
> grub_handler_t
> grub_handler_add (const char *name)
> {
> ...
> }
>
> grub_err_t
> grub_handler_remove (const char *name)
> {
>  ...
>  if (module uses handler)
>   return grub_error (...);
>  ...
> }
>
>
> /* NAME is the name of the handler, HANDLER the handler struct.  */
> grub_err_t
> grub_handler_register (const char *name, grub_handler_t handler)
> {
> ...
> }
>
>
> /* NAME is the name of the handler, HANDLER the handler struct.  */
> grub_err_t
> grub_handler_deregister (const char *name, grub_handler_t handler)
> {
> ...
> }
>
>
> /* HANDLER the handler struct.  Hook the hook function.  */
> grub_err_t
> grub_handler_register (grub_handler_t handler,
>                       int (*hook) (grub_handler_t handler))
> {
> ...
> }
>
>
> handler.h:
>
> struct grub_handler
> {
>  struct grub_handler *next;
>  const char *name;
> };
>
> #define GRUB_CREATE_HANDLER_H(name, interfaces) \
> grub_err_t grub_##name##_register (struct grub_##name##_handler h);
>
> (same for the other functions)
>
> #define GRUB_CREATE_HANDLER_FUNCS(name, interfaces)      \
> grub_err_t                                               \
> grub_##name##_register (struct grub_##name##_handler h); \
> {                                                        \
>  grub_handler_register (name, &h->handler);
> }
>
> (same for other functions)
>
>
> So the handler framework is opaque to the user.  The user just works
> with this like it works now.  For example:
>
> foo.h:
>
> struct grub_foo
> {
>  /* This is *required*.  */
>  grub_handler_t handle;
>
>  /* Every FOO needs to foo.  */
>  int (*foo) (int bar);
> };
>
> GRUB_CREATE_HANDLER_T ("foo", struct grub_foo);
>
> this creates all prototypes
>
> foo.c:
>
> GRUB_CREATE_HANDLER ("foo", struct grub_foo);
>
> this creates functions that are very light and are mainly there to
> cast the structs back and forth so we can do type checking and get
> sane warnings and maximal cleaness.  Furthermore, users can now use:
>
> grub_foo_register (...);
>
> ^^ Every foo can register itself
>
> grub_foo_iterate (...);
>
> ^^ You can iterate over every foo
>
> The only "weird" thing is that when you register a handler, you have
> to know its name.  For example disk.c would do:
>
>  {
>   err = grub_handler_add ("disk");
>   ...
>  }
>
>
> So as a summary:
>
> grub_handler_add will be used to add new handler lists,
> grub_handler_remove will remove them
>
> grub_handler_register will register specific handlers (like disk
> handlers), a macro can be used to add a wrapper that passes along the
> name.  grub_handler_register can fill in the mandatory member "handle"
> such that later on the name is not required anymore.  Requiring
> strings only at register time is not very expensive.
>
> Sorry for the crappy code, but hopefully you get the idea, otherwise I
> can hack a bit or comment on your patches :-)
>

Your idea seems fine, but there is a slightly efficiency issue. For
example, when we need to call a function in the handler, we need to
acquire it using name. We need to do this in every call, as the
handler could be changed next time.

My suggestion is to use function pointer instead of name, for example,

grub_handler_register (&grub_handler_input_head, my_handler);

Then we can always use grub_handler_input_head->getkey() to read a key.

The drawback is that we need to define grub_handler_**_head as global variable.

>>>> Then, we define an array to point to the head of handler linked list:
>>>> grub_handler[GRUB_HANDLER_NUM];
>
> Right.  Although what I proposed above you do not need an array, but
> you can register anything for free :-)
>
>>>> Head is the default selection. When we insert a new handler module, it
>>>> would automatically become the new default, although we can switch
>>>> back to old handler using a command.
>
> Are you sure this is the right way to go?  This needs more feedback
> from other people, I think...?
>

This is used to save handler switching command. Normally, when we push
a new handler, we want to use it right away. Although this behavior is
up for discussion ...

-- 
Bean