[libvirt] [PATCH 4/8] qemu: prepare secret for the graphics upfront

[John Ferlan]

On 1/16/19 2:41 AM, Ján Tomko wrote:
> Instead of hardcoding the TLS creds alias in
> qemuBuildGraphicsVNCCommandLine, store it
> in the domain private data.
> 
> Given that we only support one VNC graphics
> and thus have only one alias per-domain,
> this is overengineered, but it will allow us
> to prepare the secret upfront when we start
> supporting encrypted server TLS keys.
> 
> Note that the alias is not formatted anywhere
> since we won't need to access it after domain
> startup.
> 
> Signed-off-by: Ján Tomko <jtomko at redhat.com>
> ---
>  src/qemu/qemu_command.c |  8 ++++----
>  src/qemu/qemu_domain.c  | 44 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 822d5f8669..d130d0463c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8035,18 +8035,18 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>          virBufferAddLit(&opt, ",password");
>  
>      if (cfg->vncTLS) {
> -        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) {
> -            const char *alias = "vnc-tls-creds0";
> +        qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
> +        if (gfxPriv->tlsAlias) {
>              if (qemuBuildTLSx509CommandLine(cmd,
>                                              cfg->vncTLSx509certdir,
>                                              true,
>                                              cfg->vncTLSx509verify,
>                                              NULL,
> -                                            alias,
> +                                            gfxPriv->tlsAlias,
>                                              qemuCaps) < 0)
>                  goto error;
>  
> -            virBufferAsprintf(&opt, ",tls-creds=%s", alias);
> +            virBufferAsprintf(&opt, ",tls-creds=%s", gfxPriv->tlsAlias);
>          } else {
>              virBufferAddLit(&opt, ",tls");
>              if (cfg->vncTLSx509verify) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 63e739b778..6960f0569b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1741,6 +1741,42 @@ qemuDomainSecretChardevPrepare(virQEMUDriverConfigPtr cfg,
>  }
>  
>  
> +static void
> +qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics)
> +{
> +    qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
> +
> +    if (!gfxPriv)
> +        return;
> +
> +    VIR_FREE(gfxPriv->tlsAlias);

Free'ing of tlsAlias is handled by qemuDomainGraphicsPrivateDispose, so
this would be

   virObjectUnref(gfxPriv);
   QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics) = NULL;

The second to avoid the virDomainGraphicsDefFree doing this as well.

Still this is "unusual" (so to speak) for a qemuDomainSecret*Destroy
function. Typically they just clear/destroy the *Secinfo data.

Since you don't have that yet until patch 7, this could wait until then.
On the other hand, I don't "foresee" anything wrong with properly
free'ing the graphics def privateData now.

> +}
> +
> +
> +static int
> +qemuDomainSecretGraphicsPrepare(virQEMUDriverConfigPtr cfg,
> +                                qemuDomainObjPrivatePtr priv,
> +                                virDomainGraphicsDefPtr graphics)
> +{
> +    virQEMUCapsPtr qemuCaps = priv->qemuCaps;
> +    qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
> +
> +    if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC)
> +        return 0;
> +
> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509))
> +        return 0;
> +
> +    if (!cfg->vncTLS)
> +        return 0;
> +

Just a note/thought while reviewing... nothing all that important...

Seems to be a bit of overkill w/ graphics->privateData only being used
for VNC private data in this very specialized case. Still weighed vs the
then need for "gfxPriv && gfxPriv->...", who am I to complain ;-)

> +    if (VIR_STRDUP(gfxPriv->tlsAlias, "vnc-tls-creds0") < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
>  /* qemuDomainSecretDestroy:
>   * @vm: Domain object
>   *
> @@ -1782,6 +1818,9 @@ qemuDomainSecretDestroy(virDomainObjPtr vm)
>  
>      for (i = 0; i < vm->def->nredirdevs; i++)
>          qemuDomainSecretChardevDestroy(vm->def->redirdevs[i]->source);
> +
> +    for (i = 0; i < vm->def->ngraphics; i++)
> +        qemuDomainSecretGraphicsDestroy(vm->def->graphics[i]);


Interesting placement until one reads patch 7.

I think if patch 5 and 6 were placed before this one, then it'd be
clearer what the steps are.  I'm OK with this here now since eventually
it'd be added.

With a couple of adjustments...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

>  }
>  
>  
> @@ -1865,6 +1904,11 @@ qemuDomainSecretPrepare(virQEMUDriverPtr driver,
>              goto cleanup;
>      }
>  
> +    for (i = 0; i < vm->def->ngraphics; i++) {
> +        if (qemuDomainSecretGraphicsPrepare(cfg, priv, vm->def->graphics[i]) < 0)
> +            goto cleanup;
> +    }
> +
>      ret = 0;
>  
>   cleanup:
>