[...]
+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@redhat.com>
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