Re: [PATCH v2 3/5] msmouse: Use fifo8 instead of array

[Volker Rümelin]

Am 08.09.22 um 19:31 schrieb Arwed Meyer:

@@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
  static void msmouse_chr_accept_input(Chardev *chr)
  {
      MouseChardev *mouse = MOUSE_CHARDEV(chr);
-    int len;
+    uint32_t len_out, len;
-    len = qemu_chr_be_can_write(chr);
-    if (len > mouse->outlen) {
-        len = mouse->outlen;
-    }
-    if (!len) {
+    len_out = qemu_chr_be_can_write(chr);
+    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
          return;
      }
-
-    qemu_chr_be_write(chr, mouse->outbuf, len);
-    mouse->outlen -= len;
-    if (mouse->outlen) {
-        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
-    }
+    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
+    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
+            len_out);

Hi Arwed,

I think C function arguments are not evaluated in a defined order. It's 
not defined if the third argument of function qemu_chr_be_write() is 
len_out before or after the call to fifo8_pop_buf().

The fifo_pop_buf() function uses a ringbuffer. When the buffer wraps 
around at the end and the ringbuffer contains more than one byte you may 
need two fifo8_pop_buf() and qemu_chr_be_write() calls to write all 
bytes. The code you replace doesn't have that problem.

Some chardev frontends don't return the total number of bytes to write 
in qemu_chr_be_can_write(). They return the number of bytes that can be 
written with one qemu_chr_be_write() call. You need another 
qemu_chr_be_can_write() call after the qemu_chr_be_write() call to see 
if more bytes can be written.

The code in function gd_vc_send_chars() in ui/gtk.c could be used as a 
template to avoid the three issues above.

With best regards,
Volker

  }

  static void msmouse_queue_event(MouseChardev *mouse)