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