From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52129) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ULYdt-0005YW-8J for qemu-devel@nongnu.org; Fri, 29 Mar 2013 08:42:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ULYds-0008RH-4I for qemu-devel@nongnu.org; Fri, 29 Mar 2013 08:42:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25909) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ULYdr-0008R7-RJ for qemu-devel@nongnu.org; Fri, 29 Mar 2013 08:42:48 -0400 Date: Fri, 29 Mar 2013 18:12:43 +0530 From: Amit Shah Message-ID: <20130329124243.GF7778@amit.redhat.com> References: <9b59ac17b9d0bb3972a73fed04d415f07b391936.1362505276.git.amit.shah@redhat.com> <20130329095311.GB14019@amit.redhat.com> <87fvzefl7c.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87fvzefl7c.fsf@codemonkey.ws> Subject: Re: [Qemu-devel] [PATCH 03/20] char: add IOWatchPoll support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu list On (Fri) 29 Mar 2013 [07:24:07], Anthony Liguori wrote: > Amit Shah writes: > > > On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote: > >> From: Anthony Liguori > >> > >> 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 > >> Signed-off-by: Amit Shah > > > > > >> +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