[libvirt PATCH v2 08/16] nodedev: handle mdevs that disappear from mdevctl

[Erik Skultety]

On Tue, Aug 18, 2020 at 09:47:58AM -0500, 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 | 61 ++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 349426757e..affd707a65 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -1089,6 +1089,37 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr def,
>      }
>  }
>
> +static bool
> +mdevMatchPersistentDevices(virConnectPtr conn G_GNUC_UNUSED,
> +                           virNodeDeviceDefPtr def)
> +{
> +            return (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
> +                    def->caps->data.mdev.persistent);

Indentation is off here.

> +
> +
> +}
> +
> +/* returns a null-terminated array of strings. Free with virStringListFree() */
> +static char **
> +nodeDeviceGetMdevPersistentDevices(virNodeDeviceObjListPtr devlist)
> +{
> +    char **ret = NULL;
> +    int ndevs = virNodeDeviceObjListNumOfDevices(devlist,
> +                                                 NULL,
> +                                                 "mdev",
> +                                                 mdevMatchPersistentDevices);
> +    if (VIR_ALLOC_N(ret, ndevs+1) < 0)
> +        return NULL;
> +
> +    if (virNodeDeviceObjListGetNames(devlist, NULL,
> +                                     mdevMatchPersistentDevices,
> +                                     "mdev",
> +                                     ret,
> +                                     ndevs) < 0)
> +        VIR_FREE(ret);
> +
> +    return ret;
> +}

This suggestion will result in more code, but in the long run I think it is the
better solution and is definitely consistent with other areas of libvirt
where we iterate obj lists and have to apply modifications. Time complexity
wise, honestly it's hard to tell whether it would actually be more efficient
(there are a number of factors in play), I guess it would be about the same.

What IMO should be done instead is to extend the API set for the node device
obj list by virNodeDeviceObjListForEach and virNodeDeviceObjListRemoveLocked.
Then, modify the mdevctl JSON parser to return a hash table of devices instead
of a linked list.
Then, use the nodedev list "ForEach" iterator, pass the mdevctl hash table
defined devices as payload and then use RemoveLocked to remove elements (which
passed the MDEV cap + persistence filter) not found in the mdevctl hash table
from driver->devs.

>
>  static int
>  virMdevctlListDefined(virNodeDeviceDefPtr **devs)
> @@ -1113,6 +1144,7 @@ mdevctlEnumerateDevices(void)
>      g_autofree virNodeDeviceDefPtr *devs = NULL;
>      int ndevs;
>      size_t i;
> +    char **oldmdevs = nodeDeviceGetMdevPersistentDevices(driver->devs);
>
>      if ((ndevs = virMdevctlListDefined(&devs)) < 0) {
>          virReportSystemError(errno, "%s",
> @@ -1160,7 +1192,36 @@ mdevctlEnumerateDevices(void)
>              event = virNodeDeviceEventUpdateNew(dev->name);
>          virNodeDeviceObjEndAPI(&obj);
>          virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +
> +        virStringListRemove(&oldmdevs, dev->name);
> +    }
> +
> +    /* Any mdevs that were previously defined but were not returned by mdevctl
> +     * this time will need to be checked to see if they should be removed from
> +     * the device list */
> +    for (i = 0; i < virStringListLength((const char **)oldmdevs); i++) {
> +        const char *name = oldmdevs[i];
> +        virNodeDeviceObjPtr obj = NULL;
> +        if ((obj = virNodeDeviceObjListFindByName(driver->devs, name))) {
> +            if (!virNodeDeviceObjIsActive(obj)) {
> +                /* An existing device is not active and is no longer defined by
> +                 * mdevctl, so remove it */
> +                virObjectEventPtr event = virNodeDeviceEventLifecycleNew(name,
> +                                                                         VIR_NODE_DEVICE_EVENT_DELETED,
> +                                                                         0);
> +
> +                virNodeDeviceObjListRemove(driver->devs, obj);
> +                virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +            } else {
> +                /* An existing device is active, but no longer defined. Keep
> +                 * the device in the list, but mark it as non-persistent */
> +                virNodeDeviceDefPtr def = virNodeDeviceObjGetDef(obj);
> +                def->caps->data.mdev.persistent = false;
> +            }
> +            virNodeDeviceObjEndAPI(&obj);

If nothing else, ^this would deserve a helper.

Regards,
Erik