Add a 'testdriver' bool that we set for test_driver.c nodedevs which will skip accessing host resources via virNodeDeviceUpdateCaps Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/conf/node_device_conf.c | 3 +++ src/conf/node_device_conf.h | 1 + src/test/test_driver.c | 2 ++ 3 files changed, 6 insertions(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index fd8f4e4a9..90c940f11 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def) { virNodeDevCapsDefPtr cap = def->caps; + if (def->testdriver) + return 0; + while (cap) { switch (cap->data.type) { case VIR_NODE_DEV_CAP_SCSI_HOST: diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 685ae3034..665f766e2 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -316,6 +316,7 @@ struct _virNodeDeviceDef { char *driver; /* optional driver name */ char *devnode; /* /dev path */ char **devlinks; /* /dev links */ + bool testdriver; /* if true, skip host checks */ virNodeDevCapsDefPtr caps; /* optional device capabilities */ }; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 043caa976..39784c9fa 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1165,6 +1165,7 @@ testParseNodedevs(testDriverPtr privconn, if (!def) goto error; + def->testdriver = true; if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) { virNodeDeviceDefFree(def); goto error; @@ -5565,6 +5566,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver, caps = caps->next; } + def->testdriver = true; if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; def = NULL; -- 2.14.3

On 03/02/2018 04:02 PM, John Ferlan wrote: Okay, thanks for review. How about finding a way to remove UpdateCaps from generic conf.c implementations instead? Seems wrong that something functions needed to implement ListAllDevices touch host resources, I don't think other objects work like that Thanks, Cole

On Fri, Mar 02, 2018 at 05:53:22PM -0500, John Ferlan wrote: Or simply update_caps, but I agree with your point... Yeah, you're right, we shouldn't touch the host... Sorry for not responding earlier, this indeed has been on my radar since the date of its posting, but I couldn't come up with anything better... Exactly, there's a kernel BZ on this and so far it looks like kernel's going to remain shrugging their shoulders and say, it's application layer's responsibility to figure out whether a device has been changed or not. [...] MDEV behaves like SRIOV here, i.e. whenever the host driver for PCI changes, all the nested capabilities related to PCI are affected and as you say, we can't reliably tell when that's happened, so we have to update them every time there's a query for node devices. You need to update the caps on each API. Since you mentioned having some refresh logic, that would be nice, although I'm not sure how that would help, depends on design, whether it's an on demand API or some other internal logic, because on demand doesn't really solve the problem here. Anyways, back to the point...If you look at this patch [1], you'll see the original proposal which was doing the update before issuing the intended API itself, I agree that this solution wouldn't have caused the issue with the test driver we're having, but also if you look closely at what nodeConnectUpdateAllNodeDevicesCaps was supposed to do, it called *ObjListExport (then iterate over the whole list) only to essentially call *ObjListExport again. Even though the time complexity was still 3*O(n) ~= O(n), I felt like we were using unnecessary CPU cycles here which could have been achieved as part of generating the list, since we already apply filters on the objects to filter them out of the resulting list. And that's what I did with my series. Now, to be fair, I was a bit hasty with my patches too, since I put the cap update into virNodeDeviceMatch which is only ever called from *ObjListExport, which means that nodeNumOfDevices and nodeListDevices still didn't work properly, hence my new patch [2]. Now back to your patch. I still think that updating capabilities makes complete sense in current form, however, not for test driver. So, at first I was thinking about whether we could pass a different 'dummy' callback for the update from the test driver somehow, so that the logic of 'updating the caps' would be preserved, but I don't think it's worth it and also it might not be even possible without putting significant effort into it. We could however go back (sort of) to the original proposal and call the update explicitly after obtaining the list of devices, which as I said is a bit ineffective, or (and I prefer this) we could go your way, but here I agree with John's point to move the flag to the object rather than the definition, since @def should serve as a description of the object, and having such a flag within @def seems conceptually wrong to me. Thanks, Erik [1] https://www.redhat.com/archives/libvir-list/2018-January/msg00316.html [2] https://www.redhat.com/archives/libvir-list/2018-March/msg00280.html

On 02/23/2018 06:16 PM, Cole Robinson wrote: At least 4.2.0 now Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John