[libvirt] [PATCH 5/5] qemu: add rendernode argument

[Michal Privoznik]

On 02/14/2017 10:04 PM, marcandre.lureau at redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> 
> Add a new attribute 'rendernode' to <gl> spice element.
> 
> Give it to QEMU if qemu supports it (queued for 2.9).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>  docs/formatdomain.html.in                          |  7 +++++-
>  docs/schemas/domaincommon.rng                      |  5 ++++
>  src/conf/domain_conf.c                             | 28 +++++++++++++++++++---
>  src/conf/domain_conf.h                             |  1 +
>  src/qemu/qemu_capabilities.c                       |  2 ++
>  src/qemu/qemu_capabilities.h                       |  1 +
>  src/qemu/qemu_command.c                            | 10 ++++++++
>  .../qemuxml2argv-video-virtio-gpu-spice-gl.args    |  2 +-
>  .../qemuxml2argv-video-virtio-gpu-spice-gl.xml     |  2 +-
>  tests/qemuxml2argvtest.c                           |  1 +
>  .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml   |  2 +-
>  11 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0a115f5dc..67f486747 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5619,7 +5619,7 @@ qemu-kvm -net nic,model=? /dev/null
>    <clipboard copypaste='no'/>
>    <mouse mode='client'/>
>    <filetransfer enable='no'/>
> -  <gl enable='yes'/>
> +  <gl enable='yes' rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/>
>  </graphics></pre>
>              <p>
>                Spice supports variable compression settings for audio, images and
> @@ -5665,6 +5665,11 @@ qemu-kvm -net nic,model=? /dev/null
>                the <code>gl</code> element, by setting the <code>enable</code>
>                property. (QEMU only, <span class="since">since 1.3.3</span>).
>              </p>
> +            <p>
> +              By default, QEMU will pick the first available GPU DRM render node.
> +              You may specify a DRM render node path to use instead. (QEMU only,
> +              <span class="since">since 3.1</span>).
> +            </p>
>            </dd>
>            <dt><code>rdp</code></dt>
>            <dd>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index d715bff29..c5f101325 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3033,6 +3033,11 @@
>                  <attribute name="enable">
>                    <ref name="virYesNo"/>
>                  </attribute>
> +                <optional>
> +                  <attribute name="rendernode">
> +                    <ref name="absFilePath"/>
> +                  </attribute>
> +                </optional>
>                  <empty/>
>                </element>
>              </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1bc72a4e9..b577d0aba 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1300,6 +1300,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
>          break;
>  
>      case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> +        VIR_FREE(def->data.spice.rendernode);
>          VIR_FREE(def->data.spice.keymap);
>          virDomainGraphicsAuthDefClear(&def->data.spice.auth);
>          break;
> @@ -12141,6 +12142,7 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
>                  def->data.spice.filetransfer = enableVal;
>              } else if (xmlStrEqual(cur->name, BAD_CAST "gl")) {
>                  char *enable = virXMLPropString(cur, "enable");
> +                char *rendernode = virXMLPropString(cur, "rendernode");
>                  int enableVal;

This might be leaked if control reaches 'goto' lines in between this and
the following hunk.

>  
>                  if (!enable) {
> @@ -12159,6 +12161,8 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
>                  VIR_FREE(enable);
>  
>                  def->data.spice.gl = enableVal;
> +                def->data.spice.rendernode = rendernode;
> +
>              } else if (xmlStrEqual(cur->name, BAD_CAST "mouse")) {
>                  char *mode = virXMLPropString(cur, "mode");
>                  int modeVal;


> @@ -22839,6 +22843,23 @@ virDomainGraphicsListenDefFormatAddr(virBufferPtr buf,
>          virBufferAsprintf(buf, " listen='%s'", glisten->address);
>  }
>  
> +static int
> +virDomainSpiceGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def)
> +{
> +    if (def->data.spice.gl == VIR_TRISTATE_BOOL_ABSENT) {
> +        return 0;
> +    }

Firstly, no need for this function to return an int. We usually have
functions like this return nothing (void). Secondly, there's no need to
put curly braces around one line body.

> +
> +    virBufferAsprintf(buf, "<gl enable='%s'",
> +                      virTristateBoolTypeToString(def->data.spice.gl));
> +
> +    if (def->data.spice.rendernode)
> +        virBufferEscapeString(buf, " rendernode='%s'", def->data.spice.rendernode);

The virBufferEscapeString() is no-op if the last arg is NULL.

> +
> +    virBufferAddLit(buf, "/>\n");
> +
> +    return 0;
> +}
>  
>  static int
>  virDomainGraphicsDefFormat(virBufferPtr buf,
> @@ -23082,9 +23103,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>          if (def->data.spice.filetransfer)
>              virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n",
>                                virTristateBoolTypeToString(def->data.spice.filetransfer));
> -        if (def->data.spice.gl)
> -            virBufferAsprintf(buf, "<gl enable='%s'/>\n",
> -                              virTristateBoolTypeToString(def->data.spice.gl));
> +
> +        if (virDomainSpiceGLDefFormat(buf, def) < 0) {
> +            return -1;
> +        }

Again. This will never happen and also there's no need for {}.

>      }
>  
>      if (children) {

I am fixing all of these small nits that I've raised and pushing all of
the patches. I have couple of follow up patches ready so expect them to
be send shortly.

Michal