Hi Vladimir, I don't know if l lose any update from you. Would you give me comments about the latest hotkey patch?
Summary of the patch:
* allow function keys / delete / backspace to interrupt sleep and set
'hotkey' variable.
* remove key aliases structure.
* using grub_xasprintf instead of grub_snprintf
* unset the 'hotkey' variable quickly when it is unneeded in
grub_menu_get_hotkey
Many thanks!
Franz Hsieh
On Mon, Oct 14, 2013 at 2:02 PM, Franz Hsieh <franz.hs...@canonical.com>wrote:
> On 02.10.2013 10:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>
> >>* 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.*
> >>* *
>
> > Have you missed on-going opposition, discussion and improvement
> > proposals from me?
> > I was going to revert the patch but found out that you didn't actually
> > commit it, please see my comments to make it acceptable and don't push it.
>
> Sorry I did not say it clearly, I downloaded sources from grub-trunk, add my
> changes,
> and finished the patch file. I haven't committed the patch to trunk yet.
>
> I looked at the codes, there is no ungetkey function to do like ungetc. So
> currently
> I made it to read all keys but only accept function keys / delete / backspace
> and Esc
> keys to interrupt the sleep thread.
>
> Only function keys / delete key / backspace key are allowed for hotkey
>
>
> and will be saved to the hotkey environment variable.
>
>
>
> On Wed, Oct 2, 2013 at 4:03 PM, Franz Hsieh <franz.hs...@canonical.com>wrote:
>
>> 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
>> <franz.hs...@canonical.com>wrote:
>>
>>>
>>>
>>>
>>> On Wed, Sep 11, 2013 at 9:31 PM, Colin Watson <cjwat...@ubuntu.com>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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
