--hotkey option does not work when the menu is hidden

attached a patch made by Cyrus Lien <email address hidden>

The attachment "new_command_getfunctionkey.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

This design means that you have to press the function key at the exact
moment the command in question was being executed. You have worked
around this by introducing an arbitrary delay, but that will produce a
horrible user experience, especially if used for more than one command.

GRUB has supported a --hotkey option to menuentry since 1.99, so you can
use e.g. 'menuentry "Memory test (memtest86+)" --hotkey f9'. You should
investigate using this instead.

Hsin-Yi,

Does Colin's suggestion work for you?

@Colin , @James

Yes, I understand the patch is not perfect solution, user will need to repeat key pressing until it is trigged.

The author of the patch , Cyrus have tried the menuentry --hotkey option, but It does not fit our need because we hope the hotkey can still be trigged even menu is hidden (like sleep command does).

by current design, the --hotkey option only works when menu is not hidden .

OK, but I'm afraid the patch is not acceptable in its current form; it's
just far too painful to use. Please figure out a way to improve
--hotkey (or very similar) instead, as that kind of event-driven
approach is what we need here.

Doesn't the pressing of *any* key cause the hidden menu to be shown? Then the --hotkey should pick it up?

I suspect that the sleep command will swallow the keypress. Passing it
on to the menu somehow would be one possible avenue of attack here,
though possibly counterintuitive; I'm not sure that I expect e.g.
pressing Enter at the hiddenmenu delay to effectively bypass showing the
menu and immediately boot the first entry. So experiment and be
careful.

@Colin

I will look at the code of hotkey to see how to improve it, it seems I should add some key detection here to let grub boot to the menu entry when corresponding key is pressing.

file: ${src}/grub-core/normal/menu.c
```
  /* If timeout is 0, drawing is pointless (and ugly). */
  if (grub_menu_get_timeout () == 0)
    {
      *auto_boot = 1;
      return default_entry;
    }
```

@Colin, @Hsin-Yi
I have done a new patch that adds 100ms delay and checks hotkey in:
file: ${src}/grub-core/normal/menu.c
...
  /* If timeout is 0, drawing is pointless (and ugly). */
  if (grub_menu_get_timeout () == 0)
    {
      *auto_boot = 1;
      return default_entry;
    }
...
With this patch, we only have to set --hotkey f9 to menuentry, and users can
press f9 to enter recovery mode even grub runs in quiet mode.

(General note: please try to look at how the existing code is formatted and follow its style; this file uses a two-space indent with GNU brace style and tab-for-every-eight-spaces, and there are a few other details to match as well. When you use a different style for parts of your changes, it makes the changes very difficult to read. The third chunk of this patch in particular is hard work because the indentation has ended up reversed.)

I think it's very wrong to force a delay in normal/menu.c like this. Firstly, it's a hardcoded unconfigurable magic number. Secondly, if you asked for no timeout in your GRUB configuration, you should get no timeout, even at the cost of hotkeys; for example, we have had commercial projects with extremely tight boot deadlines that would be sensitive to a 100ms delay. So, while I do see where you're going, and it would probably work as a temporary patch in a commercial project, I'm afraid I would not approve this approach upstream or in Ubuntu.

My recommendations would be as follows. I don't promise that my colleagues upstream will feel exactly the same way, but I think this has a much better chance of being acceptable.

 * This configuration should require a non-zero GRUB_HIDDEN_TIMEOUT, in order that firmware key repeat has had a chance to kick in and something in GRUB has had a chance to see the keyboard state.

 * The terminal layer should be modified so that it's possible to save a keystroke somewhere after reading it. This would permit a keystroke to interrupt a sleep and then also be consumed by subsequent menu interface code. While doing this, be careful that the terminal layer is in the space-constrained kernel, where every byte has a cost; it would, I think, be reasonable to allow saving only a single keystroke rather than maintaining any more sophisticated structure.

 * There will need to be some interface change to the sleep command. Perhaps any keystroke should interrupt it, rather than just Escape and (in an Ubuntu-specific patch) Shift. It should then save/unget it using the interface above, so that the menu interface can get at it.

 * The menu code should then look at the saved keystroke and decide whether it's a hotkey, and if so set auto-boot, somewhat as your current patch does. (Here's where it matters that the terminal operation is an out-of-band save rather than pushing the keystroke back into the input buffer: you don't want pressing Escape to interrupt the hidden timeout and then *also* cancel the menu.)

@colin
  I rewrite a new patch that:
  1. add --function-key option to read function key and set to "hotkey" environment variable.
  2. grub reads "hotkey" environment variable and boot to the entry at the start.
  3. add GRUB_HIDDEN_TIMEOUT_KEY variable in /etc/default/grub

  Note: Currently we only support one hotkey.

Hi Colin,

Does Frank's patch work for you?

I've forwarded this upstream as https://lists.gnu.org/archive/html/grub-devel/2013-09/msg00046.html, and added my own review there. Sorry for the long delay!

What about use sleep --interruptible to get the hotkey we want to use?

- {"interruptible", 'i', 0, N_("Allow to interrupt with ESC."), 0, 0},
+ {"interruptible", 'i', 0, N_("Allow to interrupt with one key."), 0, ARG_TYPE_STRING},

I wrote a simple patch for this way. And we can use if sleep --interuptible f10 3 to interupt the sleep.

This patch has at least the following flaws:

 * It duplicates content (key names) from the normal module.
 * It has an absurdly long if chain which should be table-driven
   instead.
 * It does not meet the requirement of passing at least some hotkeys on
   to the menu; instead, you will in practice need to press the hotkey
   twice to actually boot the hotkeyed menu entry.

If anyone has further iterations of Franz's patch, could you please move
them to the thread on grub-devel instead? Otherwise I'm going to have
to relay them back and forward, which is inefficient.

I had sent the patch in comment 15 to grub mailing list http://lists.gnu.org/archive/html/grub-devel/2013-09/msg00053.html. There were some comments on it and this patch is rejected.
After discussing, the maintaner wants to use the patch in comment 14.

Franz Hsieh sent one update patch to the upstream at http://lists.gnu.org/archive/html/grub-devel/2013-10/msg00070.html and I sent one ping email to upstream at http://lists.gnu.org/archive/html/grub-devel/2013-11/msg00029.html.

But still no response from the upstream.

OK. I talked with Vladimir about this on #grub today; he shares my dislike for the hardcoded hotkey names, and between us we came up with a better plan:

 * The simplest way to work out what hotkeys to honour is to introspect the menu structure itself. However, this can only be done after all the top-level commands in grub.cfg have been executed, so it can't be done in the "sleep" command.
 * There's nothing particular that says that we have to implement the hidden timeout in a "sleep" command, although we have to take some care to ensure configuration file compatibility with older modules. Since the main timeout is implemented in normal.mod, it makes some sense for the hidden timeout to go there too, where we can get at the menu structure.
 * The simplest way to arrange for configuration file compatibility is to add a new "hiddenmenu" command that sets a "hidden_timeout" environment variable; we could also set the environment variable directly, but having a command allows us to detect the absence of that command and infer that we need to fall back to the old mechanism.

I'm working on an implementation of this plan now to check that it's viable, and will post the results on grub-devel. Modulo one slight roadblock it's looking much simpler so far.

This bug was fixed in the package grub2 - 2.00-21

---------------
grub2 (2.00-21) unstable; urgency=low

  [ Robert Millan ]
  * Backport from upstream:
    - Accept ZFS version 5000 (feature based).

  [ Colin Watson ]
  * Silence error message on initial installation when /etc/default/grub
    does not yet exist.
  * Add GRUB_RECOVERY_TITLE option, to allow the controversial "recovery
    mode" text to be customised (LP: #1240360).
  * Backport from upstream:
    - Revamp hidden timeout handling by adding a new timeout_style
      environment variable and a corresponding GRUB_TIMEOUT_STYLE
      configuration key for grub-mkconfig. This controls hidden-timeout
      handling more simply than the previous arrangements, and pressing any
      hotkeys associated with menu entries during the hidden timeout will
      now boot the corresponding menu entry immediately (LP: #1178618). As
      part of merging this, radically simplify the mess that
      quick_boot.patch had made of /etc/grub.d/30_os-prober; if it finds
      other OSes it can now just set timeout_style=menu and make sure the
      timeout is non-zero.
    - Fix build with FreeType 2.5.1.

 -- Colin Watson <email address hidden> Tue, 03 Dec 2013 16:53:32 +0000

Hello Hsin-Yi, or anyone else affected,

Accepted grub2 into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/grub2/1.99-21ubuntu3.14 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

confirmed that this bug is fixed by grub 1.99-21ubuntu3.14.

I verified this issue.
First I change the /etc/grub.d/20_memtest86+ like below.

-menuentry "Memory test (memtest86+)" {
+menuentry "Memory test (memtest86+)" --hotkey f9{

After update-grub and reboot, F9 could work.

Next, I change the /etc/default/grub, I added 'GRUB_TIMEOUT_STYLE=hidden'
After update-grub and reboot, F9 could work.

Next, I change the /etc/default/grub like below.
-GRUB_HIDDEN_TIMEOUT=0
GRUB_HIDDEN_TIMEOUT=10
After update-grub and reboot, F9 could work.

So, I thought this issue should be fixed.

Sounds good, thanks. Note that if you're using GRUB_TIMEOUT_STYLE, then
GRUB_HIDDEN_TIMEOUT is supposed to be ignored; once GRUB_TIMEOUT_STYLE
is set to indicate that we're in the new world, we use GRUB_TIMEOUT
consistently.

This bug was fixed in the package grub2 - 1.99-21ubuntu3.14

---------------
grub2 (1.99-21ubuntu3.14) precise; urgency=low

  * Handle FAT filesystems on non-512B disks (LP: #1065281).
  * Probe FusionIO devices (LP: #1237519).
  * On Linux, read partition start offsets from sysfs if possible
    (LP: #1237519).

grub2 (1.99-21ubuntu3.13) precise; urgency=low

  * Revamp hidden timeout handling by adding a new timeout_style environment
    variable and a corresponding GRUB_TIMEOUT_STYLE configuration key for
    grub-mkconfig. This controls hidden-timeout handling more simply than
    the previous arrangements, and pressing any hotkeys associated with menu
    entries during the hidden timeout will now boot the corresponding menu
    entry immediately (LP: #1178618). As part of merging this, radically
    simplify /etc/grub.d/30_os-prober; if it finds other OSes it can now
    just set timeout_style=menu and make sure the timeout is non-zero.
  * Fix mismerge of GRUB_RECOVERY_TITLE option in 1.99-21ubuntu3.12.

grub2 (1.99-21ubuntu3.12) precise; urgency=low

  * debian/build-efi-images: Where possible, make use of the device path
    derived from the EFI Loaded Image Protocol to compute the prefix
    (LP: #1097570).
  * Add GRUB_RECOVERY_TITLE option, to allow the controversial "recovery
    mode" text to be customised (LP: #1240360).
 -- Colin Watson <email address hidden> Thu, 05 Dec 2013 16:53:48 +0000

The verification of the Stable Release Update for grub2 has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.