[libvirt] [PATCH 5/6] conf: Add helper to convert list of virDomains to a list of virDomainObjs

[Peter Krempa]

On Mon, May 04, 2015 at 13:22:17 +0200, Michal Privoznik wrote:
> On 30.04.2015 14:44, Peter Krempa wrote:
> > Add virDomainObjListConvert that will take a list of virDomains, apply
> > filters and return a list of virDomainObjs.
> > ---
> >  src/conf/domain_conf.c   | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/conf/domain_conf.h   |  9 ++++++++
> >  src/libvirt_private.syms |  1 +
> >  3 files changed, 64 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 66fe470..73dc33f 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -23072,6 +23072,60 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
> > 
> > 
> >  int
> > +virDomainObjListConvert(virDomainObjListPtr domlist,
> > +                        virConnectPtr conn,
> > +                        virDomainPtr *doms,
> > +                        size_t ndoms,
> > +                        virDomainObjPtr **vms,
> > +                        size_t *nvms,
> > +                        virDomainObjListACLFilter filter,
> > +                        unsigned int flags,
> > +                        bool skip_missing)
> > +{
> > +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> > +    virDomainObjPtr vm;
> > +    size_t i;
> > +
> > +    *nvms = 0;
> > +    *vms = NULL;
> > +
> > +    virObjectLock(domlist);
> > +    for (i = 0; i < ndoms; i++) {
> > +        virDomainPtr dom = doms[i];
> > +
> > +        virUUIDFormat(dom->uuid, uuidstr);
> > +
> > +        if (!(vm = virHashLookup(domlist->objs, uuidstr))) {
> > +            if (!skip_missing) {
> > +                virReportError(VIR_ERR_NO_DOMAIN,
> > +                               _("no domain with matching uuid '%s' (%s)"),
> > +                               uuidstr, dom->name);
> > +                virObjectUnlock(domlist);
> 
> Move Unlock() before the virReportError(). Not that it would matter,
> but: a) it keeps list locked for shorter time b) I always felt like
> ReportError() and goto error should be joined together.

Okay.

> 
> > +                goto error;
> > +            }
> > +        } else {
> > +            virObjectRef(vm);
> > +
> > +            if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0)
> 
> Please unlock the @domlist before jumping anywhere.

Uhh, right. Also @vm needs to be unref'd before jumping to error.

> 
> > +                goto error;
> > +        }
> > +    }
> > +    virObjectUnlock(domlist);
> > +
> > +    virDomainObjListFilter(vms, nvms, conn, filter, flags);
> > +
> > +    return 0;
> > +
> > + error:
> > +    virObjectListFreeCount(*vms, *nvms);
> > +    *vms = NULL;
> > +    *nvms = 0;
> > +
> > +    return -1;
> > +}
> > +
> > +

I've also found the function hard to read after a week of hollidays so
I've changed the control flow a bit so that the else section is not
necessary and I've inverted the (!skip_missing) check to 'continue' the
for loop.

Thanks for the review I'll push this shortly.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150511/d2da1984/attachment-0001.sig>