Re: [PATCH v1 06/11] conf: Make graphics's GL a standalone structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/27/2018 09:34 AM, Erik Skultety wrote:
> Since we support gl with SPICE and SDL with future VNC enablement in
> sight (egl-headless), let's separate the gl-related attributes into a
> standalone structure.
> 
> Signed-off-by: Erik Skultety <eskultet@xxxxxxxxxx>
> ---
>  src/conf/domain_conf.c      | 137 +++++++++++++++++++++++++-------------------
>  src/conf/domain_conf.h      |  12 +++-
>  src/qemu/qemu_cgroup.c      |  10 ++--
>  src/qemu/qemu_command.c     |  66 ++++++++++++---------
>  src/qemu/qemu_domain.c      |   7 ++-
>  src/security/security_dac.c |   7 ++-
>  6 files changed, 138 insertions(+), 101 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 09d9bac739..6bfa3ca130 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1420,8 +1420,6 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
>          break;
>  
>      case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> -        VIR_FREE(def->data.spice.rendernode);
> -        VIR_FREE(def->data.spice.keymap);

Perhaps a bit too aggressive here?  Restore the keymap FREE - it's
allocated in virDomainGraphicsDefParseXMLSpice

>          virDomainGraphicsAuthDefClear(&def->data.spice.auth);
>          break;
>  
> @@ -1429,6 +1427,8 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
>          break;
>      }
>  
> +    virDomainGraphicsGLDefFree(def->gl);
> +
>      for (i = 0; i < def->nListens; i++)
>          virDomainGraphicsListenDefClear(&def->listens[i]);
>      VIR_FREE(def->listens);
> @@ -1436,6 +1436,18 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def)
>      VIR_FREE(def);
>  }
>  
> +
> +void
> +virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def)

The only caller is above - even after the last patch, thus this should
be static to this function and above the previous.

> +{
> +    if (!def)
> +        return;
> +
> +    VIR_FREE(def->rendernode);
> +    VIR_FREE(def);
> +}
> +
> +
>  const char *virDomainInputDefGetPath(virDomainInputDefPtr input)
>  {
>      switch ((virDomainInputType) input->type) {
> @@ -13555,6 +13567,48 @@ virDomainGraphicsListensParseXML(virDomainGraphicsDefPtr def,
>  }
>  
>  
> +static int
> +virDomainGraphicsGLDefParseXML(virDomainGraphicsDefPtr def,
> +                               xmlNodePtr node)
> +{
> +    virDomainGraphicsGLDefPtr gl = NULL;
> +    char *enable = NULL;
> +    int ret = -1;
> +
> +    if (!node)
> +        return 0;
> +
> +    if (VIR_ALLOC(gl) < 0)
> +        return -1;
> +
> +    if (!(enable = virXMLPropString(node, "enable"))) {
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("'enable' attribute is mandatory for graphics "
> +                         "<gl> element"));
> +        goto cleanup;
> +    }
> +
> +    if ((gl->enable =
> +         virTristateBoolTypeFromString(enable)) <= 0) {

This should go on the previous line - it fits, I checked.

The <= changes from the previous code which had:

-        enableVal = virTristateBoolTypeFromString(enable);
-        if (enableVal < 0) {

hopefully there's no "default"'s in the wild. But it is fine given the
previous usage model of absent being the default and being checked.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("unknown value for attribute enable '%s'"),
> +                       enable);
> +        goto cleanup;
> +    }
> +
> +    if (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)
> +       gl->rendernode = virXMLPropString(node, "rendernode");
> +
> +    VIR_STEAL_PTR(def->gl, gl);
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(enable);
> +    virDomainGraphicsGLDefFree(gl);
> +    return ret;
> +}
> +
> +
>  static int
>  virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,
>                                  xmlNodePtr node,
> @@ -13644,8 +13698,6 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
>  {
>      xmlNodePtr save = ctxt->node;
>      char *enable = NULL;
> -    int enableVal;
> -    xmlNodePtr glNode;
>      char *fullscreen = virXMLPropString(node, "fullscreen");
>      int ret = -1;
>  
> @@ -13668,23 +13720,9 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
>      def->data.sdl.xauth = virXMLPropString(node, "xauth");
>      def->data.sdl.display = virXMLPropString(node, "display");
>  
> -    glNode = virXPathNode("./gl", ctxt);
> -    if (glNode) {
> -        enable = virXMLPropString(glNode, "enable");
> -        if (!enable) {
> -            virReportError(VIR_ERR_XML_ERROR, "%s",
> -                           _("sdl gl element missing enable"));
> -            goto cleanup;
> -        }
> -
> -        enableVal = virTristateBoolTypeFromString(enable);
> -        if (enableVal < 0) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("unknown enable value '%s'"), enable);
> -            goto cleanup;
> -        }
> -        def->data.sdl.gl = enableVal;
> -    }
> +    if (virDomainGraphicsGLDefParseXML(def,
> +                                       virXPathNode("./gl[1]", ctxt)) < 0)

Move to the previous line, it fits, I checked.

> +        goto cleanup;
>  
>      ret = 0;
>   cleanup:
> @@ -14026,31 +14064,6 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,
>                  VIR_FREE(enable);
>  
>                  def->data.spice.filetransfer = enableVal;
> -            } else if (virXMLNodeNameEqual(cur, "gl")) {
> -                char *enable = virXMLPropString(cur, "enable");
> -                char *rendernode = virXMLPropString(cur, "rendernode");
> -                int enableVal;
> -
> -                if (!enable) {
> -                    virReportError(VIR_ERR_XML_ERROR, "%s",
> -                                   _("spice gl element missing enable"));
> -                    VIR_FREE(rendernode);
> -                    goto error;
> -                }
> -
> -                if ((enableVal =
> -                     virTristateBoolTypeFromString(enable)) <= 0) {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   _("unknown enable value '%s'"), enable);
> -                    VIR_FREE(enable);
> -                    VIR_FREE(rendernode);
> -                    goto error;
> -                }
> -                VIR_FREE(enable);
> -
> -                def->data.spice.gl = enableVal;
> -                def->data.spice.rendernode = rendernode;
> -

Was moving to the last else if intentional?  IDC either way, but it is
strange.

>              } else if (virXMLNodeNameEqual(cur, "mouse")) {
>                  char *mode = virXMLPropString(cur, "mode");
>                  int modeVal;
> @@ -14071,6 +14084,9 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def,

[...]

>  static int
>  virDomainGraphicsDefFormat(virBufferPtr buf,
>                             virDomainGraphicsDefPtr def,
> @@ -26247,18 +26270,12 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>          if (def->data.sdl.fullscreen)
>              virBufferAddLit(buf, " fullscreen='yes'");
>  
> -        if (!children && def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) {
> +        if (!children && def->gl) {
>              virBufferAddLit(buf, ">\n");
>              virBufferAdjustIndent(buf, 2);
>              children = true;
>          }
>  
> -        if (def->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) {
> -            virBufferAsprintf(buf, "<gl enable='%s'",
> -                              virTristateBoolTypeToString(def->data.sdl.gl));
> -            virBufferAddLit(buf, "/>\n");
> -        }
> -
>          break;
>  
>      case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
> @@ -26405,8 +26422,7 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>          if (!children && (def->data.spice.image || def->data.spice.jpeg ||
>                            def->data.spice.zlib || def->data.spice.playback ||
>                            def->data.spice.streaming || def->data.spice.copypaste ||
> -                          def->data.spice.mousemode || def->data.spice.filetransfer ||
> -                          def->data.spice.gl)) {
> +                          def->data.spice.mousemode || def->data.spice.filetransfer)) {

Interesting, it's a bit sneaky but even when not provided the above
format for <listen type...> will format to 'none' meaning we don't have
to worry about def->gl here (even if autoport isn't provided for
<graphics...>

Hopefully that remains "true" going forward in time.

>              virBufferAddLit(buf, ">\n");
>              virBufferAdjustIndent(buf, 2);
>              children = true;
> @@ -26436,9 +26452,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
>              virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n",
>                                virTristateBoolTypeToString(def->data.spice.filetransfer));
>  
> -        virDomainSpiceGLDefFormat(buf, def);
>      }
>  
> +    virDomainGraphicsGLDefFormat(buf, def);
> +
>      if (children) {
>          virBufferAdjustIndent(buf, -2);
>          virBufferAddLit(buf, "</graphics>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0924fc4f3c..20dc1334c4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h

[...]

>  
>  typedef enum {
> @@ -2837,6 +2842,7 @@ int virDomainObjWaitUntil(virDomainObjPtr vm,
>  void virDomainPanicDefFree(virDomainPanicDefPtr panic);
>  void virDomainResourceDefFree(virDomainResourceDefPtr resource);
>  void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def);
> +void virDomainGraphicsGLDefFree(virDomainGraphicsGLDefPtr def);

This won't be necessary.... The "tip off" is that the private syms file
wasn't touched...

>  const char *virDomainInputDefGetPath(virDomainInputDefPtr input);
>  void virDomainInputDefFree(virDomainInputDefPtr def);
>  virDomainDiskDefPtr virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt);

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 195d03e373..ef0be95b0f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7740,7 +7740,8 @@ qemuBuildGraphicsSDLCommandLine(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
>      virCommandAddArg(cmd, "-display");
>      virBufferAddLit(&opt, "sdl");
>  
> -    if (graphics->data.sdl.gl != VIR_TRISTATE_BOOL_ABSENT) {
> +    if (graphics->gl &&
> +        graphics->gl->enable != VIR_TRISTATE_BOOL_ABSENT) {

Ironically, because the <= exists in the Parse code, that means that if
->gl is true, then we don't have to check ABSENT I would think.

I also see other/future patches seem to use enable == YES a lot...

>          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SDL_GL)) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                             _("OpenGL for SDL is not supported with this QEMU "

[...]

> @@ -8122,10 +8127,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>          virBufferAddLit(&opt, "seamless-migration=on,");
>      }
>  
> +    /* OpenGL magic */
> +    if (graphics->gl &&
> +        graphics->gl->enable == VIR_TRISTATE_BOOL_YES &&
> +        qemuBuildGraphicsSPICEGLCommandLine(graphics->gl, &opt, qemuCaps) < 0)
> +        goto error;
> +

Not that it matters, but this is a change of order w/
"seamless-migration=on,"... No code uses both now and I don't imagine
order matters.

With recommended adjustments,

Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>

John

>      virBufferTrim(&opt, ",", -1);
>  
>      virCommandAddArg(cmd, "-spice");
>      virCommandAddArgBuffer(cmd, &opt);
> +
>      if (graphics->data.spice.keymap)
>          virCommandAddArgList(cmd, "-k",
>                               graphics->data.spice.keymap, NULL);
[...]

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux