[libvirt PATCH v4 03/25] nodedev: Add ability to filter by active state

[Erik Skultety]

On Wed, Feb 03, 2021 at 11:38:47AM -0600, Jonathon Jongsma wrote:
> Add two flag values for virConnectListAllNodeDevices() so that we can
> list only node devices that are active or inactive.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> ---
>  include/libvirt/libvirt-nodedev.h    |  9 +++--
>  src/conf/node_device_conf.h          |  8 ++++
>  src/conf/virnodedeviceobj.c          | 56 ++++++++++++++++------------
>  src/libvirt-nodedev.c                |  2 +
>  src/node_device/node_device_driver.c |  2 +-
>  5 files changed, 50 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
> index eab8abf6ab..d304283871 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -61,10 +61,9 @@ int                     virNodeListDevices      (virConnectPtr conn,
>   * virConnectListAllNodeDevices:
>   *
>   * Flags used to filter the returned node devices. Flags in each group
> - * are exclusive. Currently only one group to filter the devices by cap
> - * type.
> - */
> + * are exclusive.  */
>  typedef enum {
> +    /* filter the devices by cap type */
>      VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM        = 1 << 0,  /* System capability */
>      VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV       = 1 << 1,  /* PCI device */
>      VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV       = 1 << 2,  /* USB device */
> @@ -86,6 +85,10 @@ typedef enum {
>      VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD       = 1 << 18, /* s390 AP Card device */
>      VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE      = 1 << 19, /* s390 AP Queue */
>      VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX     = 1 << 20, /* s390 AP Matrix */
> +
> +    /* filter the devices by active state */
> +    VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE          = 1 << 29, /* Inactive devices */
> +    VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE            = 1 << 30, /* Active devices */

We don't define sentinels on public flags, so what are we saving the last value
for? Why couldn't ^this become 1U << 31?

...

>  
> +    if (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE) &&
> +        !((MATCH(VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE) &&
> +           virNodeDeviceObjIsActive(obj)) ||
> +          (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE) &&
> +           !virNodeDeviceObjIsActive(obj))))
> +            return false;

I didn't notice this in previous versions, but I think this block would read
better if written similarly as the one above it:

if (flags & (VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE)) {
    if (!(MATCH(VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE) &&
          virNodeDeviceObjIsActive(obj)) ||
          MATCH(VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE) &&
          !virNodeDeviceObjIsActive(obj))
        return false;
}

I'd also argue the new MATCH macro doesn't bring much value, but in case I was
the one who suggested it in the past I'd like to avoid contradicting myself and
thus we should keep it as is.

> +
>      return true;
>  }
>  #undef MATCH
> +#undef MATCH_CAP
>  
>  
>  typedef struct _virNodeDeviceObjListExportData virNodeDeviceObjListExportData;
> diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
> index eb8c735a8c..375b907852 100644
> --- a/src/libvirt-nodedev.c
> +++ b/src/libvirt-nodedev.c
> @@ -105,6 +105,8 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags)
>   *   VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD
>   *   VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE
>   *   VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX
> + *   VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE
> + *   VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE

An idea for a trivial follow up patch: Listing the caps in the function
commentary is counter productive, there's too many of them, it's easier to just
look up the enum definition. Also, there's no such thing as exclusive grouped
flags as the both the commentary and the header mention.

Erik

>   *
>   * Returns the number of node devices found or -1 and sets @devices to NULL in
>   * case of error.  On success, the array stored into @devices is guaranteed to
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 51b20848ce..c992251121 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -217,7 +217,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn,
>                                virNodeDevicePtr **devices,
>                                unsigned int flags)
>  {
> -    virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1);
> +    virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL, -1);
>  
>      if (virConnectListAllNodeDevicesEnsureACL(conn) < 0)
>          return -1;
> -- 
> 2.26.2
>