Re: [Qemu-devel] [PATCH] qemu-ga: isa-serial support on Windows

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Miki Mishael (2014-01-05 11:18:51)
> Add support for isa-serial method for qemu-ga on Windows,
> Added -p command line parameter for serial port name
> specification, e.g. "-p COM15".
> 
> Signed-off-by: Miki Mishael <mmishael@redhat.com>
> Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>
> ---
>  qga/channel-win32.c | 27 +++++++++++++++++++++++++--
>  qga/main.c          | 14 +++++++++++---
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/channel-win32.c b/qga/channel-win32.c
> index 8a303f3..fd42460 100644
> --- a/qga/channel-win32.c
> +++ b/qga/channel-win32.c
> @@ -284,15 +284,32 @@ GIOStatus ga_channel_write_all(GAChannel *c, const char *buf, size_t size)
>      return status;
>  }
> 
> +static void ga_serial_path_correction( gchar* newpath, const gchar *path, size_t maxdestlen)
> +{
> +    gchar *prefix = "\\\\.\\";
> +    g_strlcpy(newpath, prefix, maxdestlen);
> +    g_strlcat(newpath, path, maxdestlen);
> +}

I think this can be simplified a bit, perhaps:

#define QGA_SERIAL_PATH_PREFIX "\\\\.\\"

static void ga_serial_path_correction(gchar* newpath, const gchar *path)
{
    sprintf(newpath, QGA_SERIAL_PATH_PREFIX"%d", path);
}

At that point it probably makes more sense to just drop the func and move
this into ga_channel_open, since it's a bit clearer to understand what the
"correction" entails looking at sprintf's format parameter.

> +
>  static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
>                                  const gchar *path)
>  {
> -    if (method != GA_CHANNEL_VIRTIO_SERIAL) {
> +    COMMTIMEOUTS comTimeOut = {0};
> +    gchar newpath[MAXPATHLEN] = {0};
> +    comTimeOut.ReadIntervalTimeout = 1;
> +
> +    if (method != GA_CHANNEL_VIRTIO_SERIAL && method != GA_CHANNEL_ISA_SERIAL) {
>          g_critical("unsupported communication method");
>          return false;
>      }
> 
> -    c->handle = CreateFile(path, GENERIC_READ | GENERIC_WRITE, 0, NULL,
> +    if (method == GA_CHANNEL_ISA_SERIAL){
> +        ga_serial_path_correction(newpath, path, sizeof(newpath));
> +    }else {
> +        g_strlcpy(newpath, path, sizeof(newpath));
> +    }
> +
> +    c->handle = CreateFile(newpath, GENERIC_READ | GENERIC_WRITE, 0, NULL,
>                             OPEN_EXISTING,
>                             FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
>      if (c->handle == INVALID_HANDLE_VALUE) {
> @@ -300,6 +317,12 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
>          return false;
>      }
> 
> +    if (method == GA_CHANNEL_ISA_SERIAL && !SetCommTimeouts(c->handle,&comTimeOut)) {
> +        g_critical("error SetCommTimeouts: %d",GetLastError());

Perhaps something a little more human readable, like "error setting timeout for
comm port"

> +        CloseHandle(c->handle);
> +        return false;
> +    }
> +
>      return true;
>  }
> 
> diff --git a/qga/main.c b/qga/main.c
> index c58b26a..ad21061 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -47,9 +47,11 @@
>  #ifndef _WIN32
>  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
>  #define QGA_STATE_RELATIVE_DIR  "run"
> +#define QGA_SERIAL_PATH_DEFAULT ""

We know this will fail on linux, so we should either attempt to define some
kind of default like "/dev/ttyS0", or only attempt the default path on w32
and fail explicitly for linux.

I'm not sure setting a default makes sense for w32 either, serial is already
a non-default configuration, so requiring explicit paths seems reasonable
there, and less likely to confuse users by guessing the wrong port. Though
I guess serial might be a bit more common on w32 than linux, so I don't
really have a strong opinion either way.

>  #else
>  #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
>  #define QGA_STATE_RELATIVE_DIR  "qemu-ga"
> +#define QGA_SERIAL_PATH_DEFAULT "COM1"
>  #endif
>  #ifdef CONFIG_FSFREEZE
>  #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
> @@ -659,12 +661,18 @@ static gboolean channel_init(GAState *s, const gchar *method, const gchar *path)
>      }
> 
>      if (path == NULL) {
> -        if (strcmp(method, "virtio-serial") != 0) {
> +        if (strcmp(method, "virtio-serial") == 0 ) {
> +            /* try the default path for the virtio-serial port */
> +            path = QGA_VIRTIO_PATH_DEFAULT;
> +        }
> +        else if (strcmp(method, "isa-serial") == 0){
> +            /* try the default path for the serial port - COM1 */
> +            path = QGA_SERIAL_PATH_DEFAULT;
> +        }
> +        else {

In QEMU we generally do:

if (...) {
    ...
} else if (...) {
    ...
} else {
    ...
}

>              g_critical("must specify a path for this channel");
>              return false;
>          }
> -        /* try the default path for the virtio-serial port */
> -        path = QGA_VIRTIO_PATH_DEFAULT;
>      }
> 
>      if (strcmp(method, "virtio-serial") == 0) {
> -- 
> 1.8.3.1