[Qemu-devel] [PATCH 0/5] qemu-ga: add isa-serial support to win32

[Qemu-devel] [PATCH 0/5] qemu-ga: add isa-serial support to win32

[Luiz Capitulino]

Hi,

This series adds isa-serial support to qemu-ga. This is done by opening
the serial port in "non-blocking" mode and using the GSourceFuncs API
(full details in patch 5/5).

I've tried two other simpler ways of implementing this w/o success:

 1. By more or less doing what we do in qemu-ga POSIX, but opening
    the serial port with g_io_channel_new_file()

 2. By doing exactly what we do in qemu-ga POSIX (using open()
    inclusive), but using g_io_channel_win32_new_fd() instead of
	g_io_channel_unix_new()

Both tries failed with an assertion in g_io_channel_read_chars(), saying
that the channel wasn't readable...

Finally, I've tested this series on Windows 2008 (virtio-serial and isa-serial)
and Windows Vista (isa-serial only).

Luiz Capitulino (5):
  configure: link qemu-ga.exe in default make target
  qemu-ga: win32: add _ov suffix to current GSource functions
  qemu-ga: win32: GAChannel: store GAChannelMethod being used
  qemu-ga: win32: isolate virtio-serial specific code
  qemu-ga: win32: add isa-serial support

 configure           |   2 +-
 qga/channel-win32.c | 170 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 135 insertions(+), 37 deletions(-)

-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 1/5] configure: link qemu-ga.exe in default make target

[Luiz Capitulino]

Avoids having to type "make qemu-ga.exe" after build is complete
and also cleanups qemu-ga.exe in make clean.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 3685020..d15bf5b 100755
--- a/configure
+++ b/configure
@@ -3079,7 +3079,7 @@ if test "$softmmu" = yes ; then
       virtfs=no
     fi
   fi
-  if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
+  if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
     if [ "$guest_agent" = "yes" ]; then
       tools="qemu-ga\$(EXESUF) $tools"
     fi
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 2/5] qemu-ga: win32: add _ov suffix to current GSource functions

[Luiz Capitulino]

They are specific to virtio-serial code, which implements win32
overlaped I/O. A future commit will add a different kind of I/O.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qga/channel-win32.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index 16bf44a..c0e19a8 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -38,7 +38,7 @@ typedef struct GAWatch {
  * Called by glib prior to polling to set up poll events if polling is needed.
  *
  */
-static gboolean ga_channel_prepare(GSource *source, gint *timeout_ms)
+static gboolean ga_channel_prepare_ov(GSource *source, gint *timeout_ms)
 {
     GAWatch *watch = (GAWatch *)source;
     GAChannel *c = (GAChannel *)watch->channel;
@@ -95,7 +95,7 @@ out:
 /*
  * Called by glib after an outstanding read request is completed.
  */
-static gboolean ga_channel_check(GSource *source)
+static gboolean ga_channel_check_ov(GSource *source)
 {
     GAWatch *watch = (GAWatch *)source;
     GAChannel *c = (GAChannel *)watch->channel;
@@ -152,7 +152,7 @@ static gboolean ga_channel_check(GSource *source)
 /*
  * Called by glib after either prepare or check routines signal readiness
  */
-static gboolean ga_channel_dispatch(GSource *source, GSourceFunc unused,
+static gboolean ga_channel_dispatch_ov(GSource *source, GSourceFunc unused,
                                     gpointer user_data)
 {
     GAWatch *watch = (GAWatch *)source;
@@ -178,21 +178,21 @@ static gboolean ga_channel_dispatch(GSource *source, GSourceFunc unused,
     return success;
 }
 
-static void ga_channel_finalize(GSource *source)
+static void ga_channel_finalize_ov(GSource *source)
 {
     g_debug("finalize");
 }
 
-GSourceFuncs ga_channel_watch_funcs = {
-    ga_channel_prepare,
-    ga_channel_check,
-    ga_channel_dispatch,
-    ga_channel_finalize
+GSourceFuncs ga_channel_watch_funcs_ov = {
+    ga_channel_prepare_ov,
+    ga_channel_check_ov,
+    ga_channel_dispatch_ov,
+    ga_channel_finalize_ov
 };
 
-static GSource *ga_channel_create_watch(GAChannel *c)
+static GSource *ga_channel_create_watch_ov(GAChannel *c)
 {
-    GSource *source = g_source_new(&ga_channel_watch_funcs, sizeof(GAWatch));
+    GSource *source = g_source_new(&ga_channel_watch_funcs_ov, sizeof(GAWatch));
     GAWatch *watch = (GAWatch *)source;
 
     watch->channel = c;
@@ -326,7 +326,7 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
     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(c);
+    c->source = ga_channel_create_watch_ov(c);
     g_source_attach(c->source, NULL);
     return c;
 }
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 3/5] qemu-ga: win32: GAChannel: store GAChannelMethod being used

[Luiz Capitulino]

Next commit wants this.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qga/channel-win32.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index c0e19a8..8e259c3 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -25,6 +25,7 @@ struct GAChannel {
     GAChannelReadState rstate;
     GIOCondition pending_events; /* TODO: use GAWatch.pollfd.revents */
     GSource *source;
+    GAChannelMethod method;
 };
 
 typedef struct GAWatch {
@@ -317,6 +318,7 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
 
     c->cb = cb;
     c->user_data = opaque;
+    c->method = method;
 
     sec_attrs.nLength = sizeof(SECURITY_ATTRIBUTES);
     sec_attrs.lpSecurityDescriptor = NULL;
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 4/5] qemu-ga: win32: isolate virtio-serial specific code

[Luiz Capitulino]

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(-)

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

[Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support

[Luiz Capitulino]

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);
+        *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

Re: [Qemu-devel] [PATCH 2/5] qemu-ga: win32: add _ov suffix to current GSource functions

[Michael Roth]

On Wed, Oct 31, 2012 at 03:45:17PM -0200, Luiz Capitulino wrote:
> They are specific to virtio-serial code, which implements win32
> overlaped I/O. A future commit will add a different kind of I/O.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/channel-win32.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/qga/channel-win32.c b/qga/channel-win32.c
> index 16bf44a..c0e19a8 100644
> --- a/qga/channel-win32.c
> +++ b/qga/channel-win32.c
> @@ -38,7 +38,7 @@ typedef struct GAWatch {
>   * Called by glib prior to polling to set up poll events if polling is needed.
>   *
>   */
> -static gboolean ga_channel_prepare(GSource *source, gint *timeout_ms)
> +static gboolean ga_channel_prepare_ov(GSource *source, gint *timeout_ms)
>  {
>      GAWatch *watch = (GAWatch *)source;
>      GAChannel *c = (GAChannel *)watch->channel;
> @@ -95,7 +95,7 @@ out:
>  /*
>   * Called by glib after an outstanding read request is completed.
>   */
> -static gboolean ga_channel_check(GSource *source)
> +static gboolean ga_channel_check_ov(GSource *source)
>  {
>      GAWatch *watch = (GAWatch *)source;
>      GAChannel *c = (GAChannel *)watch->channel;
> @@ -152,7 +152,7 @@ static gboolean ga_channel_check(GSource *source)
>  /*
>   * Called by glib after either prepare or check routines signal readiness
>   */
> -static gboolean ga_channel_dispatch(GSource *source, GSourceFunc unused,
> +static gboolean ga_channel_dispatch_ov(GSource *source, GSourceFunc unused,
>                                      gpointer user_data)
>  {
>      GAWatch *watch = (GAWatch *)source;
> @@ -178,21 +178,21 @@ static gboolean ga_channel_dispatch(GSource *source, GSourceFunc unused,
>      return success;
>  }
> 
> -static void ga_channel_finalize(GSource *source)
> +static void ga_channel_finalize_ov(GSource *source)
>  {
>      g_debug("finalize");
>  }
> 
> -GSourceFuncs ga_channel_watch_funcs = {
> -    ga_channel_prepare,
> -    ga_channel_check,
> -    ga_channel_dispatch,
> -    ga_channel_finalize
> +GSourceFuncs ga_channel_watch_funcs_ov = {
> +    ga_channel_prepare_ov,
> +    ga_channel_check_ov,
> +    ga_channel_dispatch_ov,
> +    ga_channel_finalize_ov
>  };
> 
> -static GSource *ga_channel_create_watch(GAChannel *c)
> +static GSource *ga_channel_create_watch_ov(GAChannel *c)
>  {
> -    GSource *source = g_source_new(&ga_channel_watch_funcs, sizeof(GAWatch));
> +    GSource *source = g_source_new(&ga_channel_watch_funcs_ov, sizeof(GAWatch));
>      GAWatch *watch = (GAWatch *)source;
> 
>      watch->channel = c;
> @@ -326,7 +326,7 @@ GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
>      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(c);
> +    c->source = ga_channel_create_watch_ov(c);
>      g_source_attach(c->source, NULL);
>      return c;
>  }
> -- 
> 1.7.12.315.g682ce8b
> 

Re: [Qemu-devel] [PATCH 1/5] configure: link qemu-ga.exe in default make target

[Michael Roth]

On Wed, Oct 31, 2012 at 03:45:16PM -0200, Luiz Capitulino wrote:
> Avoids having to type "make qemu-ga.exe" after build is complete
> and also cleanups qemu-ga.exe in make clean.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 3685020..d15bf5b 100755
> --- a/configure
> +++ b/configure
> @@ -3079,7 +3079,7 @@ if test "$softmmu" = yes ; then
>        virtfs=no
>      fi
>    fi
> -  if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
> +  if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" -o "$mingw32" = "yes" ] ; then
>      if [ "$guest_agent" = "yes" ]; then
>        tools="qemu-ga\$(EXESUF) $tools"
>      fi
> -- 
> 1.7.12.315.g682ce8b
> 
> 

Re: [Qemu-devel] [PATCH 4/5] qemu-ga: win32: isolate virtio-serial specific code

[Michael Roth]

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
> 

Re: [Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support

[Michael Roth]

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
> 
> 

Re: [Qemu-devel] [PATCH 4/5] qemu-ga: win32: isolate virtio-serial specific code

[Luiz Capitulino]

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
> > 
> 

Re: [Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support

[Luiz Capitulino]

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
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support

[Michael Roth]

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
> > > 
> > > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH 4/5] qemu-ga: win32: isolate virtio-serial specific code

[Michael Roth]

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
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH 5/5] qemu-ga: win32: add isa-serial support

[Luiz Capitulino]

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
> > > > 
> > > > 
> > > 
> > 
> > 
>