Re: [RFC][PATCH] Allow hotkeys to interrupt hidden menu

[Franz Hsieh]

Hi, there

  I had merged the patch for enabling hotkey in grub silent mode to grub-trunk.

  The --function-key now has been removed from sleep.mod, now sleep.mod will
  monitor all function keys, delete, tab and backspace.

Thanks!

Franz Hsieh

On Fri, Sep 13, 2013 at 5:18 PM, Franz Hsieh <address@hidden> wrote:

On Wed, Sep 11, 2013 at 9:31 PM, Colin Watson <address@hidden> wrote:

Hi Franz,

Throughout this patch, please take care to adhere to the GRUB coding
style.  This is definitely an improvement over previous versions I've
reviewed, but it still has a number of places where functions are called
or declared with no space before the opening parenthesis.  That is,
"function()" should become "function ()".  I know it's a minor point,
but it makes code much easier to read when it's all in the same style.

On Wed, Sep 11, 2013 at 02:18:04PM +0100, Franz Hsieh (via Colin Watson)
wrote:

> +static struct
> +{
> +  char *name;
> +  int key;
> +} function_key_aliases[] =
> +  {
> +    {"f1", GRUB_TERM_KEY_F1},
> +    {"f2", GRUB_TERM_KEY_F2},
> +    {"f3", GRUB_TERM_KEY_F3},
> +    {"f4", GRUB_TERM_KEY_F4},
> +    {"f5", GRUB_TERM_KEY_F5},
> +    {"f6", GRUB_TERM_KEY_F6},
> +    {"f7", GRUB_TERM_KEY_F7},
> +    {"f8", GRUB_TERM_KEY_F8},
> +    {"f9", GRUB_TERM_KEY_F9},
> +    {"f10", GRUB_TERM_KEY_F10},
> +    {"f11", GRUB_TERM_KEY_F11},
> +    {"f12", GRUB_TERM_KEY_F12},
> +  };
> +

This is essentially a copy of hotkey_aliases from
grub-core/commands/menuentry.c, and there's duplicated lookup code as
well.  Can you find any way to share it?  Since we certainly don't want
to put this in the kernel, and neither the sleep module nor the normal
module really ought to depend on the other, I suspect that doing so
would require a new module for shared menu code, which may well be
overkill for this, but it's worth a look.

I also agree to remove duplicate code, but seems not easy to do it unless
we have a shared module, right? Well it would take me some time to evaluate.

At the very least it would be a good idea to bring the contents of this
structure into sync with hotkey_aliases and add comments to encourage
developers to keep them that way.

I think we should also call this kind of thing simply "hotkey"
consistently across the code, rather than it sometimes being
"function_key" or "fnkey".

 

> +      if (key == fnkey)
> +     {
> +       char hotkey[32] = {0};
> +       grub_snprintf(hotkey, 32, "%d", key);
> +       grub_env_set("hotkey", (char*)&hotkey);
> +       return 1;
> +     }

We've generally tried to get rid of this kind of static allocation.  Use
grub_xasprintf instead:

  if (key == fnkey)
    {
      char *hotkey = grub_xasprintf ("%d", key);
      grub_env_set ("hotkey", hotkey);
      grub_free (hotkey);
      return 1;
    }

> +int
> +grub_menu_get_hotkey (void)

This function is only used in this file, so it should be static.

These idea are good and I will do these changes in my patch. 

Rather than having to configure this explicitly, I have an alternative
suggestion, which ties into my earlier suggestion of making the hotkey
code a bit more shared.  Why not have sleep --interruptible look up the
hotkeys configured in the menu and automatically honour any of those?
That way you wouldn't have to configure hotkeys in two places (grub.cfg
and /etc/default/grub).  Plus, I'm always more comfortable with patches
that don't require adding configuration options.

Agree with Colin's points and I will update the patch.

By the way I would like to combine --function-key and --interruptible. I think
it is a simple way that sleep.mod always checks ESC for interrupt and f1~f12 for the

hotkey.

Thanks for your reviews and if there is any good suggestion, please feel free to tell me.

Regards,
Franz Hsieh

grub2-enable-hotkey-in-silent-mode.patch
Description: Binary data