[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [qemu-s390x] [Qemu-devel] [PATCH] chardev: Convert IOReadHandler to
Re: [qemu-s390x] [Qemu-devel] [PATCH] chardev: Convert IOReadHandler to read an unsigned number of bytes
Thu, 11 Oct 2018 17:53:20 +0200
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0
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 :)