[Qemu-devel] [PATCH v2] Fix input-linux reading from device

[Qemu-devel] [PATCH v2] Fix input-linux reading from device

[Javier Celaya]

The evdev devices in input-linux.c are read in blocks of one whole
event. If there are not enough bytes available, they are discarded,
instead of being kept for the next read operation. This results in
lost events, of even non-working devices.

This patch keeps track of the number of bytes to be read to fill up
a whole event, and then handle it.

Signed-off-by: Javier Celaya <jcelaya@gmail.com>
---
 ui/input-linux.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/ui/input-linux.c b/ui/input-linux.c
index ac31f47719..02b0d4b2fe 100644
--- a/ui/input-linux.c
+++ b/ui/input-linux.c
@@ -169,6 +169,8 @@ struct InputLinux {
     bool        has_abs_x;
     int         num_keys;
     int         num_btns;
+    struct input_event event;
+    int         to_be_read;
 
     QTAILQ_ENTRY(InputLinux) next;
 };
@@ -327,25 +329,30 @@ static void input_linux_handle_mouse(InputLinux *il, struct input_event *event)
 static void input_linux_event(void *opaque)
 {
     InputLinux *il = opaque;
-    struct input_event event;
     int rc;
+    int offset;
+    uint8_t *p = (uint8_t *)&il->event;
 
     for (;;) {
-        rc = read(il->fd, &event, sizeof(event));
-        if (rc != sizeof(event)) {
+        offset = sizeof(il->event) - il->to_be_read;
+        rc = read(il->fd, &p[offset], il->to_be_read);
+        if (rc != il->to_be_read) {
             if (rc < 0 && errno != EAGAIN) {
                 fprintf(stderr, "%s: read: %s\n", __func__, strerror(errno));
                 qemu_set_fd_handler(il->fd, NULL, NULL, NULL);
                 close(il->fd);
+            } else if (rc > 0){
+                il->to_be_read -= rc;
             }
             break;
         }
+        il->to_be_read = sizeof(il->event);
 
         if (il->num_keys) {
-            input_linux_handle_keyboard(il, &event);
+            input_linux_handle_keyboard(il, &il->event);
         }
         if (il->has_rel_x && il->num_btns) {
-            input_linux_handle_mouse(il, &event);
+            input_linux_handle_mouse(il, &il->event);
         }
     }
 }
@@ -417,6 +424,7 @@ static void input_linux_complete(UserCreatable *uc, Error **errp)
         }
     }
 
+    il->to_be_read = sizeof(il->event);
     qemu_set_fd_handler(il->fd, input_linux_event, NULL, il);
     if (il->keycount) {
         /* delay grab until all keys are released */
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2] Fix input-linux reading from device

[Gerd Hoffmann]

On So, 2017-03-26 at 11:53 +0200, Javier Celaya wrote:
> The evdev devices in input-linux.c are read in blocks of one whole
> event. If there are not enough bytes available, they are discarded,
> instead of being kept for the next read operation. This results in
> lost events, of even non-working devices.

Have you seen this happening in practice?

> +    struct input_event event;
> +    int         to_be_read;

I'd suggest to store offset (i.e. bytes already read) instead, should
make the whole logic a bit simpler and easier to read.

> +            } else if (rc > 0){

checkpatch.pl complains here:
ERROR: space required before the open brace '{'

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] Fix input-linux reading from device

[Javier Celaya]

Hi

Javi

2017-03-27 12:11 GMT+02:00 Gerd Hoffmann <kraxel@redhat.com>:

> On So, 2017-03-26 at 11:53 +0200, Javier Celaya wrote:
> > The evdev devices in input-linux.c are read in blocks of one whole
> > event. If there are not enough bytes available, they are discarded,
> > instead of being kept for the next read operation. This results in
> > lost events, of even non-working devices.
>
> Have you seen this happening in practice?
>

Yes, quite frequently, like once per hour. Totally destroys a good gaming
session :)
The curious thing is, the mouse stops working, but in the keyboard I see
some missing keyup events (the keys get stuck), but then it recovers.


>
> > +    struct input_event event;
> > +    int         to_be_read;
>
> I'd suggest to store offset (i.e. bytes already read) instead, should
> make the whole logic a bit simpler and easier to read.
>

OK


>
> > +            } else if (rc > 0){
>
> checkpatch.pl complains here:
> ERROR: space required before the open brace '{'
>

Oops, missed that


>
> cheers,
>   Gerd
>
>

Re: [Qemu-devel] [PATCH v2] Fix input-linux reading from device

[Philippe Mathieu-Daudé]

Hi Javier,

can you add a line to explain what did you change between v1/v2?

Thank,

Phil.

On 03/26/2017 06:53 AM, Javier Celaya wrote:
> The evdev devices in input-linux.c are read in blocks of one whole
> event. If there are not enough bytes available, they are discarded,
> instead of being kept for the next read operation. This results in
> lost events, of even non-working devices.
>
> This patch keeps track of the number of bytes to be read to fill up
> a whole event, and then handle it.
>
> Signed-off-by: Javier Celaya <jcelaya@gmail.com>
> ---
>  ui/input-linux.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/ui/input-linux.c b/ui/input-linux.c
> index ac31f47719..02b0d4b2fe 100644
> --- a/ui/input-linux.c
> +++ b/ui/input-linux.c
> @@ -169,6 +169,8 @@ struct InputLinux {
>      bool        has_abs_x;
>      int         num_keys;
>      int         num_btns;
> +    struct input_event event;
> +    int         to_be_read;
>
>      QTAILQ_ENTRY(InputLinux) next;
>  };
> @@ -327,25 +329,30 @@ static void input_linux_handle_mouse(InputLinux *il, struct input_event *event)
>  static void input_linux_event(void *opaque)
>  {
>      InputLinux *il = opaque;
> -    struct input_event event;
>      int rc;
> +    int offset;
> +    uint8_t *p = (uint8_t *)&il->event;
>
>      for (;;) {
> -        rc = read(il->fd, &event, sizeof(event));
> -        if (rc != sizeof(event)) {
> +        offset = sizeof(il->event) - il->to_be_read;
> +        rc = read(il->fd, &p[offset], il->to_be_read);
> +        if (rc != il->to_be_read) {
>              if (rc < 0 && errno != EAGAIN) {
>                  fprintf(stderr, "%s: read: %s\n", __func__, strerror(errno));
>                  qemu_set_fd_handler(il->fd, NULL, NULL, NULL);
>                  close(il->fd);
> +            } else if (rc > 0){
> +                il->to_be_read -= rc;
>              }
>              break;
>          }
> +        il->to_be_read = sizeof(il->event);
>
>          if (il->num_keys) {
> -            input_linux_handle_keyboard(il, &event);
> +            input_linux_handle_keyboard(il, &il->event);
>          }
>          if (il->has_rel_x && il->num_btns) {
> -            input_linux_handle_mouse(il, &event);
> +            input_linux_handle_mouse(il, &il->event);
>          }
>      }
>  }
> @@ -417,6 +424,7 @@ static void input_linux_complete(UserCreatable *uc, Error **errp)
>          }
>      }
>
> +    il->to_be_read = sizeof(il->event);
>      qemu_set_fd_handler(il->fd, input_linux_event, NULL, il);
>      if (il->keycount) {
>          /* delay grab until all keys are released */
>