Commit d18feadc0c put this into virNodeDeviceMatch, but this should have rather been part of virNodeDeviceObjHasCap from the beginning, since virNodeDeviceObjHasCap is the last helper to be called (in the calltree) whereas virNodeDeviceMatch is called from a single place only - virNodeDeviceObjListExportCallback. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/conf/virnodedeviceobj.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee4..0417664ad 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -644,6 +644,10 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, { virNodeDevCapsDefPtr cap = NULL; + /* Refresh the capabilities first, e.g. due to a driver change */ + if (virNodeDeviceUpdateCaps(obj->def) < 0) + return false; +

for (cap = obj->def->caps; cap; cap = cap->next) { if (type == cap->data.type) return true; @@ -811,10 +815,6 @@ static bool virNodeDeviceMatch(virNodeDeviceObjPtr obj, unsigned int flags) { - /* Refresh the capabilities first, e.g. due to a driver change */ - if (virNodeDeviceUpdateCaps(obj->def) < 0) - return false; - /* filter by cap type */ if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) { if (!(MATCH(SYSTEM) ||

On Wed, Mar 14, 2018 at 03:28:36PM -0400, John Ferlan wrote: > > > On 03/06/2018 10:32 AM, Erik Skultety wrote: >> Commit d18feadc0c put this into virNodeDeviceMatch, but this should have >> rather been part of virNodeDeviceObjHasCap from the beginning, since >> virNodeDeviceObjHasCap is the last helper to be called (in the calltree) >> whereas virNodeDeviceMatch is called from a single place only - >> virNodeDeviceObjListExportCallback. >> >> Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; >> --- >> src/conf/virnodedeviceobj.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c >> index ad0f27ee4..0417664ad 100644 >> --- a/src/conf/virnodedeviceobj.c >> +++ b/src/conf/virnodedeviceobj.c >> @@ -644,6 +644,10 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, >> { >> virNodeDevCapsDefPtr cap = NULL; >> >> + /* Refresh the capabilities first, e.g. due to a driver change */ >> + if (virNodeDeviceUpdateCaps(obj->def) < 0) >> + return false; >> + > > While I understand why you put it here - in order to be somewhere common > for nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and > nodeConnectListAllNodeDevices. > > However, I think this'll impact "performance" of virNodeDeviceMatch as > virNodeDeviceUpdateCaps would be called 16 times due to the logic of "if > !(MATCH() || MATCH() || MATCH() ...)))" > > Of course the alternative is calling virNodeDeviceUpdateCaps from > nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and > nodeConnectListAllNodeDevices. > > (or am I reading the !(a || b || c || ... ) logic wrong? No, you're right, unless you don't specify any flags yourself in ListAllNodeDevices, it will try to match everything.> Hmm, so as I responded to [1], the reason why I put the update where I

to avoid having to collect the list of objs twice (resulting in 3xO(n)), once to

update every single object's flags and then to actually filter the objs into the final list to be returned to the caller, so depends what you think is going to be a bigger hit for performance. Of course, I can revert it back and go with that solution which would make things easier for Cole I guess or there's another option (apart from acknowledging that there's going to be a certain performance hit either way - going with this patch or updating the devices before actually proceeding with and API) to continue with what Cole's suggested in his [1] series, add a 'update' bool to the virNodeDeviceObj structure, set it to true at the beginning of virNodeDeviceMatch, let the first MATCH's virNodeDeviceObjHasCap call update it, set it back to false, which means that a single obj will always be updated once in the span of a single API call.

Actually, there's one more way, we could revert this patch, leave the update in virNodeDeviceMatch and add the update call to virNodeDeviceObjHasCapStr and we should be settled for all the APIs, new and legacy ones.

Let me know what your thoughts on my proposals are. Erik [1] https://www.redhat.com/archives/libvir-list/2018-February/msg01136.html