[libvirt] [PATCH v3 02/12] test: Adjust cleanup/error paths for nodedev test APIs

[Erik Skultety]

> > Just a minor nit, while I was checking where testDestroyVport gets called from
> > - testStoragePoolDestroy, there's a spot that could be fixed the same way.
> > Would you mind squashing this bit in as well?
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 6422ba8fb..a397364c7 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -4547,7 +4547,7 @@ testStoragePoolDestroy(virStoragePoolPtr pool)
> >      virObjectEventPtr event = NULL;
> >
> >      if (!(privpool = testStoragePoolObjFindByName(privconn, pool->name)))
> > -        goto cleanup;
> > +        return -1;
> >
>
> I don't mind changing this as well - although I probably have that in
> another patch somewhere dealing with storage tests.  Trying to keep
> nodedev with nodedev and storage with storage.

I figured, but this is where these two world clash, you know, calling nodedev
stuff from storage... I don't care if you fix in this one or in some other
storage related patch, just so we don't forget about that.

>
>
> >      if (!virStoragePoolObjIsActive(privpool)) {
> >          virReportError(VIR_ERR_OPERATION_INVALID,
> >
> >
> >
> > [...]
> >
> >> @@ -5424,27 +5408,26 @@ testNodeDeviceListCaps(virNodeDevicePtr dev, char **const names, int maxnames)
> >>      virNodeDeviceDefPtr def;
> >>      virNodeDevCapsDefPtr caps;
> >>      int ncaps = 0;
> >> -    int ret = -1;
> >>
> >>      if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
> >> -        goto cleanup;
> >> +        return -1;
> >>      def = virNodeDeviceObjGetDef(obj);
> >>
> >>      for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) {
> >> -        if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0)
> >> -            goto cleanup;
> >> +        if (VIR_STRDUP(names[ncaps],
> >> +                       virNodeDevCapTypeToString(caps->data.type)) < 0)
> >> +            goto error;
> >> +        ncaps++;
> >
> > How about placing the increment to the iteration_expression segment of the
> > loop, aka at the end where the usual increment happens before condition
> > evaluation. Wondering whether there's a general technical term for the syntax
> > part of the loop between the parentheses.
> >
>
> Do you mean :
>
> (caps = def->caps; caps && ncaps < maxnames; caps = caps->next, ncaps++)
>
> ??
>
> If so, yuck. Since we're iterating on caps, I think what should be
> incremented is caps and not ncaps as well. Putting it after the

Well, if we only cared about the @caps in the evaluation condition, maybe, but
we also check @ncaps and I think it's pretty clear that ncaps are only
incremented when VIR_STRDUP passed, otherwise we would have dropped from the
loop in the first place. I don't intend to argue about this, as it's not a show
stopper, so I don't really care that much about that, I'm fine either way.

Erik