[libvirt PATCH v3 09/21] nodedev: handle mdevs that disappear from mdevctl

[Erik Skultety]

On Thu, Dec 24, 2020 at 08:14:33AM -0600, Jonathon Jongsma wrote:
> mdevctl does not currently provide any events when the list of defined
> devices changes, so we will need to poll mdevctl for the list of defined
> devices periodically. When a mediated device no longer exists from one
> iteration to the next, we need to treat it as an "undefine" event.
> 
> When we get such an event, we remove the device from the list if it's
> not active. Otherwise, we simply mark it as non-persistent.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  src/node_device/node_device_driver.c | 67 +++++++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 5 deletions(-)
> 
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 0267005af1..0bebd534d0 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -1163,23 +1163,73 @@ virMdevctlListDefined(virNodeDeviceDefPtr **devs)
>  }
>  
>  
> +typedef struct _virMdevctlForEachData virMdevctlForEachData;
> +struct _virMdevctlForEachData {
> +    int ndevs;
> +    virNodeDeviceDefPtr *devs;
> +};
> +
> +
> +/* This function keeps the list of persistent mediated devices consistent
> + * between the nodedev driver and mdevctl.
> + * @obj is a device that is currently known by the nodedev driver, and @opaque
> + * contains the most recent list of devices defined by mdevctl. If @obj is no
> + * longer defined in mdevctl, remove it from the driver as well. */
> +static void
> +removeMissingPersistentMdevs(virNodeDeviceObjPtr obj,
> +                             const void *opaque)
> +{
> +    const virMdevctlForEachData *data = opaque;
> +    size_t i;
> +    virNodeDeviceDefPtr def = virNodeDeviceObjGetDef(obj);
> +    virObjectEventPtr event;
> +
> +    /* transient mdevs are populated via udev, so don't remove them from the
> +     * nodedev driver just because they are not reported by by mdevctl */
> +    if (!(def->caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
> +          def->caps->data.mdev.persistent))
> +        return;
> +
> +    for (i = 0; i < data->ndevs; i++) {
> +        /* OK, this mdev is still defined by mdevctl */
> +        if (STREQ(data->devs[i]->name, def->name))
> +            return;
> +    }
> +
> +    if (virNodeDeviceObjIsActive(obj)) {
> +        /* The device is active, but no longer defined by mdevctl. Keep
> +         * the device in the list, but mark it as non-persistent */

^This commentary should most likely be outside of the 'if' condition just like
the one a few lines above.

> +        def->caps->data.mdev.persistent = false;
> +        return;
> +    }
> +
> +    /* the device is not active, and it is no longer defined by mdevctl, so
> +     * remove it. */
> +    event = virNodeDeviceEventLifecycleNew(def->name,
> +                                           VIR_NODE_DEVICE_EVENT_DELETED,
> +                                           0);
> +
> +    virNodeDeviceObjListRemoveLocked(driver->devs, obj);

For the sake of logically ordering the actions, I believe we should first
remove the device and *then* emit an event - not that it would matter much in
the end since virNodeDeviceObjListRemoveLocked doesn't return anything, so even
if it failed to remove the device for some reason we'd emit the event anyway...

> +    virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +}
> +
> +
>  int
>  nodeDeviceUpdateMediatedDevices(void)
>  {
> -    g_autofree virNodeDeviceDefPtr *devs = NULL;
> -    int ndevs;
>      size_t i;
> +    virMdevctlForEachData data = { 0, };
>  
> -    if ((ndevs = virMdevctlListDefined(&devs)) < 0) {
> +    if ((data.ndevs = virMdevctlListDefined(&data.devs)) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("failed to query mdevs from mdevctl"));
>          return -1;
>      }
>  
> -    for (i = 0; i < ndevs; i++) {
> +    for (i = 0; i < data.ndevs; i++) {
>          virNodeDeviceObjPtr obj;
>          virObjectEventPtr event;
> -        virNodeDeviceDefPtr dev = devs[i];
> +        virNodeDeviceDefPtr dev = data.devs[i];
>          bool new_device = true;
>  
>          dev->driver = g_strdup("vfio_mdev");
> @@ -1218,5 +1268,12 @@ nodeDeviceUpdateMediatedDevices(void)
>          virObjectEventStateQueue(driver->nodeDeviceEventState, event);
>      }
>  


The context is missing here, but you'd leak data.devs on line 1258:

    if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, dev))) {
        virNodeDeviceDefFree(dev);
        return -1;
    }

With the adjustments:
Reviewed-by: Erik Skultety <eskultet at redhat.com>