[libvirt] [PATCH 04/13] qemu: Introduce qemuDomainSecretMigrate{Prepare|Destroy}

[Jiri Denemark]

On Mon, Feb 20, 2017 at 15:14:21 -0500, John Ferlan wrote:
> 
> 
> On 02/20/2017 10:43 AM, Jiri Denemark wrote:
> > On Fri, Feb 17, 2017 at 14:39:21 -0500, John Ferlan wrote:
> >> Introduce API's to Prepare/Destroy a qemuDomainSecretInfoPtr to be
> >> used with a migrate or nbd TLS object
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/qemu/qemu_domain.c | 73 +++++++++++++++++++++++++++++++++++++++++
> >>  src/qemu/qemu_domain.h | 88 +++++++++++++++++++++++++++++---------------------
> >>  2 files changed, 124 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >> index be44843..dd3cfd5 100644
> >> --- a/src/qemu/qemu_domain.c
> >> +++ b/src/qemu/qemu_domain.c
> >> @@ -1370,6 +1370,77 @@ qemuDomainSecretChardevPrepare(virConnectPtr conn,
> >>  }
> >>  
> >>  
> >> +/* qemuDomainSecretMigrateDestroy:
> >> + * @migSecinfo: Pointer to the secinfo from the incoming def
> >> + *
> >> + * Clear and destroy memory associated with the secret
> >> + */
> >> +void
> >> +qemuDomainSecretMigrateDestroy(qemuDomainSecretInfoPtr *migSecinfo)
> >> +{
> >> +    if (!*migSecinfo)
> >> +        return;
> >> +
> >> +    qemuDomainSecretInfoFree(migSecinfo);
> >> +}
> > 
> > This is a useless wrapper, please drop it.
> > 
> 
> Sure - it's like an automatic reaction - have allocation need free
> function... Will use qemuDomainSecretInfoFree  instead...

It would make sense if migSecinfo was a new structure, but in this case
we would just end up with two functions usable for freeing a single
structure. Which is just asking for an inconsistent usage. And that's
what happened even in your series; sometimes qemuDomainSecretInfoFree is
used to free migSecinfo and sometime it's the new wrapper you call to do
the job.

> >> +int
> >> +qemuDomainSecretMigratePrepare(virConnectPtr conn,
> >> +                               qemuDomainObjPrivatePtr priv,
> >> +                               const char *srcAlias,
> >> +                               const char *secretUUID)
> > 
> > Almost all lines in this functions were just copy-pasted from
> > qemuDomainSecretChardevPrepare. Could you merge the two? Ideally you can
> > just make it a function which lookups the secinfo and you can do the
> > rest in the caller.
> 
> Sure. no problem... I'll create:
> 
> static qemuDomainSecretInfoPtr
> qemuDomainSecretTLSObjectPrepare(virConnectPtr conn,
>                                  qemuDomainObjPrivatePtr priv,
>                                  const char *srcAlias,
>                                  const char *secretUUID)

Great, but since the structure it creates is qemuDomainSecretInfo, can
you just call it qemuDomainSecretInfoNew or something similar to this?

Jirka