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

[Arwed Meyer]

Am 11.09.22 um 08:12 schrieb 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)

Hi Volker,

have to admit I was not completely sure about the order in which
function arguments get evaluated either. Thanks for the confirmation.
I'll change this to be safe.

I guess you probably won't see much difference between old and new code
in practice since serial I/O is slow anyway, but since I'll change the
patch anyway, I'll take the code from gd_vc_send_chars. Indeed looks better.