Re: [qemu-s390x] [Qemu-devel] [PATCH] chardev: Convert IOReadHandler to read an unsigned number of bytes

[Paolo Bonzini]

On 11/10/2018 17:24, Philippe Mathieu-Daudé wrote:
>> On the other hand, fd_chr_read_poll is not an IOCanReadHandler, and this
>> patch therefore probably doesn't compile?
> It does compile.
> 
> fd_chr_update_read_handler() uses fd_chr_read_poll with io_add_watch_poll():
> 
> GSource *io_add_watch_poll(Chardev *chr,
>                         QIOChannel *ioc,
>                         IOCanReadHandler *fd_can_read,
>                         QIOChannelFunc fd_read,
>                         gpointer user_data,
>                         GMainContext *context);

Oh, that's somewhat weird.  It could just as well return a bool.

However, this made me notice that you need to change e.g. s->max_size's
declaration (in include/chardev/char-fd.h) from int to size_t, and
likewise for: 1) all users of s->max_size, such as len in fd_chr_read;
2) all the similar variables in other char backends.

So it's probably best to structure the series as follows:

1) change fd_can_read from IOCanReadHandler to a GSourceFunc (which
returns a boolean value), changing all "return s->foo" to "return s->foo
> 0;".  Then you can remove the > 0 from

    bool now_active = iwp->fd_can_read(iwp->opaque) > 0;

(Having the > 0 repeated in all backends is now a bit ugly, but there
are future cleanup opportunities here to move the qemu_chr_be_can_write
call to qemu_chr_be_can_write; this way most chardev backends can skip
defining a read_poll function.  But I digress).

2) assert in qemu_chr_be_can_write that the returned value is >= 0

3) for each backend, change the assigned variable from int to size_t

4) now the rest of your patch, touching all front-ends.  The assertion
from (2) now does not make sense anymore, since ->can_read returns an
unsigned value, but perhaps you can keep a "fail-safe" assertion that
(ssize_t)returned_value >= 0 to catch undesired overflows.

Paolo