Re: [PATCH 6/8] nodedev: add ccwgroup node device support

On Tue, Feb 04, 2025 at 18:11:41 +0100, Boris Fiuczynski wrote: > Add ccwgroup node device type supporting qeth generic driver. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > docs/manpages/virsh.rst | 6 +- > include/libvirt/libvirt-nodedev.h | 1 + > src/conf/node_device_conf.c | 212 ++++++++++++++++++ > src/conf/node_device_conf.h | 29 ++- > src/conf/schemas/nodedev.rng | 43 ++++ > src/conf/virnodedeviceobj.c | 4 +- > src/libvirt_private.syms | 2 + > src/node_device/node_device_driver.c | 5 + > src/node_device/node_device_udev.c | 41 ++++ > src/util/virccw.c | 102 +++++++++ > src/util/virccw.h | 22 ++ > tests/nodedevschemadata/ccwgroup_0_0_bd00.xml | 20 ++ > tests/nodedevxml2xmlout/ccwgroup_0_0_bd00.xml | 1 + > tests/nodedevxml2xmltest.c | 1 + > tools/virsh-nodedev.c | 3 + > 15 files changed, 487 insertions(+), 5 deletions(-) > create mode 100644 tests/nodedevschemadata/ccwgroup_0_0_bd00.xml > create mode 120000 tests/nodedevxml2xmlout/ccwgroup_0_0_bd00.xml [...] > +static int > +udevProcessCCWGroup(struct udev_device *device, > + virNodeDeviceDef *def) > +{ > + const char *devtype = udev_device_get_devtype(device); > + virNodeDevCapData *data = &def->caps->data; > + > + data->ccwgroup_dev.address = virCCWDeviceAddressFromString(udev_device_get_sysname(device)); > + > + udevCCWGetState(device, &data->ccwgroup_dev.state); > + > + udevGenerateDeviceName(device, def, NULL); > + > + if ((data->ccwgroup_dev.type = virNodeDevCCWGroupCapTypeFromString(devtype)) < 0) > + return -1; Apart from the broken build where clang complains about assignment to the unsigned enum which I've fixed, this doesn't report an error ...

> + > + switch (data->ccwgroup_dev.type) { > + case VIR_NODE_DEV_CAP_CCWGROUP_QETH_GENERIC: > + { > + virCCWGroupTypeQeth *qeth = &data->ccwgroup_dev.qeth; > + /* process qeth device information */ > + udevGetStringSysfsAttr(device, "card_type", &qeth->card_type); > + udevGetStringSysfsAttr(device, "chpid", &qeth->chpid); > + } > + break; > + case VIR_NODE_DEV_CAP_CCWGROUP_LAST: > + return -1; > + } > + > + if (virNodeDeviceGetCCWGroupDynamicCaps(def->sysfs_path, > + &data->ccwgroup_dev) < 0) ... and neither this at least on 2 code paths: 1) the dummy impl if not running on linux 2) if the assertion that a group must be non-empty fails in the real code path inside virCCWGroupDeviceGetMembers.

The caller does seem to care about errors as other code paths do set them, thus this should as well. > + return -1; > + > + return 0; > +}