On Tue, May 31, 2016 at 8:30 AM, Pavel Grunt <pgrunt@xxxxxxxxxx> wrote: > On Tue, 2016-05-31 at 01:04 +0200, Fabiano Fidêncio wrote: >> On Mon, May 30, 2016 at 5:08 PM, Pavel Grunt <pgrunt@xxxxxxxxxx> wrote: >> > Related: rhbz#1339572 >> > --- >> > src/virt-viewer-app.c | 5 +++++ >> > tests/test-hotkeys.c | 4 ++++ >> > 2 files changed, 9 insertions(+) >> > >> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c >> > index d449f72..f8fba4c 100644 >> > --- a/src/virt-viewer-app.c >> > +++ b/src/virt-viewer-app.c >> > @@ -2077,6 +2077,11 @@ virt_viewer_app_set_hotkeys(VirtViewerApp *self, >> > const gchar *hotkeys_str) >> > gtk_accelerator_parse(accel, &accel_key, &accel_mods); >> > g_free(accel); >> > >> > + if (strlen(key + 1) != 0 && accel_key == 0 && accel_mods == 0) { >> >> Not your fault, but this key + 1 made me open the code and double >> check what's going on. >> gchar *key = strstr(*hotkey, "="); >> *key = '\0'; >> > you want to separate key from value, so you replace '=' by '\0' ("key\0value"). > If you print *hotkey, you get only the first part. That's true, hotkey is still used and I was just paying attention to the key. :-\ My bad. But I still would do: *key = '\0''; key++ instead of keeping using key + 1. > >> Why? :-) >> >> key++; would be way cleaner as we ened up using it here in your patch >> and somewhere below in the same loop. > ok, I will use key++ >> >> Okay, getting back to your patch ... strlen(key + 1) != 0 ... does it >> really happen? > > it should always happen. Let me change my question then, does it really fail at some point? Because, if it does, then we are in a situation where the hotkey is: "foo=", which shouldn't never happend. > > The condition says - if value_str is not empty and accel_key=0 and accel_key=0 > then it means the value could not be parsed as gtk accelerator. > >> what's the case? > > one can use: virt-viewer --hotkeys="toggle-fullscreen=not_an_accelerator" > which is not consider as a bug in the current code. > Okay, so, please, add this in the comment. >> I would like to see it in the commit >> log. IMO, It shouldn't happen at all. We shouldn't receive a string >> that is: "foo=" and if it does and it is provided by oVirt/RHEVM, the >> bug is there not here :-\. > > It is not about rhevm, the bug is about informing the user about incorrect usage > of hotkeys - one can have a typo in commandline and it can be hard to figure out > the issue (I know it is a stupid example). > >> Even though, I understand we should treat >> this case ... so I'd go for: if (key == NULL || ++key == NULL) > That way you would not set *key = '\0', and if key is not NULL then key + 1 is > also not NULL. Yep, as I said before, I didn't check that the hotkey would still be used, my bad. Just ignore my comment from the previous email. > >> and let >> it reach the g_warn_reached() part of the code that is just above this >> part you touched. >> >> About accel_key and accel_mode being 0, again, it only will happen if >> the parser fails. So, having the ++key == NULL in the if would save is >> this part of the code. > > Ah you mean *(++key) == '\0' Nops, it was just an way to avoid your strlen(key + 1) != 0 check, but can just be ignored. > >> >> > + g_warning("Invalid value '%s' for key '%s'", key + 1, *hotkey); >> > + continue; >> > + } >> > + >> > if (g_str_equal(*hotkey, "toggle-fullscreen")) { >> > gtk_accel_map_change_entry("<virt-viewer>/view/toggle- >> > fullscreen", accel_key, accel_mods, TRUE); >> > } else if (g_str_equal(*hotkey, "release-cursor")) { >> > diff --git a/tests/test-hotkeys.c b/tests/test-hotkeys.c >> > index d3658f5..4a45216 100644 >> > --- a/tests/test-hotkeys.c >> > +++ b/tests/test-hotkeys.c >> > @@ -96,6 +96,10 @@ test_hotkeys_bad(void) >> > "toggle-fullscreen=A,unknown_command=B", >> > G_LOG_LEVEL_WARNING, >> > "Unknown hotkey command unknown_command" >> > + },{ >> > + "secure-attention=value", >> > + G_LOG_LEVEL_WARNING, >> > + "Invalid value 'value' for key 'secure-attention'" >> > }, >> > }; >> > >> > -- >> > 2.8.3 >> > >> > _______________________________________________ >> > virt-tools-list mailing list >> > virt-tools-list@xxxxxxxxxx >> > https://www.redhat.com/mailman/listinfo/virt-tools-list >> >> ACK the code if it follows the suggestion (but feel free to convince >> me or here or on the iRC) :-) > > I would rather send v2 :) Please! > > Pavel > >> >> Best Regards, >> -- >> Fabiano Fidêncio Best Regards, _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list