[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: introduce spiceport serial backend



On Mon, Feb 03, 2014 at 05:41:00PM +0100, Martin Kletzander wrote:
> signed-off-by: martin kletzander <mkletzan redhat com>
> ---
> 
> notes:
>     this applies on top of "qemu: minor cleanups":
>     
>     https://www.redhat.com/archives/libvir-list/2014-january/msg01584.html
> 
>  docs/formatdomain.html.in                          | 22 +++++
>  docs/schemas/domaincommon.rng                      |  4 +
>  src/conf/domain_audit.c                            |  3 +-
>  src/conf/domain_conf.c                             | 40 ++++++++-
>  src/conf/domain_conf.h                             |  6 +-
>  src/qemu/qemu_capabilities.c                       |  8 ++
>  src/qemu/qemu_capabilities.h                       |  3 +-
>  src/qemu/qemu_command.c                            | 96 +++++++++++++---------
>  src/qemu/qemu_monitor_json.c                       |  3 +-
>  tests/qemucapabilitiesdata/caps_1.5.3-1.caps       |  1 +
>  tests/qemucapabilitiesdata/caps_1.6.0-1.caps       |  1 +
>  tests/qemucapabilitiesdata/caps_1.6.50-1.caps      |  1 +
>  .../qemuxml2argv-serial-spiceport-nospice.args     |  6 ++
>  .../qemuxml2argv-serial-spiceport-nospice.xml      | 40 +++++++++
>  .../qemuxml2argv-serial-spiceport.args             | 13 +++
>  .../qemuxml2argv-serial-spiceport.xml              | 43 ++++++++++
>  tests/qemuxml2argvtest.c                           |  7 ++
>  tests/qemuxml2xmltest.c                            |  2 +
>  18 files changed, 255 insertions(+), 44 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport-nospice.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport-nospice.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-spiceport.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index fa1ecb5..8cdd0e9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -437,7 +437,8 @@ VIR_ENUM_IMPL(virDomainChr, VIR_DOMAIN_CHR_TYPE_LAST,
>                "udp",
>                "tcp",
>                "unix",
> -              "spicevmc")
> +              "spicevmc",
> +              "spiceport")
> 
>  VIR_ENUM_IMPL(virDomainChrTcpProtocol, VIR_DOMAIN_CHR_TCP_PROTOCOL_LAST,
>                "raw",
> @@ -1583,6 +1584,12 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
>              STREQ_NULLABLE(src->data.nix.path, tgt->data.nix.path);
>          break;
> 
> +    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> +        return STREQ_NULLABLE(src->data.spiceport.channel,
> +                              tgt->data.spiceport.channel);
> +        return true;

this 'return true' is not needed

> +        break;
> +
>      case VIR_DOMAIN_CHR_TYPE_NULL:
>      case VIR_DOMAIN_CHR_TYPE_VC:
>      case VIR_DOMAIN_CHR_TYPE_STDIO:
> @@ -7090,6 +7097,9 @@ error:
>      return ret;
>  }
> 
> +#define SERIAL_CHANNEL_NAME_CHARS \
> +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-."
> +
>  /* Parse the source half of the XML definition for a character device,
>   * where node is the first element of node->children of the parent
>   * element.  def->type must already be valid.  Return -1 on failure,
> @@ -7110,6 +7120,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>      char *path = NULL;
>      char *mode = NULL;
>      char *protocol = NULL;
> +    char *channel = NULL;
>      int remaining = 0;
> 
>      while (cur != NULL) {
> @@ -7154,6 +7165,11 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>                          VIR_FREE(mode);
>                      break;
> 
> +                case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> +                    if (!channel)
> +                        channel = virXMLPropString(cur, "channel");
> +                    break;
> +
>                  case VIR_DOMAIN_CHR_TYPE_LAST:
>                  case VIR_DOMAIN_CHR_TYPE_NULL:
>                  case VIR_DOMAIN_CHR_TYPE_VC:
> @@ -7293,6 +7309,21 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>          def->data.nix.path = path;
>          path = NULL;
>          break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> +        if (!channel) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Missing source channel attribute for char device"));
> +            goto error;
> +        }

Code before that uses VIR_ERR_INTERNAL_ERROR for missing attributes, but
VIR_ERR_XML_ERROR seems indeed better.

> +        if (strcspn(channel, SERIAL_CHANNEL_NAME_CHARS)) {

If I understood the man page correctly, strcspn will return the number of
characters at the beginning of channel which are not in
SERIAL_CHANNEL_NAME_CHARS. It seems this won't catch "org.éâò".
The net code does:
if (strspn(channel, SERIAL_CHANNEL_NAME_CHARS) < strlen(channel)) {
which seems ok.

> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Invalid character in source channel for char device"));

The network code uses VIR_ERR_INVALID_ARG here.

> +            goto error;
> +        }
> +        def->data.spiceport.channel = channel;
> +        channel = NULL;
> +        break;
>      }
> 
>  cleanup:
> @@ -7303,6 +7334,7 @@ cleanup:
>      VIR_FREE(connectHost);
>      VIR_FREE(connectService);
>      VIR_FREE(path);
> +    VIR_FREE(channel);
> 
>      return remaining;
> 
> @@ -15651,6 +15683,12 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>          virBufferEscapeString(buf, " path='%s'", def->data.nix.path);
>          virBufferAddLit(buf, "/>\n");
>          break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> +        virBufferAsprintf(buf, "<source channel='%s'/>\n",
> +                          def->data.spiceport.channel);
> +        break;
> +
>      }
>      virBufferAdjustIndent(buf, -6);
> 
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index d8f2e49..b07aa8f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1,7 +1,7 @@
>  /*
>   * domain_conf.h: domain XML processing
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   * Copyright (C) 2006-2008 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -1104,6 +1104,7 @@ enum virDomainChrType {
>      VIR_DOMAIN_CHR_TYPE_TCP,
>      VIR_DOMAIN_CHR_TYPE_UNIX,
>      VIR_DOMAIN_CHR_TYPE_SPICEVMC,
> +    VIR_DOMAIN_CHR_TYPE_SPICEPORT,
> 
>      VIR_DOMAIN_CHR_TYPE_LAST
>  };
> @@ -1152,6 +1153,9 @@ struct _virDomainChrSourceDef {
>              bool listen;
>          } nix;
>          int spicevmc;
> +        struct {
> +            char *channel;
> +        } spiceport;
>      } data;
>  };
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 8aec293..317b374 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -248,6 +248,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "pvpanic",
>                "enable-fips",
>                "spice-file-xfer-disable",
> +              "spiceport",
>      );
> 
>  struct _virQEMUCaps {
> @@ -1012,6 +1013,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV);
>          if (strstr(help, "-chardev spicevmc"))
>              virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEVMC);
> +        if (strstr(help, "-chardev spiceport"))
> +            virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT);
>      }
>      if (strstr(help, "-balloon"))
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_BALLOON);
> @@ -2570,6 +2573,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>      if (qemuCaps->version >= 1006000)
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> 
> +    /* -chardev spiceport is supported from 1.4.0,
> +     * but it's in qapi only since 1.5.0 */
> +    if (qemuCaps->version >= 1005000)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT);
> +

I'd tend to put this before
     if (qemuCaps->version >= 1006000)
         virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
so that the qemu version tests are sorted by version.

>

>      if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
>          goto cleanup;
>      if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 23dccce..a4eecb6 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_capabilities.h: QEMU capabilities generation
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -202,6 +202,7 @@ enum virQEMUCapsFlags {
>      QEMU_CAPS_DEVICE_PANIC       = 161, /* -device pvpanic */
>      QEMU_CAPS_ENABLE_FIPS        = 162, /* -enable-fips */
>      QEMU_CAPS_SPICE_FILE_XFER_DISABLE = 163, /* -spice disable-agent-file-xfer */
> +    QEMU_CAPS_CHARDEV_SPICEPORT  = 164, /* -chardev spiceport */
> 
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7e1cd53..c1635e0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5977,6 +5977,16 @@ qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias,
>                            virDomainChrSpicevmcTypeToString(dev->data.spicevmc));
>          break;
> 
> +    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_SPICEPORT)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("spiceport not supported in this QEMU binary"));
> +            goto error;
> +        }
> +        virBufferAsprintf(&buf, "spiceport,id=char%s,name=%s", alias,
> +                          dev->data.spiceport.channel);
> +        break;
> +
>      default:
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("unsupported chardev '%s'"),
> @@ -6075,6 +6085,8 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char *prefix)
> 
>      case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
>          /* spicevmc doesn't have any '-serial' compatible option */
> +    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> +        /* spiceport doesn't have any '-serial' compatible option */

As you suggested when I raised this for TYPE_SPICEVMC, you can remove the
confusing comment for TYPE_SPICE_PORT too.

Rest of the patch looks good.

Christophe

Attachment: pgpAB5eRI5TFV.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]