[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]