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