Re: [RFC PATCH v1 03/15] nodedev: immediate update of active config on udev add

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

On 4/12/24 8:36 AM, Marc Hartmayer wrote:
> From: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>
> When an udev add event occurs the mdev active config data requires an
> update via mdevctl as the udev does not contain all config data.
> This update needs to occur immediate and to be finished before the

s/immediate/immediately/

> libvirt CREATE event is issued to keep the API usage reliable.
>
> After this change, scheduleMdevctlUpdate call is already called in
> `udevAddOneDevice` and can therefore be removed in `udevHandleOneDevice`.
>
> Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
> ---
>   src/node_device/node_device_udev.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 6613528d0e37..03ef0c14371a 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1535,6 +1535,7 @@ udevSetParent(struct udev_device *device,
>   static int
>   udevAddOneDevice(struct udev_device *device)
>   {
> +    g_autofree char *sysfs_path = NULL;
>       virNodeDeviceDef *def = NULL;
>       virNodeDeviceObj *obj = NULL;
>       virNodeDeviceDef *objdef;
> @@ -1549,6 +1550,9 @@ udevAddOneDevice(struct udev_device *device)
>       def = g_new0(virNodeDeviceDef, 1);
>   
>       def->sysfs_path = g_strdup(udev_device_get_syspath(device));
> +    /* Create a copy of sysfs_path so it can be safely accessed, even without
> +     * holding the @obj lock during the VIR_WARN(...) call at the end. */
> +    sysfs_path = g_strdup(def->sysfs_path);
>   
>       udevGetStringProperty(device, "DRIVER", &def->driver);
>   
> @@ -1608,6 +1612,13 @@ udevAddOneDevice(struct udev_device *device)
>       if (has_mdev_types)
>           scheduleMdevctlUpdate(driver->privateData, false);
>   
> +    /* The added mdev needs an immediate active config update before
> +     * the event is issued to allow sane API usage. */

How about simply something like "so that full device information is 
available at the time that the 'created' event is emitted"

> +    if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) {
> +        VIR_WARN("Update of mediated device %s failed",
> +                 NULLSTR_EMPTY(sysfs_path));
> +    }
> + >       ret = 0;
>   
>    cleanup:
> @@ -1758,19 +1769,12 @@ nodeStateCleanup(void)
>   static int
>   udevHandleOneDevice(struct udev_device *device)
>   {
> -    virNodeDevCapType dev_cap_type;
>       const char *action = udev_device_get_action(device);
>   
>       VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
>   
> -    if (STREQ(action, "add") || STREQ(action, "change")) {
> -        int ret = udevAddOneDevice(device);
> -        if (ret == 0 &&
> -            udevGetDeviceType(device, &dev_cap_type) == 0 &&
> -            dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
> -            scheduleMdevctlUpdate(driver->privateData, false);
> -        return ret;
> -    }
> +    if (STREQ(action, "add") || STREQ(action, "change"))
> +        return udevAddOneDevice(device);
>   
>       if (STREQ(action, "remove"))
>           return udevRemoveOneDevice(device);

Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx