On Thu, Feb 18, 2021 at 04:05:19PM -0600, Jonathon Jongsma wrote:
> On Mon, 15 Feb 2021 18:22:41 +0100
> Erik Skultety <eskultet@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
> > > ---
> > > 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
Doesn't sound correct. GCC will accommodate the underlying enum type according
to the needs, IOW it will select the type so that it can hold all the values
inside. Note that I forced "1U" in there which explicitly asks GCC to select
unsigned int for the enum, I could have gone with 1ULL << 61 (which would break
our docs generator :D), but our API only supports unsigned int flags anyway.
Erik
