| Message ID | 1351705520-24589-6-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <lcapitulino@redhat.com> > --- > 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() > + *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 > >
On Thu, 1 Nov 2012 11:31:05 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> 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 <lcapitulino@redhat.com> > > --- > > 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... 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); > > > + *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 > > > > >
On Thu, Nov 01, 2012 at 02:55:20PM -0200, Luiz Capitulino wrote: > On Thu, 1 Nov 2012 11:31:05 -0500 > Michael Roth <mdroth@linux.vnet.ibm.com> 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 <lcapitulino@redhat.com> > > > --- > > > 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. > > 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. 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. - 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 > > > > > > > > > >
On Thu, 1 Nov 2012 14:13:52 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> 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 <lcapitulino@redhat.com> > > > > --- > > > > 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 > > > > > > > > > > > > > > > >
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); + *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;
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 <lcapitulino@redhat.com> --- qga/channel-win32.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)