On 11/01/2010 12:17 PM, Daniel P. Berrange wrote:
This adds an element
<graphics type='spice' port='5903' tlsPort='5904' autoport='yes' listen='127.0.0.1'/>
This is the bare minimum that should be exposed in the guest config for SPICE. Other parameters are better handled as per host level configuration tunables
* docs/schemas/domain.rng: Define the SPICE <graphics> schema * src/domain_conf.h, src/domain_conf.c: Add parsing and formatting for SPICE graphics config * src/qemu_conf.c: Complain about unsupported graphics types --- docs/schemas/domain.rng | 38 ++++++++++++++++++++++ src/conf/domain_conf.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++ src/qemu/qemu_conf.c | 2 +- 4 files changed, 127 insertions(+), 2 deletions(-)
Also missing docs/formatdomain.html.in changes; but unlike patch 2/10 where the change was a trivial one-word addition, this needs a full paragraph, so I'd like to see this before giving an ack. Rearranging my review a bit...
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 78f28d0..0238f92 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -511,6 +511,7 @@ enum virDomainGraphicsType { VIR_DOMAIN_GRAPHICS_TYPE_VNC, VIR_DOMAIN_GRAPHICS_TYPE_RDP, VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP, + VIR_DOMAIN_GRAPHICS_TYPE_SPICE,
VIR_DOMAIN_GRAPHICS_TYPE_LAST, }; @@ -543,6 +544,14 @@ struct _virDomainGraphicsDef { char *display; unsigned int fullscreen :1; } desktop; + struct { + int port; + int tlsPort;
Should these two be unsigned short, rather than int?
+ char *listenAddr; + char *keymap; + char *passwd; + unsigned int autoport :1;
Or, maybe keep port as int, and use a sentinel of -1 to mean autoport, rather than wasting another 4/8 bytes of struct space for one bit for autoport?
+ } spice; } data; };
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index c8beffc..3163257 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1090,6 +1090,44 @@ </group> <group> <attribute name="type"> + <value>spice</value> + </attribute>
HALLELUJAH! The stupid Thunderbird bug that corrupted spacing in message replies prior to a word starting with <, >, or & appears to be fixed in the latest F13 build now! (.rng patches were so much harder for me to review when t-bird insisted on left-flushing the entire .rng chunk) :)
+ <optional> + <attribute name="listen"> + <ref name="addrIP"/>
addrIP is hard-coded to IPv4 at the moment; is this something that would need adjustment for IPv6?
@@ -3202,6 +3209,50 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, int flags) { def->data.desktop.fullscreen = 0;
def->data.desktop.display = virXMLPropString(node, "display"); + } else if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + char *port = virXMLPropString(node, "port"); + char *tlsPort; + char *autoport; + + if (port) { + if (virStrToLong_i(port, NULL, 10, &def->data.spice.port) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse spice port %s"), port); + VIR_FREE(port); + goto error; + } + VIR_FREE(port); + } else { + def->data.spice.port = 5900; + }
Should we validate that def->data.spice.port < 64k? That is, port='100000' should be rejected.
+ + tlsPort = virXMLPropString(node, "tlsPort"); + if (tlsPort) { + if (virStrToLong_i(tlsPort, NULL, 10, &def->data.spice.tlsPort) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse spice tlsPort %s"), tlsPort); + VIR_FREE(tlsPort); + goto error; + } + VIR_FREE(tlsPort); + } else { + def->data.spice.tlsPort = 0; + }
Likewise for tlsPort.
+ def->data.spice.listenAddr = virXMLPropString(node, "listen"); ... + + if (def->data.spice.listenAddr) + virBufferVSprintf(buf, " listen='%s'", + def->data.spice.listenAddr);
Should we be storing listenAddr as a raw string, or should we be converting it to/from virSocketAddr? Conversion to virSocketAddr would also allow validation
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 149dcee..aa42e04 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4949,7 +4949,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
if (def->nvideos) { if (def->nvideos > 1) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("only one video card is currently supported"));
This seems like an independent change worth putting in at any time. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org