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. > O >> >>> + 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. > Hmm... Looks like I got distracted whilst writing and didn't finish my though.... I too have the gift of choosing names that cause angst for reviewers. Anyway - it's just a strange name for something that I think ends up being (what I call) the adapter or controller, e.g. in an .args file: -device vfio-pci,id=hostdev0,\ sysfsdev=/sys/bus/mdev/devices/53764d0e-85a0-42b4-af5c-2046b460b1dc,bus=pci.0,\ addr=0x3 the "vfio-pci" is less a "deviceAPI" to me and more the "device mechanism or name" to handle the traffic. But I see that "device_api" is

what the file name ends up being and that I assume was agreed upon by the consortium of those who have been arguing about the vGPU/mdev sysfs layout for far longer than I wanted to pay close attention, so we just get to deal with it. Now as for availableInstances - thankfully there's cut-n-paste to deal with that long name. Ironically the name is far longer than the value as opposed to something like uuid/wwnn/wwpn where the name is far shorter than what the value turns out to be. Glad I don't have to type a uuid/wwnn/wwpn value too often and even happier I have cut-n-paste

John