Comment 11 for bug 1178618

Revision history for this message
Colin Watson (cjwatson) wrote :

(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.)