Re: [PATCH] Handler support

[Bean]

On Sun, Feb 15, 2009 at 3:32 PM, Vesa Jääskeläinen <address@hidden> wrote:
> Bean wrote:
>> On Sat, Feb 14, 2009 at 11:11 PM, Felix Zielcke <address@hidden> wrote:
>>>     A. Am Samstag, den 14.02.2009, 22:46 +0800 schrieb Bean:
>>>
>>>> If there is no objection, I'd commit this patch in a few days.
>>> Hello Bean,
>>>
>>> if you want to drop termin_input and terminal_output command then please
>>> don't forget to update util/grub.d/00_header.in
>>>
>>> +GRUB_MOD_INIT(handler)
>>> +{
>>> +  (void)mod;                   /* To stop warning. */
>>> +  grub_register_command ("handler", grub_cmd_handler,
>>> GRUB_COMMAND_FLAG_BOTH,
>>> +                        "handler [class [handler]]",
>>> +                        "List or select a handler", 0);
>>> +}
>>> +
>>> +GRUB_MOD_FINI(handler)
>>> +{
>>> +  grub_unregister_command ("hello");
>>> +}
>>>
>>> This should probable be grub_unregister_command ("handler").
>>
>> Hi,
>>
>> Thanks for pointing out, this new patch should fix the problem.
>
> Hi,
>
> Ok. I a way this functionality could be used. I just don't like how it
> is visible to the user.
>
>> +if handler output ${GRUB_TERMINAL_OUTPUT} ; then true ; else
>> +  # For backward compatibility with versions that uses terminal.mod instead
>> +  # of handler.mod
>> +  if terminal_output ${GRUB_TERMINAL_OUTPUT} ; then true ; else
>> +    # For backward compatibility with versions of terminal.mod that don't
>> +    # understand terminal_output
>> +    terminal ${GRUB_TERMINAL_OUTPUT}
>> +  fi
>
> If we look at this snippet, for me keyword handler doesn't say a thing.
> where it could be terminal for terminal handlers.
>
> So I would propose to change it in following ways:
>
> 1. Do not register handler command unless you need to enumerate those or
> allow more flexibility to configuration
> 2. Register command functions/alias to more understandable grub commands.
>
> So following could be:
>
> terminal output <whatever>
>
> or keep it what it is (I would prefer above line)
>
> terminal_output
>
> Good idea with this handler stuff would be that if someone writes following:
>
> terminal console
>
> It would lookup if this console provides input and output service and
> would use them out of the box.
>
> Now if some-one writes something like:
>
> terminal --input usbkbd --input console --output console
>
> or:
>
> terminal --input usbkbd
> terminal --input console
> terminal --output console
>
> or
>
> terminal_input usbkbd
> terminal_input console
> terminal_output console
>
> or
>
> terminal_input usbkbd console
> terminal_output console
>
> or
>
> terminal_input console
> terminal_output console
> termianl_input --add usbkbd
>
> User would now get two input sources like ps2 keyboard and usb keyboard
> and then output to console.
>
> Anyway... my point being... handler does not say anything to me in a
> sens what does it logically give to user. It hinders the understanding
> of the user for grub 2 usage...

Hi,

Good point. However, currently this can't be implemented in a generic
way. The main obstacle is that the command function doesn't allow
custom parameter, so one function can only serve one command, it's not
possible to use a single function, such as grub_cmd_handler, to handle
multiple command with slightly different behavior.

One quick fix is to use more descriptive class name, such as
terminal_input and terminal_output, so the command looks like this:
handler terminal_input console

In a long term, it would be better to improve the command interface,
which is my next goal. I'm planning to merge the normal and rescue
command into one single command set, so that any command can be used
in both environment. This also eliminates commands like linux, chain,
etc, whose function is simply to call the underlying command _linux
and _chain.

-- 
Bean