[libvirt PATCH v2 06/16] nodedev: add STOPPED/STARTED lifecycle events

[Erik Skultety]

On Tue, Aug 18, 2020 at 09:47:56AM -0500, Jonathon Jongsma wrote:
> Since a mediated device can be persistently defined by the mdevctl
> backend, we need additional lifecycle events beyond CREATED/DELETED to
> indicate that e.g. the device has been stopped but the device definition
> still exists.

I think this patch should come a bit later where there already are the
necessary backing functionality present.

>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  examples/c/misc/event-test.c         |  4 ++++
>  include/libvirt/libvirt-nodedev.h    |  2 ++
>  src/conf/node_device_conf.h          |  1 +
>  src/node_device/node_device_driver.c |  1 +
>  src/node_device/node_device_udev.c   | 25 +++++++++++++++++++++++--
>  tools/virsh-nodedev.c                |  4 +++-
>  6 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c
> index 52caa8ffa8..c40819969c 100644
> --- a/examples/c/misc/event-test.c
> +++ b/examples/c/misc/event-test.c
> @@ -381,6 +381,10 @@ nodeDeviceEventToString(int event)
>              return "Created";
>          case VIR_NODE_DEVICE_EVENT_DELETED:
>              return "Deleted";
> +        case VIR_NODE_DEVICE_EVENT_STOPPED:
> +            return "Stopped";
> +        case VIR_NODE_DEVICE_EVENT_STARTED:
> +            return "Started";
>          case VIR_NODE_DEVICE_EVENT_LAST:
>              break;
>      }
> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
> index 98a972e60d..423a583d45 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -192,6 +192,8 @@ int virConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
>  typedef enum {
>      VIR_NODE_DEVICE_EVENT_CREATED = 0,
>      VIR_NODE_DEVICE_EVENT_DELETED = 1,
> +    VIR_NODE_DEVICE_EVENT_STOPPED = 2,
> +    VIR_NODE_DEVICE_EVENT_STARTED = 3,
>
>  # ifdef VIR_ENUM_SENTINELS
>      VIR_NODE_DEVICE_EVENT_LAST
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 353dbebaf0..7e825ca6a9 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -144,6 +144,7 @@ struct _virNodeDevCapMdev {
>      char *uuid;
>      virMediatedDeviceAttrPtr *attributes;
>      size_t nattributes;
> +    bool persistent;

^This attribute should live in the object instead...

>  };
>
>  typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev;
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 3b042e9a45..b47ecba10a 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -894,6 +894,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>                  child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
>
>                  mdev = &child->caps->data.mdev;
> +                mdev->persistent = true;

...and be set outside of the JSON parsing.

>                  mdev->uuid = g_strdup(uuid);
>                  mdev->type =
>                      g_strdup(virJSONValueObjectGetString(props, "mdev_type"));
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 93b74b1f24..ac7986bc70 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1213,6 +1213,8 @@ udevRemoveOneDeviceSysPath(const char *path)
>      virNodeDeviceObjPtr obj = NULL;
>      virNodeDeviceDefPtr def;
>      virObjectEventPtr event = NULL;
> +    virNodeDevCapsDefPtr cap;
> +    int event_type = VIR_NODE_DEVICE_EVENT_DELETED;
>
>      if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) {
>          VIR_DEBUG("Failed to find device to remove that has udev path '%s'",
> @@ -1221,13 +1223,32 @@ udevRemoveOneDeviceSysPath(const char *path)
>      }
>      def = virNodeDeviceObjGetDef(obj);
>
> +    /* If the device is a mediated device that has been 'stopped', it may still
> +     * be defined by mdevctl and can therefore be started again. Don't drop it
> +     * from the list of node devices */
> +    cap = def->caps;
> +    while (cap != NULL) {
> +        if (cap->data.type == VIR_NODE_DEV_CAP_MDEV) {
> +            if (cap->data.mdev.persistent) {
> +                VIR_FREE(def->sysfs_path);

Once we move the persistent attribute to the 'obj' instead, we can then
simplify ^this whole bit to merely:

if (obj->persitent) {
    VIR_FREE(def->sysfs_path);
    virNodeDeviceObjSetActive(obj, false);
}

I'd also like to see the commentary above enhanced with an explanation that we
only need to free the sysfs path, since that part is the only one that is
dynamic, so the rest doesn't change per the definition.

> +                event_type = VIR_NODE_DEVICE_EVENT_STOPPED;
> +                break;
> +            }
> +        }
> +        cap = cap->next;
> +    }
> +
>      event = virNodeDeviceEventLifecycleNew(def->name,
> -                                           VIR_NODE_DEVICE_EVENT_DELETED,
> +                                           event_type,
>                                             0);
>
>      VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
>                def->name, path);
> -    virNodeDeviceObjListRemove(driver->devs, obj);
> +
> +    if (event_type == VIR_NODE_DEVICE_EVENT_DELETED)
> +        virNodeDeviceObjListRemove(driver->devs, obj);
> +    else
> +        virNodeDeviceObjSetActive(obj, false);

^This 'else' branch could be moved to the "persistence" check above as
demonstrated.

Erik