Re: [Qemu-devel] [PATCH 03/20] char: add IOWatchPoll support

[Amit Shah <amit.shah@redhat.com>]

On (Fri) 29 Mar 2013 [07:24:07], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote:
> >> From: Anthony Liguori <aliguori@us.ibm.com>
> >> 
> >> This is a special GSource that supports CharDriverState style
> >> poll callbacks.
> >> 
> >> For reviewability and bisectability, this code is #if 0'd out in this
> >> patch to avoid unused warnings since all of the functions are static.
> >> 
> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >
> >
> >> +static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
> >> +{
> >> +    GIOStatus status;
> >> +    gsize bytes_written;
> >> +    int len;
> >> +    const uint8_t *buf = _buf;
> >> +
> >> +    len = len1;
> >> +    while (len > 0) {
> >> +        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
> >> +                                          &bytes_written, NULL);
> >> +        if (status != G_IO_STATUS_NORMAL) {
> >> +            if (status != G_IO_STATUS_AGAIN) {
> >> +                return -1;
> >> +            }
> >
> > It's not quite right to return -1 here; previous iterations of the
> > while loop could have successfully written data, and (len1 - len)
> > could be +ve.
> 
> Once -1 is returned, it's a terminal error.  It doesn't matter that we
> may have written some data.

Why do you say that?

Try this:

Start a guest with a virtio-serial channel.

In the guest, start writing something to the channel, say a 1M file.

In the host, open the chardev but don't read from it.

This will make the guest send some data, fill the vq and the char
buffers, and then make it stop.

The flow control code in virtio-console.c and virtio-serial-bus.c will
be triggered, and a watch will be registered for the pending data to
be sent once the chardev becomes writable.

If one starts reading from the chardev, we'll get duplicate data in
the current case (16 bytes were written, 20 not, but a retry for all
the 36 bytes will be tried in this case).

Instead, we only want to continue writing from the offset that was
last written.

Having a chardev open but not reading from it causing data to stop
flowing isn't a terminal error.

		Amit