Re: [libvirt] [PATCH] conf: nodedev: Update capabilities from within virNodeDeviceObjHasCap
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>
> ---
> 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 did was
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