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@xxxxxxxxxx> > > --- > > > > 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list