[libvirt PATCH 0/7] Enable autostarting mediated devices
This series replaces the initial patch that was recently reverted. It implements the autostart feature using a new virNodeDeviceGet/SetAutostart() API that is consistent with how other libvirt objects handle autostart. It also adds a counterpart virsh command nodedev-autostart. In order to easily check the 'autostart' status of the device (since it is no longer part of the device xml), a new virsh command is introduced: nodedev-info. This also presents a few more basic bits of information about the device, including 'active' and 'persistent' status, which requires exposing new APIs on the node device: IsActive() and IsPersistent(). These APIs are consistent with existing libvirt objects. Jonathon Jongsma (7): api: add virNodeDevice(Get|Set)Autostart() nodedev: implement virNodeDevice(Get|Set)Autostart() nodedev: Add tests for mdevctl autostart command virsh: add nodedev-autostart api: add virNodeDeviceIsPersistent()/IsActive() nodedev: Implement virNodeDeviceIsPersistent()/IsActive() virsh: add nodedev-info docs/manpages/virsh.rst | 27 +++ include/libvirt/libvirt-nodedev.h | 10 ++ src/conf/virnodedeviceobj.c | 16 ++ src/conf/virnodedeviceobj.h | 6 + src/driver-nodedev.h | 18 ++ src/libvirt-nodedev.c | 141 +++++++++++++++ src/libvirt_private.syms | 2 + src/libvirt_public.syms | 6 + src/node_device/node_device_driver.c | 166 ++++++++++++++++++ src/node_device/node_device_driver.h | 19 ++ src/node_device/node_device_udev.c | 30 +++- src/remote/remote_driver.c | 6 +- src/remote/remote_protocol.x | 59 ++++++- src/remote_protocol-structs | 26 +++ .../nodedevmdevctldata/mdevctl-autostart.argv | 8 + tests/nodedevmdevctltest.c | 54 ++++++ tools/virsh-nodedev.c | 139 +++++++++++++++ 17 files changed, 727 insertions(+), 6 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-autostart.argv -- 2.31.1
This will allow persistent mediated devices to be configured to be restarted automatically when the host reboots. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 6 +++ src/driver-nodedev.h | 10 ++++ src/libvirt-nodedev.c | 76 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 4 ++ src/remote/remote_driver.c | 4 +- src/remote/remote_protocol.x | 29 +++++++++++- src/remote_protocol-structs | 12 +++++ 7 files changed, 139 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 067d914d37..0e841ada8a 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -141,6 +141,12 @@ int virNodeDeviceUndefine(virNodeDevicePtr dev, int virNodeDeviceCreate(virNodeDevicePtr dev, unsigned int flags); +int virNodeDeviceSetAutostart(virNodeDevicePtr dev, + int autostart); + +int virNodeDeviceGetAutostart(virNodeDevicePtr dev, + int *autostart); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index dd56421a54..125f4cfd9e 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -87,6 +87,14 @@ typedef int (*virDrvNodeDeviceCreate)(virNodeDevicePtr dev, unsigned int flags); +typedef int +(*virDrvNodeDeviceSetAutostart)(virNodeDevicePtr dev, + int autostart); + +typedef int +(*virDrvNodeDeviceGetAutostart)(virNodeDevicePtr dev, + int *autostart); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -128,4 +136,6 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceDefineXML nodeDeviceDefineXML; virDrvNodeDeviceUndefine nodeDeviceUndefine; virDrvNodeDeviceCreate nodeDeviceCreate; + virDrvNodeDeviceSetAutostart nodeDeviceSetAutostart; + virDrvNodeDeviceGetAutostart nodeDeviceGetAutostart; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index e416c12534..68fc83203d 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -979,3 +979,79 @@ virConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, virDispatchError(conn); return -1; } + +/** + * virNodeDeviceSetAutostart: + * @dev: the device object + * @autostart: whether the device should be automatically started + * + * Configure the node device to be automatically started when the host machine + * boots or the parent device becomes available. + * + * Returns -1 in case of error, 0 in case of success + */ +int +virNodeDeviceSetAutostart(virNodeDevicePtr dev, + int autostart) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceSetAutostart) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceSetAutostart(dev, autostart); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + +/** + * virNodeDeviceGetAutostart: + * @dev: the device object + * @autostart: the value returned + * + * Provides a boolean value indicating whether the node device is configured to + * be automatically started when the host machine boots or the parent device + * becomes available. + * + * Returns -1 in case of error, 0 in case of success + */ +int +virNodeDeviceGetAutostart(virNodeDevicePtr dev, + int *autostart) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceGetAutostart) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceGetAutostart(dev, autostart); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 5678a13cda..a21bd6ac7d 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -896,4 +896,8 @@ LIBVIRT_7.3.0 { virNodeDeviceCreate; } LIBVIRT_7.2.0; +LIBVIRT_7.5.0 { + virNodeDeviceSetAutostart; + virNodeDeviceGetAutostart; +} LIBVIRT_7.3.0; # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 48423f3619..9070f648e2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8717,7 +8717,9 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceCreate = remoteNodeDeviceCreate, /* 7.3.0 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.3.0 */ .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.3.0 */ - .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ + .nodeDeviceDestroy = remoteNodeDeviceDestroy, /* 0.6.3 */ + .nodeDeviceGetAutostart = remoteNodeDeviceGetAutostart, /* 7.5.0 */ + .nodeDeviceSetAutostart = remoteNodeDeviceSetAutostart, /* 7.5.0 */ }; static virNWFilterDriver nwfilter_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index de69704b68..a98ab49f2e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2164,6 +2164,19 @@ struct remote_node_device_create_args { unsigned int flags; }; +struct remote_node_device_get_autostart_args { + remote_nonnull_string name; +}; + +struct remote_node_device_get_autostart_ret { + int autostart; +}; + +struct remote_node_device_set_autostart_args { + remote_nonnull_string name; + int autostart; +}; + /* * Events Register/Deregister: @@ -6784,6 +6797,20 @@ enum remote_procedure { * @priority: high * @acl: node_device:start */ - REMOTE_PROC_NODE_DEVICE_CREATE = 430 + REMOTE_PROC_NODE_DEVICE_CREATE = 430, + + /** + * @generate: both + * @priority: high + * @acl: node_device:read + */ + REMOTE_PROC_NODE_DEVICE_GET_AUTOSTART = 431, + + /** + * @generate: both + * @priority: high + * @acl: node_device:write + */ + REMOTE_PROC_NODE_DEVICE_SET_AUTOSTART = 432 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 6b46328adc..b17372537b 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1615,6 +1615,16 @@ struct remote_node_device_create_args { remote_nonnull_string name; u_int flags; }; +struct remote_node_device_get_autostart_args { + remote_nonnull_string name; +}; +struct remote_node_device_get_autostart_ret { + int autostart; +}; +struct remote_node_device_set_autostart_args { + remote_nonnull_string name; + int autostart; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3623,4 +3633,6 @@ enum remote_procedure { REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, REMOTE_PROC_NODE_DEVICE_CREATE = 430, + REMOTE_PROC_NODE_DEVICE_GET_AUTOSTART = 431, + REMOTE_PROC_NODE_DEVICE_SET_AUTOSTART = 432, }; -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:50PM -0500, Jonathon Jongsma wrote:
This will allow persistent mediated devices to be configured to be restarted automatically when the host reboots.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 6 +++ src/driver-nodedev.h | 10 ++++ src/libvirt-nodedev.c | 76 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 4 ++ src/remote/remote_driver.c | 4 +- src/remote/remote_protocol.x | 29 +++++++++++- src/remote_protocol-structs | 12 +++++ 7 files changed, 139 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Implement autostart functionality for mediated devices. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++ src/conf/virnodedeviceobj.h | 6 ++ src/libvirt_private.syms | 2 + src/node_device/node_device_driver.c | 116 +++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 13 +++ src/node_device/node_device_udev.c | 19 ++++- 6 files changed, 171 insertions(+), 1 deletion(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b213592b56..deee796440 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -41,6 +41,7 @@ struct _virNodeDeviceObj { used by testdriver */ bool active; bool persistent; + bool autostart; }; struct _virNodeDeviceObjList { @@ -1026,6 +1027,21 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, } +bool +virNodeDeviceObjIsAutostart(virNodeDeviceObj *obj) +{ + return obj->autostart; +} + + +void +virNodeDeviceObjSetAutostart(virNodeDeviceObj *obj, + bool autostart) +{ + obj->autostart = autostart; +} + + struct virNodeDeviceObjListRemoveHelperData { virNodeDeviceObjListRemoveIterator callback; diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 7353e4619b..78188be2a2 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -135,6 +135,12 @@ virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); void virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, bool persistent); +bool +virNodeDeviceObjIsAutostart(virNodeDeviceObj *obj); + +void +virNodeDeviceObjSetAutostart(virNodeDeviceObj *obj, + bool autostart); typedef bool (*virNodeDeviceObjListRemoveIterator)(virNodeDeviceObj *obj, const void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ced2a7990..dd46fe8750 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1278,6 +1278,7 @@ virNetworkPortDefSaveStatus; virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjIsActive; +virNodeDeviceObjIsAutostart; virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; @@ -1294,6 +1295,7 @@ virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; virNodeDeviceObjListRemoveLocked; virNodeDeviceObjSetActive; +virNodeDeviceObjSetAutostart; virNodeDeviceObjSetPersistent; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8a0a2c3847..9ebe609aa4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -962,6 +962,46 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg) } +/* gets a virCommand object that executes a mdevctl command to set the + * 'autostart' property of the device to the specified value + */ +virCommand* +nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, + bool autostart, + char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "modify", + "--uuid", + def->caps->data.mdev.uuid, + NULL); + + if (autostart) + virCommandAddArg(cmd, "--auto"); + else + virCommandAddArg(cmd, "--manual"); + + virCommandSetErrorBuffer(cmd, errmsg); + + return cmd; +} + + +static int +virMdevctlSetAutostart(virNodeDeviceDef *def, bool autostart, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlSetAutostartCommand(def, autostart, errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output, @@ -1688,3 +1728,79 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, return ret; } + + +int +nodeDeviceSetAutostart(virNodeDevice *device, + int autostart) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceSetAutostartEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot set autostart for transient device")); + goto cleanup; + } + + if (autostart != virNodeDeviceObjIsAutostart(obj)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlSetAutostart(def, autostart, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set autostart on '%s': %s"), + def->name, + errmsg && errmsg[0] != '\0' ? errmsg : _("Unknown Error")); + goto cleanup; + } + virNodeDeviceObjSetAutostart(obj, autostart); + } + ret = 0; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceGetAutostart(virNodeDevice *device, + int *autostart) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceGetAutostartEnsureACL(device->conn, def) < 0) + goto cleanup; + + *autostart = virNodeDeviceObjIsAutostart(obj); + ret = 0; + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index edd763f0e4..d178a18180 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -171,3 +171,16 @@ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, int nodeDeviceCreate(virNodeDevice *dev, unsigned int flags); + +int +nodeDeviceSetAutostart(virNodeDevice *dev, + int autostart); + +int +nodeDeviceGetAutostart(virNodeDevice *dev, + int *autostart); + +virCommand* +nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, + bool autostart, + char **errmsg); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f99578414d..21273083a6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1488,6 +1488,8 @@ udevAddOneDevice(struct udev_device *device) bool new_device = true; int ret = -1; bool was_persistent = false; + bool autostart = true; + bool is_mdev; def = g_new0(virNodeDeviceDef, 1); @@ -1509,17 +1511,28 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup; + is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV; + if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj); - if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV) + if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); was_persistent = virNodeDeviceObjIsPersistent(obj); + autostart = virNodeDeviceObjIsAutostart(obj); + /* If the device was defined by mdevctl and was never instantiated, it * won't have a sysfs path. We need to emit a CREATED event... */ new_device = (objdef->sysfs_path == NULL); virNodeDeviceObjEndAPI(&obj); + } else { + /* All non-mdev devices report themselves as autostart since they + * should still be present and active after a reboot unless the device + * is removed from the host. Mediated devices can only be persistent if + * they are in already in the device list from parsing the mdevctl + * output. */ + autostart = !is_mdev; } /* If this is a device change, the old definition will be freed @@ -1527,6 +1540,7 @@ udevAddOneDevice(struct udev_device *device) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; virNodeDeviceObjSetPersistent(obj, was_persistent); + virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj); if (new_device) @@ -1930,6 +1944,7 @@ udevSetupSystemDev(void) goto cleanup; virNodeDeviceObjSetActive(obj, true); + virNodeDeviceObjSetAutostart(obj, true); virNodeDeviceObjEndAPI(&obj); @@ -2331,6 +2346,8 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.3.0 */ .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.3.0 */ .nodeDeviceCreate = nodeDeviceCreate, /* 7.3.0 */ + .nodeDeviceSetAutostart = nodeDeviceSetAutostart, /* 7.5.0 */ + .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.5.0 */ }; -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:51PM -0500, Jonathon Jongsma wrote:
Implement autostart functionality for mediated devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++ src/conf/virnodedeviceobj.h | 6 ++ src/libvirt_private.syms | 2 + src/node_device/node_device_driver.c | 116 +++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 13 +++ src/node_device/node_device_udev.c | 19 ++++- 6 files changed, 171 insertions(+), 1 deletion(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b213592b56..deee796440 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -41,6 +41,7 @@ struct _virNodeDeviceObj { used by testdriver */ bool active; bool persistent; + bool autostart; };
struct _virNodeDeviceObjList { @@ -1026,6 +1027,21 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, }
+bool +virNodeDeviceObjIsAutostart(virNodeDeviceObj *obj) +{ + return obj->autostart; +} + + +void +virNodeDeviceObjSetAutostart(virNodeDeviceObj *obj, + bool autostart) +{ + obj->autostart = autostart; +} + + struct virNodeDeviceObjListRemoveHelperData { virNodeDeviceObjListRemoveIterator callback; diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 7353e4619b..78188be2a2 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -135,6 +135,12 @@ virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); void virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, bool persistent); +bool +virNodeDeviceObjIsAutostart(virNodeDeviceObj *obj); + +void +virNodeDeviceObjSetAutostart(virNodeDeviceObj *obj, + bool autostart);
typedef bool (*virNodeDeviceObjListRemoveIterator)(virNodeDeviceObj *obj, const void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ced2a7990..dd46fe8750 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1278,6 +1278,7 @@ virNetworkPortDefSaveStatus; virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjIsActive; +virNodeDeviceObjIsAutostart; virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; @@ -1294,6 +1295,7 @@ virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; virNodeDeviceObjListRemoveLocked; virNodeDeviceObjSetActive; +virNodeDeviceObjSetAutostart; virNodeDeviceObjSetPersistent;
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8a0a2c3847..9ebe609aa4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -962,6 +962,46 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg) }
+/* gets a virCommand object that executes a mdevctl command to set the + * 'autostart' property of the device to the specified value + */ +virCommand* +nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, + bool autostart, + char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "modify", + "--uuid", + def->caps->data.mdev.uuid, + NULL); + + if (autostart) + virCommandAddArg(cmd, "--auto"); + else + virCommandAddArg(cmd, "--manual"); + + virCommandSetErrorBuffer(cmd, errmsg); + + return cmd; +} + + +static int +virMdevctlSetAutostart(virNodeDeviceDef *def, bool autostart, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlSetAutostartCommand(def, autostart, errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output, @@ -1688,3 +1728,79 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
return ret; } + + +int +nodeDeviceSetAutostart(virNodeDevice *device, + int autostart) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceSetAutostartEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot set autostart for transient device")); + goto cleanup; + } + + if (autostart != virNodeDeviceObjIsAutostart(obj)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlSetAutostart(def, autostart, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set autostart on '%s': %s"), + def->name, + errmsg && errmsg[0] != '\0' ? errmsg : _("Unknown Error")); + goto cleanup; + } + virNodeDeviceObjSetAutostart(obj, autostart); + } + ret = 0; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceGetAutostart(virNodeDevice *device, + int *autostart) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceGetAutostartEnsureACL(device->conn, def) < 0) + goto cleanup; + + *autostart = virNodeDeviceObjIsAutostart(obj); + ret = 0; + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index edd763f0e4..d178a18180 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -171,3 +171,16 @@ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, int nodeDeviceCreate(virNodeDevice *dev, unsigned int flags); + +int +nodeDeviceSetAutostart(virNodeDevice *dev, + int autostart); + +int +nodeDeviceGetAutostart(virNodeDevice *dev, + int *autostart); + +virCommand* +nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, + bool autostart, + char **errmsg); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f99578414d..21273083a6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1488,6 +1488,8 @@ udevAddOneDevice(struct udev_device *device) bool new_device = true; int ret = -1; bool was_persistent = false; + bool autostart = true; + bool is_mdev;
def = g_new0(virNodeDeviceDef, 1);
@@ -1509,17 +1511,28 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup;
+ is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV; + if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj);
- if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV) + if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); was_persistent = virNodeDeviceObjIsPersistent(obj); + autostart = virNodeDeviceObjIsAutostart(obj); + /* If the device was defined by mdevctl and was never instantiated, it * won't have a sysfs path. We need to emit a CREATED event... */ new_device = (objdef->sysfs_path == NULL);
virNodeDeviceObjEndAPI(&obj); + } else { + /* All non-mdev devices report themselves as autostart since they + * should still be present and active after a reboot unless the device + * is removed from the host. Mediated devices can only be persistent if + * they are in already in the device list from parsing the mdevctl + * output. */ + autostart = !is_mdev;
Don't we need to be querying the mdevctl configuration to see if it is autostart or manual ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jun 4, 2021 at 3:07 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Thu, Jun 03, 2021 at 03:11:51PM -0500, Jonathon Jongsma wrote:
Implement autostart functionality for mediated devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++ src/conf/virnodedeviceobj.h | 6 ++ src/libvirt_private.syms | 2 + src/node_device/node_device_driver.c | 116 +++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 13 +++ src/node_device/node_device_udev.c | 19 ++++- 6 files changed, 171 insertions(+), 1 deletion(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index b213592b56..deee796440 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -41,6 +41,7 @@ struct _virNodeDeviceObj { used by testdriver */ bool active; bool persistent; + bool autostart; };
struct _virNodeDeviceObjList { @@ -1026,6 +1027,21 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, }
+bool +virNodeDeviceObjIsAutostart(virNodeDeviceObj *obj) +{ + return obj->autostart; +} + + +void +virNodeDeviceObjSetAutostart(virNodeDeviceObj *obj, + bool autostart) +{ + obj->autostart = autostart; +} + + struct virNodeDeviceObjListRemoveHelperData { virNodeDeviceObjListRemoveIterator callback; diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 7353e4619b..78188be2a2 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -135,6 +135,12 @@ virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); void virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, bool persistent); +bool +virNodeDeviceObjIsAutostart(virNodeDeviceObj *obj); + +void +virNodeDeviceObjSetAutostart(virNodeDeviceObj *obj, + bool autostart);
typedef bool (*virNodeDeviceObjListRemoveIterator)(virNodeDeviceObj *obj, const void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ced2a7990..dd46fe8750 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1278,6 +1278,7 @@ virNetworkPortDefSaveStatus; virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjIsActive; +virNodeDeviceObjIsAutostart; virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; @@ -1294,6 +1295,7 @@ virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; virNodeDeviceObjListRemoveLocked; virNodeDeviceObjSetActive; +virNodeDeviceObjSetAutostart; virNodeDeviceObjSetPersistent;
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 8a0a2c3847..9ebe609aa4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -962,6 +962,46 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg) }
+/* gets a virCommand object that executes a mdevctl command to set the + * 'autostart' property of the device to the specified value + */ +virCommand* +nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, + bool autostart, + char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "modify", + "--uuid", + def->caps->data.mdev.uuid, + NULL); + + if (autostart) + virCommandAddArg(cmd, "--auto"); + else + virCommandAddArg(cmd, "--manual"); + + virCommandSetErrorBuffer(cmd, errmsg); + + return cmd; +} + + +static int +virMdevctlSetAutostart(virNodeDeviceDef *def, bool autostart, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlSetAutostartCommand(def, autostart, errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output, @@ -1688,3 +1728,79 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
return ret; } + + +int +nodeDeviceSetAutostart(virNodeDevice *device, + int autostart) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceSetAutostartEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot set autostart for transient device")); + goto cleanup; + } + + if (autostart != virNodeDeviceObjIsAutostart(obj)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlSetAutostart(def, autostart, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set autostart on '%s': %s"), + def->name, + errmsg && errmsg[0] != '\0' ? errmsg : _("Unknown Error")); + goto cleanup; + } + virNodeDeviceObjSetAutostart(obj, autostart); + } + ret = 0; + } else { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceGetAutostart(virNodeDevice *device, + int *autostart) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceGetAutostartEnsureACL(device->conn, def) < 0) + goto cleanup; + + *autostart = virNodeDeviceObjIsAutostart(obj); + ret = 0; + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index edd763f0e4..d178a18180 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -171,3 +171,16 @@ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, int nodeDeviceCreate(virNodeDevice *dev, unsigned int flags); + +int +nodeDeviceSetAutostart(virNodeDevice *dev, + int autostart); + +int +nodeDeviceGetAutostart(virNodeDevice *dev, + int *autostart); + +virCommand* +nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, + bool autostart, + char **errmsg); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f99578414d..21273083a6 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1488,6 +1488,8 @@ udevAddOneDevice(struct udev_device *device) bool new_device = true; int ret = -1; bool was_persistent = false; + bool autostart = true; + bool is_mdev;
def = g_new0(virNodeDeviceDef, 1);
@@ -1509,17 +1511,28 @@ udevAddOneDevice(struct udev_device *device) if (udevSetParent(device, def) != 0) goto cleanup;
+ is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV; + if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj);
- if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV) + if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); was_persistent = virNodeDeviceObjIsPersistent(obj); + autostart = virNodeDeviceObjIsAutostart(obj); + /* If the device was defined by mdevctl and was never instantiated, it * won't have a sysfs path. We need to emit a CREATED event... */ new_device = (objdef->sysfs_path == NULL);
virNodeDeviceObjEndAPI(&obj); + } else { + /* All non-mdev devices report themselves as autostart since they + * should still be present and active after a reboot unless the device + * is removed from the host. Mediated devices can only be persistent if + * they are in already in the device list from parsing the mdevctl + * output. */ + autostart = !is_mdev;
Don't we need to be querying the mdevctl configuration to see if it is autostart or manual ?
Yes, clearly. I accidentally left it out of this patch. New version coming shortly. Jonathon
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- .../nodedevmdevctldata/mdevctl-autostart.argv | 8 +++ tests/nodedevmdevctltest.c | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-autostart.argv diff --git a/tests/nodedevmdevctldata/mdevctl-autostart.argv b/tests/nodedevmdevctldata/mdevctl-autostart.argv new file mode 100644 index 0000000000..9b441e9466 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-autostart.argv @@ -0,0 +1,8 @@ +mdevctl \ +modify \ +--uuid d069d019-36ea-4111-8f0a-8c9a70e21366 \ +--auto +mdevctl \ +modify \ +--uuid d069d019-36ea-4111-8f0a-8c9a70e21366 \ +--manual diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 8ba1d2da70..3f9d4c84b9 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -118,6 +118,55 @@ testMdevctlHelper(const void *data) } +static int +testMdevctlAutostart(const void *data G_GNUC_UNUSED) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) enablecmd = NULL; + g_autoptr(virCommand) disablecmd = NULL; + g_autofree char *errmsg = NULL; + /* just concatenate both calls into the same output file */ + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-autostart.argv", + abs_srcdir); + g_autofree char *mdevxml = + g_strdup_printf("%s/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml", + abs_srcdir); + g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew(); + + if (!(def = virNodeDeviceDefParseFile(mdevxml, CREATE_DEVICE, VIRT_TYPE))) + return -1; + + virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL); + + if (!(enablecmd = nodeDeviceGetMdevctlSetAutostartCommand(def, true, &errmsg))) + goto cleanup; + + if (virCommandRun(enablecmd, NULL) < 0) + goto cleanup; + + if (!(disablecmd = nodeDeviceGetMdevctlSetAutostartCommand(def, false, &errmsg))) + goto cleanup; + + if (virCommandRun(disablecmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (virTestCompareToFileFull(actualCmdline, cmdlinefile, false) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + static int testMdevctlListDefined(const void *data G_GNUC_UNUSED) { @@ -348,6 +397,9 @@ mymain(void) #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("list defined mdevs", testMdevctlListDefined, NULL) +#define DO_TEST_AUTOSTART() \ + DO_TEST_FULL("autostart mdevs", testMdevctlAutostart, NULL) + #define DO_TEST_PARSE_JSON(filename) \ DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) @@ -370,6 +422,8 @@ mymain(void) DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST_AUTOSTART(); + done: nodedevTestDriverFree(driver); -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:52PM -0500, Jonathon Jongsma wrote:
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- .../nodedevmdevctldata/mdevctl-autostart.argv | 8 +++ tests/nodedevmdevctltest.c | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-autostart.argv
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Add ability to set node devices to autostart on boot or parent device availability. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/manpages/virsh.rst | 15 +++++++++ tools/virsh-nodedev.c | 71 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 87668f2b9a..e9023ba17b 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5135,6 +5135,21 @@ When *--timestamp* is used, a human-readable timestamp will be printed before the event. +nodedev-autostart +----------------- + +**Syntax:** + +:: + + nodedev-autostart [--disable] device + +Configure a device to be automatically started when the host machine boots or +the parent device becomes available. With *--disable*, the device will be set +to manual mode and will no longer be automatically started by the host. This +command is only supported for persistently-defined mediated devices. + + VIRTUAL NETWORK COMMANDS ======================== diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 8c2086b71f..845effcb81 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1162,6 +1162,71 @@ cmdNodeDeviceStart(vshControl *ctl, const vshCmd *cmd) } +/* + * "nodedev-autostart" command + */ +static const vshCmdInfo info_node_device_autostart[] = { + {.name = "help", + .data = N_("autostart a defined node device") + }, + {.name = "desc", + .data = N_("Configure a node device to be automatically started at boot.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_autostart[] = { + {.name = "device", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device name or wwn pair in 'wwnn,wwpn' format"), + .completer = virshNodeDeviceNameCompleter, + }, + {.name = "disable", + .type = VSH_OT_BOOL, + .help = N_("disable autostarting") + }, + {.name = NULL} +}; + +static bool +cmdNodeDeviceAutostart(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevice *dev = NULL; + bool ret = false; + const char *name = NULL; + int autostart; + + if (vshCommandOptStringReq(ctl, cmd, "device", &name) < 0) + return false; + + dev = vshFindNodeDevice(ctl, name); + + if (!dev) goto cleanup; + + autostart = !vshCommandOptBool(cmd, "disable"); + + if (virNodeDeviceSetAutostart(dev, autostart) < 0) { + if (autostart) + vshError(ctl, _("failed to mark device %s as autostarted"), name); + else + vshError(ctl, _("failed to unmark device %s as autostarted"), name); + goto cleanup; + } + + if (autostart) + vshPrintExtra(ctl, _("Device %s marked as autostarted\n"), name); + else + vshPrintExtra(ctl, _("Device %s unmarked as autostarted\n"), name); + + ret = true; + cleanup: + if (dev) + virNodeDeviceFree(dev); + return ret; +} + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1233,5 +1298,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_start, .flags = 0 }, + {.name = "nodedev-autostart", + .handler = cmdNodeDeviceAutostart, + .opts = opts_node_device_autostart, + .info = info_node_device_autostart, + .flags = 0 + }, {.name = NULL} }; -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:53PM -0500, Jonathon Jongsma wrote:
Add ability to set node devices to autostart on boot or parent device availability.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/manpages/virsh.rst | 15 +++++++++ tools/virsh-nodedev.c | 71 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
These two public APIs are implemented for almost all other objects that have a concept of persistent definition and activatability. Now that we have node devices (mdevs) that can be defined and inactive, it will be useful to query the persistent/active state of node devices as well. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 8 ++++ src/libvirt-nodedev.c | 65 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 32 ++++++++++++++- src/remote_protocol-structs | 14 +++++++ 7 files changed, 126 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 0e841ada8a..e492634217 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -147,6 +147,10 @@ int virNodeDeviceSetAutostart(virNodeDevicePtr dev, int virNodeDeviceGetAutostart(virNodeDevicePtr dev, int *autostart); +int virNodeDeviceIsPersistent(virNodeDevicePtr dev); + +int virNodeDeviceIsActive(virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index 125f4cfd9e..167a8166dd 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -95,6 +95,12 @@ typedef int (*virDrvNodeDeviceGetAutostart)(virNodeDevicePtr dev, int *autostart); +typedef int +(*virDrvNodeDeviceIsPersistent)(virNodeDevicePtr dev); + +typedef int +(*virDrvNodeDeviceIsActive)(virNodeDevicePtr dev); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -138,4 +144,6 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceCreate nodeDeviceCreate; virDrvNodeDeviceSetAutostart nodeDeviceSetAutostart; virDrvNodeDeviceGetAutostart nodeDeviceGetAutostart; + virDrvNodeDeviceIsPersistent nodeDeviceIsPersistent; + virDrvNodeDeviceIsActive nodeDeviceIsActive; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 68fc83203d..8ad1e9cb9e 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -1055,3 +1055,68 @@ virNodeDeviceGetAutostart(virNodeDevicePtr dev, virDispatchError(dev->conn); return -1; } + +/** + * virNodeDeviceIsPersistent: + * @dev: pointer to the nodedev object + * + * Determine if the node device has a persistent configuration + * which means it will still exist after shutting down + * + * Returns 1 if persistent, 0 if transient, -1 on error + */ +int +virNodeDeviceIsPersistent(virNodeDevicePtr dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceIsPersistent) { + int ret; + ret = dev->conn->nodeDeviceDriver->nodeDeviceIsPersistent(dev); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(dev->conn); + return -1; +} + + +/** + * virNodeDeviceIsActive: + * @dev: pointer to the node device object + * + * Determine if the node device is currently active + * + * Returns 1 if active, 0 if inactive, -1 on error + */ +int virNodeDeviceIsActive(virNodeDevicePtr dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceIsActive) { + int ret; + ret = dev->conn->nodeDeviceDriver->nodeDeviceIsActive(dev); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(dev->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index a21bd6ac7d..c2b4b46a92 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -899,5 +899,7 @@ LIBVIRT_7.3.0 { LIBVIRT_7.5.0 { virNodeDeviceSetAutostart; virNodeDeviceGetAutostart; + virNodeDeviceIsPersistent; + virNodeDeviceIsActive; } LIBVIRT_7.3.0; # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9070f648e2..edc01437e3 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8720,6 +8720,8 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceDestroy = remoteNodeDeviceDestroy, /* 0.6.3 */ .nodeDeviceGetAutostart = remoteNodeDeviceGetAutostart, /* 7.5.0 */ .nodeDeviceSetAutostart = remoteNodeDeviceSetAutostart, /* 7.5.0 */ + .nodeDeviceIsPersistent = remoteNodeDeviceIsPersistent, /* 7.5.0 */ + .nodeDeviceIsActive = remoteNodeDeviceIsActive, /* 7.5.0 */ }; static virNWFilterDriver nwfilter_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a98ab49f2e..49218cb1df 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2177,6 +2177,22 @@ struct remote_node_device_set_autostart_args { int autostart; }; +struct remote_node_device_is_persistent_args { + remote_nonnull_string name; +}; + +struct remote_node_device_is_persistent_ret { + int persistent; +}; + +struct remote_node_device_is_active_args { + remote_nonnull_string name; +}; + +struct remote_node_device_is_active_ret { + int active; +}; + /* * Events Register/Deregister: @@ -6811,6 +6827,20 @@ enum remote_procedure { * @priority: high * @acl: node_device:write */ - REMOTE_PROC_NODE_DEVICE_SET_AUTOSTART = 432 + REMOTE_PROC_NODE_DEVICE_SET_AUTOSTART = 432, + + /** + * @generate: both + * @priority: high + * @acl: node_device:read + */ + REMOTE_PROC_NODE_DEVICE_IS_PERSISTENT = 433, + + /** + * @generate: both + * @priority: high + * @acl: node_device:read + */ + REMOTE_PROC_NODE_DEVICE_IS_ACTIVE = 434 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index b17372537b..7717b04804 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1625,6 +1625,18 @@ struct remote_node_device_set_autostart_args { remote_nonnull_string name; int autostart; }; +struct remote_node_device_is_persistent_args { + remote_nonnull_string name; +}; +struct remote_node_device_is_persistent_ret { + int persistent; +}; +struct remote_node_device_is_active_args { + remote_nonnull_string name; +}; +struct remote_node_device_is_active_ret { + int active; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3635,4 +3647,6 @@ enum remote_procedure { REMOTE_PROC_NODE_DEVICE_CREATE = 430, REMOTE_PROC_NODE_DEVICE_GET_AUTOSTART = 431, REMOTE_PROC_NODE_DEVICE_SET_AUTOSTART = 432, + REMOTE_PROC_NODE_DEVICE_IS_PERSISTENT = 433, + REMOTE_PROC_NODE_DEVICE_IS_ACTIVE = 434, }; -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:54PM -0500, Jonathon Jongsma wrote:
These two public APIs are implemented for almost all other objects that have a concept of persistent definition and activatability. Now that we have node devices (mdevs) that can be defined and inactive, it will be useful to query the persistent/active state of node devices as well.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 8 ++++ src/libvirt-nodedev.c | 65 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + src/remote/remote_driver.c | 2 + src/remote/remote_protocol.x | 32 ++++++++++++++- src/remote_protocol-structs | 14 +++++++ 7 files changed, 126 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Implement these new API functions in the nodedev driver. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart); +int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev; @@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device) if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj); /* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device) virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */ + persistent = !is_mdev; autostart = !is_mdev; } @@ -1539,7 +1541,7 @@ udevAddOneDevice(struct udev_device *device) * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjSetPersistent(obj, was_persistent); + virNodeDeviceObjSetPersistent(obj, persistent); virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj); @@ -1945,6 +1947,7 @@ udevSetupSystemDev(void) virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetAutostart(obj, true); + virNodeDeviceObjSetPersistent(obj, true); virNodeDeviceObjEndAPI(&obj); @@ -2348,6 +2351,8 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreate = nodeDeviceCreate, /* 7.3.0 */ .nodeDeviceSetAutostart = nodeDeviceSetAutostart, /* 7.5.0 */ .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.5.0 */ + .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.5.0 */ + .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.5.0 */ }; -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:55PM -0500, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart);
+int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev;
@@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj);
/* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */
The assumption for all non-mdev devices ends up very misleading. For example: The parent device of an mdev needs to be bound to a vfio device driver. Without it the device ends up without the capability to create mdevs. If this driver binding is not persisted (e.g. with setting up driverctl) but instead the device is just manually being rebound to a vfio device driver than after reboot neither the mdev capability on the parent device nor the mdev device as a child device will be existing. A user calling nodedev-info before the reboot gets on the parent device Persistent: yes Autostart: yes and on the mdev device Persistent: yes Autostart: yes After a reboot he ends up with with nodedev-info on the parent device Persistent: yes Autostart: yes and the mdev device does not exist. I suggest to use three states like yes, no, unknown or not showing Persistent and Autostart on devices that libvirt does not manage/know about their persistence and autostart configuration.
+ persistent = !is_mdev; autostart = !is_mdev; }
@@ -1539,7 +1541,7 @@ udevAddOneDevice(struct udev_device *device) * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjSetPersistent(obj, was_persistent); + virNodeDeviceObjSetPersistent(obj, persistent); virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj);
@@ -1945,6 +1947,7 @@ udevSetupSystemDev(void)
virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetAutostart(obj, true); + virNodeDeviceObjSetPersistent(obj, true);
virNodeDeviceObjEndAPI(&obj);
@@ -2348,6 +2351,8 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreate = nodeDeviceCreate, /* 7.3.0 */ .nodeDeviceSetAutostart = nodeDeviceSetAutostart, /* 7.5.0 */ .nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.5.0 */ + .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.5.0 */ + .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.5.0 */ };
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart);
+int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev;
@@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj);
/* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */
The assumption for all non-mdev devices ends up very misleading. For example: The parent device of an mdev needs to be bound to a vfio device driver. Without it the device ends up without the capability to create mdevs. If this driver binding is not persisted (e.g. with setting up driverctl) but instead the device is just manually being rebound to a vfio device driver than after reboot neither the mdev capability on the parent device nor the mdev device as a child device will be existing. A user calling nodedev-info before the reboot gets on the parent device Persistent: yes Autostart: yes and on the mdev device Persistent: yes Autostart: yes After a reboot he ends up with with nodedev-info on the parent device Persistent: yes Autostart: yes and the mdev device does not exist.
Before I get to the rest of your suggestion, I'd like to know more about this. If the mdev device is persistent (i.e. "defined" in mdevctl terminology), then it should still exist after a reboot, even if it's not started. If it doesn't, then it's a bug. An mdev can be defined even if its parent device is not present. Does this device show up if you run 'mdevctl list --defined'? Does it show up if you run `virsh nodedev-list --cap mdev --all`?
I suggest to use three states like yes, no, unknown or not showing Persistent and Autostart on devices that libvirt does not manage/know about their persistence and autostart configuration.
Well, I suppose that we have the error value (-1) that could be used for this case, but it doesn't exactly seem like an error. Adding a separate tri-state return value would make this API inconsistent with all of the other IsActive()/IsPersistent() APIs in libvirt, so I don't think that's a very good idea. Jonathon
On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart);
+int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev;
@@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj);
/* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */
The assumption for all non-mdev devices ends up very misleading. For example: The parent device of an mdev needs to be bound to a vfio device driver. Without it the device ends up without the capability to create mdevs. If this driver binding is not persisted (e.g. with setting up driverctl) but instead the device is just manually being rebound to a vfio device driver than after reboot neither the mdev capability on the parent device nor the mdev device as a child device will be existing. A user calling nodedev-info before the reboot gets on the parent device Persistent: yes Autostart: yes and on the mdev device Persistent: yes Autostart: yes After a reboot he ends up with with nodedev-info on the parent device Persistent: yes Autostart: yes and the mdev device does not exist.
Before I get to the rest of your suggestion, I'd like to know more about this. If the mdev device is persistent (i.e. "defined" in mdevctl terminology), then it should still exist after a reboot, even if it's not started. If it doesn't, then it's a bug. An mdev can be defined even if its parent device is not present.
Does this device show up if you run 'mdevctl list --defined'?
Yes, the mdev definition exists. Here is the information before the reboot with a vfio-ccw device setup by manually binding it to the vfio-ccw device driver. Manually means that I did not use driverctl to persist the device driver binding. # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: yes Persistent: yes Autostart: yes # mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto (active) # virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 <device> <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name> <path>/sys/devices/css0/0.0.0033/e60cef97-3f6b-485e-ac46-0520f9f66ac2</path> <parent>css_0_0_0033</parent> <driver> <name>vfio_mdev</name> </driver> <capability type='mdev'> <type id='vfio_ccw-io'/> <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid> <iommuGroup number='1'/> </capability> </device> # virsh nodedev-info css_0_0_0033 Name: css_0_0_0033 Parent: computer Active: yes Persistent: yes Autostart: yes This is the state before rebooting. After the reboot: # virsh nodedev-list --tree ... +- css_0_0_0033 | | | +- ccw_0_0_c670 | | | +- block_dasdb_IBM_750000000KMV11_c600_70 | +- css_0_0_0034 | | | +- ccw_0_0_c671 | | | +- block_dasda_IBM_750000000KMV11_c600_71 ... # virsh nodedev-info css_0_0_0033 Name: css_0_0_0033 Parent: computer Active: yes Persistent: yes Autostart: yes # virsh nodedev-info ccw_0_0_c670 Name: ccw_0_0_c670 Parent: css_0_0_0033 Active: yes Persistent: yes Autostart: yes # virsh nodedev-dumpxml css_0_0_0033 <device> <name>css_0_0_0033</name> <path>/sys/devices/css0/0.0.0033</path> <parent>computer</parent> <driver> <name>io_subchannel</name> </driver> <capability type='css'> <cssid>0x0</cssid> <ssid>0x0</ssid> <devno>0x0033</devno> </capability> </device> # virsh nodedev-list --cap mdev --all mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: 0.0.0033 Active: no Persistent: yes Autostart: yes As you can see the default device driver is bound to the parent device now and therefore the mdev does not get created although being defined in mdevctl. Also note that Persistent ond Autostart are misleading as they show on the mdev definition as well as the ccw device being set both "yes". On s390 there is also cio_ignore which allows devices to be ignored. This can also cause that devices exist before the reboot afterwards due to being ignored no longer exist on the system. For these reasons I think that these two new attributes show something that libvirt should not make assumptions on unless knowing about it.
Does it show up if you run `virsh nodedev-list --cap mdev --all`?
I suggest to use three states like yes, no, unknown or not showing Persistent and Autostart on devices that libvirt does not manage/know about their persistence and autostart configuration.
Well, I suppose that we have the error value (-1) that could be used for this case, but it doesn't exactly seem like an error. Adding a separate tri-state return value would make this API inconsistent with all of the other IsActive()/IsPersistent() APIs in libvirt, so I don't think that's a very good idea.
Jonathon
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Jun 16, 2021 at 8:30 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart);
+int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev;
@@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj);
/* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */
The assumption for all non-mdev devices ends up very misleading. For example: The parent device of an mdev needs to be bound to a vfio device driver. Without it the device ends up without the capability to create mdevs. If this driver binding is not persisted (e.g. with setting up driverctl) but instead the device is just manually being rebound to a vfio device driver than after reboot neither the mdev capability on the parent device nor the mdev device as a child device will be existing. A user calling nodedev-info before the reboot gets on the parent device Persistent: yes Autostart: yes and on the mdev device Persistent: yes Autostart: yes After a reboot he ends up with with nodedev-info on the parent device Persistent: yes Autostart: yes and the mdev device does not exist.
Before I get to the rest of your suggestion, I'd like to know more about this. If the mdev device is persistent (i.e. "defined" in mdevctl terminology), then it should still exist after a reboot, even if it's not started. If it doesn't, then it's a bug. An mdev can be defined even if its parent device is not present.
Does this device show up if you run 'mdevctl list --defined'?
Yes, the mdev definition exists. Here is the information before the reboot with a vfio-ccw device setup by manually binding it to the vfio-ccw device driver. Manually means that I did not use driverctl to persist the device driver binding.
# virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: yes Persistent: yes Autostart: yes
# mdevctl list -d e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto (active)
# virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 <device> <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name>
<path>/sys/devices/css0/0.0.0033/e60cef97-3f6b-485e-ac46-0520f9f66ac2</path> <parent>css_0_0_0033</parent> <driver> <name>vfio_mdev</name> </driver> <capability type='mdev'> <type id='vfio_ccw-io'/> <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid> <iommuGroup number='1'/> </capability> </device>
# virsh nodedev-info css_0_0_0033 Name: css_0_0_0033 Parent: computer Active: yes Persistent: yes Autostart: yes
This is the state before rebooting. After the reboot: # virsh nodedev-list --tree ... +- css_0_0_0033 | | | +- ccw_0_0_c670 | | | +- block_dasdb_IBM_750000000KMV11_c600_70 | +- css_0_0_0034 | | | +- ccw_0_0_c671 | | | +- block_dasda_IBM_750000000KMV11_c600_71 ...
# virsh nodedev-info css_0_0_0033 Name: css_0_0_0033 Parent: computer Active: yes Persistent: yes Autostart: yes
# virsh nodedev-info ccw_0_0_c670 Name: ccw_0_0_c670 Parent: css_0_0_0033 Active: yes Persistent: yes Autostart: yes
# virsh nodedev-dumpxml css_0_0_0033 <device> <name>css_0_0_0033</name> <path>/sys/devices/css0/0.0.0033</path> <parent>computer</parent> <driver> <name>io_subchannel</name> </driver> <capability type='css'> <cssid>0x0</cssid> <ssid>0x0</ssid> <devno>0x0033</devno> </capability> </device>
# virsh nodedev-list --cap mdev --all mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
# virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: 0.0.0033 Active: no Persistent: yes Autostart: yes
As you can see the default device driver is bound to the parent device now and therefore the mdev does not get created although being defined in mdevctl. Also note that Persistent ond Autostart are misleading as they show on the mdev definition as well as the ccw device being set both "yes".
On s390 there is also cio_ignore which allows devices to be ignored. This can also cause that devices exist before the reboot afterwards due to being ignored no longer exist on the system.
For these reasons I think that these two new attributes show something that libvirt should not make assumptions on unless knowing about it.
This is an interesting case. The vast majority of these devices will be "persistent" for all practical purposes, because they will still be there on the next reboot, and they will still be using the same drivers. On the other hand, it is true that there is no record of the device that remains after the physical device is removed, so strictly speaking they're not persistently defined. After reflecting, I've kind of flip-flopped and decided that maybe the best choice would be to mark them as non-persistent (and also non-autostart?). Would there be any downside to such a designation? They don't exactly match the behavior of other transient objects (domains, etc) because they (mostly) will still be present after a reboot...I still don't really like the idea of introducing a tristate return type. Opinions from longtime libvirt developers particularly appreciated. Jonathon
On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart);
+int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev;
@@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj);
/* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */
The assumption for all non-mdev devices ends up very misleading. For example: The parent device of an mdev needs to be bound to a vfio device driver. Without it the device ends up without the capability to create mdevs. If this driver binding is not persisted (e.g. with setting up driverctl) but instead the device is just manually being rebound to a vfio device driver than after reboot neither the mdev capability on the parent device nor the mdev device as a child device will be existing. A user calling nodedev-info before the reboot gets on the parent device Persistent: yes Autostart: yes and on the mdev device Persistent: yes Autostart: yes After a reboot he ends up with with nodedev-info on the parent device Persistent: yes Autostart: yes and the mdev device does not exist.
Before I get to the rest of your suggestion, I'd like to know more about this. If the mdev device is persistent (i.e. "defined" in mdevctl terminology), then it should still exist after a reboot, even if it's not started. If it doesn't, then it's a bug. An mdev can be defined even if its parent device is not present.
Does this device show up if you run 'mdevctl list --defined'? Does it show up if you run `virsh nodedev-list --cap mdev --all`?
I suggest to use three states like yes, no, unknown or not showing Persistent and Autostart on devices that libvirt does not manage/know about their persistence and autostart configuration.
Well, I suppose that we have the error value (-1) that could be used for this case, but it doesn't exactly seem like an error. Adding a separate tri-state return value would make this API inconsistent with all of the other IsActive()/IsPersistent() APIs in libvirt, so I don't think that's a very good idea.
Jonathon
Just noticed something while playing around with this. The default "yes" seems to be not really a good choice even for mdevs. See below: # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: yes Persistent: yes Autostart: yes # virsh nodedev-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' # mdevctl list --defined e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: no Persistent: yes Autostart: yes # virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 <device> <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name> <parent>css_0_0_0033</parent> <capability type='mdev'> <type id='vfio_ccw-io'/> <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid> <iommuGroup number='1'/> </capability> </device> # virsh nodedev-start mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 started # virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2' # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: yes Persistent: no Autostart: yes # mdevctl list --defined # mdevctl list e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Jun 22, 2021 at 2:08 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
Implement these new API functions in the nodedev driver.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++ src/node_device/node_device_udev.c | 21 +++++++----- 3 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 9ebe609aa4..75391f18b8 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device, virNodeDeviceObjEndAPI(&obj); return ret; } + + +int +nodeDeviceIsPersistent(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsPersistent(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + +int +nodeDeviceIsActive(virNodeDevice *device) +{ + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + int ret = -1; + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0) + goto cleanup; + + ret = virNodeDeviceObjIsActive(obj); + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index d178a18180..744dd42632 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -180,6 +180,12 @@ int nodeDeviceGetAutostart(virNodeDevice *dev, int *autostart);
+int +nodeDeviceIsPersistent(virNodeDevice *dev); + +int +nodeDeviceIsActive(virNodeDevice *dev); + virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 21273083a6..eb15ccce7f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEvent *event = NULL; bool new_device = true; int ret = -1; - bool was_persistent = false; + bool persistent = true; bool autostart = true; bool is_mdev;
@@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
if (is_mdev) nodeDeviceDefCopyFromMdevctl(def, objdef); - was_persistent = virNodeDeviceObjIsPersistent(obj); + + persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj);
/* If the device was defined by mdevctl and was never instantiated, it @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
virNodeDeviceObjEndAPI(&obj); } else { - /* All non-mdev devices report themselves as autostart since they - * should still be present and active after a reboot unless the device - * is removed from the host. Mediated devices can only be persistent if - * they are in already in the device list from parsing the mdevctl - * output. */ + /* All non-mdev devices report themselves as persistent and autostart + * since they should still be present and active after a reboot unless + * the device is removed from the host. Mediated devices can only be + * persistent if they are in already in the device list from parsing + * the mdevctl output. */
The assumption for all non-mdev devices ends up very misleading. For example: The parent device of an mdev needs to be bound to a vfio device driver. Without it the device ends up without the capability to create mdevs. If this driver binding is not persisted (e.g. with setting up driverctl) but instead the device is just manually being rebound to a vfio device driver than after reboot neither the mdev capability on the parent device nor the mdev device as a child device will be existing. A user calling nodedev-info before the reboot gets on the parent device Persistent: yes Autostart: yes and on the mdev device Persistent: yes Autostart: yes After a reboot he ends up with with nodedev-info on the parent device Persistent: yes Autostart: yes and the mdev device does not exist.
Before I get to the rest of your suggestion, I'd like to know more about this. If the mdev device is persistent (i.e. "defined" in mdevctl terminology), then it should still exist after a reboot, even if it's not started. If it doesn't, then it's a bug. An mdev can be defined even if its parent device is not present.
Does this device show up if you run 'mdevctl list --defined'? Does it show up if you run `virsh nodedev-list --cap mdev --all`?
I suggest to use three states like yes, no, unknown or not showing Persistent and Autostart on devices that libvirt does not manage/know about their persistence and autostart configuration.
Well, I suppose that we have the error value (-1) that could be used for this case, but it doesn't exactly seem like an error. Adding a separate tri-state return value would make this API inconsistent with all of the other IsActive()/IsPersistent() APIs in libvirt, so I don't think that's a very good idea.
Jonathon
Just noticed something while playing around with this. The default "yes" seems to be not really a good choice even for mdevs. See below:
# virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: yes Persistent: yes Autostart: yes
# virsh nodedev-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# mdevctl list --defined e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto
# virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: no Persistent: yes Autostart: yes
# virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 <device> <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name> <parent>css_0_0_0033</parent> <capability type='mdev'> <type id='vfio_ccw-io'/> <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid> <iommuGroup number='1'/> </capability> </device>
# virsh nodedev-start mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 started
# virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
# virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Name: mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 Parent: css_0_0_0033 Active: yes Persistent: no Autostart: yes
So it appears that there is a bug where an mdev is still marked as autostart even after it's undefined. Was there anything else you were trying to demonstrate? Jonathon
On 6/22/21 4:33 PM, Jonathon Jongsma wrote:
So it appears that there is a bug where an mdev is still marked as autostart even after it's undefined. Was there anything else you were trying to demonstrate?
Jonathon
Don't you need to resync with mdevctl on nodedev-info? If you would resync and fail to find the definition for the mdev your bugfix would be to set the autostart to "no". The shortcut would be to set autostart to "no" when undefine is called but that would leave out direct usage of mdectl. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Jun 22, 2021 at 10:03 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/22/21 4:33 PM, Jonathon Jongsma wrote:
So it appears that there is a bug where an mdev is still marked as autostart even after it's undefined. Was there anything else you were trying to demonstrate?
Jonathon
Don't you need to resync with mdevctl on nodedev-info? If you would resync and fail to find the definition for the mdev your bugfix would be to set the autostart to "no". The shortcut would be to set autostart to "no" when undefine is called but that would leave out direct usage of mdectl.
Yes, it appears that there's a bug in my patch that needs to be fixed. But your email said
"The default "yes" seems to be not really a good choice even for mdevs.
That makes it sound like you think there's something more fundamentally wrong than just a simple bug where a value didn't get updated properly. Jonathon
On 6/22/21 5:08 PM, Jonathon Jongsma wrote:
On Tue, Jun 22, 2021 at 10:03 AM Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 6/22/21 4:33 PM, Jonathon Jongsma wrote:
So it appears that there is a bug where an mdev is still marked as autostart even after it's undefined. Was there anything else you were trying to demonstrate?
Jonathon
Don't you need to resync with mdevctl on nodedev-info? If you would resync and fail to find the definition for the mdev your bugfix would be to set the autostart to "no". The shortcut would be to set autostart to "no" when undefine is called but that would leave out direct usage of mdectl.
Yes, it appears that there's a bug in my patch that needs to be fixed.
But your email said
"The default "yes" seems to be not really a good choice even for mdevs.
That makes it sound like you think there's something more fundamentally wrong than just a simple bug where a value didn't get updated properly.
Jonathon
Is the current code resyncing with mdevctl on nodedev-info calls persisted and autostart? Is using mdevctl directly something libvirt needs to worry about with regard to data consistency? -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
This is currently the only way to view the 'autostart' property for a node device. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/manpages/virsh.rst | 12 ++++++++ tools/virsh-nodedev.c | 68 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index e9023ba17b..5958f6d1f7 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5061,6 +5061,18 @@ be either device name or wwn pair in "wwnn,wwpn" format (only works for HBA). +nodedev-info +------------ + +**Syntax:** + +:: + + nodedev-info device + +Returns basic information about the *device* object. + + nodedev-list ------------ diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 845effcb81..c8da37f104 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1227,6 +1227,68 @@ cmdNodeDeviceAutostart(vshControl *ctl, const vshCmd *cmd) } +/* + * "nodedev-info" command + */ +static const vshCmdInfo info_node_device_info[] = { + {.name = "help", + .data = N_("node device details in XML") + }, + {.name = "desc", + .data = N_("Output the node device details as an XML dump to stdout.") + }, + {.name = NULL} +}; + + +static const vshCmdOptDef opts_node_device_info[] = { + {.name = "device", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device name or wwn pair in 'wwnn,wwpn' format"), + .completer = virshNodeDeviceNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNodeDeviceInfo(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevicePtr device = NULL; + const char *device_value = NULL; + bool ret = false; + int autostart; + const char *parent = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; + + device = vshFindNodeDevice(ctl, device_value); + + if (!device) + goto cleanup; + + parent = virNodeDeviceGetParent(device); + vshPrint(ctl, "%-15s %s\n", _("Name:"), virNodeDeviceGetName(device)); + vshPrint(ctl, "%-15s %s\n", _("Parent:"), parent ? parent : ""); + vshPrint(ctl, "%-15s %s\n", _("Active:"), virNodeDeviceIsActive(device) ? + _("yes") : _("no")); + vshPrint(ctl, "%-15s %s\n", _("Persistent:"), + virNodeDeviceIsPersistent(device) ? _("yes") : _("no")); + if (virNodeDeviceGetAutostart(device, &autostart) < 0) + vshPrint(ctl, "%-15s %s\n", _("Autostart:"), _("no autostart")); + else + vshPrint(ctl, "%-15s %s\n", _("Autostart:"), autostart ? _("yes") : _("no")); + + ret = true; + cleanup: + if (device) + virNodeDeviceFree(device); + return ret; +} + + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1304,5 +1366,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_autostart, .flags = 0 }, + {.name = "nodedev-info", + .handler = cmdNodeDeviceInfo, + .opts = opts_node_device_info, + .info = info_node_device_info, + .flags = 0 + }, {.name = NULL} }; -- 2.31.1
On Thu, Jun 03, 2021 at 03:11:56PM -0500, Jonathon Jongsma wrote:
This is currently the only way to view the 'autostart' property for a node device.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/manpages/virsh.rst | 12 ++++++++ tools/virsh-nodedev.c | 68 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
This is currently the only way to view the 'autostart' property for a node device.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/manpages/virsh.rst | 12 ++++++++ tools/virsh-nodedev.c | 68 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index e9023ba17b..5958f6d1f7 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5061,6 +5061,18 @@ be either device name or wwn pair in "wwnn,wwpn" format (only works for HBA).
+nodedev-info +------------ + +**Syntax:** + +:: + + nodedev-info device + +Returns basic information about the *device* object. + + nodedev-list ------------
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 845effcb81..c8da37f104 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1227,6 +1227,68 @@ cmdNodeDeviceAutostart(vshControl *ctl, const vshCmd *cmd) }
+/* + * "nodedev-info" command + */ +static const vshCmdInfo info_node_device_info[] = {
The information for help and description need to be updated from dumpxml to info.
+ {.name = "help", + .data = N_("node device details in XML") "node device information"
+ }, + {.name = "desc", + .data = N_("Output the node device details as an XML dump to stdout.") "Returns basic information about the node device."
+ }, + {.name = NULL} +}; + + +static const vshCmdOptDef opts_node_device_info[] = { + {.name = "device", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device name or wwn pair in 'wwnn,wwpn' format"), + .completer = virshNodeDeviceNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNodeDeviceInfo(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevicePtr device = NULL; + const char *device_value = NULL; + bool ret = false; + int autostart; + const char *parent = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; + + device = vshFindNodeDevice(ctl, device_value); + + if (!device) + goto cleanup; + + parent = virNodeDeviceGetParent(device); + vshPrint(ctl, "%-15s %s\n", _("Name:"), virNodeDeviceGetName(device)); + vshPrint(ctl, "%-15s %s\n", _("Parent:"), parent ? parent : ""); + vshPrint(ctl, "%-15s %s\n", _("Active:"), virNodeDeviceIsActive(device) ? + _("yes") : _("no")); + vshPrint(ctl, "%-15s %s\n", _("Persistent:"), + virNodeDeviceIsPersistent(device) ? _("yes") : _("no")); + if (virNodeDeviceGetAutostart(device, &autostart) < 0) + vshPrint(ctl, "%-15s %s\n", _("Autostart:"), _("no autostart")); + else + vshPrint(ctl, "%-15s %s\n", _("Autostart:"), autostart ? _("yes") : _("no")); + + ret = true; + cleanup: + if (device) + virNodeDeviceFree(device); + return ret; +} + + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1304,5 +1366,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_autostart, .flags = 0 }, + {.name = "nodedev-info", + .handler = cmdNodeDeviceInfo, + .opts = opts_node_device_info, + .info = info_node_device_info, + .flags = 0 + }, {.name = NULL} };
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (3)
-
Boris Fiuczynski -
Daniel P. Berrangé -
Jonathon Jongsma