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

[Arwed Meyer <arwed.meyer@gmx.de>]

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.