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)