Patchwork [4/5] qemu-ga: win32: isolate virtio-serial specific code

login
register
mail settings
Submitter Luiz Capitulino
Date Oct. 31, 2012, 5:45 p.m.
Message ID <1351705520-24589-5-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/195973/
State New
Headers show

Comments

Luiz Capitulino - Oct. 31, 2012, 5:45 p.m.
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(-)
Michael Roth - Nov. 1, 2012, 4:22 p.m.
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
>
Luiz Capitulino - Nov. 1, 2012, 4:33 p.m.
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
> > 
>
Michael Roth - Nov. 1, 2012, 7:15 p.m.
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
> > > 
> > 
>

Patch

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)