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.shcherba...@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 :|
