[libvirt PATCH 1/2] nodedev: fix parent device of inactive mdevs

[Michal Prívozník]

On 7/7/21 11:29 PM, Jonathon Jongsma wrote:
> Inactive mdevs were simply formatting their parent name as the value
> received from mdevctl rather than looking up the libvirt nodedev name of
> the parent device. This resulted in a parent value of e.g.
> '0000:5b:00.0' instead of 'pci_0000_5b_00_0'. This prevented defining a
> new mdev device from the output of nodedev-dumpxml.
> 
> Unfortunately, it's not simple to fix this comprehensively due to the
> fact that mdevctl supports defining (inactive) mdevs for parent devices
> that do not actually exist on the host (yet). So for those persistent
> mdev definitions that do not have a valid parent in the device list, the
> parent device will be set to the root "computer" device.
> 
> Unfortunately, because the value of the 'parent' field now depends on
> the configuration of the host, the mdevctl parsing test will output
> 'computer' for all test devices. Fixing this would require a more
> extensive mock test environment.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1979761
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> 
> fixup
> ---
>  src/node_device/node_device_driver.c          | 20 ++++++++++++++++++-
>  .../mdevctl-list-multiple.out.xml             |  8 ++++----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index b4dd57e5f4..26b0c2f032 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -1051,6 +1051,7 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
>      virJSONValue *props;
>      virJSONValue *attrs;
>      g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
> +    g_autofree char *parent_sysfs_path = NULL;
>  
>      /* the child object should have a single key equal to its uuid.
>       * The value is an object describing the properties of the mdev */
> @@ -1060,7 +1061,24 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
>      uuid = virJSONValueObjectGetKey(json, 0);
>      props = virJSONValueObjectGetValue(json, 0);
>  
> -    child->parent = g_strdup(parent);
> +    /* Look up id of parent device. mdevctl supports defining mdevs for parent
> +     * devices that are not present on the system (to support starting mdevs on
> +     * hotplug, etc) so the parent may not actually exist. */
> +    parent_sysfs_path = g_strdup_printf("/sys/class/mdev_bus/%s", parent);
> +    if (virFileExists(parent_sysfs_path)) {
> +        g_autofree char *canon_syspath = realpath(parent_sysfs_path, NULL);

I wonder why syntax-check did not catch this. We prefer
virFileCanonicalizePath().

> +        virNodeDeviceObj *parentobj = NULL;
> +        if ((parentobj = virNodeDeviceObjListFindBySysfsPath(driver->devs,

And we also like an empty line between these two ^^ to break the block
into two.

> +                                                             canon_syspath))) {
> +            virNodeDeviceDef *parentdef = virNodeDeviceObjGetDef(parentobj);
> +            child->parent = g_strdup(parentdef->name);
> +            virNodeDeviceObjEndAPI(&parentobj);
> +
> +            child->parent_sysfs_path = g_steal_pointer(&canon_syspath);
> +        }
> +    }
> +    if (!child->parent)
> +        child->parent = g_strdup("computer");
>      child->caps = g_new0(virNodeDevCapsDef, 1);
>      child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
>  

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal