5:04 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org