[libvirt] [PATCH] conf: nodedev: Update capabilities from within virNodeDeviceObjHasCap

[John Ferlan]

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 at 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?

John

>      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)        ||
>