From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TU0VY-0008PZ-Ba for qemu-devel@nongnu.org; Thu, 01 Nov 2012 15:32:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TU0VW-0006Pf-Cs for qemu-devel@nongnu.org; Thu, 01 Nov 2012 15:32:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TU0VW-0006Pa-4Z for qemu-devel@nongnu.org; Thu, 01 Nov 2012 15:32:50 -0400 Date: Thu, 1 Nov 2012 17:33:24 -0200 From: Luiz Capitulino Message-ID: <20121101173324.5500c992@doriath.home> In-Reply-To: <20121101191352.GL16157@illuin> References: <1351705520-24589-1-git-send-email-lcapitulino@redhat.com> <1351705520-24589-6-git-send-email-lcapitulino@redhat.com> <20121101163105.GK16157@illuin> <20121101145520.333245b7@doriath.home> <20121101191352.GL16157@illuin> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: qemu-devel@nongnu.org On Thu, 1 Nov 2012 14:13:52 -0500 Michael Roth wrote: > On Thu, Nov 01, 2012 at 02:55:20PM -0200, Luiz Capitulino wrote: > > On Thu, 1 Nov 2012 11:31:05 -0500 > > Michael Roth wrote: > > > > > On Wed, Oct 31, 2012 at 03:45:20PM -0200, Luiz Capitulino wrote: > > > > It's implemented by opening the serial port in "non-blocking" mode > > > > and using the GSourceFuncs API in the following way: > > > > > > > > o prepare(): issue a read request. If something has been read, it's > > > > stored in rs->buf and rs->pending will store the number > > > > of bytes read. If there wasn't any bytes available, we > > > > set the timeout to 500 ms and return > > > > > > > > o check(): returns true if prepare() was able to read anything, > > > > otherwise return false > > > > > > > > o finalize(): does nothing > > > > > > > > At dispatch() time we simply copy the bytes read to the buffer passed > > > > to ga_channel_read(). > > > > > > > > Signed-off-by: Luiz Capitulino > > > > --- > > > > qga/channel-win32.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 87 insertions(+) > > > > > > > > diff --git a/qga/channel-win32.c b/qga/channel-win32.c > > > > index c173270..887fe5f 100644 > > > > --- a/qga/channel-win32.c > > > > +++ b/qga/channel-win32.c > > > > @@ -179,6 +179,46 @@ static gboolean ga_channel_dispatch_ov(GSource *source, GSourceFunc unused, > > > > return success; > > > > } > > > > > > > > +static gboolean ga_channel_prepare(GSource *source, gint *timeout_ms) > > > > +{ > > > > + GAWatch *watch = (GAWatch *)source; > > > > + GAChannel *c = (GAChannel *)watch->channel; > > > > + GAChannelReadState *rs = &c->rstate; > > > > + DWORD count_read = 0; > > > > + bool success; > > > > + > > > > + success = ReadFile(c->handle, rs->buf, rs->buf_size, &count_read, NULL); > > > > + if (success) { > > > > + if (count_read == 0) { > > > > + *timeout_ms = 500; > > > > + return false; > > > > + } > > > > + rs->pending = count_read; > > > > + return true; > > > > + } > > > > + > > > > + return false; > > > > +} > > > > + > > > > +static gboolean ga_channel_check(GSource *source) > > > > +{ > > > > + GAWatch *watch = (GAWatch *)source; > > > > + GAChannel *c = (GAChannel *)watch->channel; > > > > + GAChannelReadState *rs = &c->rstate; > > > > + > > > > + return !!rs->pending; > > > > +} > > > > + > > > > +static gboolean ga_channel_dispatch(GSource *source, GSourceFunc unused, > > > > + gpointer user_data) > > > > +{ > > > > + GAWatch *watch = (GAWatch *)source; > > > > + GAChannel *c = (GAChannel *)watch->channel; > > > > + > > > > + g_assert(c->cb); > > > > + return c->cb(0, c->user_data); > > > > +} > > > > + > > > > static void ga_channel_finalize_ov(GSource *source) > > > > { > > > > g_debug("finalize"); > > > > @@ -203,6 +243,30 @@ static GSource *ga_channel_create_watch_ov(GAChannel *c) > > > > return source; > > > > } > > > > > > > > +static void ga_channel_finalize(GSource *source) > > > > +{ > > > > + g_debug("finalize"); > > > > +} > > > > + > > > > +GSourceFuncs ga_channel_watch_funcs = { > > > > + ga_channel_prepare, > > > > + ga_channel_check, > > > > + ga_channel_dispatch, > > > > + ga_channel_finalize > > > > +}; > > > > + > > > > +static GSource *ga_channel_create_watch(GAChannel *c) > > > > +{ > > > > + GSource *source = g_source_new(&ga_channel_watch_funcs, sizeof(GAWatch)); > > > > + GAWatch *watch = (GAWatch *)source; > > > > + > > > > + watch->channel = c; > > > > + watch->pollfd.fd = -1; > > > > + g_source_add_poll(source, &watch->pollfd); > > > > + > > > > + return source; > > > > +} > > > > + > > > > GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count) > > > > { > > > > GAChannelReadState *rs = &c->rstate; > > > > @@ -225,6 +289,12 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count) > > > > status = G_IO_STATUS_AGAIN; > > > > } > > > > break; > > > > + case GA_CHANNEL_ISA_SERIAL: > > > > + memcpy(buf, rs->buf, rs->pending); > > > > > > Do we really want to ignore size? Seems like we can overrun their buffer > > > if it's smaller than ours. You can use rs->start to track the number of > > > bytes theyve read so far and offset from that in prepare() > > > > size == rs->buf_size, so we're ok. Although this is sensible, as both buffers > > are not tied... > > Hmm, true. But there's no contract that users of ga_channel_read() have to > use size==QGA_READ_COUNT_DEFAULT, so this could cause problems if that > ever changed. That's correct. > > > > > I think that the intermediate buffer handling complicate things, and for > > isa-serial this is not needed. What about returning the buffer from > > ga_channel_read()? > > > > I mean, instead of: > > > > gchar buf[QGA_READ_COUNT_DEFAULT+1]; > > gsize count; > > ... > > > > ga_channel_read(s->channel, buf, QGA_READ_COUNT_DEFAULT, &count); > > > > We could have: > > > > gchar *buf; > > gsize count; > > ... > > > > ga_channel_read(s->channel, &buf, &count); > > We don't expose our internal buffer for virtio-serial because if it > fills up, and there were partial reads (which might occur if size != > QGA_READ_COUNT_DEFAULT) there may be unused space at the beginning of > the buffer, so we'll shift the used portion back to the beginning to > read as much as possible for each IO request. It's minor optimization, > but only requires a few lines of code to implement so I think it makes > sense. So that prevents us from passing the buffer directly to users > in that case. I meant to do that only for isa-serial, but what you suggest below is better. > If we want to avoid this for isa-serial, I'd avoiding the use of the > internal buffer completely. What you might be able to do instead is: > > - have your prepare() function call ClearCommError(), then check the returned > lpStat information for the cbInQue value, which will give you the number of > bytes available. If it's > 0, prepare() can set c->pending to this > amount, and avoid the ReadFile() or using the internal buffer. I can't tell you how much time I spent looking for a function that would give me that info! This is really the right of of implementing this. > - in ga_channel_read(), you can read MIN(size, c->pending) bytes directly > into the buf it's called with via ReadFile(). > > > > > > > > > > + *count = rs->pending; > > > > + rs->pending = 0; > > > > + status = G_IO_STATUS_NORMAL; > > > > + break; > > > > default: > > > > abort(); /* impossible */ > > > > } > > > > @@ -303,6 +373,7 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > > > > { > > > > GAChannel *c = g_malloc0(sizeof(GAChannel)); > > > > SECURITY_ATTRIBUTES sec_attrs; > > > > + COMMTIMEOUTS timeouts; > > > > > > > > switch (method) { > > > > case GA_CHANNEL_VIRTIO_SERIAL: > > > > @@ -322,6 +393,22 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > > > > > > > > c->source = ga_channel_create_watch_ov(c); > > > > break; > > > > + case GA_CHANNEL_ISA_SERIAL: > > > > + if (!ga_channel_open(c, path, 0)) { > > > > + g_critical("error opening channel (path: %s)", path); > > > > + goto out_err; > > > > + } > > > > + > > > > + /* non-blocking I/O */ > > > > + memset(&timeouts, 0, sizeof(timeouts)); > > > > + timeouts.ReadIntervalTimeout = MAXDWORD; > > > > + if (!SetCommTimeouts(c->handle, &timeouts)) { > > > > + CloseHandle(c->handle); > > > > + goto out_err; > > > > + } > > > > + > > > > + c->source = ga_channel_create_watch(c); > > > > + break; > > > > default: > > > > g_critical("unsupported communication method"); > > > > goto out_err; > > > > -- > > > > 1.7.12.315.g682ce8b > > > > > > > > > > > > > > > >