[libvirt PATCH v5 3/7] Add PCI VPD-related helper functions to virpci

[Daniel P. Berrangé]

On Mon, Sep 27, 2021 at 10:30:49PM +0300, Dmitrii Shcherbakov wrote:
> Add helper functions to virpci to provide means of checking for a VPD
> file presence and for VPD resource retrieval using the PCI VPD parser.
> 
> The added test assesses the basic functionality of VPD retrieval while
> the full parser is tested by virpcivpdtest.
> 
> Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov at canonical.com>
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/virpci.c        | 62 ++++++++++++++++++++++++++++++++++++++
>  src/util/virpci.h        |  3 ++
>  tests/virpcimock.c       | 30 +++++++++++++++++++
>  tests/virpcitest.c       | 64 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 161 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index bb9b380599..ebee46ce9b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2988,7 +2988,9 @@ virPCIDeviceGetReprobe;
>  virPCIDeviceGetStubDriver;
>  virPCIDeviceGetUnbindFromStub;
>  virPCIDeviceGetUsedBy;
> +virPCIDeviceGetVPDResources;
>  virPCIDeviceHasPCIExpressLink;
> +virPCIDeviceHasVPD;
>  virPCIDeviceIsAssignable;
>  virPCIDeviceIsPCIExpress;
>  virPCIDeviceListAdd;
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index f307580a53..480d91c17f 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -37,6 +37,7 @@
>  #include "virkmod.h"
>  #include "virstring.h"
>  #include "viralloc.h"
> +#include "virpcivpd.h"
>  
>  VIR_LOG_INIT("util.pci");
>  
> @@ -2640,6 +2641,53 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
>      return 0;
>  }
>  
> +
> +gboolean
> +virPCIDeviceHasVPD(virPCIDevice *dev)
> +{
> +    g_autofree char *vpdPath = NULL;
> +    vpdPath = virPCIFile(dev->name, "vpd");
> +    if (!virFileExists(vpdPath)) {
> +        VIR_INFO("Device VPD file does not exist %s", vpdPath);

IIUC this is normal case for most PCI devices, so DEBUG level is
better.

> +        return false;
> +    } else if (!virFileIsRegular(vpdPath)) {
> +        VIR_WARN("VPD path does not point to a regular file %s", vpdPath);
> +        return false;
> +    }
> +    return true;
> +}
> +
> +/**
> + * virPCIDeviceGetVPDResources:
> + * @dev: a PCI device to get a list of VPD resources for.
> + *
> + * Obtain resources stored in the PCI device's Vital Product Data (VPD).
> + * VPD is optional in both PCI Local Bus and PCIe specifications so there is
> + * no guarantee it will be there for a particular device.
> + *
> + * Returns: a pointer to a list of VPDResource types which needs to be freed by the caller or
> + * NULL if getting it failed for some reason.
> + */
> +GList *
> +virPCIDeviceGetVPDResources(virPCIDevice *dev)
> +{
> +    g_autofree char *vpdPath = NULL;
> +    int fd;
> +
> +    vpdPath = virPCIFile(dev->name, "vpd");
> +    if (!virPCIDeviceHasVPD(dev)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s does not have a VPD"),
> +                virPCIDeviceGetName(dev));

Nit-pick - align with the '(' above.

> +        return NULL;
> +    }
> +    if ((fd = open(vpdPath, O_RDONLY)) < 0) {
> +        virReportSystemError(-fd,
> +                             _("Failed to open a VPD file '%s'"), vpdPath);
> +        return NULL;
> +    }
> +    return virPCIVPDParse(fd);

Nothing is closing 'fd' here, unless virPCIVPDParse is doing it, which
I would consider an unexpected design.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|