Re: [Qemu-devel] [PULL 7/7] ps2: fix sending of PAUSE/BREAK scancodes

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Jul 27, 2017 at 04:00:25PM +0200, Gerd Hoffmann wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> The processing of the scancodes for PAUSE/BREAK  has been broken since
> the conversion to qcodes in:
> 
>   commit 8c10e0baf0260b59a4e984744462a18016662e3e
>   Author: Hervé Poussineau <hpoussin@reactos.org>
>   Date:   Thu Sep 15 22:06:26 2016 +0200
> 
>     ps2: use QEMU qcodes instead of scancodes
> 
> When using a VNC client, with the raw scancode extension, the client
> will send a scancode of 0xc6 for both PAUSE and BREAK. There is mistakenly
> no entry in the qcode_to_number table for this scancode, so
> ps2_keyboard_event() just generates a log message and discards the
> scancode
> 
> When using a SPICE client, it will also send 0xc6 for BREAK, but
> will send 0xe1 0x1d 0x45 0xe1 0x9d 0xc5 for PAUSE. There is no
> entry in the qcode_to_number table for the scancode 0xe1 because
> it is a special XT keyboard prefix not mapping to any QKeyCode.
> Again ps2_keyboard_event() just generates a log message and discards
> the scancode. The following 0x1d, 0x45, 0x9d, 0xc5 scancodes get
> handled correctly. Rather than trying to handle 3 byte sequences
> of scancodes in the PS/2 driver, special case the SPICE input
> code so that it captures the 3 byte pause sequence and turns it
> into a Pause QKeyCode.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> Message-id: 20170727113243.23991-1-berrange@redhat.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/keymaps.h      |  1 +
>  ui/input-keymap.c |  1 +
>  ui/spice-input.c  | 20 ++++++++++++++++++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/ui/keymaps.h b/ui/keymaps.h
> index 47d061343e..8757465529 100644
> --- a/ui/keymaps.h
> +++ b/ui/keymaps.h
> @@ -59,6 +59,7 @@ typedef struct {
>  /* "grey" keys will usually need a 0xe0 prefix */
>  #define SCANCODE_GREY   0x80
>  #define SCANCODE_EMUL0  0xE0
> +#define SCANCODE_EMUL1  0xE1
>  /* "up" flag */
>  #define SCANCODE_UP     0x80
>  
> diff --git a/ui/input-keymap.c b/ui/input-keymap.c
> index f96adf4165..0d9ddde9c9 100644
> --- a/ui/input-keymap.c
> +++ b/ui/input-keymap.c
> @@ -233,6 +233,7 @@ static const int qcode_to_number[] = {
>      [Q_KEY_CODE_KP_ENTER] = 0x9c,
>      [Q_KEY_CODE_KP_DECIMAL] = 0x53,
>      [Q_KEY_CODE_SYSRQ] = 0x54,
> +    [Q_KEY_CODE_PAUSE] = 0xc6,
>  
>      [Q_KEY_CODE_KP_0] = 0x52,
>      [Q_KEY_CODE_KP_1] = 0x4f,
> diff --git a/ui/spice-input.c b/ui/spice-input.c
> index 918580239d..cda9976469 100644
> --- a/ui/spice-input.c
> +++ b/ui/spice-input.c
> @@ -32,6 +32,7 @@ typedef struct QemuSpiceKbd {
>      SpiceKbdInstance sin;
>      int ledstate;
>      bool emul0;
> +    size_t pauseseq;
>  } QemuSpiceKbd;
>  
>  static void kbd_push_key(SpiceKbdInstance *sin, uint8_t frag);
> @@ -64,6 +65,25 @@ static void kbd_push_key(SpiceKbdInstance *sin, uint8_t scancode)
>          keycode |= SCANCODE_GREY;
>      }
>  
> +    if (scancode == SCANCODE_EMUL1) {
> +        kbd->pauseseq++;
> +        return;
> +    } else if (kbd->pauseseq == 1) {
> +        if (keycode == 0x1d) {
> +            kbd->pauseseq++;
> +            return;
> +        } else {
> +            kbd->pauseseq = 0;
> +        }
> +    } else if (kbd->pauseseq == 2) {
> +        if (keycode == 0x45) {
> +            qemu_input_event_send_key_qcode(NULL, Q_KEY_CODE_PAUSE, !up);
> +            kbd->pauseseq = 0;
> +            return;
> +        }
> +        kbd->pauseseq = 0;
> +    }
> +
>      qemu_input_event_send_key_number(NULL, keycode, !up);
>  }

Self-NACK.

This impl is unfortunately wrong because it sends two Q_KEY_CODE_PAUSE
events, because it mistakenly considered the make + break codes as
separate sequences.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|