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';
Why? :-)
key++; would be way cleaner as we ened up using it here in your patch
and somewhere below in the same loop.
Okay, getting back to your patch ... strlen(key + 1) != 0 ... does it
really happen? what's the case? 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 :-\. Even though, I understand we should treat
this case ... so I'd go for: if (key == NULL || ++key == NULL) 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.
> + 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) :-)
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
