On 2/21/24 18:27, Jonathon Jongsma wrote:
On 2/16/24 8:52 AM, Boris Fiuczynski wrote:
Allow to modify a node device by using virNodeDeviceDefineXML() to align
its behavior with other drivers define methods.
Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
---
NEWS.rst | 5 +++
src/libvirt-nodedev.c | 4 +-
src/node_device/node_device_driver.c | 64 ++++++++++++++++++++++------
tools/virsh-nodedev.c | 8 ++--
4 files changed, 64 insertions(+), 17 deletions(-)
diff --git a/NEWS.rst b/NEWS.rst
index aa2fee3533..9b8b94dea4 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -32,6 +32,11 @@ v10.1.0 (unreleased)
* **Improvements**
+* nodedev: Add ability to update persistent mediated devices by
defining them
+
+ Existing persistent mediated devices can now also be updated by
+ ``virNodeDeviceDefineXML()`` as long as parent and UUID remain
unchanged.
+
* **Bug fixes**
* qemu_process: Skip over non-virtio non-TAP NIC models when
refreshing rx-filter
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index b97c199a3a..f242c0a8f6 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -780,7 +780,9 @@ virNodeDeviceDestroy(virNodeDevicePtr dev)
* @xmlDesc: string containing an XML description of the device to
be defined
* @flags: bitwise-OR of supported virNodeDeviceDefineXMLFlags
*
- * Define a new device on the VM host machine, for example, a
mediated device
+ * Define a new inactive persistent device or modify an existing
persistent
+ * one from the XML description on the VM host machine, for example,
a mediated
+ * device.
*
* virNodeDeviceFree should be used to free the resources after the
* node device object is no longer needed.
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index 11bc9af92e..66de46513d 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -1544,12 +1544,33 @@
nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def,
}
+static virNodeDeviceObj*
+findPersistentMdevNodeDevice(virNodeDeviceDef *def)
+{
+ virNodeDeviceObj *obj = NULL;
+
+ if (!nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV) &&
!def->parent)
+ return NULL;
These checks are duplicating checks that already exist in
nodeDeviceDefineXML(). If we remove them, I'm not sure it's even worth
factoring this out into a separate function.
The check for mdev ensures the later access to mdev data works as
expected and checking for parent here protects running a search that is
not required. I will remove the parent check.
+
+ if (def->caps->data.mdev.uuid &&
def->caps->data.mdev.parent_addr) { > +
mdevGenerateDeviceName(def);
I don't like the fact that calling this function has the side-effect of
(potentially) changing the name of the device definition by calling
mdevGenerateDeviceName(). While you're only currently calling this
function from one location where this won't cause any problems, I'd
rather not make assumptions within the function. It'd be better to make
sure that the name is set before calling this function (if you keep this
as a separate function). Or better yet, just use
virNodeDeviceObjListFindMediatedDeviceByUUID() so you don't even need to
generate a name for lookup.
+ if ((obj = nodeDeviceObjFindByName(def->name)) &&
+ virNodeDeviceObjIsPersistent(obj))
The locking needs to be fixed in case the object is not persistent here.
I will no longer use mdevGenerateDeviceName() and use
virNodeDeviceObjListFindMediatedDeviceByUUID() as you suggested but I
need to include a virNodeDeviceObjEndAPI() here.
+ return obj;
+ }
+
+ return NULL;
+}
+
+
virNodeDevice*
nodeDeviceDefineXML(virConnect *conn,
const char *xmlDesc,
unsigned int flags)
{
g_autoptr(virNodeDeviceDef) def = NULL;
+ virNodeDevicePtr new_nodedev = NULL;
+ virNodeDeviceObj *persistent_obj = NULL;
const char *virt_type = NULL;
g_autofree char *uuid = NULL;
g_autofree char *name = NULL;
@@ -1566,28 +1587,43 @@ nodeDeviceDefineXML(virConnect *conn,
&driver->parserCallbacks,
NULL, validate)))
return NULL;
+ persistent_obj = findPersistentMdevNodeDevice(def);
+
if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0)
- return NULL;
+ goto cleanup;
if (!nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Unsupported device type"));
- return NULL;
+ goto cleanup;
}
if (!def->parent) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("cannot define a mediated device without a
parent"));
- return NULL;
+ goto cleanup;
}
- if (virMdevctlDefine(def, &uuid) < 0) {
- return NULL;
- }
+ if (persistent_obj) {
+ // virNodeDeviceObjUpdateModificationImpact() is not required
we will
+ // modify the persitent config only.
typo: persitent -> persistent
It seems to be my favorite typo... :(
+ // nodeDeviceDefValidateUpdate() is not required as uuid and
parent are
+ // machting if def was found and changing the type in the
persistent
typo: machting -> matching
Fixed
+ // config is allowed.
+ VIR_DEBUG("Update node device '%s' with mdevctl", def->name);
+ if (virMdevctlModify(def, true, false) < 0)
+ goto cleanup;
- if (uuid && uuid[0]) {
- g_free(def->caps->data.mdev.uuid);
- def->caps->data.mdev.uuid = g_steal_pointer(&uuid);
+ virNodeDeviceObjEndAPI(&persistent_obj);
You're calling EndAPI() here and then calling it in the cleanup label
again later. Although that won't cause any problems (since the
peristent_obj variable will be set to NULL by the first call), it
^-- seems like I am not the only one... ;)
doesn't feel quite right. One way to avoid this might be to move the
findPersistentMdevNodeDevice() call after all of the other checks so
that you no longer need the cleanup label below.
I am going to remove the cleanup label, move
findPersistentMdevNodeDevice() into the if condition and do the
virNodeDeviceObjEndAPI() here after calling virMdevctlModify().
Expect a v5 today.
+ } else {
+ VIR_DEBUG("Define node device '%s' with mdevctl", def->name);
+ if (virMdevctlDefine(def, &uuid) < 0)
+ goto cleanup;
+
+ if (uuid && uuid[0]) {
+ g_free(def->caps->data.mdev.uuid);
+ def->caps->data.mdev.uuid = g_steal_pointer(&uuid);
+ }
}
mdevGenerateDeviceName(def);
@@ -1601,9 +1637,13 @@ nodeDeviceDefineXML(virConnect *conn,
* add the provisional device to the list and return it
immediately and
* avoid this long delay. */
if (nodeDeviceUpdateMediatedDevice(g_steal_pointer(&def), true)
< 0) > - return NULL;
+ goto cleanup;
+
+ new_nodedev = virGetNodeDevice(conn, name);
- return virGetNodeDevice(conn, name);
+ cleanup:
+ virNodeDeviceObjEndAPI(&persistent_obj);
+ return new_nodedev;
}
@@ -2237,7 +2277,7 @@ nodeDeviceDefValidateUpdate(virNodeDeviceDef *def,
cap_mdev->uuid);
return -1;
}
-
+
/* A live update cannot change the mdev type. Since the new
config is
* stored in defined_config, compare that to the mdev type of
the current
* live config to make sure they match */
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index 7b80747819..912975dfed 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -1083,12 +1083,12 @@ cmdNodeDeviceUndefine(vshControl *ctl, const
vshCmd *cmd G_GNUC_UNUSED)
*/
static const vshCmdInfo info_node_device_define[] = {
{.name = "help",
- .data = N_("Define a device by an xml file on a node")
+ .data = N_("Define or modify a device by an xml file on a node")
},
{.name = "desc",
- .data = N_("Defines a persistent device on the node that can be "
- "assigned to a domain. The device must be started
before "
- "it can be assigned to a domain.")
+ .data = N_("Defines or modifies a persistent device on the node
that "
+ "can be assigned to a domain. The device must be
started "
+ "before it can be assigned to a domain.")
},
{.name = NULL}
};
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
