[libvirt] [PATCH] qemu: introduce spiceport serial backend
Martin Kletzander
mkletzan at redhat.com
Wed Feb 5 15:39:34 UTC 2014
On Wed, Feb 05, 2014 at 03:38:49PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 03, 2014 at 05:41:00PM +0100, Martin Kletzander wrote:
> > signed-off-by: martin kletzander <mkletzan at 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
>
I was probably cleaning this out so many times I over-cleaned it to
double returns.
> > + 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.
>
Yes, at least from what I remembered the previous one should be
XML_ERROR as well, but I didn't want to mix the cleanup with
introducing spiceport.
> > + 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.
>
I missed that it calculates only the "prefix" length and used strcspn
instead of strspn, so this code is all wrong.
> > + virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("Invalid character in source channel for char device"));
>
> The network code uses VIR_ERR_INVALID_ARG here.
>
Hard to say what's better, it's an argument in XML, so both are
sensible, but I'll change that for v2.
> > + 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.
>
Definitely, I just posted it before seeing that. Thanks for the
review, v2 coming up.
Martin
> Rest of the patch looks good.
>
> Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140205/c9936af8/attachment-0001.sig>
More information about the libvir-list
mailing list