[libvirt] [PATCH v4 4/6] nodedev: Introduce the mdev capability to a PCI parent device

[John Ferlan]

[...]

>>> +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 at 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