[libvirt] [PATCH 03/10] Add a <graphics> type for SPICE protocol

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101102/035ec264/attachment-0001.sig>