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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
