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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list