[libvirt] [PATCH v4 04/14] util: Introduce new module virmdev

[Erik Skultety]

> > +
> > +static int
> > +virMediatedDeviceGetSysfsDeviceAPI(virMediatedDevicePtr dev,
> > +                                   char **device_api)
> > +{
> > +    int ret = -1;
> > +    char *buf = NULL;
> > +    char *tmp = NULL;
> > +    char *file = NULL;
> 
> Since buf and file both point to strings that we need to free, and tmp
> doesn't, I think it would be better if tmp was after the others and not
> initialized since it's never used before it's set) to differentiate it
> from the other two (you can't differentiate by making it const, since
> it's used to replace the \n with 0. A comment saying that anything it
> points to doesn't need to be freed might be nice too. (Generally I
> assume that const char* doesn't need its data freed, but char* does.
> Seeing it not freed sets off alarms that then need brain cells to suppress).

Well, the name 'tmp' sort of indicates that it is going to be used for
something temporary/volatile which could be anything, so the pointer could be
used further in the code for something that indeed needs freeing, combine it
with forgetting to remove the comment in that case and you created an
unnecessary confusion. Although I get your point and internally agree that it
could be useful, I also think that generally the best approach is just to scroll
through the code and look at how a specific variable is used
(however sad it sounds...).

[..]

> > +
> > +
> > +static int
> > +virMediatedDeviceCheckModel(virMediatedDevicePtr dev,
> > +                            virMediatedDeviceModelType model)
> > +{
> > +    int ret = -1;
> > +    char *dev_api = NULL;
> > +    int actual_model;
> > +
> > +    if (virMediatedDeviceGetSysfsDeviceAPI(dev, &dev_api) < 0)
> > +        return -1;
> > +
> > +    /* safeguard in case we've got an older libvirt which doesn't know newer
> > +     * device_api models yet
> > +     */
> > +    if ((actual_model = virMediatedDeviceModelTypeFromString(dev_api)) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("Device API '%s' not supported yet"),
> > +                       dev_api);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (actual_model != model) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("Invalid device API '%s' for device %s: "
> > +                         "device only supports '%s'"),
> > +                       virMediatedDeviceModelTypeToString(model),
> > +                       dev->path, dev_api);
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = 0;
> > + cleanup:
> > +    VIR_FREE(dev_api);
> > +    return ret;
> > +}
> > +
> > +
> > +#ifdef __linux__
> 
> Why did you only start the #ifdef part here - surely most of the stuff
> up above would never be used on a non-linux system either?
> virMediatedDeviceGetSysfsDeviceAPI for example references files in linux
> sysfs.

As per [1] I dropped the #ifdef part covering all the functions, but I think
it would be a good practice to at least cover the *CheckModel and
*GetSysfsDeviceAPI functions since you're right about it, although it's not
necessary at the moment because they're only being called from
virMediatedDeviceNew which is correctly guarded by the #ifdef.

[1] https://www.redhat.com/archives/libvir-list/2017-February/msg00171.html

> > +virMediatedDevicePtr
> > +virMediatedDeviceNew(virPCIDeviceAddressPtr pciaddr ATTRIBUTE_UNUSED,
> > +                     const char *uuidstr ATTRIBUTE_UNUSED)
> > +{
> > +    virRerportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                    _("not supported on non-linux platforms"));
> 
> Maybe say "mediate devices ...." in case someone sees/reports the log
> message from the output of virsh (where the function name isn't a part
> of the log message).
> 
> > +    return NULL;
> > +int
> > +virMediatedDeviceGetIOMMUGroupNum(virMediatedDevicePtr dev)
> > +{
> > +    char *vfio_path = NULL;
> > +    char *group_num_str = NULL;
> > +    unsigned int group_num = -1;
> > +
> > +    if (!(vfio_path = virMediatedDeviceGetIOMMUGroupDev(dev)))
> > +        return -1;
> > +
> > +    group_num_str = last_component(vfio_path);
> > +    ignore_value(virStrToLong_ui(group_num_str, NULL, 10, &group_num));
> 
> I'm assuming the caller logs a message on failure? If so, then the
> ignore_value is okay.

Well, they have to. If you think about it, we're converting string which we
obtained by ourselves from sysfs. If virMediatedDeviceGetIOMMUGroupDev failed,
it'd log error by itself, so the only point of failure in this function is
"last_component" from gnulib. So supposing that our code obtaining file paths
is correct, there is really no way how virStrToLong* could fail reasonably, so
I decided to just ignore the return value.

[...]
> 
> 
> (end of comments)
> 
> To sum it up: ACK if you:
> 
> * remove the unnecessary #includes
> 
> * make it obvious when defined that tmp doesn't "own" any memory
>   that should be freed (just to make life easier for maintainers)

- I'd like to, but see my comment above, in any case, it can be added later

> 
> * modify the "unsupported" log message to start with "mediated
>   devices"
> 
> * maybe put more/most/all(?) of the functions inside #ifdef __linux__
> 

Adjusted, thanks.

Erik