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

[Colin Watson]

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.

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.

> Index: grub2-1.99/util/grub-mkconfig.in
> ===================================================================
> --- grub2-1.99.orig/util/grub-mkconfig.in     2013-06-13 10:15:09.322977487 
> +0800
> +++ grub2-1.99/util/grub-mkconfig.in  2013-06-13 10:16:33.954980977 +0800
> @@ -251,7 +251,8 @@
>    GRUB_INIT_TUNE \
>    GRUB_SAVEDEFAULT \
>    GRUB_BADRAM \
> -  GRUB_RECORDFAIL_TIMEOUT
> +  GRUB_RECORDFAIL_TIMEOUT \
> +  GRUB_HIDDEN_TIMEOUT_HOTKEY
>  
>  if test "x${grub_cfg}" != "x"; then
>    rm -f ${grub_cfg}.new
> Index: grub2-1.99/util/grub.d/30_os-prober.in
> ===================================================================
> --- grub2-1.99.orig/util/grub.d/30_os-prober.in       2013-06-13 
> 10:15:09.010977474 +0800
> +++ grub2-1.99/util/grub.d/30_os-prober.in    2013-06-13 10:22:12.534994943 
> +0800
> @@ -34,6 +34,12 @@
>       verbose=" --verbose"
>        fi
>  
> +      if [ "x${GRUB_HIDDEN_TIMEOUT_HOTKEY}" = "x" ] ; then
> +        hotkey=
> +      else
> +        hotkey=" --function-key ${GRUB_HIDDEN_TIMEOUT_HOTKEY}"
> +      fi
> +
>        if [ "x${1}" = "x0" ] ; then
>       cat <<EOF
>  if [ "x\${timeout}" != "x-1" ]; then
> @@ -44,7 +50,7 @@
>        set timeout=0
>      fi
>    else
> -    if sleep$verbose --interruptible 3 ; then
> +    if sleep$verbose --interruptible 3$hotkey ; then
>        set timeout=0
>      fi
>    fi
> @@ -53,7 +59,7 @@
>        else
>       cat << EOF
>  if [ "x\${timeout}" != "x-1" ]; then
> -  if sleep$verbose --interruptible ${GRUB_HIDDEN_TIMEOUT} ; then
> +  if sleep$verbose --interruptible ${GRUB_HIDDEN_TIMEOUT}$hotkey ; then
>      set timeout=0
>    fi
>  fi

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.

Thanks,

-- 
Colin Watson                                       address@hidden