| Message ID | 1351705520-24589-5-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 31, 2012 at 03:45:19PM -0200, Luiz Capitulino wrote: > This commit prepares ga_channel_new(), ga_channel_read() and > ga_channel_open() for isa-serial support. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> Though I'm wondering if it'd be nicer to: 1) Add a GAChannelOps struct with pointers for init/read/write/write_all 2) Move all the method-specific init stuff from ga_channel_new() into method-specific init functions (which all use ga_channel_open() as a general helper function, or maybe roll the CreateFile() into the init routines with the proper flags hard-coded 3) Have ga_channel_new() simply hang the proper GAChannelOps* off of the GAChannel it create and then call GAChannel->ops->init(self) 4) Make ga_channel_read/write/write_all(self) wrappers around GAChannel->ops->read/write/write_all(self) > --- > qga/channel-win32.c | 75 ++++++++++++++++++++++++++++++----------------------- > 1 file changed, 42 insertions(+), 33 deletions(-) > > diff --git a/qga/channel-win32.c b/qga/channel-win32.c > index 8e259c3..c173270 100644 > --- a/qga/channel-win32.c > +++ b/qga/channel-win32.c > @@ -213,14 +213,20 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count) > return G_IO_STATUS_ERROR; > } > > - *count = to_read = MIN(size, rs->pending); > - if (to_read) { > - memcpy(buf, rs->buf + rs->cur, to_read); > - rs->cur += to_read; > - rs->pending -= to_read; > - status = G_IO_STATUS_NORMAL; > - } else { > - status = G_IO_STATUS_AGAIN; > + switch (c->method) { > + case GA_CHANNEL_VIRTIO_SERIAL: > + *count = to_read = MIN(size, rs->pending); > + if (to_read) { > + memcpy(buf, rs->buf + rs->cur, to_read); > + rs->cur += to_read; > + rs->pending -= to_read; > + status = G_IO_STATUS_NORMAL; > + } else { > + status = G_IO_STATUS_AGAIN; > + } > + break; > + default: > + abort(); /* impossible */ > } > > return status; > @@ -285,23 +291,11 @@ GIOStatus ga_channel_write_all(GAChannel *c, const char *buf, size_t size) > return status; > } > > -static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method, > - const gchar *path) > +static gboolean ga_channel_open(GAChannel *c, const gchar *path, int flags) > { > - if (!method == GA_CHANNEL_VIRTIO_SERIAL) { > - g_critical("unsupported communication method"); > - return false; > - } > - > c->handle = CreateFile(path, GENERIC_READ | GENERIC_WRITE, 0, NULL, > - OPEN_EXISTING, > - FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL); > - if (c->handle == INVALID_HANDLE_VALUE) { > - g_critical("error opening path"); > - return false; > - } > - > - return true; > + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING | flags, NULL); > + return (c->handle == INVALID_HANDLE_VALUE) ? false : true; > } > > GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > @@ -310,27 +304,42 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > GAChannel *c = g_malloc0(sizeof(GAChannel)); > SECURITY_ATTRIBUTES sec_attrs; > > - if (!ga_channel_open(c, method, path)) { > - g_critical("error opening channel"); > - g_free(c); > - return NULL; > + switch (method) { > + case GA_CHANNEL_VIRTIO_SERIAL: > + if (!ga_channel_open(c, path, FILE_FLAG_OVERLAPPED)) { > + g_critical("error opening channel (path: %s)", path); > + goto out_err; > + } > + > + sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); > + sec_attrs.lpSecurityDescriptor = NULL; > + sec_attrs.bInheritHandle = false; > + c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); > + if (!c->rstate.ov.hEvent) { > + g_critical("can't create event"); > + goto out_err; > + } > + > + c->source = ga_channel_create_watch_ov(c); > + break; > + default: > + g_critical("unsupported communication method"); > + goto out_err; > } > > c->cb = cb; > c->user_data = opaque; > c->method = method; > > - sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); > - sec_attrs.lpSecurityDescriptor = NULL; > - sec_attrs.bInheritHandle = false; > - > c->rstate.buf_size = QGA_READ_COUNT_DEFAULT; > c->rstate.buf = g_malloc(QGA_READ_COUNT_DEFAULT); > - c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); > > - c->source = ga_channel_create_watch_ov(c); > g_source_attach(c->source, NULL); > return c; > + > +out_err: > + g_free(c); > + return NULL; > } > > void ga_channel_free(GAChannel *c) > -- > 1.7.12.315.g682ce8b >
On Thu, 1 Nov 2012 11:22:13 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > On Wed, Oct 31, 2012 at 03:45:19PM -0200, Luiz Capitulino wrote: > > This commit prepares ga_channel_new(), ga_channel_read() and > > ga_channel_open() for isa-serial support. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > Though I'm wondering if it'd be nicer to: > > 1) Add a GAChannelOps struct with pointers for init/read/write/write_all > 2) Move all the method-specific init stuff from ga_channel_new() into > method-specific init functions (which all use ga_channel_open() as > a general helper function, or maybe roll the CreateFile() into the > init routines with the proper flags hard-coded > 3) Have ga_channel_new() simply hang the proper GAChannelOps* off of > the GAChannel it create and then call GAChannel->ops->init(self) > 4) Make ga_channel_read/write/write_all(self) wrappers around > GAChannel->ops->read/write/write_all(self) Can this be a future improvement? I was also wondering if it would be good to have GAOps instead of having global functions for win32 & posix. > > > --- > > qga/channel-win32.c | 75 ++++++++++++++++++++++++++++++----------------------- > > 1 file changed, 42 insertions(+), 33 deletions(-) > > > > diff --git a/qga/channel-win32.c b/qga/channel-win32.c > > index 8e259c3..c173270 100644 > > --- a/qga/channel-win32.c > > +++ b/qga/channel-win32.c > > @@ -213,14 +213,20 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count) > > return G_IO_STATUS_ERROR; > > } > > > > - *count = to_read = MIN(size, rs->pending); > > - if (to_read) { > > - memcpy(buf, rs->buf + rs->cur, to_read); > > - rs->cur += to_read; > > - rs->pending -= to_read; > > - status = G_IO_STATUS_NORMAL; > > - } else { > > - status = G_IO_STATUS_AGAIN; > > + switch (c->method) { > > + case GA_CHANNEL_VIRTIO_SERIAL: > > + *count = to_read = MIN(size, rs->pending); > > + if (to_read) { > > + memcpy(buf, rs->buf + rs->cur, to_read); > > + rs->cur += to_read; > > + rs->pending -= to_read; > > + status = G_IO_STATUS_NORMAL; > > + } else { > > + status = G_IO_STATUS_AGAIN; > > + } > > + break; > > + default: > > + abort(); /* impossible */ > > } > > > > return status; > > @@ -285,23 +291,11 @@ GIOStatus ga_channel_write_all(GAChannel *c, const char *buf, size_t size) > > return status; > > } > > > > -static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method, > > - const gchar *path) > > +static gboolean ga_channel_open(GAChannel *c, const gchar *path, int flags) > > { > > - if (!method == GA_CHANNEL_VIRTIO_SERIAL) { > > - g_critical("unsupported communication method"); > > - return false; > > - } > > - > > c->handle = CreateFile(path, GENERIC_READ | GENERIC_WRITE, 0, NULL, > > - OPEN_EXISTING, > > - FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL); > > - if (c->handle == INVALID_HANDLE_VALUE) { > > - g_critical("error opening path"); > > - return false; > > - } > > - > > - return true; > > + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING | flags, NULL); > > + return (c->handle == INVALID_HANDLE_VALUE) ? false : true; > > } > > > > GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > > @@ -310,27 +304,42 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > > GAChannel *c = g_malloc0(sizeof(GAChannel)); > > SECURITY_ATTRIBUTES sec_attrs; > > > > - if (!ga_channel_open(c, method, path)) { > > - g_critical("error opening channel"); > > - g_free(c); > > - return NULL; > > + switch (method) { > > + case GA_CHANNEL_VIRTIO_SERIAL: > > + if (!ga_channel_open(c, path, FILE_FLAG_OVERLAPPED)) { > > + g_critical("error opening channel (path: %s)", path); > > + goto out_err; > > + } > > + > > + sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); > > + sec_attrs.lpSecurityDescriptor = NULL; > > + sec_attrs.bInheritHandle = false; > > + c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); > > + if (!c->rstate.ov.hEvent) { > > + g_critical("can't create event"); > > + goto out_err; > > + } > > + > > + c->source = ga_channel_create_watch_ov(c); > > + break; > > + default: > > + g_critical("unsupported communication method"); > > + goto out_err; > > } > > > > c->cb = cb; > > c->user_data = opaque; > > c->method = method; > > > > - sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); > > - sec_attrs.lpSecurityDescriptor = NULL; > > - sec_attrs.bInheritHandle = false; > > - > > c->rstate.buf_size = QGA_READ_COUNT_DEFAULT; > > c->rstate.buf = g_malloc(QGA_READ_COUNT_DEFAULT); > > - c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); > > > > - c->source = ga_channel_create_watch_ov(c); > > g_source_attach(c->source, NULL); > > return c; > > + > > +out_err: > > + g_free(c); > > + return NULL; > > } > > > > void ga_channel_free(GAChannel *c) > > -- > > 1.7.12.315.g682ce8b > > >
On Thu, Nov 01, 2012 at 02:33:29PM -0200, Luiz Capitulino wrote: > On Thu, 1 Nov 2012 11:22:13 -0500 > Michael Roth <mdroth@linux.vnet.ibm.com> wrote: > > > On Wed, Oct 31, 2012 at 03:45:19PM -0200, Luiz Capitulino wrote: > > > This commit prepares ga_channel_new(), ga_channel_read() and > > > ga_channel_open() for isa-serial support. > > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > > > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > > > Though I'm wondering if it'd be nicer to: > > > > 1) Add a GAChannelOps struct with pointers for init/read/write/write_all > > 2) Move all the method-specific init stuff from ga_channel_new() into > > method-specific init functions (which all use ga_channel_open() as > > a general helper function, or maybe roll the CreateFile() into the > > init routines with the proper flags hard-coded > > 3) Have ga_channel_new() simply hang the proper GAChannelOps* off of > > the GAChannel it create and then call GAChannel->ops->init(self) > > 4) Make ga_channel_read/write/write_all(self) wrappers around > > GAChannel->ops->read/write/write_all(self) > > Can this be a future improvement? I was also wondering if it would be good > to have GAOps instead of having global functions for win32 & posix. Sure, I wouldn't hold up the series for it. I agree it seems like a good future improvement. > > > > > > --- > > > qga/channel-win32.c | 75 ++++++++++++++++++++++++++++++----------------------- > > > 1 file changed, 42 insertions(+), 33 deletions(-) > > > > > > diff --git a/qga/channel-win32.c b/qga/channel-win32.c > > > index 8e259c3..c173270 100644 > > > --- a/qga/channel-win32.c > > > +++ b/qga/channel-win32.c > > > @@ -213,14 +213,20 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count) > > > return G_IO_STATUS_ERROR; > > > } > > > > > > - *count = to_read = MIN(size, rs->pending); > > > - if (to_read) { > > > - memcpy(buf, rs->buf + rs->cur, to_read); > > > - rs->cur += to_read; > > > - rs->pending -= to_read; > > > - status = G_IO_STATUS_NORMAL; > > > - } else { > > > - status = G_IO_STATUS_AGAIN; > > > + switch (c->method) { > > > + case GA_CHANNEL_VIRTIO_SERIAL: > > > + *count = to_read = MIN(size, rs->pending); > > > + if (to_read) { > > > + memcpy(buf, rs->buf + rs->cur, to_read); > > > + rs->cur += to_read; > > > + rs->pending -= to_read; > > > + status = G_IO_STATUS_NORMAL; > > > + } else { > > > + status = G_IO_STATUS_AGAIN; > > > + } > > > + break; > > > + default: > > > + abort(); /* impossible */ > > > } > > > > > > return status; > > > @@ -285,23 +291,11 @@ GIOStatus ga_channel_write_all(GAChannel *c, const char *buf, size_t size) > > > return status; > > > } > > > > > > -static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method, > > > - const gchar *path) > > > +static gboolean ga_channel_open(GAChannel *c, const gchar *path, int flags) > > > { > > > - if (!method == GA_CHANNEL_VIRTIO_SERIAL) { > > > - g_critical("unsupported communication method"); > > > - return false; > > > - } > > > - > > > c->handle = CreateFile(path, GENERIC_READ | GENERIC_WRITE, 0, NULL, > > > - OPEN_EXISTING, > > > - FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL); > > > - if (c->handle == INVALID_HANDLE_VALUE) { > > > - g_critical("error opening path"); > > > - return false; > > > - } > > > - > > > - return true; > > > + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING | flags, NULL); > > > + return (c->handle == INVALID_HANDLE_VALUE) ? false : true; > > > } > > > > > > GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > > > @@ -310,27 +304,42 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, > > > GAChannel *c = g_malloc0(sizeof(GAChannel)); > > > SECURITY_ATTRIBUTES sec_attrs; > > > > > > - if (!ga_channel_open(c, method, path)) { > > > - g_critical("error opening channel"); > > > - g_free(c); > > > - return NULL; > > > + switch (method) { > > > + case GA_CHANNEL_VIRTIO_SERIAL: > > > + if (!ga_channel_open(c, path, FILE_FLAG_OVERLAPPED)) { > > > + g_critical("error opening channel (path: %s)", path); > > > + goto out_err; > > > + } > > > + > > > + sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); > > > + sec_attrs.lpSecurityDescriptor = NULL; > > > + sec_attrs.bInheritHandle = false; > > > + c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); > > > + if (!c->rstate.ov.hEvent) { > > > + g_critical("can't create event"); > > > + goto out_err; > > > + } > > > + > > > + c->source = ga_channel_create_watch_ov(c); > > > + break; > > > + default: > > > + g_critical("unsupported communication method"); > > > + goto out_err; > > > } > > > > > > c->cb = cb; > > > c->user_data = opaque; > > > c->method = method; > > > > > > - sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); > > > - sec_attrs.lpSecurityDescriptor = NULL; > > > - sec_attrs.bInheritHandle = false; > > > - > > > c->rstate.buf_size = QGA_READ_COUNT_DEFAULT; > > > c->rstate.buf = g_malloc(QGA_READ_COUNT_DEFAULT); > > > - c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); > > > > > > - c->source = ga_channel_create_watch_ov(c); > > > g_source_attach(c->source, NULL); > > > return c; > > > + > > > +out_err: > > > + g_free(c); > > > + return NULL; > > > } > > > > > > void ga_channel_free(GAChannel *c) > > > -- > > > 1.7.12.315.g682ce8b > > > > > >
diff --git a/qga/channel-win32.c b/qga/channel-win32.c index 8e259c3..c173270 100644 --- a/qga/channel-win32.c +++ b/qga/channel-win32.c @@ -213,14 +213,20 @@ GIOStatus ga_channel_read(GAChannel *c, char *buf, size_t size, gsize *count) return G_IO_STATUS_ERROR; } - *count = to_read = MIN(size, rs->pending); - if (to_read) { - memcpy(buf, rs->buf + rs->cur, to_read); - rs->cur += to_read; - rs->pending -= to_read; - status = G_IO_STATUS_NORMAL; - } else { - status = G_IO_STATUS_AGAIN; + switch (c->method) { + case GA_CHANNEL_VIRTIO_SERIAL: + *count = to_read = MIN(size, rs->pending); + if (to_read) { + memcpy(buf, rs->buf + rs->cur, to_read); + rs->cur += to_read; + rs->pending -= to_read; + status = G_IO_STATUS_NORMAL; + } else { + status = G_IO_STATUS_AGAIN; + } + break; + default: + abort(); /* impossible */ } return status; @@ -285,23 +291,11 @@ GIOStatus ga_channel_write_all(GAChannel *c, const char *buf, size_t size) return status; } -static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method, - const gchar *path) +static gboolean ga_channel_open(GAChannel *c, const gchar *path, int flags) { - if (!method == GA_CHANNEL_VIRTIO_SERIAL) { - g_critical("unsupported communication method"); - return false; - } - c->handle = CreateFile(path, GENERIC_READ | GENERIC_WRITE, 0, NULL, - OPEN_EXISTING, - FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL); - if (c->handle == INVALID_HANDLE_VALUE) { - g_critical("error opening path"); - return false; - } - - return true; + OPEN_EXISTING, FILE_FLAG_NO_BUFFERING | flags, NULL); + return (c->handle == INVALID_HANDLE_VALUE) ? false : true; } GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, @@ -310,27 +304,42 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path, GAChannel *c = g_malloc0(sizeof(GAChannel)); SECURITY_ATTRIBUTES sec_attrs; - if (!ga_channel_open(c, method, path)) { - g_critical("error opening channel"); - g_free(c); - return NULL; + switch (method) { + case GA_CHANNEL_VIRTIO_SERIAL: + if (!ga_channel_open(c, path, FILE_FLAG_OVERLAPPED)) { + g_critical("error opening channel (path: %s)", path); + goto out_err; + } + + sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); + sec_attrs.lpSecurityDescriptor = NULL; + sec_attrs.bInheritHandle = false; + c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); + if (!c->rstate.ov.hEvent) { + g_critical("can't create event"); + goto out_err; + } + + c->source = ga_channel_create_watch_ov(c); + break; + default: + g_critical("unsupported communication method"); + goto out_err; } c->cb = cb; c->user_data = opaque; c->method = method; - sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES); - sec_attrs.lpSecurityDescriptor = NULL; - sec_attrs.bInheritHandle = false; - c->rstate.buf_size = QGA_READ_COUNT_DEFAULT; c->rstate.buf = g_malloc(QGA_READ_COUNT_DEFAULT); - c->rstate.ov.hEvent = CreateEvent(&sec_attrs, FALSE, FALSE, NULL); - c->source = ga_channel_create_watch_ov(c); g_source_attach(c->source, NULL); return c; + +out_err: + g_free(c); + return NULL; } void ga_channel_free(GAChannel *c)
This commit prepares ga_channel_new(), ga_channel_read() and ga_channel_open() for isa-serial support. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qga/channel-win32.c | 75 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 33 deletions(-)