Re: [libvirt] [PATCH v4 4/6] nodedev: Introduce the mdev capability to a PCI parent device

>> +static int >> +udevFillMdevType(struct udev_device *device, >> + const char *dir, >> + virNodeDevCapMdevTypePtr type) >> +{ >> + int ret = -1; >> + char *attrpath = NULL; >> + >> +#define MDEV_GET_SYSFS_ATTR(attr_name, cb, ...) \ >> + do { \ >> + if (virAsprintf(&attrpath, "%s/%s", dir, #attr_name) < 0) \ >> + goto cleanup; \ >> + \ >> + if (cb(device, attrpath, __VA_ARGS__) < 0) \ >> + goto cleanup; \ >> + \ >> + VIR_FREE(attrpath); \ >> + } while (0) \ >> + >> + if (VIR_STRDUP(type->id, last_component(dir)) < 0) >> + goto cleanup; >> + >> + /* query udev for the attributes under subdirectories using the relative >> + * path stored in @dir, i.e. 'mdev_supported_types/<type_id>' >> + */ >> + MDEV_GET_SYSFS_ATTR(name, udevGetStringSysfsAttr, &type->name); > > Does this imply that name isn't necessarily optional as defined in RNG? > Can '@name' not exist and if it is optional how does that adjust the macro? There's no need for the macro to be changed - type->name == NULL in that case which means it won't be formatted to the XML, there's no issue in that the NULL's going to be handled gracefully all the way down from udevGetStringSysfsAttr.

> >> + MDEV_GET_SYSFS_ATTR(device_api, udevGetStringSysfsAttr, &type->device_api); >> + MDEV_GET_SYSFS_ATTR(available_instances, udevGetUintSysfsAttr, >> + &type->available_instances, 10); >> + >> +#undef MDEV_GET_SYSFS_ATTR >> + >> + ret = 0; >> + cleanup: >> + VIR_FREE(attrpath); >> + return ret; >> +} >> + > > > With changes to > > 1. fix the Coverity issues > 2. determine whether the virNodeDeviceObjHasCap change is needed > 3. address the optional name ^ see my comment above > > You have my: > > Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; > > While I'm not a fan of 'deviceAPI' Why so? I'm open to any suggestions, choosing the right name for basically anything is a gift I unfortunately do not possess, but truly desire to.