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

[Ján Tomko]

On Thu, Jan 17, 2019 at 11:08:03AM -0500, John Ferlan wrote:
>
>
>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.
>

It would be more unusual for a Secret*Destroy function to free the
private data.

IIUC the point is to not have the temporary data allocated during the
whole time the domain is running.

>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 ;-)
>

I could convert it so that privateData is only allocated when needed,
but I considered the marginal memory usage increase worth the considency
and not forgetting to allocate/clean it up in some other case
(especially if we decide to use privateData for something else too).

So I'd rather not Unref the whole privateData in the Secret.*Dispose
function.

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190121/80bd391a/attachment-0001.sig>