On Thu, May 18, 2017 at 06:48:48AM -0400, John Ferlan wrote:
[...]
+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
You're right, but this way, we at least stay consistent with arguably poor naming under sysfs - I know, might have been better off with letting the fantasy off the leash, but hopefully this one's going to bite us in the back less than usual....yeah, right....
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
Thanks for review, I'm going to push the series in a moment. I also created BZ 1452072 to track the feature and pasted the URL to patches 3-6. Erik
John