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@xxxxxxxxxx>
> >>
> >> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
