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

[Philippe Mathieu-Daudé]

On 11/10/2018 17:34, Paolo Bonzini wrote:
> 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.

OK! thanks for sorting this :)