> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
