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@redhat.com>
Reviewed-by: Erik Skultety <eskultet@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 */
 } virConnectListAllNodeDeviceFlags;
 
 int                     virConnectListAllNodeDevices (virConnectPtr conn,
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index c67b8e2aeb..3d7872fd6e 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -422,6 +422,14 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps);
                  VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE      | \
                  VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX)
 
+#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE \
+    VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE |\
+    VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE
+
+#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL \
+    VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP | \
+    VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE
+
 int
 virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host);
 
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 92f58dbf7d..6e9291264a 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -848,8 +848,10 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs,
 }
 
 
-#define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \
-                     virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG))
+#define MATCH_CAP(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \
+                         virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG))
+#define MATCH(FLAG) (flags & (FLAG))
+
 static bool
 virNodeDeviceObjMatch(virNodeDeviceObjPtr obj,
                       unsigned int flags)
@@ -861,33 +863,41 @@ virNodeDeviceObjMatch(virNodeDeviceObjPtr obj,
 
     /* filter by cap type */
     if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) {
-        if (!(MATCH(SYSTEM)        ||
-              MATCH(PCI_DEV)       ||
-              MATCH(USB_DEV)       ||
-              MATCH(USB_INTERFACE) ||
-              MATCH(NET)           ||
-              MATCH(SCSI_HOST)     ||
-              MATCH(SCSI_TARGET)   ||
-              MATCH(SCSI)          ||
-              MATCH(STORAGE)       ||
-              MATCH(FC_HOST)       ||
-              MATCH(VPORTS)        ||
-              MATCH(SCSI_GENERIC)  ||
-              MATCH(DRM)           ||
-              MATCH(MDEV_TYPES)    ||
-              MATCH(MDEV)          ||
-              MATCH(CCW_DEV)       ||
-              MATCH(CSS_DEV)       ||
-              MATCH(VDPA)          ||
-              MATCH(AP_CARD)       ||
-              MATCH(AP_QUEUE)      ||
-              MATCH(AP_MATRIX)))
+        if (!(MATCH_CAP(SYSTEM)        ||
+              MATCH_CAP(PCI_DEV)       ||
+              MATCH_CAP(USB_DEV)       ||
+              MATCH_CAP(USB_INTERFACE) ||
+              MATCH_CAP(NET)           ||
+              MATCH_CAP(SCSI_HOST)     ||
+              MATCH_CAP(SCSI_TARGET)   ||
+              MATCH_CAP(SCSI)          ||
+              MATCH_CAP(STORAGE)       ||
+              MATCH_CAP(FC_HOST)       ||
+              MATCH_CAP(VPORTS)        ||
+              MATCH_CAP(SCSI_GENERIC)  ||
+              MATCH_CAP(DRM)           ||
+              MATCH_CAP(MDEV_TYPES)    ||
+              MATCH_CAP(MDEV)          ||
+              MATCH_CAP(CCW_DEV)       ||
+              MATCH_CAP(CSS_DEV)       ||
+              MATCH_CAP(VDPA)          ||
+              MATCH_CAP(AP_CARD)       ||
+              MATCH_CAP(AP_QUEUE)      ||
+              MATCH_CAP(AP_MATRIX)))
             return false;
     }
 
+    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;
+
     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
  *
  * 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

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@redhat.com>
> Reviewed-by: Erik Skultety <eskultet@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
> 

On Mon, 15 Feb 2021 18:22:41 +0100
Erik Skultety <eskultet@redhat.com> wrote:

> 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@redhat.com>
> > Reviewed-by: Erik Skultety <eskultet@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?
> 
> ...

Because gcc implements the enum as a signed int and 1<<31 overflows the
maximum positive integer value:

In file included from ../include/libvirt/libvirt.h:42,
                 from ../src/internal.h:65,
                 from ../src/util/vircgroupv1.c:30:
../include/libvirt/libvirt-nodedev.h:91:57: error: result of ‘1 << 31’
requires 33 bits to represent, but ‘int’ only has 32 bits
[-Werror=shift-overflow=] 91 |     VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE
           = 1 << 31, /* Active devices */ |
                             ^~ cc1: all warnings being treated as errors


Jonathon