Re: [libvirt PATCH v4 03/25] nodedev: Add ability to filter by active state
On Mon, 15 Feb 2021 18:22:41 +0100
Erik Skultety <eskultet(a)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(a)redhat.com>
> Reviewed-by: Erik Skultety <eskultet(a)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