client gets broken keycodes #666

Open
jrosenboom opened this Issue Sep 20, 2016 · 35 comments

Projects

None yet

10 participants

@jrosenboom

See https://bugs.launchpad.net/horizon/+bug/1622684 for a detailed description, result is messages like

[ 41.750245] atkbd serio0: Unknown key released (translated set 2,
code 0x0 on isa0060/serio0).
[ 41.750591] atkbd serio0: Use 'setkeycodes 00 <keycode>' to make it known.

being shown on the console. Reverting to 4e0c36d solves the issue.

@samhed
Member
samhed commented Sep 20, 2016

@devicenull seems to have the same issue in #596

I'm quoting @danielhb here:

If you are using the qemu keyboard option when starting the guest, remove it

@DirectXMan12 DirectXMan12 added the bug label Sep 20, 2016
@SilviaFc

What if I'm using devstack to install openstack? How to revert the version?
Thanks

@devicenull
Contributor

I haven't actually had time to go and gather debug logs for you. I can tell you we do not have a keyboard/keymap option present on the qemu command line

@danielhb
Contributor
danielhb commented Sep 20, 2016 edited

I will go ahead and try installing Openstack Horizon to see if I can reproduce it here when I have the chance. Meanwhile any other information (operational system, browser version) is welcome. This info is missing from the original launchpad bug.

@devicenull
Contributor

If it helps, I can provide access (https URL, user, pw) to a websockify session that's having this issue. I can also provide SSH access to the VM itself. I cannot provide access to the host though.

@danielhb
Contributor

On 09/20/2016 01:18 PM, devicenull wrote:

If it helps, I can provide access (https URL, user, pw) to a
websockify session that's having this issue. I can also provide SSH
access to the VM itself. I cannot provide access to the host though.

Feel free to send me the credentials for this websockify session that is
experiencing the issue. I'll have a look as soon as I am able. It will
be faster than installing Horizon and hopefully will be enough.

@devicenull
Contributor

I sent you an email with login details

@danielhb
Contributor

My first findings are that the remote QEMU VNC server is expecting a field (keysym) that the code isn't supplying. I can see that because the guest is receiving the numpad keys with NumLock ON as expected, the only cases where the keysym is sent. However, we can't make the code send the keysym every time - this is exactly the problem we're trying to avoid with the extension.

What QEMU version is running on the host? Is the host running Cent OS 7/RHEL 7? This wouldn't be the first time that people complains about this code in a Cent OS 7 host, but back then I didn't find out why. I am guessing that these OSes have older QEMU versions that behaves differently from the newer ones (requiring the keysym field every time). If that's the case we can try to disable the QEMU extension in the cases where the protocol is older than a given version, falling back to the former noVNC keyboard handling. I can ask around the QEMU community to see if this is somewhat a known issue with the older versions and if there is a patch available.

@badcrc
badcrc commented Sep 21, 2016

Hi,

I updated today from an old fork of mine of noVNC and I got this problem too, every time I pressed a key I got disconnected.

I'm using xvp to connect to multiple XenServers, if I "disable" the QEMU extension (I'm just commenting _this.keyboard.setQEMUVNCKeyboardHandler(); I got wrong characters, every keypress adds '^[' in from of the actual key (s transforms into ^[s, etc..).

In the xvp log I got this error:

Sep 21 17:02:26 xvp[7956]: Error: Unrecognised client message type -1

So I just assume that xvp is old and doesn't have those qemu extensions.

@danielhb
Contributor

@badcrc to disable the code altogether you need to comment the following lines:

            if (keyboardEvent.code !== undefined) {
                this._qemuExtKeyEventSupported = true;
                this._keyboard.setQEMUVNCKeyboardHandler();
            }

Commenting only the line you've mentioned will mess up the logic due to the boolean set to 'true'. Comment all these will make noVNC fall back to the regular flow.

@badcrc
badcrc commented Sep 21, 2016

Hi,

seems to do the trick!

Thanks!

@devicenull
Contributor

@danielhb The one I gave you is running via qemu 2.5.1 . I was just able to reproduce this on a qemu 2.7.0 install as well, so I'm not sure it's necessarily caused by older versions. I'll send you the details for that

@danielhb
Contributor

@devicenull thanks, that was helpful. Yeah, it is happening in this case too - we can discard the 'version is too old' problem.

I've investigated it more and the only way I can reliably reproduce the behavior (using qemu 2.4.1 in Fedora 23) is by setting a keymap in the guest. This can be done by:

1 - the '-k' flag in the qemu command line. QEMU recommends that this flag must not be used at all after the qemu extension got implemented;

2 - keymap="..." property of the " <graphics type='vnc' (...)" element in the guest XML

I am aware that you already said that at least (1) isn't happening. Can you please check again (1) and (2)? If there's indeed no keymap being defined then we'll need to discuss an alternative.

@devicenull
Contributor

Yes, I can confirm that we have a keymap of 'en-us' set. I removed it from that first server I sent you, and it appears to be working properly now. Is there a way that can be detected via VNC?

@danielhb
Contributor

Glad it worked!

About detecting via VNC if the keymap was set in QEMU/libvirt, I don't think it's possible in the RFB (the VNC protocol) level. Libvirt recommends to get rid of all keymap definitions altogether since the QEMU extension was develop (2009 I guess) so I think it's safer to get rid of all keymap definitions of guests. In fact, I believe they stopped supporting the option altogether since then:

https://www.redhat.com/archives/libvir-list/2009-May/msg00234.html

"Our recommendation is to never set the '-k' /keymap option at all these
days. Recent QEMU / KVM and GTK-VNC releases support a VNC extension for
sending raw keycodes, instead of localized keysyms. Thus is best to leave
off the keymap in the config, and just configure it inside the guest OS."

@berrange

Removing use of keymap should NOT be needed to make extended keyboard event messages work.

The Extended keyboard event message contains both the scancode and keysym. The keysym sent should be exactly the same as the keysym sent with the non-extended keyboard event message. If QEMU has a keymap set, it will then ignore the scancode and only use the keysym. IOW, if keymap is set, QEMU will behaviour identically whether it gets a extended or non-extended keyboard event - it will always use the keysym.

If this is not working, then it suggests that the keysym being included in the extended keyboard event message is somehow different from the keysym being sent in the non-extended keyboard event message. ie a client app bug

This bit of code in core/rfb.js looks pretty suspect

_handleKeyPress: function (keyevent) {
    if (this._view_only) { return; } // View only, skip keyboard, events

    var down = (keyevent.type == 'keydown');
    if (this._qemuExtKeyEventSupported) {
        var scancode = XtScancode[keyevent.code];
        if (scancode) {
            var keysym = keyevent.keysym;
            RFB.messages.QEMUExtendedKeyEvent(this._sock, keysym, down, scancode);
        } else {
            Util.Error('Unable to find a xt scancode for code = ' + keyevent.code);
        }
    } else {
        keysym = keyevent.keysym.keysym;
        RFB.messages.keyEvent(this._sock, keysym, down);
    }
},

Notice that in the extended event scenario it is sending "keyevent.keysym" but in the non-extended scenario it is sending "keyevent.keysym.keysym". Surely those should both be keyevent.keysym.keysym

@jrosenboom

@berrange I think you are on the right track, but the real bug seems to be somewhere deeper in core/input/util.js:KeyboardUtil.QEMUKeyEventDecoder.process() which almost always sets result.keysym = 0.

If I replace that with result.keysym = evt.keyCode; I can see the proper (though upper case) letters for letter keypresses, but am still getting error messages for meta keys like shift.

@berrange

That whole bit of code looks rather crazy. Having completely separate KeyboardUtil.KeyEventDecoder and KeyboardUtil.QEMUKeyEventDecoder methods is rather dubious, as they need to have identical logic for determining the 'keysym' value.

@danielhb
Contributor
danielhb commented Sep 23, 2016 edited

I have forked the logic like that for two reasons. First one is that I didn`t want to assume that every VNC server has the QEMU extension support, so this was an easy way to not mess with the existing logic.

Second, it is not possible to determine every keysym from noVNC level for several reasons. We can't get the keymap information from the browser so, even having the keycode, we can't calculcate it. The JS attributes that gives the keysym are sometimes sent in keyPressed messages, which can be a combo of two or more key presses. This means that sometimes modifiers such as Shift or Alt will not produce any keysym. noVNC tried to speculate the keysym in this scenario in the former logic and it is very complex and it has a lot of problems.

The scenarios where keysym is sent in the QEMU extension are the numPad keys because the numPad is not layout dependent (as far as I`ve investigated) and I have experienced issues with the numLock state when ignoring the keysym field in the KeyEvent message (VNC server seems to consider keysym values to determine whether the state is On or Off).

A good question here is why a QEMU guest started with a keymap setting will behave differently from a guest without it for the same Extended KeyEvent message. While I cant tell if its a bug or intended, I don`t see a reason why not have an option here in noVNC to disable the extension option and allow noVNC to work with this kind of guests.

@berrange

As I mentioned above, if QEMU is launched with a keymap set, QEMU will behave identically whether your using the extended or non-extended keyboard VNC protocol message. If there is any difference in behaviour is is because the client is sending different data for the keysym field. This design was precisely so that clients do not need to have a flag to disable the extension.

@danielhb
Contributor

On 09/23/2016 09:32 AM, Daniel Berrange wrote:

As I mentioned above, if QEMU is launched with a keymap set, QEMU will
behave /identically/ whether your using the extended or non-extended
keyboard VNC protocol message.
I am not saying that. Let me rephrase.

Given two scenarios, both using the QEMU extension, in one of them the
guest was launched with a '-k' option and in the other without it, they
behave differently. The first scenario will check the 'keysym' field of
the QEMU KeyEvent message every time, the second won't.

@danielhb
Contributor

Poor choice of words. It's not like the second won't read the keysym field, it does (the numLock scenario I mentioned above for example). What I mean is that I can zero the keysym field and key press is interpreted.

@berrange

As the client when sending the extended key event, you have no way of knowing whether the QEMU server has a keymap set or not, so you should never zero the keysym field. You must always supply valid keysym field values.

@danielhb
Contributor

I agree, noVNC should sent valid keysym values in every case to be immune to server-side settings such as guest keymap. But it won't. As I said above, we can't reliably give a valid keysym value in noVNC due to browser limitations/specifics. This is the current state of the QEMU extension in noVNC and, unless there are browser improvements to give us reliable information to calculate the keysym in every key press, it will stay that way.

And this is why I believe that providing an option to disable this extension in noVNC is an acceptable compromise. If the user can't remove the '-k' option from the guest, he/she can turn the qemu noVNC extension off and have at least some connectivity from noVNC keysym logic.

@berrange

we can't reliably give a valid keysym value in noVNC due to browser limitations/specifics.

Prior to adding support for the extended key event to noVNC, it was capable of providing keysym values. Yes, these keysyms were not perfect due to browser limitations, but you can at least continue to provide the same data as before, when using extended key events.

@danielhb
Contributor

I am reluctant in doing that because the existing keysym calculation can't handle AZERTY, DVORAK or any other layout other that en_us derivatives. The keysym calculation is done by assuming that the underlying keymap is en_us QWERTY.

It'll break the QEMU extension for the other scenarios (guests started without '-k') that are currently working. Instead of working fine with everyone else and not working with '-k' guests (and this can be handled by disabling the extension in noVNC), we would fall back to the previous state where there was no way to use noVNC with other layouts at all. Feel free to browse through all the problems of the current keysym calculation in noVNC with non-us layouts here:

#21

@berrange

It'll break the QEMU extension for the other scenarios (guests started without '-k') that are currently working.

No, it wont. If the guest is started without -k, then the keysym field in the extended key event is completely ignored.

If you provide the keysym value in exactly the same way as noVNC has always done in the past, and also provide the scancode, then guests with -k have same behaviour they've always had, and guests without -k have the new improved behaviour.

@danielhb
Contributor

On 09/23/2016 10:33 AM, Daniel Berrange wrote:

No, it wont. If the guest is started without -k, then the keysym field
in the extended key event is completely ignored.

I can tell you that it is not completely ignored. I've fixed a numLock
issue by setting the keysym in those cases as I've said before. Zeroing
the keysym field would turn off the Num Lock state of the guest prior of
executing the key. And that in a guest without a QEMU keymap. Sending
the keysym makes the numeric keys work as expected.

I haven't investigated it further - perhaps the keysym is read only in
the numPad keys case. Seeing how the keysym calculation was done and how
much bugs it has I was very happy to zero the keysym field in the
message and not worry about it.

I'll give it a try and set the keysym to en_us values and see what
happens when using a non-us layout.

@berrange

There is a little bit of magic that happens with keysym in QEMU related to tracking modifier state which could impact NumLock, but in terms of what's actually injected into the guest, that's scancode based. Any problems in the area of NumLock would likely already be a problem with non-extended events, so that likely needs to be fixed independently.

@danielhb
Contributor

Just did a simple test by setting the keysyms of the keys QWERTY and then changing the layout of the guest from en_us to fr, which was started without the '-k' option. It worked. More testing would be needed but, at first, it seems that setting the keysym of the message to the en_us value does not affect the behavior of the guests without keymap.

Assuming that we go this route, this would mean that the current QEMU extension code would need to be integrated with the existing noVNC keysym logic (or the other way around) to retrieve the keysyms calculated by noVNC. Given that this keysym logic does not send KeyEvent messages at every keystroke (it holds key presses to calculate keysym with modifiers such as 'ç' or 'â' , it does not send dead keys right away, etc) this is not a trivial task and it will require some time to get it done right.

@danielhb
Contributor

ps: just to not give the wrong idea: I will investigate this issue (testing if sending the keysym everytime is viable, then changing the code) whenever I can but, at this moment, I can't focus in it, so no ETA. If anyone is in a hurry for this fix I suggest any of these workarounds:

  • remove the '-k' option when starting the QEMU guest;
  • disabling the current QEMU extension in those cases where the option can't be disabled (I suggest #667 )

If workaround isn't an option, feel free to help with testing and merging both logics together. As I said I will not be able to focus into this now but I'll help anyone trying to make this change as best as I can.

@kanaka
Member
kanaka commented Sep 23, 2016

Thanks everyone for trying to get to the bottom of this. @danielhb sorry for the crazy algorithm for handling key presses. Unfortunately, the state of browser key events at the time sucked and so this ended up being the best workaround. I think browser support is now pretty high for UI Events. Ostensibly that fixes some of the craziness around key handling, but I haven't had a chance (or much motivation) to look at in in depth.

@danielhb
Contributor

@kanaka no problem, I understand that the complexity of the existing keysym algorithm is justified by the poor browser support concerning keyboard events.

@DirectXMan12 DirectXMan12 added this to the Future Features milestone Sep 30, 2016
@DirectXMan12
Member

[to head off any issues about this being marked "Future Features": I've marked #667 as being in milestone v0.7.0, so at the very least we'll have a short-term fix for the next release 😉 ]

@olivierlambert

For people having problems with XenServer and consoles, we reported this: https://bugs.xenserver.org/browse/XSO-650

I hope they will remove the -k en-us, therefore allowing it to work with the latest version of noVNC.

Thanks @danielhb for the very insightful details you provided here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment