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