[libvirt PATCH v6 00/30] Add support for persistent mediated devices

This patch series follows the previously-merged series which added support for transient mediated devices. This series expands mdev support to include persistent device definitions. Again, it relies on mdevctl as the backend. It follows the common libvirt pattern of APIs by adding the following new APIs for node devices: - virNodeDeviceDefineXML() - defines a persistent device - virNodeDeviceUndefine() - undefines a persistent device - virNodeDeviceCreate() - starts a previously-defined device It also adds virsh commands mapping to these new APIs: nodedev-define, nodedev-undefine, and nodedev-start. Since we rely on mdevctl for the definition of mediated devices, we need a way to stay up-to-date with devices that are defined by mdevctl (outside of libvirt). The method for staying up-to-date is currently a little bit crude due to the fact that mdevctl does not emit any events when new devices are added or removed. As a workaround, we create a file monitor for the mdevctl config directory and re-query mdevctl when we detect changes within that directory. In the future, mdevctl may introduce a more elegant solution. Changes in v6: - rebase to git master again - remove typedefs for various *Ptr types, since they're now discouraged in libvirt. Changes in v5: - Rebase to git master - updated new API version info to 7.2.0 - capture and relay stderr message from mdevctl - changed to using GHashTable functions directly instead of deprecated virHash functions - protected mdevctlMonitors with a mutex - added a couple patches to fix the 5s delay when defining a new mdev. These are currently separate patches added to the end of the series, but could be squashed into the earlier commits if that's preferred. - various other minor review fixes Changes in v4: - rebase to git master - switch to throwaway thread for querying mdevctl - fixed a bug when removing devices because I was accidentally using virHashForEach() instead of virHashForeachSafe() - use DEFINED/UNDEFINED events instead of STARTED/STOPPED events - changes related to merging information about mdev devices from both udev a= nd mdevctl: - Re-used the same function to copy extra data from mdevctl regardless of whether we're processing a udev event or a mdevctl event (recommended by Erik). This results in slightly more complex handling of the object lifetimes (see patch 9), but it consolidates some code. - nodeDeviceDefCopyFromMdevctl() previously only copied the data that was unique to mdevctl and didn't exist in udev. It now copies additional data (possibly overwriting some udev). This solves a problem where a device = is defined but not active (i.e. we have not gotten any data from udev), and then changed (e.g. somebody calls 'mdevctl modify' to change the mdev type), but libvirt was not updating to the new definition. - fix a bug where we were mistakenly emitting 'update' events for devices th= at had not changed - Added the ability to specify a uuid for an mdev via device XML. - split some commits into multiple patches - updated new API version info to 7.1.0 - Fixed a bug reported by Yan Fu which hangs the client when attempting to destroy a nodedev that is in use by an active vm. See https://www.redhat.com/archives/libvir-list/2021-February/msg00116.html for solution suggested by Alex. - numerous smaller fixes from review findings changes in v3: - streamlined tests -- removed some unnecessary duplication - split out patch to factor out node device name generation function - split nodeDeviceParseMdevctlChildDevice() into a separate function - added follow-up patch to remove space-padded alignment in header - refactored the mdevctl update handling significantly: - no longer a separate persistent thread that gets signaled by a timer - now piggybacks onto the existing udev thread and signals the thread in t= he same way that the udev event does. - Daniel suggested spawning a throw-away thread to handle mdevctl updates, but that introduces the complexity of possibly serializing multiple throw-away threads (e.g. if we get an 'created' event followed immediate= ly by a 'deleted' event, two threads may be spawned and we'd need to ensure they are properly ordered) - added virNodeDeviceObjListForEach() and virNodeDeviceObjListRemoveLocked() to simplify removing devices that are removed from mdevctl. - coding style fixes - NOTE: per Erik's request, I experimented with changing the way that mdevctl commands were generated and tested (e.g. introducing something like virMdevctlGetCommand(def, MDEVCTL_COMMAND_<SUBCOMMAND>, ...)), but it was too invasive and awkward and didn't seem worthwhile Changes in v2: - rebase to latest git master Jonathon Jongsma (30): nodedev: capture and report stderror from mdevctl tests: remove extra trailing semicolon nodedev: introduce concept of 'active' node devices nodedev: Add ability to filter by active state nodedev: fix docs for virConnectListAllNodeDevices() nodedev: expose internal helper for naming devices nodedev: add ability to parse mdevs from mdevctl nodedev: add ability to list defined mdevs nodedev: add persistence to virNodeDeviceObj nodedev: add DEFINED/UNDEFINED lifecycle events nodedev: add mdevctl devices to node device list nodedev: add helper functions to remove node devices nodedev: handle mdevs that disappear from mdevctl nodedev: Refresh mdev devices when changes are detected nodedev: add function to generate mdevctl define command api: add virNodeDeviceDefineXML() virsh: Add --inactive, --all to nodedev-list virsh: add nodedev-define command nodedev: refactor tests to support mdev undefine api: add virNodeDeviceUndefine() virsh: Factor out function to find node device virsh: add nodedev-undefine command api: add virNodeDeviceCreate() virsh: add "nodedev-start" command nodedev: add <uuid> element to mdev caps nodedev: add ability to specify UUID for new mdevs nodedev: fix hang when destroying an mdev in use nodedev: add docs about mdev attribute order nodedev: factor out function to add mediated devices nodedev: avoid delay when defining a new mdev docs/formatnode.html.in | 5 +- docs/schemas/nodedev.rng | 43 +- examples/c/misc/event-test.c | 4 + include/libvirt/libvirt-nodedev.h | 20 +- src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 + src/conf/node_device_conf.c | 14 + src/conf/node_device_conf.h | 8 + src/conf/virnodedeviceobj.c | 147 +++- src/conf/virnodedeviceobj.h | 24 + src/driver-nodedev.h | 14 + src/libvirt-nodedev.c | 141 +++- src/libvirt_private.syms | 6 + src/libvirt_public.syms | 3 + src/node_device/node_device_driver.c | 743 +++++++++++++++++- src/node_device/node_device_driver.h | 50 +- src/node_device/node_device_udev.c | 217 ++++- src/remote/remote_driver.c | 3 + src/remote/remote_protocol.x | 40 +- src/remote_protocol-structs | 16 + src/rpc/gendispatch.pl | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 2 + ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 + ...019_36ea_4111_8f0a_8c9a70e21366-start.argv | 3 +- ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 + tests/nodedevmdevctldata/mdevctl-create.argv | 1 + .../mdevctl-list-defined.argv | 1 + .../mdevctl-list-multiple.json | 59 ++ .../mdevctl-list-multiple.out.xml | 43 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 232 +++++- ...v_d069d019_36ea_4111_8f0a_8c9a70e21366.xml | 1 + tools/virsh-nodedev.c | 269 ++++++- 36 files changed, 1931 insertions(+), 193 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9= a70e21366-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9= a70e21366-define.json create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb= 3f14c23d9-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb= 3f14c23d9-define.json create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871= d16c13076-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871= d16c13076-define.json create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv --=20 2.26.3

When an mdevctl command fails, there is not much information available to the user about why it failed. This is partly because we were not making use of the error message that mdevctl itself prints upon failure. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 46 +++++++++++++++++----------- src/node_device/node_device_driver.h | 6 ++-- tests/nodedevmdevctltest.c | 6 ++-- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 543e5bb90a..3851a3568f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -700,7 +700,8 @@ nodeDeviceFindAddressByName(const char *name) virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out) + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -725,15 +726,17 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, virCommandSetInputBuffer(cmd, json); virCommandSetOutputBuffer(cmd, uuid_out); + virCommandSetErrorBuffer(cmd, errmsg); return cmd; } static int -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { int status; - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid); + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid, + errmsg); if (!cmd) return -1; @@ -754,6 +757,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) { g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; if (!def->parent) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -761,9 +765,10 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; } - if (virMdevctlStart(def, &uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to start mediated device")); + if (virMdevctlStart(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start mediated device '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown Error"); return NULL; } @@ -828,23 +833,25 @@ nodeDeviceCreateXML(virConnectPtr conn, virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid) -{ - return virCommandNewArgList(MDEVCTL, - "stop", - "-u", - uuid, - NULL); +nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg) +{ + virCommandPtr cmd = virCommandNewArgList(MDEVCTL, + "stop", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; } static int -virMdevctlStop(virNodeDeviceDefPtr def) +virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { int status; g_autoptr(virCommand) cmd = NULL; - cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid); + cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, errmsg); if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; @@ -901,9 +908,12 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { - if (virMdevctlStop(def) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to stop mediated device")); + g_autofree char *errmsg = NULL; + + if (virMdevctlStop(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown error"); goto cleanup; } ret = 0; diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 2113d2b0a5..4a40aa51f6 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -115,6 +115,8 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out); + char **uuid_out, + char **errmsg); virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid); +nodeDeviceGetMdevctlStopCommand(const char *uuid, + char **errmsg); diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 1bad65549b..c12feaac55 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -58,6 +58,7 @@ testMdevctlStart(const char *virt_type, const char *actualCmdline = NULL; int ret = -1; g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; g_autofree char *stdinbuf = NULL; g_autoptr(virCommand) cmd = NULL; @@ -66,7 +67,7 @@ testMdevctlStart(const char *virt_type, /* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid); + cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg); if (!cmd) goto cleanup; @@ -117,11 +118,12 @@ testMdevctlStop(const void *data) const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL; g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid); + cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg); if (!cmd) goto cleanup; -- 2.26.3

On Fri, Mar 26, 2021 at 11:47:57AM -0500, Jonathon Jongsma wrote:
When an mdevctl command fails, there is not much information available to the user about why it failed. This is partly because we were not making use of the error message that mdevctl itself prints upon failure.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 46 +++++++++++++++++----------- src/node_device/node_device_driver.h | 6 ++-- tests/nodedevmdevctltest.c | 6 ++-- 3 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 543e5bb90a..3851a3568f 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -700,7 +700,8 @@ nodeDeviceFindAddressByName(const char *name)
virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out) + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -725,15 +726,17 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def,
virCommandSetInputBuffer(cmd, json); virCommandSetOutputBuffer(cmd, uuid_out); + virCommandSetErrorBuffer(cmd, errmsg);
return cmd; }
static int -virMdevctlStart(virNodeDeviceDefPtr def, char **uuid) +virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { int status; - g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid); + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid, + errmsg); if (!cmd) return -1;
@@ -754,6 +757,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) { g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL;
if (!def->parent) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -761,9 +765,10 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; }
- if (virMdevctlStart(def, &uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to start mediated device")); + if (virMdevctlStart(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to start mediated device '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown Error");
"Unknown Error" is fairly unhelpful to the user. It should also be reported *only* when errmsg exists otherwise an error should have been logged from other internal helpers up the stack. The scenario when I hit it was when I tried to define an mdev with parent "0000:06:00" not knowing where exactly it failed. Turns out, both nodeDeviceGetMdevctlDefineStartCommand and nodeDeviceFindAddressByName call virReportError which would tell me more about it, but little did I know that VIR_ERR_NO_XYZ are intended/reserved error codes for public APIs, not internal use. That's because we call daemonErrorLogFilter just before logging the error to perform the final log priority change which demotes every error message generated internally with an error code of VIR_ERROR_NO_XYZ (see [1]). Therefore we'll have to fix the two callers mentioned above to report an internal error instead, but we can easily do that in a follow up patch - let's just not forget about it :). Otherwise the patch looks good. With the "Unknown Error" message gone: Reviewed-by: Erik Skultety <eskultet@redhat.com> [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/remote/remote_daemon.c#...)
return NULL; }
@@ -828,23 +833,25 @@ nodeDeviceCreateXML(virConnectPtr conn,
virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid) -{ - return virCommandNewArgList(MDEVCTL, - "stop", - "-u", - uuid, - NULL); +nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg) +{ + virCommandPtr cmd = virCommandNewArgList(MDEVCTL, + "stop", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd;
}
static int -virMdevctlStop(virNodeDeviceDefPtr def) +virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { int status; g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid); + cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0) return -1; @@ -901,9 +908,12 @@ nodeDeviceDestroy(virNodeDevicePtr device)
ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { - if (virMdevctlStop(def) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to stop mediated device")); + g_autofree char *errmsg = NULL; + + if (virMdevctlStop(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': %s"), def->name, + errmsg && errmsg[0] ? errmsg : "Unknown error"); goto cleanup; } ret = 0; diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 2113d2b0a5..4a40aa51f6 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -115,6 +115,8 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out); + char **uuid_out, + char **errmsg); virCommandPtr -nodeDeviceGetMdevctlStopCommand(const char *uuid); +nodeDeviceGetMdevctlStopCommand(const char *uuid, + char **errmsg); diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 1bad65549b..c12feaac55 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -58,6 +58,7 @@ testMdevctlStart(const char *virt_type, const char *actualCmdline = NULL; int ret = -1; g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; g_autofree char *stdinbuf = NULL; g_autoptr(virCommand) cmd = NULL;
@@ -66,7 +67,7 @@ testMdevctlStart(const char *virt_type,
/* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid); + cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg);
if (!cmd) goto cleanup; @@ -117,11 +118,12 @@ testMdevctlStop(const void *data) const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; + g_autofree char *errmsg = NULL; g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", abs_srcdir);
- cmd = nodeDeviceGetMdevctlStopCommand(uuid); + cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg);
if (!cmd) goto cleanup; -- 2.26.3

The macro should not have a trailing semicolon so that when the macro is used, the user can add a semicolon themselves. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tests/nodedevmdevctltest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index c12feaac55..650e46a29f 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -273,7 +273,7 @@ mymain(void) struct startTestInfo info = { virt_type, create, filename }; \ DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ } \ - while (0); + while (0) #define DO_TEST_START(filename) \ DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename) -- 2.26.3

we will be able to define mediated devices that can be started or stopped, so we need to be able to indicate whether the device is active or not, similar to other resources (storage pools, domains, etc.) Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++++++++++++++ src/conf/virnodedeviceobj.h | 7 +++++++ src/libvirt_private.syms | 2 ++ src/node_device/node_device_udev.c | 3 +++ 4 files changed, 28 insertions(+) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ecae3d0479..f0ceb61a19 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -39,6 +39,7 @@ struct _virNodeDeviceObj { virNodeDeviceDefPtr def; /* device definition */ bool skipUpdateCaps; /* whether to skip checking host caps, used by testdriver */ + bool active; }; struct _virNodeDeviceObjList { @@ -976,3 +977,18 @@ virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj, { obj->skipUpdateCaps = skipUpdateCaps; } + + +bool +virNodeDeviceObjIsActive(virNodeDeviceObj *obj) +{ + return obj->active; +} + + +void +virNodeDeviceObjSetActive(virNodeDeviceObj *obj, + bool active) +{ + obj->active = active; +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 6efdb23d36..e786a70f51 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -121,3 +121,10 @@ virNodeDeviceObjSetSkipUpdateCaps(virNodeDeviceObjPtr obj, virNodeDeviceObjPtr virNodeDeviceObjListFindMediatedDeviceByUUID(virNodeDeviceObjListPtr devs, const char *uuid); + +bool +virNodeDeviceObjIsActive(virNodeDeviceObj *obj); + +void +virNodeDeviceObjSetActive(virNodeDeviceObj *obj, + bool active); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cb9fe7c80a..09957c943b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1272,6 +1272,7 @@ virNetworkPortDefSaveStatus; # conf/virnodedeviceobj.h virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; +virNodeDeviceObjIsActive; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; @@ -1284,6 +1285,7 @@ virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; +virNodeDeviceObjSetActive; # conf/virnwfilterbindingdef.h diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 3daa5c90ad..373f36c41f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1544,6 +1544,7 @@ udevAddOneDevice(struct udev_device *device) else event = virNodeDeviceEventUpdateNew(objdef->name); + virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjEndAPI(&obj); ret = 0; @@ -1930,6 +1931,8 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + virNodeDeviceObjSetActive(obj, true); + virNodeDeviceObjEndAPI(&obj); ret = 0; -- 2.26.3

Add two flag values for virConnectListAllNodeDevices() so that we can list only node devices that are active or inactive. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 9 +++-- src/conf/node_device_conf.h | 8 ++++ src/conf/virnodedeviceobj.c | 57 +++++++++++++++++----------- src/libvirt-nodedev.c | 2 + src/node_device/node_device_driver.c | 2 +- 5 files changed, 51 insertions(+), 27 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index eab8abf6ab..1a0e60b81f 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -61,10 +61,9 @@ int virNodeListDevices (virConnectPtr conn, * virConnectListAllNodeDevices: * * Flags used to filter the returned node devices. Flags in each group - * are exclusive. Currently only one group to filter the devices by cap - * type. - */ + * are exclusive. */ typedef enum { + /* filter the devices by cap type */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0, /* System capability */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV = 1 << 1, /* PCI device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV = 1 << 2, /* USB device */ @@ -86,6 +85,10 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD = 1 << 18, /* s390 AP Card device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE = 1 << 19, /* s390 AP Queue */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX = 1 << 20, /* s390 AP Matrix */ + + /* filter the devices by active state */ + VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE = 1 << 30, /* Inactive devices */ + VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE = 1U << 31, /* Active devices */ } virConnectListAllNodeDeviceFlags; int virConnectListAllNodeDevices (virConnectPtr conn, diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index c67b8e2aeb..3d7872fd6e 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -422,6 +422,14 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX) +#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE \ + VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE |\ + VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE + +#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL \ + VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP | \ + VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE + int virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index f0ceb61a19..ab663fd5a0 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -848,8 +848,10 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs, } -#define MATCH(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ - virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG)) +#define MATCH_CAP(FLAG) ((flags & (VIR_CONNECT_LIST_NODE_DEVICES_CAP_ ## FLAG)) && \ + virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_ ## FLAG)) +#define MATCH(FLAG) (flags & (FLAG)) + static bool virNodeDeviceObjMatch(virNodeDeviceObjPtr obj, unsigned int flags) @@ -861,33 +863,42 @@ virNodeDeviceObjMatch(virNodeDeviceObjPtr obj, /* filter by cap type */ if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) { - if (!(MATCH(SYSTEM) || - MATCH(PCI_DEV) || - MATCH(USB_DEV) || - MATCH(USB_INTERFACE) || - MATCH(NET) || - MATCH(SCSI_HOST) || - MATCH(SCSI_TARGET) || - MATCH(SCSI) || - MATCH(STORAGE) || - MATCH(FC_HOST) || - MATCH(VPORTS) || - MATCH(SCSI_GENERIC) || - MATCH(DRM) || - MATCH(MDEV_TYPES) || - MATCH(MDEV) || - MATCH(CCW_DEV) || - MATCH(CSS_DEV) || - MATCH(VDPA) || - MATCH(AP_CARD) || - MATCH(AP_QUEUE) || - MATCH(AP_MATRIX))) + if (!(MATCH_CAP(SYSTEM) || + MATCH_CAP(PCI_DEV) || + MATCH_CAP(USB_DEV) || + MATCH_CAP(USB_INTERFACE) || + MATCH_CAP(NET) || + MATCH_CAP(SCSI_HOST) || + MATCH_CAP(SCSI_TARGET) || + MATCH_CAP(SCSI) || + MATCH_CAP(STORAGE) || + MATCH_CAP(FC_HOST) || + MATCH_CAP(VPORTS) || + MATCH_CAP(SCSI_GENERIC) || + MATCH_CAP(DRM) || + MATCH_CAP(MDEV_TYPES) || + MATCH_CAP(MDEV) || + MATCH_CAP(CCW_DEV) || + MATCH_CAP(CSS_DEV) || + MATCH_CAP(VDPA) || + MATCH_CAP(AP_CARD) || + MATCH_CAP(AP_QUEUE) || + MATCH_CAP(AP_MATRIX))) + return false; + } + + if (flags & (VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE)) { + if (!((MATCH(VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE) && + virNodeDeviceObjIsActive(obj)) || + (MATCH(VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE) && + !virNodeDeviceObjIsActive(obj)))) return false; } return true; } #undef MATCH +#undef MATCH_CAP typedef struct _virNodeDeviceObjListExportData virNodeDeviceObjListExportData; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index eb8c735a8c..375b907852 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -105,6 +105,8 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX + * VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE + * VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 3851a3568f..920fd815f2 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -217,7 +217,7 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, virNodeDevicePtr **devices, unsigned int flags) { - virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP, -1); + virCheckFlags(VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ALL, -1); if (virConnectListAllNodeDevicesEnsureACL(conn) < 0) return -1; -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:00AM -0500, Jonathon Jongsma wrote:
Add two flag values for virConnectListAllNodeDevices() so that we can list only node devices that are active or inactive.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 9 +++-- src/conf/node_device_conf.h | 8 ++++ src/conf/virnodedeviceobj.c | 57 +++++++++++++++++----------- src/libvirt-nodedev.c | 2 + src/node_device/node_device_driver.c | 2 +- 5 files changed, 51 insertions(+), 27 deletions(-)
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index eab8abf6ab..1a0e60b81f 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -61,10 +61,9 @@ int virNodeListDevices (virConnectPtr conn, * virConnectListAllNodeDevices: * * Flags used to filter the returned node devices. Flags in each group - * are exclusive. Currently only one group to filter the devices by cap - * type. - */ + * are exclusive. */ typedef enum { + /* filter the devices by cap type */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0, /* System capability */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV = 1 << 1, /* PCI device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV = 1 << 2, /* USB device */ @@ -86,6 +85,10 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD = 1 << 18, /* s390 AP Card device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE = 1 << 19, /* s390 AP Queue */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX = 1 << 20, /* s390 AP Matrix */ + + /* filter the devices by active state */ + VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE = 1 << 30, /* Inactive devices */ + VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE = 1U << 31, /* Active devices */ } virConnectListAllNodeDeviceFlags;
int virConnectListAllNodeDevices (virConnectPtr conn, diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index c67b8e2aeb..3d7872fd6e 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -422,6 +422,14 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX)
+#define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_ACTIVE \ + VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE |\ nitpick: 1 extra white space ^here
My R-b still stands. Erik

It doesn't make sense to list all of the flag values in the function documentation. This is unnecessary duplication, we already refer to the enum type. Also, remove reference to exclusive groups of flags, since that does not apply to this API. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 3 +-- src/libvirt-nodedev.c | 30 +----------------------------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 1a0e60b81f..2deead0791 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -60,8 +60,7 @@ int virNodeListDevices (virConnectPtr conn, /* * virConnectListAllNodeDevices: * - * Flags used to filter the returned node devices. Flags in each group - * are exclusive. */ + * Flags used to filter the returned node devices. */ typedef enum { /* filter the devices by cap type */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM = 1 << 0, /* System capability */ diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 375b907852..fb707b570f 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -78,35 +78,7 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * objects. * * Normally, all node devices are returned; however, @flags can be used to - * filter the results for a smaller list of targeted node devices. The valid - * flags are divided into groups, where each group contains bits that - * describe mutually exclusive attributes of a node device, and where all bits - * within a group describe all possible node devices. - * - * Only one group of the @flags is provided to filter the node devices by - * capability type, flags include: - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_USB_INTERFACE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_NET - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_HOST - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_TARGET - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_STORAGE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_FC_HOST - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPORTS - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_SCSI_GENERIC - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE - * VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX - * VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE - * VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE + * filter the results for a smaller list of targeted node devices. * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:01AM -0500, Jonathon Jongsma wrote:
It doesn't make sense to list all of the flag values in the function documentation. This is unnecessary duplication, we already refer to the enum type. Also, remove reference to exclusive groups of flags, since that does not apply to this API.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

Expose a helper function that can be used by udev and mdevctl to generate device names for node devices. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 25 +++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++++ src/node_device/node_device_udev.c | 19 +++---------------- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 920fd815f2..bada5bbf00 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -978,3 +978,28 @@ nodedevRegister(void) return udevNodeRegister(); #endif } + + +void +nodeDeviceGenerateName(virNodeDeviceDef *def, + const char *subsystem, + const char *sysname, + const char *s) +{ + size_t i; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s_%s", + subsystem, + sysname); + + if (s != NULL) + virBufferAsprintf(&buf, "_%s", s); + + def->name = virBufferContentAndReset(&buf); + + for (i = 0; i < strlen(def->name); i++) { + if (!(g_ascii_isalnum(*(def->name + i)))) + *(def->name + i) = '_'; + } +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 4a40aa51f6..76a40c05ad 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -120,3 +120,9 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); + +void +nodeDeviceGenerateName(virNodeDeviceDef *def, + const char *subsystem, + const char *sysname, + const char *s); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 373f36c41f..eae301cc95 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -311,22 +311,9 @@ udevGenerateDeviceName(struct udev_device *device, virNodeDeviceDefPtr def, const char *s) { - size_t i; - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - virBufferAsprintf(&buf, "%s_%s", - udev_device_get_subsystem(device), - udev_device_get_sysname(device)); - - if (s != NULL) - virBufferAsprintf(&buf, "_%s", s); - - def->name = virBufferContentAndReset(&buf); - - for (i = 0; i < strlen(def->name); i++) { - if (!(g_ascii_isalnum(*(def->name + i)))) - *(def->name + i) = '_'; - } + nodeDeviceGenerateName(def, + udev_device_get_subsystem(device), + udev_device_get_sysname(device), s); return 0; } -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:02AM -0500, Jonathon Jongsma wrote:
Expose a helper function that can be used by udev and mdevctl to generate device names for node devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 25 +++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++++++ src/node_device/node_device_udev.c | 19 +++---------------- 3 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 920fd815f2..bada5bbf00 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -978,3 +978,28 @@ nodedevRegister(void) return udevNodeRegister(); #endif } + + +void +nodeDeviceGenerateName(virNodeDeviceDef *def, + const char *subsystem, + const char *sysname, + const char *s) +{ + size_t i; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAsprintf(&buf, "%s_%s", + subsystem, + sysname); + + if (s != NULL) + virBufferAsprintf(&buf, "_%s", s); + + def->name = virBufferContentAndReset(&buf);
Actually, there's a leak ^here than I only just noticed: 11 bytes in 1 blocks are definitely lost in loss record 222 of 3,093 ==629005== at 0x4C34F0B: malloc (vg_replace_malloc.c:307) ==629005== by 0x59A92A5: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.4) ==629005== by 0x59C2BE2: g_strdup (in /usr/lib64/libglib-2.0.so.0.5600.4) ==629005== by 0x504B37C: virNodeDeviceDefParseXML (node_device_conf.c:2122) ==629005== by 0x504BA0E: virNodeDeviceDefParseNode (node_device_conf.c:2242) ==629005== by 0x504BADD: virNodeDeviceDefParse (node_device_conf.c:2256) ==629005== by 0x504BB1E: virNodeDeviceDefParseString (node_device_conf.c:2270) ==629005== by 0x262229FF: nodeDeviceDefineXML (node_device_driver.c:1290) ==629005== by 0x520D415: virNodeDeviceDefineXML (libvirt-nodedev.c:768) ==629005== by 0x14AEED: remoteDispatchNodeDeviceDefineXML (remote_daemon_dispatch_stubs.h:1 5032 ==629005== by 0x14AE78: remoteDispatchNodeDeviceDefineXMLHelper (remote_daemon_dispatch_stu bs.h:15013) What happens is that we start with something like: ... def->name = g_strdup("new_device") ... and we never free it before re-writing it. My R-b still stands. Erik

This function will parse the list of mediated devices that are returned by mdevctl and convert it into our internal node device representation. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 143 ++++++++++++++++++ src/node_device/node_device_driver.h | 4 + .../mdevctl-list-multiple.json | 59 ++++++++ .../mdevctl-list-multiple.out.xml | 39 +++++ tests/nodedevmdevctltest.c | 56 ++++++- 5 files changed, 299 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.json create mode 100644 tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index bada5bbf00..a646692870 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -860,6 +860,149 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +static void mdevGenerateDeviceName(virNodeDeviceDef *dev) +{ + nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid, NULL); +} + + +static virNodeDeviceDef* +nodeDeviceParseMdevctlChildDevice(const char *parent, + virJSONValue *json) +{ + virNodeDevCapMdev *mdev; + const char *uuid; + virJSONValue *props; + virJSONValue *attrs; + g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1); + + /* the child object should have a single key equal to its uuid. + * The value is an object describing the properties of the mdev */ + if (virJSONValueObjectKeysNumber(json) != 1) + return NULL; + + uuid = virJSONValueObjectGetKey(json, 0); + props = virJSONValueObjectGetValue(json, 0); + + child->parent = g_strdup(parent); + child->caps = g_new0(virNodeDevCapsDef, 1); + child->caps->data.type = VIR_NODE_DEV_CAP_MDEV; + + mdev = &child->caps->data.mdev; + mdev->uuid = g_strdup(uuid); + mdev->type = + g_strdup(virJSONValueObjectGetString(props, "mdev_type")); + + attrs = virJSONValueObjectGet(props, "attrs"); + + if (attrs && virJSONValueIsArray(attrs)) { + size_t i; + int nattrs = virJSONValueArraySize(attrs); + + mdev->attributes = g_new0(virMediatedDeviceAttr*, nattrs); + mdev->nattributes = nattrs; + + for (i = 0; i < nattrs; i++) { + virJSONValue *attr = virJSONValueArrayGet(attrs, i); + virMediatedDeviceAttr *attribute; + virJSONValue *value; + + if (!virJSONValueIsObject(attr) || + virJSONValueObjectKeysNumber(attr) != 1) + return NULL; + + attribute = g_new0(virMediatedDeviceAttr, 1); + attribute->name = g_strdup(virJSONValueObjectGetKey(attr, 0)); + value = virJSONValueObjectGetValue(attr, 0); + attribute->value = g_strdup(virJSONValueGetString(value)); + mdev->attributes[i] = attribute; + } + } + mdevGenerateDeviceName(child); + + return g_steal_pointer(&child); +} + + +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDef ***devs) +{ + int n; + g_autoptr(virJSONValue) json_devicelist = NULL; + virNodeDeviceDef **outdevs = NULL; + size_t noutdevs = 0; + size_t i; + size_t j; + + json_devicelist = virJSONValueFromString(jsonstring); + + if (!json_devicelist || !virJSONValueIsArray(json_devicelist)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("mdevctl JSON response contains no devices")); + goto error; + } + + n = virJSONValueArraySize(json_devicelist); + + for (i = 0; i < n; i++) { + virJSONValue *obj = virJSONValueArrayGet(json_devicelist, i); + const char *parent; + virJSONValue *child_array; + int nchildren; + + if (!virJSONValueIsObject(obj)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parent device is not an object")); + goto error; + } + + /* mdevctl returns an array of objects. Each object is a parent device + * object containing a single key-value pair which maps from the name + * of the parent device to an array of child devices */ + if (virJSONValueObjectKeysNumber(obj) != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected format for parent device object")); + goto error; + } + + parent = virJSONValueObjectGetKey(obj, 0); + child_array = virJSONValueObjectGetValue(obj, 0); + + if (!virJSONValueIsArray(child_array)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Parent device's JSON object data is not an array")); + goto error; + } + + nchildren = virJSONValueArraySize(child_array); + + for (j = 0; j < nchildren; j++) { + g_autoptr(virNodeDeviceDef) child = NULL; + virJSONValue *child_obj = virJSONValueArrayGet(child_array, j); + + if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to parse child device")); + goto error; + } + + if (VIR_APPEND_ELEMENT(outdevs, noutdevs, child) < 0) + goto error; + } + } + + *devs = outdevs; + return noutdevs; + + error: + for (i = 0; i < noutdevs; i++) + virNodeDeviceDefFree(outdevs[i]); + VIR_FREE(outdevs); + return -1; +} + + int nodeDeviceDestroy(virNodeDevicePtr device) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 76a40c05ad..f1c93f4b21 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -121,6 +121,10 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +int +nodeDeviceParseMdevctlJSON(const char *jsonstring, + virNodeDeviceDef ***devs); + void nodeDeviceGenerateName(virNodeDeviceDef *def, const char *subsystem, diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.json b/tests/nodedevmdevctldata/mdevctl-list-multiple.json new file mode 100644 index 0000000000..eefcd90c62 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.json @@ -0,0 +1,59 @@ +[ + { + "0000:00:02.0": [ + { + "200f228a-c80a-4d50-bfb7-f5a0e4e34045": { + "mdev_type": "i915-GVTg_V5_4", + "start": "manual" + } + }, + { + "de807ffc-1923-4d5f-b6c9-b20ecebc6d4b": { + "mdev_type": "i915-GVTg_V5_4", + "start": "auto" + } + }, + { + "435722ea-5f43-468a-874f-da34f1217f13": { + "mdev_type": "i915-GVTg_V5_8", + "start": "manual", + "attrs": [ + { + "testattr": "42" + } + ] + } + } + ] + }, + { + "matrix": [ + { "783e6dbb-ea0e-411f-94e2-717eaad438bf": { + "mdev_type": "vfio_ap-passthrough", + "start": "manual", + "attrs": [ + { + "assign_adapter": "5" + }, + { + "assign_adapter": "6" + }, + { + "assign_domain": "0xab" + }, + { + "assign_control_domain": "0xab" + }, + { + "assign_domain": "4" + }, + { + "assign_control_domain": "4" + } + ] + } + } + ] + } +] + diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml new file mode 100644 index 0000000000..543ad916b7 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml @@ -0,0 +1,39 @@ +<device> + <name>mdev_200f228a_c80a_4d50_bfb7_f5a0e4e34045</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_4'/> + <iommuGroup number='0'/> + </capability> +</device> +<device> + <name>mdev_de807ffc_1923_4d5f_b6c9_b20ecebc6d4b</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_4'/> + <iommuGroup number='0'/> + </capability> +</device> +<device> + <name>mdev_435722ea_5f43_468a_874f_da34f1217f13</name> + <parent>0000:00:02.0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/> + <attr name='testattr' value='42'/> + </capability> +</device> +<device> + <name>mdev_783e6dbb_ea0e_411f_94e2_717eaad438bf</name> + <parent>matrix</parent> + <capability type='mdev'> + <type id='vfio_ap-passthrough'/> + <iommuGroup number='0'/> + <attr name='assign_adapter' value='5'/> + <attr name='assign_adapter' value='6'/> + <attr name='assign_domain' value='0xab'/> + <attr name='assign_control_domain' value='0xab'/> + <attr name='assign_domain' value='4'/> + <attr name='assign_control_domain' value='4'/> + </capability> +</device> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 650e46a29f..6c92d839d1 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -145,6 +145,53 @@ testMdevctlStop(const void *data) return ret; } + +static int +testMdevctlParse(const void *data) +{ + g_autofree char *buf = NULL; + const char *filename = data; + g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s.json", + abs_srcdir, filename); + g_autofree char *xmloutfile = g_strdup_printf("%s/nodedevmdevctldata/%s.out.xml", + abs_srcdir, filename); + int ret = -1; + int nmdevs = 0; + virNodeDeviceDef **mdevs = NULL; + virBuffer xmloutbuf = VIR_BUFFER_INITIALIZER; + size_t i; + + if (virFileReadAll(jsonfile, 1024*1024, &buf) < 0) { + VIR_TEST_DEBUG("Unable to read file %s", jsonfile); + return -1; + } + + if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) { + VIR_TEST_DEBUG("Unable to parse json for %s", filename); + return -1; + } + + for (i = 0; i < nmdevs; i++) { + g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]); + if (!devxml) + goto cleanup; + virBufferAddStr(&xmloutbuf, devxml); + } + + if (nodedevCompareToFile(virBufferCurrentContent(&xmloutbuf), xmloutfile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&xmloutbuf); + for (i = 0; i < nmdevs; i++) + virNodeDeviceDefFree(mdevs[i]); + g_free(mdevs); + + return ret; +} + static void nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv) { @@ -265,13 +312,13 @@ mymain(void) } #define DO_TEST_FULL(desc, func, info) \ - if (virTestRun(desc, func, &info) < 0) \ + if (virTestRun(desc, func, info) < 0) \ ret = -1; #define DO_TEST_START_FULL(virt_type, create, filename) \ do { \ struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ + DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, &info); \ } \ while (0) @@ -281,6 +328,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) +#define DO_TEST_PARSE_JSON(filename) \ + DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) + /* Test mdevctl start commands */ DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); DO_TEST_START("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); @@ -289,6 +339,8 @@ mymain(void) /* Test mdevctl stop command, pass an arbitrary uuid */ DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576"); + DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + done: nodedevTestDriverFree(driver); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:03AM -0500, Jonathon Jongsma wrote:
This function will parse the list of mediated devices that are returned by mdevctl and convert it into our internal node device representation.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> ---
...
@@ -265,13 +312,13 @@ mymain(void) }
#define DO_TEST_FULL(desc, func, info) \ - if (virTestRun(desc, func, &info) < 0) \ + if (virTestRun(desc, func, info) < 0) \ ret = -1;
#define DO_TEST_START_FULL(virt_type, create, filename) \ do { \ struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, info); \ + DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, &info); \ } \ while (0)
^This IMO deserves a trivial standalone patch. As for the rest of the code - per the discussion that happened in v4: Reviewed-by: Erik Skultety <eskultet@redhat.com>
@@ -281,6 +328,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid)
+#define DO_TEST_PARSE_JSON(filename) \ + DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) + /* Test mdevctl start commands */ DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); DO_TEST_START("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); @@ -289,6 +339,8 @@ mymain(void) /* Test mdevctl stop command, pass an arbitrary uuid */ DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576");
+ DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + done: nodedevTestDriverFree(driver);
-- 2.26.3

This adds an internal API to query for persistent mediated devices that are defined by mdevctl. Upcoming commits will make use of this information. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 18 +++++++++ src/node_device/node_device_driver.h | 3 ++ .../mdevctl-list-defined.argv | 1 + tests/nodedevmdevctltest.c | 39 +++++++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a646692870..6911fa36da 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -860,6 +860,24 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +virCommand* +nodeDeviceGetMdevctlListCommand(bool defined, + char **output) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "list", + "--dumpjson", + NULL); + + if (defined) + virCommandAddArg(cmd, "--defined"); + + virCommandSetOutputBuffer(cmd, output); + + return cmd; +} + + static void mdevGenerateDeviceName(virNodeDeviceDef *dev) { nodeDeviceGenerateName(dev, "mdev", dev->caps->data.mdev.uuid, NULL); diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f1c93f4b21..705d83f827 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -121,6 +121,9 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +virCommandPtr +nodeDeviceGetMdevctlListCommand(bool defined, char **output); + int nodeDeviceParseMdevctlJSON(const char *jsonstring, virNodeDeviceDef ***devs); diff --git a/tests/nodedevmdevctldata/mdevctl-list-defined.argv b/tests/nodedevmdevctldata/mdevctl-list-defined.argv new file mode 100644 index 0000000000..72b5906e9e --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-list-defined.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ list --dumpjson --defined diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 6c92d839d1..1ab4776a23 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -145,6 +145,40 @@ testMdevctlStop(const void *data) return ret; } +static int +testMdevctlListDefined(const void *data G_GNUC_UNUSED) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *output = NULL; + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-list-defined.argv", + abs_srcdir); + + cmd = nodeDeviceGetMdevctlListCommand(true, &output); + + if (!cmd) + goto cleanup; + + virCommandSetDryRun(&buf, NULL, NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (nodedevCompareToFile(actualCmdline, cmdlinefile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + virCommandSetDryRun(NULL, NULL, NULL); + return ret; +} static int testMdevctlParse(const void *data) @@ -328,6 +362,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) +#define DO_TEST_LIST_DEFINED() \ + DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) + #define DO_TEST_PARSE_JSON(filename) \ DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename) @@ -339,6 +376,8 @@ mymain(void) /* Test mdevctl stop command, pass an arbitrary uuid */ DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576"); + DO_TEST_LIST_DEFINED(); + DO_TEST_PARSE_JSON("mdevctl-list-multiple"); done: -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:04AM -0500, Jonathon Jongsma wrote:
This adds an internal API to query for persistent mediated devices that are defined by mdevctl. Upcoming commits will make use of this information.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 18 +++++++++ src/node_device/node_device_driver.h | 3 ++ .../mdevctl-list-defined.argv | 1 + tests/nodedevmdevctltest.c | 39 +++++++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-list-defined.argv
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index a646692870..6911fa36da 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -860,6 +860,24 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) }
+virCommand* +nodeDeviceGetMdevctlListCommand(bool defined, + char **output) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "list", + "--dumpjson", + NULL); + + if (defined) + virCommandAddArg(cmd, "--defined"); + + virCommandSetOutputBuffer(cmd, output);
Since we'll capture error output from all the other mdevctl commands, I think we should do it here as well, even though 99.9% of cases won't use it. The rest looks fine. Reviewed-by: Erik Skultety <eskultet@redhat.com>

Consistent with other objects (e.g. virDomainObj), add a field to indicate whether the node device is persistent or transient. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 16 ++++++++++++++++ src/conf/virnodedeviceobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 24 insertions(+) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ab663fd5a0..ce84e4d8c1 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -40,6 +40,7 @@ struct _virNodeDeviceObj { bool skipUpdateCaps; /* whether to skip checking host caps, used by testdriver */ bool active; + bool persistent; }; struct _virNodeDeviceObjList { @@ -1003,3 +1004,18 @@ virNodeDeviceObjSetActive(virNodeDeviceObj *obj, { obj->active = active; } + + +bool +virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj) +{ + return obj->persistent; +} + + +void +virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, + bool persistent) +{ + obj->persistent = persistent; +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index e786a70f51..7f682b9dca 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -128,3 +128,9 @@ virNodeDeviceObjIsActive(virNodeDeviceObj *obj); void virNodeDeviceObjSetActive(virNodeDeviceObj *obj, bool active); +bool +virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); + +void +virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, + bool persistent); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 09957c943b..047314ec19 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1273,6 +1273,7 @@ virNetworkPortDefSaveStatus; virNodeDeviceObjEndAPI; virNodeDeviceObjGetDef; virNodeDeviceObjIsActive; +virNodeDeviceObjIsPersistent; virNodeDeviceObjListAssignDef; virNodeDeviceObjListExport; virNodeDeviceObjListFindByName; @@ -1286,6 +1287,7 @@ virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; virNodeDeviceObjSetActive; +virNodeDeviceObjSetPersistent; # conf/virnwfilterbindingdef.h -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:05AM -0500, Jonathon Jongsma wrote:
Consistent with other objects (e.g. virDomainObj), add a field to indicate whether the node device is persistent or transient.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

Since a mediated device can be persistently defined by the mdevctl backend, we need additional lifecycle events beyond CREATED/DELETED to indicate that e.g. the device has been stopped but the device definition still exists. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- examples/c/misc/event-test.c | 4 ++++ include/libvirt/libvirt-nodedev.h | 2 ++ tools/virsh-nodedev.c | 4 +++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c index 76d4f3f6e8..10c707e66b 100644 --- a/examples/c/misc/event-test.c +++ b/examples/c/misc/event-test.c @@ -381,6 +381,10 @@ nodeDeviceEventToString(int event) return "Created"; case VIR_NODE_DEVICE_EVENT_DELETED: return "Deleted"; + case VIR_NODE_DEVICE_EVENT_DEFINED: + return "Defined"; + case VIR_NODE_DEVICE_EVENT_UNDEFINED: + return "Undefined"; case VIR_NODE_DEVICE_EVENT_LAST: break; } diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 2deead0791..77d814935e 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -196,6 +196,8 @@ int virConnectNodeDeviceEventDeregisterAny(virConnectPtr conn, typedef enum { VIR_NODE_DEVICE_EVENT_CREATED = 0, VIR_NODE_DEVICE_EVENT_DELETED = 1, + VIR_NODE_DEVICE_EVENT_DEFINED = 2, + VIR_NODE_DEVICE_EVENT_UNDEFINED = 3, # ifdef VIR_ENUM_SENTINELS VIR_NODE_DEVICE_EVENT_LAST diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index fedc8497f8..b9fe9b8be1 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -775,7 +775,9 @@ VIR_ENUM_DECL(virshNodeDeviceEvent); VIR_ENUM_IMPL(virshNodeDeviceEvent, VIR_NODE_DEVICE_EVENT_LAST, N_("Created"), - N_("Deleted")); + N_("Deleted"), + N_("Defined"), + N_("Undefined")); static const char * virshNodeDeviceEventToString(int event) -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:06AM -0500, Jonathon Jongsma wrote:
Since a mediated device can be persistently defined by the mdevctl backend, we need additional lifecycle events beyond CREATED/DELETED to indicate that e.g. the device has been stopped but the device definition still exists.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

At startup, query devices that are defined by 'mdevctl' and add them to the node device list. This adds a complication: we now have two potential sources of information for a node device: - udev for all devices and for activated mediated devices - mdevctl for persistent mediated devices Unfortunately, neither backend returns full information for a mediated device. For example, if a persistent mediated device in the list (with information provided from mdevctl) is 'started', that same device will now be detected by udev. If we simply overwrite the existing device definition with the new one provided by the udev backend, we will lose extra information that was provided by mdevctl (e.g. attributes, etc). To avoid this, make sure to copy the extra information into the new device definition. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 153 +++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 6 ++ src/node_device/node_device_udev.c | 31 +++++- 3 files changed, 185 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 6911fa36da..efa524f317 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1164,3 +1164,156 @@ nodeDeviceGenerateName(virNodeDeviceDef *def, *(def->name + i) = '_'; } } + + +static int +virMdevctlListDefined(virNodeDeviceDef ***devs) +{ + int status; + g_autofree char *output = NULL; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlListCommand(true, &output); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + if (!output) + return -1; + + return nodeDeviceParseMdevctlJSON(output, devs); +} + + +int +nodeDeviceUpdateMediatedDevices(void) +{ + g_autofree virNodeDeviceDef **defs = NULL; + int ndefs; + size_t i; + + if ((ndefs = virMdevctlListDefined(&defs)) < 0) { + virReportSystemError(errno, "%s", + _("failed to query mdevs from mdevctl")); + return -1; + } + + for (i = 0; i < ndefs; i++) { + virNodeDeviceObj *obj; + virObjectEvent *event; + g_autoptr(virNodeDeviceDef) def = defs[i]; + g_autofree char *name = g_strdup(def->name); + bool defined = false; + + def->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + virNodeDeviceDef *d = g_steal_pointer(&def); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, def); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + continue; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + } + + return 0; +} + + +/* returns true if any attributes were copied, else returns false */ +static bool +virMediatedDeviceAttrsCopy(virNodeDevCapMdev *dst, + virNodeDevCapMdev *src) +{ + bool ret = false; + size_t i; + + if (src->nattributes != dst->nattributes) { + ret = true; + for (i = 0; i < dst->nattributes; i++) + virMediatedDeviceAttrFree(dst->attributes[i]); + g_free(dst->attributes); + + dst->nattributes = src->nattributes; + dst->attributes = g_new0(virMediatedDeviceAttr*, + src->nattributes); + for (i = 0; i < dst->nattributes; i++) + dst->attributes[i] = virMediatedDeviceAttrNew(); + } + + for (i = 0; i < src->nattributes; i++) { + if (STRNEQ_NULLABLE(src->attributes[i]->name, + dst->attributes[i]->name)) { + ret = true; + g_free(dst->attributes[i]->name); + dst->attributes[i]->name = + g_strdup(src->attributes[i]->name); + } + if (STRNEQ_NULLABLE(src->attributes[i]->value, + dst->attributes[i]->value)) { + ret = true; + g_free(dst->attributes[i]->value); + dst->attributes[i]->value = + g_strdup(src->attributes[i]->value); + } + } + + return ret; +} + + +/* A mediated device definitions from mdevctl contains additional info that is + * not available from udev. Transfer this data to the new definition. + * Returns true if anything was copied, else returns false */ +bool +nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, + virNodeDeviceDef *src) +{ + bool ret = false; + virNodeDevCapMdev *srcmdev = &src->caps->data.mdev; + virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev; + + if (STRNEQ_NULLABLE(dstmdev->type, srcmdev->type)) { + ret = true; + g_free(dstmdev->type); + dstmdev->type = g_strdup(srcmdev->type); + } + + if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) { + ret = true; + g_free(dstmdev->uuid); + dstmdev->uuid = g_strdup(srcmdev->uuid); + } + + if (virMediatedDeviceAttrsCopy(dstmdev, srcmdev)) + ret = true; + + return ret; +} diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 705d83f827..9b54efe4e3 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -128,8 +128,14 @@ int nodeDeviceParseMdevctlJSON(const char *jsonstring, virNodeDeviceDef ***devs); +int +nodeDeviceUpdateMediatedDevices(void); + void nodeDeviceGenerateName(virNodeDeviceDef *def, const char *subsystem, const char *sysname, const char *s); + +bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, + virNodeDeviceDef *src); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index eae301cc95..38ebe7b5c5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1421,9 +1421,17 @@ udevRemoveOneDeviceSysPath(const char *path) VIR_NODE_DEVICE_EVENT_DELETED, 0); - VIR_DEBUG("Removing device '%s' with sysfs path '%s'", - def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + /* If the device is a mediated device that has been 'stopped', it may still + * be defined by mdevctl and can therefore be started again. Don't drop it + * from the list of node devices */ + if (virNodeDeviceObjIsPersistent(obj)) { + VIR_FREE(def->sysfs_path); + virNodeDeviceObjSetActive(obj, false); + } else { + VIR_DEBUG("Removing device '%s' with sysfs path '%s'", + def->name, path); + virNodeDeviceObjListRemove(driver->devs, obj); + } virNodeDeviceObjEndAPI(&obj); virObjectEventStateQueue(driver->nodeDeviceEventState, event); @@ -1481,7 +1489,6 @@ udevSetParent(struct udev_device *device, return 0; } - static int udevAddOneDevice(struct udev_device *device) { @@ -1491,6 +1498,7 @@ udevAddOneDevice(struct udev_device *device) virObjectEventPtr event = NULL; bool new_device = true; int ret = -1; + bool was_persistent = false; def = g_new0(virNodeDeviceDef, 1); @@ -1514,14 +1522,23 @@ udevAddOneDevice(struct udev_device *device) goto cleanup; if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + objdef = virNodeDeviceObjGetDef(obj); + + if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV) + nodeDeviceDefCopyFromMdevctl(def, objdef); + was_persistent = virNodeDeviceObjIsPersistent(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); - new_device = false; } /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + virNodeDeviceObjSetPersistent(obj, was_persistent); objdef = virNodeDeviceObjGetDef(obj); if (new_device) @@ -1941,6 +1958,10 @@ nodeStateInitializeEnumerate(void *opaque) /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) goto error; + /* Load persistent mdevs (which might not be activated yet) and additional + * information about active mediated devices from mdevctl */ + if (nodeDeviceUpdateMediatedDevices() != 0) + goto error; nodeDeviceLock(); driver->initialized = true; -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:07AM -0500, Jonathon Jongsma wrote:
At startup, query devices that are defined by 'mdevctl' and add them to the node device list.
This adds a complication: we now have two potential sources of information for a node device: - udev for all devices and for activated mediated devices - mdevctl for persistent mediated devices
Unfortunately, neither backend returns full information for a mediated device. For example, if a persistent mediated device in the list (with information provided from mdevctl) is 'started', that same device will now be detected by udev. If we simply overwrite the existing device definition with the new one provided by the udev backend, we will lose extra information that was provided by mdevctl (e.g. attributes, etc). To avoid this, make sure to copy the extra information into the new device definition.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

When a mediated device is stopped or undefined by an application outside of libvirt, we need to remove it from our list of node devices within libvirt. This patch introduces virNodeDeviceObjListRemoveLocked() and virNodeDeviceObjListForEachRemove() (which are analogous to other types of object lists in libvirt) to facilitate that. They will be used in coming commits. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 58 ++++++++++++++++++++++++++++++++++--- src/conf/virnodedeviceobj.h | 11 +++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ce84e4d8c1..97e7d7ab11 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -507,23 +507,29 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - virNodeDeviceDefPtr def; - if (!obj) return; - def = obj->def; virObjectRef(obj); virObjectUnlock(obj); virObjectRWLockWrite(devs); virObjectLock(obj); - virHashRemoveEntry(devs->objs, def->name); + virNodeDeviceObjListRemoveLocked(devs, obj); virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(devs); } +/* The caller must hold lock on 'devs' */ +void +virNodeDeviceObjListRemoveLocked(virNodeDeviceObjList *devs, + virNodeDeviceObj *dev) +{ + virHashRemoveEntry(devs->objs, dev->def->name); +} + + /* * Return the NPIV dev's parent device name */ @@ -1019,3 +1025,47 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, { obj->persistent = persistent; } + + +struct virNodeDeviceObjListRemoveData +{ + virNodeDeviceObjListRemoveIter iter; + void *opaque; +}; + +static int virNodeDeviceObjListRemoveCb(void *key G_GNUC_UNUSED, + void *value, + void *opaque) +{ + struct virNodeDeviceObjListRemoveData *data = opaque; + + return data->iter(value, data->opaque); +} + + +/** + * virNodeDeviceObjListForEachRemove + * @devs: Pointer to object list + * @iter: function to call for each device object + * @opaque: Opaque data to use as argument to helper + * + * For each object in @devs, call the @iter helper using @opaque as + * an argument. If @iter returns true, that item will be removed from the + * object list. + */ +void +virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, + virNodeDeviceObjListRemoveIter iter, + void *opaque) +{ + struct virNodeDeviceObjListRemoveData data = { + .iter = iter, + .opaque = opaque + }; + + virObjectRWLockWrite(devs); + g_hash_table_foreach_remove(devs->objs, + virNodeDeviceObjListRemoveCb, + &data); + virObjectRWUnlock(devs); +} diff --git a/src/conf/virnodedeviceobj.h b/src/conf/virnodedeviceobj.h index 7f682b9dca..249115cf68 100644 --- a/src/conf/virnodedeviceobj.h +++ b/src/conf/virnodedeviceobj.h @@ -80,6 +80,10 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr dev); +void +virNodeDeviceObjListRemoveLocked(virNodeDeviceObjList *devs, + virNodeDeviceObj *dev); + int virNodeDeviceObjListGetParentHost(virNodeDeviceObjListPtr devs, virNodeDeviceDefPtr def); @@ -134,3 +138,10 @@ virNodeDeviceObjIsPersistent(virNodeDeviceObj *obj); void virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, bool persistent); + +typedef bool (*virNodeDeviceObjListRemoveIter)(virNodeDeviceObj *obj, + const void *opaque); + +void virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, + virNodeDeviceObjListRemoveIter iter, + void *opaque); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 047314ec19..f36400b5f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1280,12 +1280,14 @@ virNodeDeviceObjListFindByName; virNodeDeviceObjListFindBySysfsPath; virNodeDeviceObjListFindMediatedDeviceByUUID; virNodeDeviceObjListFindSCSIHostByWWNs; +virNodeDeviceObjListForEachRemove; virNodeDeviceObjListFree; virNodeDeviceObjListGetNames; virNodeDeviceObjListGetParentHost; virNodeDeviceObjListNew; virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; +virNodeDeviceObjListRemoveLocked; virNodeDeviceObjSetActive; virNodeDeviceObjSetPersistent; -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:08AM -0500, Jonathon Jongsma wrote:
When a mediated device is stopped or undefined by an application outside of libvirt, we need to remove it from our list of node devices within libvirt. This patch introduces virNodeDeviceObjListRemoveLocked() and virNodeDeviceObjListForEachRemove() (which are analogous to other types of object lists in libvirt) to facilitate that. They will be used in coming commits.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/conf/virnodedeviceobj.c | 58 ++++++++++++++++++++++++++++++++++--- src/conf/virnodedeviceobj.h | 11 +++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ce84e4d8c1..97e7d7ab11 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -507,23 +507,29 @@ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) { - virNodeDeviceDefPtr def; - if (!obj) return; - def = obj->def;
virObjectRef(obj); virObjectUnlock(obj); virObjectRWLockWrite(devs); virObjectLock(obj); - virHashRemoveEntry(devs->objs, def->name); + virNodeDeviceObjListRemoveLocked(devs, obj); virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(devs); }
+/* The caller must hold lock on 'devs' */ +void +virNodeDeviceObjListRemoveLocked(virNodeDeviceObjList *devs, + virNodeDeviceObj *dev) +{ + virHashRemoveEntry(devs->objs, dev->def->name); +} + + /* * Return the NPIV dev's parent device name */ @@ -1019,3 +1025,47 @@ virNodeDeviceObjSetPersistent(virNodeDeviceObj *obj, { obj->persistent = persistent; } + + +struct virNodeDeviceObjListRemoveData
Looking at other libvirt code, the precedent seems to be for this be named xyzListRemoveHelperData
+{ + virNodeDeviceObjListRemoveIter iter;
and ^this one should IMO be named xyzListRemoveIterator callback;
+ void *opaque; +}; + +static int virNodeDeviceObjListRemoveCb(void *key G_GNUC_UNUSED, + void *value, + void *opaque) +{ + struct virNodeDeviceObjListRemoveData *data = opaque; + + return data->iter(value, data->opaque);
data->callback
+} + + +/** + * virNodeDeviceObjListForEachRemove + * @devs: Pointer to object list + * @iter: function to call for each device object
s/iter/callback (it sounds odd that we're iterating over something and call another iterator over each item --> hence "callback")
+ * @opaque: Opaque data to use as argument to helper + * + * For each object in @devs, call the @iter helper using @opaque as + * an argument. If @iter returns true, that item will be removed from the + * object list. + */ +void +virNodeDeviceObjListForEachRemove(virNodeDeviceObjList *devs, + virNodeDeviceObjListRemoveIter iter, + void *opaque) +{ + struct virNodeDeviceObjListRemoveData data = { + .iter = iter, + .opaque = opaque + }; + + virObjectRWLockWrite(devs); + g_hash_table_foreach_remove(devs->objs, + virNodeDeviceObjListRemoveCb,
xyzListRemoveHelper... Reviewed-by: Erik Skultety <eskultet@redhat.com>

mdevctl does not currently provide any events when the list of defined devices changes, so we will need to poll mdevctl for the list of defined devices periodically. When a mediated device no longer exists from one iteration to the next, we need to treat it as an "undefine" event. When we get such an event, we remove the device from the list if it's not active. Otherwise, we simply mark it as non-persistent. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 67 ++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index efa524f317..4be650ddef 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1183,20 +1183,81 @@ virMdevctlListDefined(virNodeDeviceDef ***devs) } +typedef struct _virMdevctlForEachData virMdevctlForEachData; +struct _virMdevctlForEachData { + int ndefs; + virNodeDeviceDef **defs; +}; + + +/* This function keeps the list of persistent mediated devices consistent + * between the nodedev driver and mdevctl. + * @obj is a device that is currently known by the nodedev driver, and @opaque + * contains the most recent list of devices defined by mdevctl. If @obj is no + * longer defined in mdevctl, mark it as undefined and possibly remove it from + * the driver as well. Returning 'true' from this function indicates that the + * device should be removed from the nodedev driver list. */ +static bool +removeMissingPersistentMdevs(virNodeDeviceObj *obj, + const void *opaque) +{ + bool remove = false; + const virMdevctlForEachData *data = opaque; + size_t i; + virNodeDeviceDef *def = virNodeDeviceObjGetDef(obj); + virObjectEvent *event; + + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return false; + + /* transient mdevs are populated via udev, so don't remove them from the + * nodedev driver just because they are not reported by by mdevctl */ + if (!virNodeDeviceObjIsPersistent(obj)) + return false; + + for (i = 0; i < data->ndefs; i++) { + /* OK, this mdev is still defined by mdevctl */ + if (STREQ(data->defs[i]->name, def->name)) + return false; + } + + event = virNodeDeviceEventLifecycleNew(def->name, + VIR_NODE_DEVICE_EVENT_UNDEFINED, + 0); + + /* The device is active, but no longer defined by mdevctl. Keep the device + * in the list, but mark it as non-persistent */ + if (virNodeDeviceObjIsActive(obj)) + virNodeDeviceObjSetPersistent(obj, false); + else + remove = true; + + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return remove; +} + + int nodeDeviceUpdateMediatedDevices(void) { g_autofree virNodeDeviceDef **defs = NULL; - int ndefs; + virMdevctlForEachData data = { 0, }; size_t i; - if ((ndefs = virMdevctlListDefined(&defs)) < 0) { + if ((data.ndefs = virMdevctlListDefined(&defs)) < 0) { virReportSystemError(errno, "%s", _("failed to query mdevs from mdevctl")); return -1; } - for (i = 0; i < ndefs; i++) { + /* Any mdevs that were previously defined but were not returned in the + * latest mdevctl query should be removed from the device list */ + data.defs = defs; + virNodeDeviceObjListForEachRemove(driver->devs, + removeMissingPersistentMdevs, &data); + + for (i = 0; i < data.ndefs; i++) { virNodeDeviceObj *obj; virObjectEvent *event; g_autoptr(virNodeDeviceDef) def = defs[i]; -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:09AM -0500, Jonathon Jongsma wrote:
mdevctl does not currently provide any events when the list of defined devices changes, so we will need to poll mdevctl for the list of defined devices periodically. When a mediated device no longer exists from one iteration to the next, we need to treat it as an "undefine" event.
When we get such an event, we remove the device from the list if it's not active. Otherwise, we simply mark it as non-persistent.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 67 ++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index efa524f317..4be650ddef 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1183,20 +1183,81 @@ virMdevctlListDefined(virNodeDeviceDef ***devs) }
+typedef struct _virMdevctlForEachData virMdevctlForEachData; +struct _virMdevctlForEachData { + int ndefs; + virNodeDeviceDef **defs; +}; + + +/* This function keeps the list of persistent mediated devices consistent + * between the nodedev driver and mdevctl. + * @obj is a device that is currently known by the nodedev driver, and @opaque + * contains the most recent list of devices defined by mdevctl. If @obj is no + * longer defined in mdevctl, mark it as undefined and possibly remove it from + * the driver as well. Returning 'true' from this function indicates that the + * device should be removed from the nodedev driver list. */ +static bool +removeMissingPersistentMdevs(virNodeDeviceObj *obj,
Since this handles a single device at a time --> removeMissingPersistentMdev Reviewed-by: Erik Skultety <eskultet@redhat.com>

We need to query mdevctl for changes to device definitions since an administrator can define new devices by executing mdevctl outside of libvirt. In the future, mdevctl may add a way to signal device add/remove via events, but for now we resort to a bit of a workaround: monitoring the mdevctl config directory for changes to files. When a change is detected, we query mdevctl and update our device list. The mdevctl querying is handled in a throwaway thread, and these threads are synchronized with a mutex. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_udev.c | 161 +++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 38ebe7b5c5..78144b7762 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -19,6 +19,7 @@ */ #include <config.h> +#include <gio/gio.h> #include <libudev.h> #include <pciaccess.h> #include <scsi/scsi.h> @@ -69,6 +70,10 @@ struct _udevEventData { /* init thread */ virThread initThread; + + GList *mdevctlMonitors; + virMutex mdevctlLock; + int mdevctlTimeout; }; static virClassPtr udevEventDataClass; @@ -89,6 +94,11 @@ udevEventDataDispose(void *obj) udev_monitor_unref(priv->udev_monitor); udev_unref(udev); + virMutexLock(&priv->mdevctlLock); + g_list_free_full(priv->mdevctlMonitors, g_object_unref); + virMutexUnlock(&priv->mdevctlLock); + virMutexDestroy(&priv->mdevctlLock); + virCondDestroy(&priv->threadCond); } @@ -120,6 +130,11 @@ udevEventDataNew(void) return NULL; } + if (virMutexInit(&ret->mdevctlLock) < 0) { + virObjectUnref(ret); + return NULL; + } + ret->watch = -1; return ret; } @@ -2004,6 +2019,137 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) } +static void +mdevctlHandlerThread(void *opaque G_GNUC_UNUSED) +{ + udevEventData *priv = driver->privateData; + + /* ensure only a single thread can query mdevctl at a time */ + virMutexLock(&priv->mdevctlLock); + if (nodeDeviceUpdateMediatedDevices() < 0) + VIR_WARN("mdevctl failed to updated mediated devices"); + virMutexUnlock(&priv->mdevctlLock); +} + + +static void +scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque) +{ + udevEventData *priv = opaque; + virThread thread; + + if (priv->mdevctlTimeout > 0) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (virThreadCreateFull(&thread, false, mdevctlHandlerThread, + "mdevctl-thread", false, NULL) < 0) { + virReportSystemError(errno, "%s", + _("failed to create mdevctl thread")); + } +} + + +static void +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, + GFile *file, + GFile *other_file G_GNUC_UNUSED, + GFileMonitorEvent event_type, + gpointer user_data); + + +/* Recursively monitors a directory and its subdirectories for file changes and + * returns a GList of GFileMonitor objects */ +static GList* +monitorFileRecursively(udevEventData *udev, + GFile *file) +{ + GList *monitors = NULL; + g_autoptr(GError) error = NULL; + g_autoptr(GFileEnumerator) children = NULL; + GFileMonitor *mon; + + if (!(children = g_file_enumerate_children(file, "standard::*", + G_FILE_QUERY_INFO_NONE, NULL, &error))) + goto error; + + if (!(mon = g_file_monitor(file, G_FILE_MONITOR_NONE, NULL, &error))) + goto error; + + g_signal_connect(mon, "changed", + G_CALLBACK(mdevctlEventHandleCallback), udev); + + monitors = g_list_append(monitors, mon); + + while (true) { + GFileInfo *info = NULL; + GFile *child = NULL; + GList *child_monitors = NULL; + + if (!g_file_enumerator_iterate(children, &info, &child, NULL, &error)) + goto error; + + if (!info) + break; + + if (g_file_query_file_type(child, G_FILE_QUERY_INFO_NONE, NULL) == + G_FILE_TYPE_DIRECTORY) { + + child_monitors = monitorFileRecursively(udev, child); + if (child_monitors) + monitors = g_list_concat(monitors, child_monitors); + } + } + + return monitors; + + error: + g_list_free_full(monitors, g_object_unref); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to monitor directory: %s"), error->message); + g_clear_error(&error); + return NULL; +} + + +static void +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, + GFile *file, + GFile *other_file G_GNUC_UNUSED, + GFileMonitorEvent event_type, + gpointer user_data) +{ + udevEventData *priv = user_data; + /* if a new directory appears, monitor that directory for changes */ + if (event_type == G_FILE_MONITOR_EVENT_CREATED && + g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) == + G_FILE_TYPE_DIRECTORY) { + GList *newmonitors = monitorFileRecursively(priv, file); + virMutexLock(&priv->mdevctlLock); + priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, newmonitors); + virMutexUnlock(&priv->mdevctlLock); + } + + /* When mdevctl creates a device, it can result in multiple notify events + * emitted for a single logical change (e.g. several CHANGED events, or a + * CREATED and CHANGED event followed by CHANGES_DONE_HINT). To avoid + * spawning a mdevctl thread multiple times for a single logical + * configuration change, try to coalesce these changes by waiting for the + * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the + * signal if that event never comes */ + if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + if (priv->mdevctlTimeout > 0) + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = virEventAddTimeout(100, scheduleMdevctlHandler, + priv, NULL); + return; + } + + scheduleMdevctlHandler(-1, priv); +} + + static int nodeStateInitialize(bool privileged, const char *root, @@ -2012,6 +2158,7 @@ nodeStateInitialize(bool privileged, { udevEventDataPtr priv = NULL; struct udev *udev = NULL; + g_autoptr(GFile) mdevctlConfigDir = g_file_new_for_path("/etc/mdevctl.d"); if (root != NULL) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2113,6 +2260,20 @@ nodeStateInitialize(bool privileged, if (priv->watch == -1) goto unlock; + /* mdevctl may add notification events in the future: + * https://github.com/mdevctl/mdevctl/issues/27. For now, fall back to + * monitoring the mdevctl configuration directory for changes. + * mdevctl configuration is stored in a directory tree within + * /etc/mdevctl.d/. There is a directory for each parent device, which + * contains a file defining each mediated device */ + virMutexLock(&priv->mdevctlLock); + if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, + mdevctlConfigDir))) { + virMutexUnlock(&priv->mdevctlLock); + goto cleanup; + } + virMutexUnlock(&priv->mdevctlLock); + virObjectUnlock(priv); /* Create a fictional 'computer' device to root the device tree. */ -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:10AM -0500, Jonathon Jongsma wrote:
We need to query mdevctl for changes to device definitions since an administrator can define new devices by executing mdevctl outside of libvirt.
In the future, mdevctl may add a way to signal device add/remove via events, but for now we resort to a bit of a workaround: monitoring the mdevctl config directory for changes to files. When a change is detected, we query mdevctl and update our device list. The mdevctl querying is handled in a throwaway thread, and these threads are synchronized with a mutex.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- ...
+static void +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, + GFile *file, + GFile *other_file G_GNUC_UNUSED, + GFileMonitorEvent event_type, + gpointer user_data) +{ + udevEventData *priv = user_data; + /* if a new directory appears, monitor that directory for changes */ + if (event_type == G_FILE_MONITOR_EVENT_CREATED && + g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL) == + G_FILE_TYPE_DIRECTORY) {
^probably better written as: if (event_type == G_FILE_MONITOR_EVENT_CREATED) { file_type = g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL); if (file_type == G_FILE_TYPE_DIRECTORY) { ... } }
+ GList *newmonitors = monitorFileRecursively(priv, file);
newline here for better optical code separation
+ virMutexLock(&priv->mdevctlLock); + priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, newmonitors); + virMutexUnlock(&priv->mdevctlLock); + } + + /* When mdevctl creates a device, it can result in multiple notify events + * emitted for a single logical change (e.g. several CHANGED events, or a + * CREATED and CHANGED event followed by CHANGES_DONE_HINT). To avoid + * spawning a mdevctl thread multiple times for a single logical + * configuration change, try to coalesce these changes by waiting for the + * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the + * signal if that event never comes */ + if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + if (priv->mdevctlTimeout > 0) + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = virEventAddTimeout(100, scheduleMdevctlHandler, + priv, NULL); + return; + } + + scheduleMdevctlHandler(-1, priv); +}
Reviewed-by: Erik Skultety <eskultet@redhat.com>

Abstract out the function used to generate the commandline for 'mdevctl start' since they take the same arguments. Add tests to ensure that we're generating the command properly. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 34 +++++++-- src/node_device/node_device_driver.h | 6 ++ ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 1 + ...19_36ea_4111_8f0a_8c9a70e21366-define.json | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 1 + ...39_495e_4243_ad9f_beb3f14c23d9-define.json | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.argv | 1 + ...16_1ca8_49ac_b176_871d16c13076-define.json | 1 + tests/nodedevmdevctltest.c | 76 +++++++++++++------ 9 files changed, 95 insertions(+), 27 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 4be650ddef..abd45a6eab 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -698,10 +698,14 @@ nodeDeviceFindAddressByName(const char *name) } -virCommandPtr -nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, - char **uuid_out, - char **errmsg) +/* the mdevctl 'start' and 'define' commands accept almost the exact same + * arguments, so provide a common implementation that can be wrapped by a more + * specific function */ +static virCommand* +nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def, + const char *subcommand, + char **uuid_out, + char **errmsg) { virCommandPtr cmd; g_autofree char *json = NULL; @@ -719,7 +723,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return NULL; } - cmd = virCommandNewArgList(MDEVCTL, "start", + cmd = virCommandNewArgList(MDEVCTL, subcommand, "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL); @@ -731,6 +735,26 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, return cmd; } +virCommand* +nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def, + char **uuid_out, + char **errmsg) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out, + errmsg); +} + +virCommand* +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def, + char **uuid_out, + char **errmsg) +{ + return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out, + errmsg); +} + + + static int virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) { diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 9b54efe4e3..b319990f0f 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -117,6 +117,12 @@ virCommandPtr nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, char **uuid_out, char **errmsg); + +virCommand* +nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def, + char **uuid_out, + char **errmsg); + virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json new file mode 100644 index 0000000000..bfc6dcace3 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual"} \ No newline at end of file diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json new file mode 100644 index 0000000000..e5b22b2c44 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute-1":"attribute-value-1"},{"example-attribute-2":"attribute-value-2"}]} \ No newline at end of file diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv new file mode 100644 index 0000000000..773e98b963 --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json new file mode 100644 index 0000000000..2e03d0bd8e --- /dev/null +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.json @@ -0,0 +1 @@ +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute":"attribute-value"}]} \ No newline at end of file diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 1ab4776a23..99fa328d24 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -10,10 +10,16 @@ #define VIR_FROM_THIS VIR_FROM_NODEDEV +typedef enum { + MDEVCTL_CMD_START, + MDEVCTL_CMD_DEFINE, +} MdevctlCmd; + struct startTestInfo { const char *virt_type; int create; const char *filename; + MdevctlCmd command; }; /* capture stdin passed to command */ @@ -45,12 +51,17 @@ nodedevCompareToFile(const char *actual, return virTestCompareToFile(replacedCmdline, filename); } + +typedef virCommand* (*MdevctlCmdFunc)(virNodeDeviceDef *, char **, char **); + + static int -testMdevctlStart(const char *virt_type, - int create, - const char *mdevxml, - const char *startcmdfile, - const char *startjsonfile) +testMdevctlStartOrDefine(const char *virt_type, + int create, + MdevctlCmdFunc mdevctl_cmd_func, + const char *mdevxml, + const char *cmdfile, + const char *jsonfile) { g_autoptr(virNodeDeviceDef) def = NULL; virNodeDeviceObjPtr obj = NULL; @@ -67,7 +78,7 @@ testMdevctlStart(const char *virt_type, /* this function will set a stdin buffer containing the json configuration * of the device. The json value is captured in the callback above */ - cmd = nodeDeviceGetMdevctlStartCommand(def, &uuid, &errmsg); + cmd = mdevctl_cmd_func(def, &uuid, &errmsg); if (!cmd) goto cleanup; @@ -79,10 +90,10 @@ testMdevctlStart(const char *virt_type, if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup; - if (nodedevCompareToFile(actualCmdline, startcmdfile) < 0) + if (nodedevCompareToFile(actualCmdline, cmdfile) < 0) goto cleanup; - if (virTestCompareToFile(stdinbuf, startjsonfile) < 0) + if (virTestCompareToFile(stdinbuf, jsonfile) < 0) goto cleanup; ret = 0; @@ -94,20 +105,34 @@ testMdevctlStart(const char *virt_type, } static int -testMdevctlStartHelper(const void *data) +testMdevctlStartOrDefineHelper(const void *data) { const struct startTestInfo *info = data; + const char *cmd; + MdevctlCmdFunc func; + g_autofree char *mdevxml = NULL; + g_autofree char *cmdlinefile = NULL; + g_autofree char *jsonfile = NULL; + + if (info->command == MDEVCTL_CMD_START) { + cmd = "start"; + func = nodeDeviceGetMdevctlStartCommand; + } else if (info->command == MDEVCTL_CMD_DEFINE) { + cmd = "define"; + func = nodeDeviceGetMdevctlDefineCommand; + } else { + return -1; + } - g_autofree char *mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", - abs_srcdir, info->filename); - g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.argv", - abs_srcdir, info->filename); - g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.json", - abs_srcdir, info->filename); + mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir, + info->filename); + cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.argv", + abs_srcdir, info->filename, cmd); + jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir, + info->filename, cmd); - return testMdevctlStart(info->virt_type, - info->create, mdevxml, cmdlinefile, - jsonfile); + return testMdevctlStartOrDefine(info->virt_type, info->create, func, + mdevxml, cmdlinefile, jsonfile); } static int @@ -349,15 +374,18 @@ mymain(void) if (virTestRun(desc, func, info) < 0) \ ret = -1; -#define DO_TEST_START_FULL(virt_type, create, filename) \ +#define DO_TEST_CMD(desc, virt_type, create, filename, command) \ do { \ - struct startTestInfo info = { virt_type, create, filename }; \ - DO_TEST_FULL("mdevctl start " filename, testMdevctlStartHelper, &info); \ + struct startTestInfo info = { virt_type, create, filename, command }; \ + DO_TEST_FULL(desc, testMdevctlStartOrDefineHelper, &info); \ } \ while (0) #define DO_TEST_START(filename) \ - DO_TEST_START_FULL("QEMU", CREATE_DEVICE, filename) + DO_TEST_CMD("mdevctl start " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_START) + +#define DO_TEST_DEFINE(filename) \ + DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) #define DO_TEST_STOP(uuid) \ DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) @@ -380,6 +408,10 @@ mymain(void) DO_TEST_PARSE_JSON("mdevctl-list-multiple"); + DO_TEST_DEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); + DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + done: nodedevTestDriverFree(driver); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:11AM -0500, Jonathon Jongsma wrote:
Abstract out the function used to generate the commandline for 'mdevctl start' since they take the same arguments. Add tests to ensure that we're generating the command properly.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

With mediated devices, we can now define persistent node devices that can be started and stopped. In order to take advantage of this, we need an API to define new node devices. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 6 +++ src/libvirt-nodedev.c | 42 ++++++++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 71 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 5 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++- src/remote_protocol-structs | 8 ++++ src/rpc/gendispatch.pl | 1 + 11 files changed, 156 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 77d814935e..33eb46b3cd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -131,6 +131,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn, int virNodeDeviceDestroy (virNodeDevicePtr dev); +virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index d0fc7f19cf..64a0a7c473 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -74,6 +74,11 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev); +typedef virNodeDevice* +(*virDrvNodeDeviceDefineXML)(virConnect *conn, + const char *xmlDesc, + unsigned int flags); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -113,4 +118,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceListCaps nodeDeviceListCaps; virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; + virDrvNodeDeviceDefineXML nodeDeviceDefineXML; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index fb707b570f..cfc0c9de5b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -737,6 +737,48 @@ virNodeDeviceDestroy(virNodeDevicePtr dev) } +/** + * virNodeDeviceDefineXML: + * @conn: pointer to the hypervisor connection + * @xmlDesc: string containing an XML description of the device to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Define a new device 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. + * + * Returns a node device object if successful, NULL in case of failure + */ +virNodeDevice* +virNodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, xmlDesc=%s, flags=0x%x", conn, NULLSTR(xmlDesc), flags); + + virResetLastError(); + + virCheckConnectReturn(conn, NULL); + virCheckReadOnlyGoto(conn->flags, error); + virCheckNonNullArgGoto(xmlDesc, error); + + if (conn->nodeDeviceDriver && + conn->nodeDeviceDriver->nodeDeviceDefineXML) { + virNodeDevice *dev = conn->nodeDeviceDriver->nodeDeviceDefineXML(conn, xmlDesc, flags); + if (dev == NULL) + goto error; + return dev; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return NULL; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 51a3d7265a..3d8176351c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -887,6 +887,7 @@ LIBVIRT_7.1.0 { LIBVIRT_7.2.0 { global: virDomainStartDirtyRateCalc; + virNodeDeviceDefineXML; } LIBVIRT_7.1.0; # .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index abd45a6eab..418faa9fb9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -776,6 +776,26 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) } +static int +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlDefineCommand(def, uuid, errmsg); + if (!cmd) + return -1; + + /* an auto-generated uuid is returned via stdout if no uuid is specified in + * the mdevctl args */ + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + /* remove newline */ + *uuid = g_strstrip(*uuid); + + return 0; +} + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) @@ -1112,6 +1132,57 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; } +virNodeDevice* +nodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virNodeDevice *device = NULL; + const char *virt_type = NULL; + g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; + + virCheckFlags(0, NULL); + + if (nodeDeviceWaitInit() < 0) + return NULL; + + virt_type = virConnectGetType(conn); + + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) + return NULL; + + if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0) + return NULL; + + if (!nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + return NULL; + } + + if (!def->parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot define a mediated device without a parent")); + return NULL; + } + + if (virMdevctlDefine(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to define mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + return NULL; + } + + def->caps->data.mdev.uuid = g_strdup(uuid); + mdevGenerateDeviceName(def); + device = nodeDeviceFindNewMediatedDevice(conn, uuid); + + return device; +} + + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b319990f0f..f626e9ac8a 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -102,6 +102,11 @@ nodeDeviceCreateXML(virConnectPtr conn, int nodeDeviceDestroy(virNodeDevicePtr dev); +virNodeDevice* +nodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 78144b7762..9de658cab5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2321,6 +2321,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceListCaps = nodeDeviceListCaps, /* 0.7.3 */ .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ + .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62ada08344..15c592b5b5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8696,6 +8696,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 7fdc65f029..a95ed65f12 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2145,6 +2145,15 @@ struct remote_node_device_destroy_args { remote_nonnull_string name; }; +struct remote_node_device_define_xml_args { + remote_nonnull_string xml_desc; + unsigned int flags; +}; + +struct remote_node_device_define_xml_ret { + remote_nonnull_node_device dev; +}; + /* * Events Register/Deregister: @@ -6745,5 +6754,11 @@ enum remote_procedure { * @generate: both * @acl: domain:read */ - REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427 + REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, + + /** + * @generate: both + * @acl: node_device:write + */ + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index d13dc81a82..3488659da1 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1600,6 +1600,13 @@ struct remote_node_device_create_xml_ret { struct remote_node_device_destroy_args { remote_nonnull_string name; }; +struct remote_node_device_define_xml_args { + remote_nonnull_string xml_desc; + u_int flags; +}; +struct remote_node_device_define_xml_ret { + remote_nonnull_node_device dev; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3605,4 +3612,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, REMOTE_PROC_DOMAIN_GET_MESSAGES = 426, REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 9dcd8c3e89..9f5bf0e316 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -567,6 +567,7 @@ elsif ($mode eq "server") { if ($argtype =~ m/^remote_node_device_/ and !($argtype =~ m/^remote_node_device_lookup_by_name_/) and !($argtype =~ m/^remote_node_device_create_xml_/) and + !($argtype =~ m/^remote_node_device_define_xml_/) and !($argtype =~ m/^remote_node_device_lookup_scsi_host_by_wwn_/)) { $has_node_device = 1; push(@vars_list, "virNodeDevicePtr dev = NULL"); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:12AM -0500, Jonathon Jongsma wrote:
With mediated devices, we can now define persistent node devices that can be started and stopped. In order to take advantage of this, we need an API to define new node devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 6 +++ src/libvirt-nodedev.c | 42 ++++++++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 71 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 5 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++- src/remote_protocol-structs | 8 ++++ src/rpc/gendispatch.pl | 1 + 11 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 77d814935e..33eb46b3cd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -131,6 +131,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn,
int virNodeDeviceDestroy (virNodeDevicePtr dev);
+virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index d0fc7f19cf..64a0a7c473 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -74,6 +74,11 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
+typedef virNodeDevice* +(*virDrvNodeDeviceDefineXML)(virConnect *conn, + const char *xmlDesc, + unsigned int flags); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -113,4 +118,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceListCaps nodeDeviceListCaps; virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; + virDrvNodeDeviceDefineXML nodeDeviceDefineXML; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index fb707b570f..cfc0c9de5b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -737,6 +737,48 @@ virNodeDeviceDestroy(virNodeDevicePtr dev) }
+/** + * virNodeDeviceDefineXML: + * @conn: pointer to the hypervisor connection + * @xmlDesc: string containing an XML description of the device to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Define a new device 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. + * + * Returns a node device object if successful, NULL in case of failure + */ +virNodeDevice* +virNodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, xmlDesc=%s, flags=0x%x", conn, NULLSTR(xmlDesc), flags); + + virResetLastError(); + + virCheckConnectReturn(conn, NULL); + virCheckReadOnlyGoto(conn->flags, error); + virCheckNonNullArgGoto(xmlDesc, error); + + if (conn->nodeDeviceDriver && + conn->nodeDeviceDriver->nodeDeviceDefineXML) { + virNodeDevice *dev = conn->nodeDeviceDriver->nodeDeviceDefineXML(conn, xmlDesc, flags); + if (dev == NULL)
if (!dev) goto error;
+ goto error; + return dev; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return NULL; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 51a3d7265a..3d8176351c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -887,6 +887,7 @@ LIBVIRT_7.1.0 { LIBVIRT_7.2.0 { global: virDomainStartDirtyRateCalc; + virNodeDeviceDefineXML;
Obvious note: this will have to be bumped 1 last time :).
} LIBVIRT_7.1.0;
# .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index abd45a6eab..418faa9fb9 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -776,6 +776,26 @@ virMdevctlStart(virNodeDeviceDefPtr def, char **uuid, char **errmsg) }
+static int +virMdevctlDefine(virNodeDeviceDefPtr def, char **uuid, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlDefineCommand(def, uuid, errmsg);
newline here^
+ if (!cmd) + return -1; + + /* an auto-generated uuid is returned via stdout if no uuid is specified in + * the mdevctl args */ + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + /* remove newline */ + *uuid = g_strstrip(*uuid); + + return 0; +} + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDefPtr def) @@ -1112,6 +1132,57 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; }
+virNodeDevice* +nodeDeviceDefineXML(virConnect *conn, + const char *xmlDesc, + unsigned int flags) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virNodeDevice *device = NULL; + const char *virt_type = NULL; + g_autofree char *uuid = NULL; + g_autofree char *errmsg = NULL; + + virCheckFlags(0, NULL); + + if (nodeDeviceWaitInit() < 0) + return NULL; + + virt_type = virConnectGetType(conn); + + if (!(def = virNodeDeviceDefParseString(xmlDesc, CREATE_DEVICE, virt_type))) + return NULL; + + if (virNodeDeviceDefineXMLEnsureACL(conn, def) < 0) + return NULL; + + if (!nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + return NULL; + } + + if (!def->parent) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot define a mediated device without a parent")); + return NULL; + } + + if (virMdevctlDefine(def, &uuid, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to define mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error");
My comment from 1/30 would apply ^here too. Reviewed-by: Erik Skultety <eskultet@redhat.com>

On Fri, Mar 26, 2021 at 11:48:12AM -0500, Jonathon Jongsma wrote:
With mediated devices, we can now define persistent node devices that can be started and stopped. In order to take advantage of this, we need an API to define new node devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 6 +++ src/libvirt-nodedev.c | 42 ++++++++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 71 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 5 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++- src/remote_protocol-structs | 8 ++++ src/rpc/gendispatch.pl | 1 + 11 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 77d814935e..33eb46b3cd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -131,6 +131,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn,
int virNodeDeviceDestroy (virNodeDevicePtr dev);
+virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index d0fc7f19cf..64a0a7c473 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -74,6 +74,11 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
+typedef virNodeDevice*
typedef virNodeDevice *
+(*virDrvNodeDeviceDefineXML)(virConnect *conn, + const char *xmlDesc, + unsigned int flags); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -113,4 +118,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceListCaps nodeDeviceListCaps; virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; + virDrvNodeDeviceDefineXML nodeDeviceDefineXML; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index fb707b570f..cfc0c9de5b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -737,6 +737,48 @@ virNodeDeviceDestroy(virNodeDevicePtr dev) }
+/** + * virNodeDeviceDefineXML: + * @conn: pointer to the hypervisor connection + * @xmlDesc: string containing an XML description of the device to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Define a new device 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. + * + * Returns a node device object if successful, NULL in case of failure + */ +virNodeDevice*
Actually, I think we should keep the Ptr variant in the public APIs. I remember the conclusion to the Ptr discussion was to keep the typedef wrt to public headers and API entrypoints. This is a new API so it's a bit different, but I think for consistency reasons we ought to keep using Ptr for public API everywhere, even new APIs (this is the case for all the APIs you added). My R-b still stands, ^this is not a show-stopper. Erik

On Wed, 31 Mar 2021 08:31:11 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Fri, Mar 26, 2021 at 11:48:12AM -0500, Jonathon Jongsma wrote:
With mediated devices, we can now define persistent node devices that can be started and stopped. In order to take advantage of this, we need an API to define new node devices.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 4 ++ src/driver-nodedev.h | 6 +++ src/libvirt-nodedev.c | 42 ++++++++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 71 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 5 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++- src/remote_protocol-structs | 8 ++++ src/rpc/gendispatch.pl | 1 + 11 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 77d814935e..33eb46b3cd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -131,6 +131,10 @@ virNodeDevicePtr virNodeDeviceCreateXML (virConnectPtr conn, int virNodeDeviceDestroy (virNodeDevicePtr dev); +virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, + const char *xmlDesc, + unsigned int flags); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index d0fc7f19cf..64a0a7c473 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -74,6 +74,11 @@ typedef virNodeDevicePtr typedef int (*virDrvNodeDeviceDestroy)(virNodeDevicePtr dev);
+typedef virNodeDevice*
typedef virNodeDevice *
+(*virDrvNodeDeviceDefineXML)(virConnect *conn, + const char *xmlDesc, + unsigned int flags); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -113,4 +118,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceListCaps nodeDeviceListCaps; virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; + virDrvNodeDeviceDefineXML nodeDeviceDefineXML; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index fb707b570f..cfc0c9de5b 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -737,6 +737,48 @@ virNodeDeviceDestroy(virNodeDevicePtr dev) }
+/** + * virNodeDeviceDefineXML: + * @conn: pointer to the hypervisor connection + * @xmlDesc: string containing an XML description of the device to be defined + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Define a new device 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. + * + * Returns a node device object if successful, NULL in case of failure + */ +virNodeDevice*
Actually, I think we should keep the Ptr variant in the public APIs. I remember the conclusion to the Ptr discussion was to keep the typedef wrt to public headers and API entrypoints. This is a new API so it's a bit different, but I think for consistency reasons we ought to keep using Ptr for public API everywhere, even new APIs (this is the case for all the APIs you added).
Agreed. I had come to that conclusion as well, but apparently I accidentally changed more than I had intended to. I'll change these to use the public Ptr types. Jonathon

Now that we can filter active and inactive node devices in virConnectListAllNodeDevices(), add these switches to the virsh command. Eventual output (once everything is hooked up): virsh # nodedev-list --cap mdev mdev_bd2ea955_3402_4252_8c17_7468083a0f26 virsh # nodedev-list --inactive --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c virsh # nodedev-list --all --cap mdev mdev_07d8b8b0_7e04_4c0f_97ed_9214ce12723c mdev_927c040f_ae7d_4a35_966e_286ba6ebbe1c mdev_bd2ea955_3402_4252_8c17_7468083a0f26 Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index b9fe9b8be1..7ab5b264fc 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -378,6 +378,14 @@ static const vshCmdOptDef opts_node_list_devices[] = { .completer = virshNodeDeviceCapabilityNameCompleter, .help = N_("capability names, separated by comma") }, + {.name = "inactive", + .type = VSH_OT_BOOL, + .help = N_("list inactive devices") + }, + {.name = "all", + .type = VSH_OT_BOOL, + .help = N_("list inactive & active devices") + }, {.name = NULL} }; @@ -393,18 +401,26 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) int ncaps = 0; virshNodeDeviceListPtr list = NULL; int cap_type = -1; + bool inactive = vshCommandOptBool(cmd, "inactive"); + bool all = vshCommandOptBool(cmd, "all"); ignore_value(vshCommandOptStringQuiet(ctl, cmd, "cap", &cap_str)); if (cap_str) { - if (tree) { - vshError(ctl, "%s", _("Options --tree and --cap are incompatible")); - return false; - } if ((ncaps = vshStringToArray(cap_str, &caps)) < 0) return false; } + if (all && inactive) { + vshError(ctl, "%s", _("Option --all is incompatible with --inactive")); + return false; + } + + if (tree && (cap_str || inactive || all)) { + vshError(ctl, "%s", _("Option --tree is incompatible with other options")); + return false; + } + for (i = 0; i < ncaps; i++) { if ((cap_type = virNodeDevCapTypeFromString(caps[i])) < 0) { vshError(ctl, "%s", _("Invalid capability type")); @@ -481,6 +497,11 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) } } + if (inactive || all) + flags |= VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE; + if (!inactive) + flags |= VIR_CONNECT_LIST_NODE_DEVICES_ACTIVE; + if (!(list = virshNodeDeviceListCollect(ctl, caps, ncaps, flags))) { ret = false; goto cleanup; -- 2.26.3

Add a virsh command that maps to virNodeDeviceDefineXML(). Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 58 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 7ab5b264fc..1cbf2e082b 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1013,6 +1013,58 @@ cmdNodeDeviceEvent(vshControl *ctl, const vshCmd *cmd) } +/* + * "nodedev-define" command + */ +static const vshCmdInfo info_node_device_define[] = { + {.name = "help", + .data = N_("Define 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.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_define[] = { + VIRSH_COMMON_OPT_FILE(N_("file containing an XML description " + "of the device")), + {.name = NULL} +}; + +static bool +cmdNodeDeviceDefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) +{ + virNodeDevice *dev = NULL; + const char *from = NULL; + bool ret = true; + char *buffer; + virshControl *priv = ctl->privData; + + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) + return false; + + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) + return false; + + dev = virNodeDeviceDefineXML(priv->conn, buffer, 0); + VIR_FREE(buffer); + + if (dev != NULL) { + vshPrintExtra(ctl, _("Node device '%s' defined from '%s'\n"), + virNodeDeviceGetName(dev), from); + virNodeDeviceFree(dev); + } else { + vshError(ctl, _("Failed to define node device from '%s'"), from); + ret = false; + } + + return ret; +} + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1066,5 +1118,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_event, .flags = 0 }, + {.name = "nodedev-define", + .handler = cmdNodeDeviceDefine, + .opts = opts_node_device_define, + .info = info_node_device_define, + .flags = 0 + }, {.name = NULL} }; -- 2.26.3

mdevctl 'stop' and 'undefine' commands take the same uuid parameter, so refactor the test infrastructure to share common implementation for both of these commands. The 'undefine' command will be introduced in a following patch. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- tests/nodedevmdevctltest.c | 48 +++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 99fa328d24..7cf9fa7c67 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -12,7 +12,9 @@ typedef enum { MDEVCTL_CMD_START, + MDEVCTL_CMD_STOP, MDEVCTL_CMD_DEFINE, + MDEVCTL_CMD_UNDEFINE } MdevctlCmd; struct startTestInfo { @@ -135,20 +137,22 @@ testMdevctlStartOrDefineHelper(const void *data) mdevxml, cmdlinefile, jsonfile); } +typedef virCommand* (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf); +struct UuidCommandTestInfo { + const char *uuid; + MdevctlCmd command; +}; + static int -testMdevctlStop(const void *data) +testMdevctlUuidCommand(const char *uuid, GetStopUndefineCmdFunc func, const char *outfile) { - const char *uuid = data; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const char *actualCmdline = NULL; int ret = -1; g_autoptr(virCommand) cmd = NULL; g_autofree char *errmsg = NULL; - g_autofree char *cmdlinefile = - g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", - abs_srcdir); - cmd = nodeDeviceGetMdevctlStopCommand(uuid, &errmsg); + cmd = func(uuid, &errmsg); if (!cmd) goto cleanup; @@ -160,7 +164,7 @@ testMdevctlStop(const void *data) if (!(actualCmdline = virBufferCurrentContent(&buf))) goto cleanup; - if (nodedevCompareToFile(actualCmdline, cmdlinefile) < 0) + if (nodedevCompareToFile(actualCmdline, outfile) < 0) goto cleanup; ret = 0; @@ -170,6 +174,27 @@ testMdevctlStop(const void *data) return ret; } +static int +testMdevctlUuidCommandHelper(const void *data) +{ + const struct UuidCommandTestInfo *info = data; + GetStopUndefineCmdFunc func; + const char *cmd; + g_autofree char *cmdlinefile = NULL; + + if (info->command == MDEVCTL_CMD_STOP) { + cmd = "stop"; + func = nodeDeviceGetMdevctlStopCommand; + } else { + return -1; + } + + cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-%s.argv", + abs_srcdir, cmd); + + return testMdevctlUuidCommand(info->uuid, func, cmdlinefile); +} + static int testMdevctlListDefined(const void *data G_GNUC_UNUSED) { @@ -387,8 +412,15 @@ mymain(void) #define DO_TEST_DEFINE(filename) \ DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE, filename, MDEVCTL_CMD_DEFINE) +#define DO_TEST_UUID_COMMAND_FULL(desc, uuid, command) \ + do { \ + struct UuidCommandTestInfo info = { uuid, command }; \ + DO_TEST_FULL(desc, testMdevctlUuidCommandHelper, &info); \ + } \ + while (0) + #define DO_TEST_STOP(uuid) \ - DO_TEST_FULL("mdevctl stop " uuid, testMdevctlStop, uuid) + DO_TEST_UUID_COMMAND_FULL("mdevctl stop " uuid, uuid, MDEVCTL_CMD_STOP) #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:15AM -0500, Jonathon Jongsma wrote:
mdevctl 'stop' and 'undefine' commands take the same uuid parameter, so refactor the test infrastructure to share common implementation for both of these commands. The 'undefine' command will be introduced in a following patch.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl' Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++ src/driver-nodedev.h | 4 + src/libvirt-nodedev.c | 36 +++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 73 +++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++- src/remote_protocol-structs | 4 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 8 ++ 14 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 33eb46b3cd..623017f1fd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -135,6 +135,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags); +int virNodeDeviceUndefine(virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c index 33db7752b6..d4a0c98b9b 100644 --- a/src/access/viraccessperm.c +++ b/src/access/viraccessperm.c @@ -70,7 +70,7 @@ VIR_ENUM_IMPL(virAccessPermNodeDevice, VIR_ACCESS_PERM_NODE_DEVICE_LAST, "getattr", "read", "write", "start", "stop", - "detach", + "detach", "delete", ); VIR_ENUM_IMPL(virAccessPermNWFilter, diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h index 42996b9741..051246a7b6 100644 --- a/src/access/viraccessperm.h +++ b/src/access/viraccessperm.h @@ -500,6 +500,12 @@ typedef enum { */ VIR_ACCESS_PERM_NODE_DEVICE_DETACH, + /** + * @desc: Delete node device + * @message: Deleting node device driver requires authorization + */ + VIR_ACCESS_PERM_NODE_DEVICE_DELETE, + VIR_ACCESS_PERM_NODE_DEVICE_LAST } virAccessPermNodeDevice; diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index 64a0a7c473..c352462dda 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -79,6 +79,9 @@ typedef virNodeDevice* const char *xmlDesc, unsigned int flags); +typedef int +(*virDrvNodeDeviceUndefine)(virNodeDevice *dev); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -119,4 +122,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceCreateXML nodeDeviceCreateXML; virDrvNodeDeviceDestroy nodeDeviceDestroy; virDrvNodeDeviceDefineXML nodeDeviceDefineXML; + virDrvNodeDeviceUndefine nodeDeviceUndefine; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index cfc0c9de5b..1d397c6610 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -779,6 +779,42 @@ virNodeDeviceDefineXML(virConnect *conn, } +/** + * virNodeDeviceUndefine: + * @dev: a device object + * + * Undefine the device object. The virtual device is removed from the host + * operating system. This function may require privileged access. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceUndefine(virNodeDevice *dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceUndefine) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceUndefine(dev); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 3d8176351c..99b46587b9 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -888,6 +888,7 @@ LIBVIRT_7.2.0 { global: virDomainStartDirtyRateCalc; virNodeDeviceDefineXML; + virNodeDeviceUndefine; } LIBVIRT_7.1.0; # .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 418faa9fb9..48b4b1f438 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -889,6 +889,18 @@ nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg) } +virCommand* +nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "undefine", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; +} + static int virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { @@ -904,6 +916,22 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) } +static int +virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlUndefineCommand(def->caps->data.mdev.uuid, + errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1183,6 +1211,51 @@ nodeDeviceDefineXML(virConnect *conn, } +int +nodeDeviceUndefine(virNodeDevice *device) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def; + + if (nodeDeviceWaitInit() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceUndefineEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Node device '%s' is not defined"), + def->name); + goto cleanup; + } + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlUndefine(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to undefine mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + goto cleanup; + } + ret = 0; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f626e9ac8a..92bb72ee5d 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -107,6 +107,9 @@ nodeDeviceDefineXML(virConnect *conn, const char *xmlDesc, unsigned int flags); +int +nodeDeviceUndefine(virNodeDevice *dev); + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr dev, @@ -132,6 +135,10 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg); +virCommand* +nodeDeviceGetMdevctlUndefineCommand(const char *uuid, + char **errmsg); + virCommandPtr nodeDeviceGetMdevctlListCommand(bool defined, char **output); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9de658cab5..b870446c55 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2322,6 +2322,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 15c592b5b5..d3e21ea797 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8697,6 +8697,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ }; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a95ed65f12..10d4233d69 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2154,6 +2154,10 @@ struct remote_node_device_define_xml_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_undefine_args { + remote_nonnull_string name; +}; + /* * Events Register/Deregister: @@ -6760,5 +6764,13 @@ enum remote_procedure { * @generate: both * @acl: node_device:write */ - REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428 + REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, + + /** + * @generate: both + * @priority: high + * @acl: node_device:delete + */ + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429 + }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 3488659da1..792e858770 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1607,6 +1607,9 @@ struct remote_node_device_define_xml_args { struct remote_node_device_define_xml_ret { remote_nonnull_node_device dev; }; +struct remote_node_device_undefine_args { + remote_nonnull_string name; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3613,4 +3616,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_MESSAGES = 426, REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, }; diff --git a/tests/nodedevmdevctldata/mdevctl-undefine.argv b/tests/nodedevmdevctldata/mdevctl-undefine.argv new file mode 100644 index 0000000000..54717455f7 --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-undefine.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ undefine -u d76a6b78-45ed-4149-a325-005f9abc5281 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 7cf9fa7c67..e471e2e6eb 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -185,6 +185,9 @@ testMdevctlUuidCommandHelper(const void *data) if (info->command == MDEVCTL_CMD_STOP) { cmd = "stop"; func = nodeDeviceGetMdevctlStopCommand; + } else if (info->command == MDEVCTL_CMD_UNDEFINE) { + cmd = "undefine"; + func = nodeDeviceGetMdevctlUndefineCommand; } else { return -1; } @@ -422,6 +425,9 @@ mymain(void) #define DO_TEST_STOP(uuid) \ DO_TEST_UUID_COMMAND_FULL("mdevctl stop " uuid, uuid, MDEVCTL_CMD_STOP) +#define DO_TEST_UNDEFINE(uuid) \ + DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid, MDEVCTL_CMD_UNDEFINE) + #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) @@ -444,6 +450,8 @@ mymain(void) DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + done: nodedevTestDriverFree(driver); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:16AM -0500, Jonathon Jongsma wrote:
This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl'
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
...
+/** + * virNodeDeviceUndefine: + * @dev: a device object + * + * Undefine the device object. The virtual device is removed from the host + * operating system. This function may require privileged access. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceUndefine(virNodeDevice *dev)
For consistency reasons ^this should remain virNodeDevicePtr ...
+virCommand*
virCommand * I noticed this pattern repeating across the whole series, some of the occurrences I commented on (when I noticed), some of them I forgot...so please fix all of them. Reviewed-by: Erik Skultety <eskultet@redhat.com>
+nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "undefine", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; +} + static int virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { @@ -904,6 +916,22 @@ virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) }
+static int +virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlUndefineCommand(def->caps->data.mdev.uuid, + errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1183,6 +1211,51 @@ nodeDeviceDefineXML(virConnect *conn, }
+int +nodeDeviceUndefine(virNodeDevice *device) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def; + + if (nodeDeviceWaitInit() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceUndefineEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (!virNodeDeviceObjIsPersistent(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Node device '%s' is not defined"), + def->name); + goto cleanup; + } + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlUndefine(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to undefine mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error");
"Unknown Error" case which has already been mentioned...

On Fri, Mar 26, 2021 at 11:48:16AM -0500, Jonathon Jongsma wrote:
This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl'
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++ src/driver-nodedev.h | 4 + src/libvirt-nodedev.c | 36 +++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 73 +++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++- src/remote_protocol-structs | 4 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 8 ++ 14 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 33eb46b3cd..623017f1fd 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -135,6 +135,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags);
+int virNodeDeviceUndefine(virNodeDevicePtr dev);
This API doesn't follow our best practice which is to *always* have an "unsigned int flags" parameter, even if we don't currently think we need it. I think this needs fixing asap since it affects public API, wire protocol and language bindings, and we're not yet locked into the API design. 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 a Friday in 2021, Jonathon Jongsma wrote:
This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl'
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++ src/driver-nodedev.h | 4 + src/libvirt-nodedev.c | 36 +++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 73 +++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++- src/remote_protocol-structs | 4 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 8 ++ 14 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9de658cab5..b870446c55 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2322,6 +2322,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ };
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 15c592b5b5..d3e21ea797 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8697,6 +8697,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ };
7.2.0 is already released. These APIs will be a part of 7.3.0, so the comment also needs bumping. Jano

On Fri, Apr 09, 2021 at 03:57:12PM +0200, Ján Tomko wrote:
On a Friday in 2021, Jonathon Jongsma wrote:
This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl'
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++ src/driver-nodedev.h | 4 + src/libvirt-nodedev.c | 36 +++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 73 +++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++- src/remote_protocol-structs | 4 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 8 ++ 14 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9de658cab5..b870446c55 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2322,6 +2322,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ };
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 15c592b5b5..d3e21ea797 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8697,6 +8697,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ };
7.2.0 is already released. These APIs will be a part of 7.3.0, so the comment also needs bumping.
Sigh, the libvirt_public.syms file is wrong too, as it put the symbols in the previous release 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 a Friday in 2021, Daniel P. Berrangé wrote:
On Fri, Apr 09, 2021 at 03:57:12PM +0200, Ján Tomko wrote:
On a Friday in 2021, Jonathon Jongsma wrote:
This interface allows you to undefine a persistently defined (but inactive) mediated devices. It is implemented via 'mdevctl'
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++ src/driver-nodedev.h | 4 + src/libvirt-nodedev.c | 36 +++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 73 +++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++- src/remote_protocol-structs | 4 + .../nodedevmdevctldata/mdevctl-undefine.argv | 1 + tests/nodedevmdevctltest.c | 8 ++ 14 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9de658cab5..b870446c55 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2322,6 +2322,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceCreateXML = nodeDeviceCreateXML, /* 0.7.3 */ .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ };
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 15c592b5b5..d3e21ea797 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8697,6 +8697,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ + .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ };
7.2.0 is already released. These APIs will be a part of 7.3.0, so the comment also needs bumping.
Sigh, the libvirt_public.syms file is wrong too, as it put the symbols in the previous release
It is wrong on-list, but the final version pushed to git has the syms file rebased correctly. It's just the comments that got left behind. Jano

Several functions accept providing a node device by name or by wwnn,wwpn pair. Extract the logic to do this into a function that can be used by both callers. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 62 +++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 1cbf2e082b..5589a5862d 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -111,23 +111,18 @@ static const vshCmdOptDef opts_node_device_destroy[] = { {.name = NULL} }; -static bool -cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +static virNodeDevice* +vshFindNodeDevice(vshControl *ctl, const char *value) { virNodeDevicePtr dev = NULL; - bool ret = false; - const char *device_value = NULL; char **arr = NULL; int narr; virshControlPtr priv = ctl->privData; - if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) - return false; - - if (strchr(device_value, ',')) { - narr = vshStringToArray(device_value, &arr); + if (strchr(value, ',')) { + narr = vshStringToArray(value, &arr); if (narr != 2) { - vshError(ctl, _("Malformed device value '%s'"), device_value); + vshError(ctl, _("Malformed device value '%s'"), value); goto cleanup; } @@ -136,14 +131,33 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) dev = virNodeDeviceLookupSCSIHostByWWN(priv->conn, arr[0], arr[1], 0); } else { - dev = virNodeDeviceLookupByName(priv->conn, device_value); + dev = virNodeDeviceLookupByName(priv->conn, value); } if (!dev) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + vshError(ctl, "%s '%s'", _("Could not find matching device"), value); goto cleanup; } + cleanup: + g_strfreev(arr); + return dev; +} + +static bool +cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) +{ + virNodeDevice *dev = NULL; + bool ret = false; + const char *device_value = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; + + dev = vshFindNodeDevice(ctl, device_value); + if (!dev) + goto cleanup; + if (virNodeDeviceDestroy(dev) == 0) { vshPrintExtra(ctl, _("Destroyed node device '%s'\n"), device_value); } else { @@ -153,7 +167,6 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); if (dev) virNodeDeviceFree(dev); return ret; @@ -578,33 +591,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device = NULL; char *xml = NULL; const char *device_value = NULL; - char **arr = NULL; - int narr; bool ret = false; - virshControlPtr priv = ctl->privData; if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) return false; - if (strchr(device_value, ',')) { - narr = vshStringToArray(device_value, &arr); - if (narr != 2) { - vshError(ctl, _("Malformed device value '%s'"), device_value); - goto cleanup; - } + device = vshFindNodeDevice(ctl, device_value); - if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1])) - goto cleanup; - - device = virNodeDeviceLookupSCSIHostByWWN(priv->conn, arr[0], arr[1], 0); - } else { - device = virNodeDeviceLookupByName(priv->conn, device_value); - } - - if (!device) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), device_value); + if (!device) goto cleanup; - } if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) goto cleanup; @@ -613,7 +608,6 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - g_strfreev(arr); VIR_FREE(xml); if (device) virNodeDeviceFree(device); -- 2.26.3

Add a virsh command that maps to virNodeDeviceUndefine(). Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 59 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 5589a5862d..d5b82ba285 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1007,6 +1007,59 @@ cmdNodeDeviceEvent(vshControl *ctl, const vshCmd *cmd) } +/* + * "nodedev-undefine" command + */ +static const vshCmdInfo info_node_device_undefine[] = { + {.name = "help", + .data = N_("Undefine an inactive node device") + }, + {.name = "desc", + .data = N_("Undefines the configuration for an inactive node device") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_undefine[] = { + {.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 +cmdNodeDeviceUndefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) +{ + virNodeDevice *dev = NULL; + bool ret = false; + const char *device_value = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "device", &device_value) < 0) + return false; + + dev = vshFindNodeDevice(ctl, device_value); + + if (!dev) + goto cleanup; + + if (virNodeDeviceUndefine(dev) == 0) { + vshPrintExtra(ctl, _("Undefined node device '%s'\n"), device_value); + } else { + vshError(ctl, _("Failed to undefine node device '%s'"), device_value); + goto cleanup; + } + + ret = true; + cleanup: + if (dev) + virNodeDeviceFree(dev); + return ret; +} + + /* * "nodedev-define" command */ @@ -1118,5 +1171,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_define, .flags = 0 }, + {.name = "nodedev-undefine", + .handler = cmdNodeDeviceUndefine, + .opts = opts_node_device_undefine, + .info = info_node_device_undefine, + .flags = 0 + }, {.name = NULL} }; -- 2.26.3

This new API function provides a way to start a persistently-defined mediate device that was defined by virNodeDeviceDefineXML() (or one that was defined externally via mdevctl) Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/driver-nodedev.h | 4 ++ src/libvirt-nodedev.c | 35 ++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 68 ++++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 +++- src/remote_protocol-structs | 4 ++ tests/nodedevmdevctldata/mdevctl-create.argv | 1 + tests/nodedevmdevctltest.c | 11 +++- 12 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 623017f1fd..cf51c3d085 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -137,6 +137,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn, int virNodeDeviceUndefine(virNodeDevicePtr dev); +int virNodeDeviceCreate(virNodeDevicePtr dev); + /** * VIR_NODE_DEVICE_EVENT_CALLBACK: * diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h index c352462dda..22fe53bfc0 100644 --- a/src/driver-nodedev.h +++ b/src/driver-nodedev.h @@ -82,6 +82,9 @@ typedef virNodeDevice* typedef int (*virDrvNodeDeviceUndefine)(virNodeDevice *dev); +typedef int +(*virDrvNodeDeviceCreate)(virNodeDevice *def); + typedef int (*virDrvConnectNodeDeviceEventRegisterAny)(virConnectPtr conn, virNodeDevicePtr dev, @@ -123,4 +126,5 @@ struct _virNodeDeviceDriver { virDrvNodeDeviceDestroy nodeDeviceDestroy; virDrvNodeDeviceDefineXML nodeDeviceDefineXML; virDrvNodeDeviceUndefine nodeDeviceUndefine; + virDrvNodeDeviceCreate nodeDeviceCreate; }; diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index 1d397c6610..a1b9e44ab3 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -815,6 +815,41 @@ virNodeDeviceUndefine(virNodeDevice *dev) } +/** + * virNodeDeviceCreate: + * @dev: a device object + * + * Start a defined node device: + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNodeDeviceCreate(virNodeDevice *dev) +{ + VIR_DEBUG("dev=%p", dev); + + virResetLastError(); + + virCheckNodeDeviceReturn(dev, -1); + virCheckReadOnlyGoto(dev->conn->flags, error); + + if (dev->conn->nodeDeviceDriver && + dev->conn->nodeDeviceDriver->nodeDeviceCreate) { + int retval = dev->conn->nodeDeviceDriver->nodeDeviceCreate(dev); + if (retval < 0) + goto error; + + return 0; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dev->conn); + return -1; +} + + /** * virConnectNodeDeviceEventRegisterAny: * @conn: pointer to the connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 99b46587b9..3a8f5d8fc6 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -889,6 +889,7 @@ LIBVIRT_7.2.0 { virDomainStartDirtyRateCalc; virNodeDeviceDefineXML; virNodeDeviceUndefine; + virNodeDeviceCreate; } LIBVIRT_7.1.0; # .... define new API here using predicted next version number .... diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 48b4b1f438..cd7677633a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -901,6 +901,18 @@ nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg) return cmd; } +virCommand* +nodeDeviceGetMdevctlCreateCommand(const char *uuid, char **errmsg) +{ + virCommand *cmd = virCommandNewArgList(MDEVCTL, + "start", + "-u", + uuid, + NULL); + virCommandSetErrorBuffer(cmd, errmsg); + return cmd; +} + static int virMdevctlStop(virNodeDeviceDefPtr def, char **errmsg) { @@ -932,6 +944,21 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg) } +static int +virMdevctlCreate(virNodeDeviceDef *def, char **errmsg) +{ + int status; + g_autoptr(virCommand) cmd = NULL; + + cmd = nodeDeviceGetMdevctlCreateCommand(def->caps->data.mdev.uuid, errmsg); + + if (virCommandRun(cmd, &status) < 0 || status != 0) + return -1; + + return 0; +} + + virCommand* nodeDeviceGetMdevctlListCommand(bool defined, char **output) @@ -1257,6 +1284,47 @@ nodeDeviceUndefine(virNodeDevice *device) } +int +nodeDeviceCreate(virNodeDevice *device) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def = NULL; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + + if (virNodeDeviceObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Device is already active")); + goto cleanup; + } + def = virNodeDeviceObjGetDef(obj); + + if (virNodeDeviceCreateEnsureACL(device->conn, def) < 0) + goto cleanup; + + if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + g_autofree char *errmsg = NULL; + + if (virMdevctlCreate(def, &errmsg) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to create mediated device: %s"), + errmsg && errmsg[0] ? errmsg : "Unknown Error"); + goto cleanup; + } + ret = 0; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type")); + } + + cleanup: + virNodeDeviceObjEndAPI(&obj); + return ret; +} + + int nodeConnectNodeDeviceEventRegisterAny(virConnectPtr conn, virNodeDevicePtr device, diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index 92bb72ee5d..27e4095299 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -157,3 +157,10 @@ nodeDeviceGenerateName(virNodeDeviceDef *def, bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, virNodeDeviceDef *src); + +virCommand* +nodeDeviceGetMdevctlCreateCommand(const char *uuid, + char **errmsg); + +int +nodeDeviceCreate(virNodeDevice *dev); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b870446c55..d80e81a5a9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2323,6 +2323,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = { .nodeDeviceDestroy = nodeDeviceDestroy, /* 0.7.3 */ .nodeDeviceDefineXML = nodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceUndefine = nodeDeviceUndefine, /* 7.2.0 */ + .nodeDeviceCreate = nodeDeviceCreate, /* 7.2.0 */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d3e21ea797..31b293ac4a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8696,6 +8696,7 @@ static virNodeDeviceDriver node_device_driver = { .nodeDeviceNumOfCaps = remoteNodeDeviceNumOfCaps, /* 0.5.0 */ .nodeDeviceListCaps = remoteNodeDeviceListCaps, /* 0.5.0 */ .nodeDeviceCreateXML = remoteNodeDeviceCreateXML, /* 0.6.3 */ + .nodeDeviceCreate = remoteNodeDeviceCreate, /* 7.2.0 */ .nodeDeviceDefineXML = remoteNodeDeviceDefineXML, /* 7.2.0 */ .nodeDeviceUndefine = remoteNodeDeviceUndefine, /* 7.2.0 */ .nodeDeviceDestroy = remoteNodeDeviceDestroy /* 0.6.3 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 10d4233d69..273ae7531e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2158,6 +2158,10 @@ struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; + /* * Events Register/Deregister: @@ -6771,6 +6775,13 @@ enum remote_procedure { * @priority: high * @acl: node_device:delete */ - REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429 + REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, + + /** + * @generate: both + * @priority: high + * @acl: node_device:start + */ + REMOTE_PROC_NODE_DEVICE_CREATE = 430 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 792e858770..15bd7ae8f0 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1610,6 +1610,9 @@ struct remote_node_device_define_xml_ret { struct remote_node_device_undefine_args { remote_nonnull_string name; }; +struct remote_node_device_create_args { + remote_nonnull_string name; +}; struct remote_connect_domain_event_register_ret { int cb_registered; }; @@ -3617,4 +3620,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_START_DIRTY_RATE_CALC = 427, REMOTE_PROC_NODE_DEVICE_DEFINE_XML = 428, REMOTE_PROC_NODE_DEVICE_UNDEFINE = 429, + REMOTE_PROC_NODE_DEVICE_CREATE = 430, }; diff --git a/tests/nodedevmdevctldata/mdevctl-create.argv b/tests/nodedevmdevctldata/mdevctl-create.argv new file mode 100644 index 0000000000..802109340c --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-create.argv @@ -0,0 +1 @@ +$MDEVCTL_BINARY$ start -u 8a05ad83-3472-497d-8631-8142f31460e8 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index e471e2e6eb..af5e073e8b 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -14,7 +14,8 @@ typedef enum { MDEVCTL_CMD_START, MDEVCTL_CMD_STOP, MDEVCTL_CMD_DEFINE, - MDEVCTL_CMD_UNDEFINE + MDEVCTL_CMD_UNDEFINE, + MDEVCTL_CMD_CREATE, } MdevctlCmd; struct startTestInfo { @@ -188,6 +189,9 @@ testMdevctlUuidCommandHelper(const void *data) } else if (info->command == MDEVCTL_CMD_UNDEFINE) { cmd = "undefine"; func = nodeDeviceGetMdevctlUndefineCommand; + }else if (info->command == MDEVCTL_CMD_CREATE) { + cmd = "create"; + func = nodeDeviceGetMdevctlCreateCommand; } else { return -1; } @@ -428,6 +432,9 @@ mymain(void) #define DO_TEST_UNDEFINE(uuid) \ DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid, MDEVCTL_CMD_UNDEFINE) +#define DO_TEST_CREATE(uuid) \ + DO_TEST_UUID_COMMAND_FULL("mdevctl create " uuid, uuid, MDEVCTL_CMD_CREATE) + #define DO_TEST_LIST_DEFINED() \ DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL) @@ -452,6 +459,8 @@ mymain(void) DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281"); + DO_TEST_CREATE("8a05ad83-3472-497d-8631-8142f31460e8"); + done: nodedevTestDriverFree(driver); -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:19AM -0500, Jonathon Jongsma wrote:
This new API function provides a way to start a persistently-defined mediate device that was defined by virNodeDeviceDefineXML() (or one that was defined externally via mdevctl)
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- include/libvirt/libvirt-nodedev.h | 2 + src/driver-nodedev.h | 4 ++ src/libvirt-nodedev.c | 35 ++++++++++ src/libvirt_public.syms | 1 + src/node_device/node_device_driver.c | 68 ++++++++++++++++++++ src/node_device/node_device_driver.h | 7 ++ src/node_device/node_device_udev.c | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 +++- src/remote_protocol-structs | 4 ++ tests/nodedevmdevctldata/mdevctl-create.argv | 1 + tests/nodedevmdevctltest.c | 11 +++- 12 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv
diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index 623017f1fd..cf51c3d085 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -137,6 +137,8 @@ virNodeDevicePtr virNodeDeviceDefineXML(virConnectPtr conn,
int virNodeDeviceUndefine(virNodeDevicePtr dev);
+int virNodeDeviceCreate(virNodeDevicePtr dev);
This API has the same problem of missing "unsigned int flags" and is even more critical for the Create method IMHO. 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 :|

This virsh command maps to virNodeDeviceCreate(), which starts a node device that has been previously defined by virNodeDeviceDefineXML(). This is only supported for mediated devices. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 57 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index d5b82ba285..e13c15b4b0 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -1112,6 +1112,57 @@ cmdNodeDeviceDefine(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) } +/* + * "nodedev-start" command + */ +static const vshCmdInfo info_node_device_start[] = { + {.name = "help", + .data = N_("Start an inactive node device") + }, + {.name = "desc", + .data = N_("Starts an inactive node device that was previously defined") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_node_device_start[] = { + {.name = "device", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("device name"), + .completer = virshNodeDeviceNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNodeDeviceStart(vshControl *ctl, const vshCmd *cmd) +{ + const char *name = NULL; + virNodeDevice *device; + bool ret = true; + virshControl *priv = ctl->privData; + + if (vshCommandOptStringReq(ctl, cmd, "device", &name) < 0) + return false; + + if (!(device = virNodeDeviceLookupByName(priv->conn, name))) { + vshError(ctl, _("Could not find matching device '%s'"), name); + return false; + } + + if (virNodeDeviceCreate(device) == 0) { + vshPrintExtra(ctl, _("Device %s started\n"), name); + } else { + vshError(ctl, _("Failed to start device %s"), name); + ret = false; + } + + virNodeDeviceFree(device); + return ret; +} + + const vshCmdDef nodedevCmds[] = { {.name = "nodedev-create", .handler = cmdNodeDeviceCreate, @@ -1177,5 +1228,11 @@ const vshCmdDef nodedevCmds[] = { .info = info_node_device_undefine, .flags = 0 }, + {.name = "nodedev-start", + .handler = cmdNodeDeviceStart, + .opts = opts_node_device_start, + .info = info_node_device_start, + .flags = 0 + }, {.name = NULL} }; -- 2.26.3

It will be useful to be able to specify a particular UUID for a mediated device when defining the node device. To accomodate that, allow this to be specified in the xml schema. This patch also parses and formats that value to the xml, but does not yet use it. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- docs/schemas/nodedev.rng | 43 +++++++++++-------- src/conf/node_device_conf.c | 14 ++++++ .../mdevctl-list-multiple.out.xml | 4 ++ 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 5840dc9f0d..777227c38a 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -606,27 +606,34 @@ </define> <define name="capmdev"> - <attribute name="type"> - <value>mdev</value> - </attribute> - <element name="type"> - <attribute name="id"> - <data type="string"/> + <interleave> + <attribute name="type"> + <value>mdev</value> </attribute> - </element> - <optional> - <element name="iommuGroup"> - <attribute name="number"> - <ref name="unsignedInt"/> + <element name="type"> + <attribute name="id"> + <data type="string"/> </attribute> </element> - </optional> - <zeroOrMore> - <element name="attr"> - <attribute name="name"/> - <attribute name="value"/> - </element> - </zeroOrMore> + <optional> + <element name="iommuGroup"> + <attribute name="number"> + <ref name="unsignedInt"/> + </attribute> + </element> + </optional> + <optional> + <element name="uuid"> + <ref name="UUID"/> + </element> + </optional> + <zeroOrMore> + <element name="attr"> + <attribute name="name"/> + <attribute name="value"/> + </element> + </zeroOrMore> + </interleave> </define> <define name="capccwdev"> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index b1739ceb01..d167da6171 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -527,6 +527,7 @@ virNodeDeviceCapMdevDefFormat(virBufferPtr buf, size_t i; virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.type); + virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid); virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n", data->mdev.iommuGroupNumber); @@ -1948,6 +1949,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, int nattrs = 0; g_autofree xmlNodePtr *attrs = NULL; size_t i; + g_autofree char *uuidstr = NULL; ctxt->node = node; @@ -1957,6 +1959,18 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, return -1; } + if ((uuidstr = virXPathString("string(./uuid[1])", ctxt))) { + unsigned char uuidbuf[VIR_UUID_BUFLEN]; + /* make sure that the provided uuid is valid */ + if (virUUIDParse(uuidstr, uuidbuf) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid uuid '%s' for new mdev device"), uuidstr); + return -1; + } + mdev->uuid = g_new0(char, VIR_UUID_STRING_BUFLEN); + virUUIDFormat(uuidbuf, mdev->uuid); + } + /* 'iommuGroup' is optional, only report an error if the supplied value is * invalid (-2), not if it's missing (-1) */ if (virXPathUInt("number(./iommuGroup[1]/@number)", diff --git a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml index 543ad916b7..cf7e966256 100644 --- a/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml +++ b/tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml @@ -3,6 +3,7 @@ <parent>0000:00:02.0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_4'/> + <uuid>200f228a-c80a-4d50-bfb7-f5a0e4e34045</uuid> <iommuGroup number='0'/> </capability> </device> @@ -11,6 +12,7 @@ <parent>0000:00:02.0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_4'/> + <uuid>de807ffc-1923-4d5f-b6c9-b20ecebc6d4b</uuid> <iommuGroup number='0'/> </capability> </device> @@ -19,6 +21,7 @@ <parent>0000:00:02.0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_8'/> + <uuid>435722ea-5f43-468a-874f-da34f1217f13</uuid> <iommuGroup number='0'/> <attr name='testattr' value='42'/> </capability> @@ -28,6 +31,7 @@ <parent>matrix</parent> <capability type='mdev'> <type id='vfio_ap-passthrough'/> + <uuid>783e6dbb-ea0e-411f-94e2-717eaad438bf</uuid> <iommuGroup number='0'/> <attr name='assign_adapter' value='5'/> <attr name='assign_adapter' value='6'/> -- 2.26.3

Use the new <uuid> element in the mdev caps to define and start devices with a specific UUID. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 19 ++++++++++++++++--- ...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 3 ++- ...019_36ea_4111_8f0a_8c9a70e21366-start.argv | 3 ++- ...v_d069d019_36ea_4111_8f0a_8c9a70e21366.xml | 1 + 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index cd7677633a..b1b3217afb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -729,6 +729,10 @@ nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def, NULL); virCommandSetInputBuffer(cmd, json); + + if (def->caps->data.mdev.uuid) + virCommandAddArgPair(cmd, "--uuid", def->caps->data.mdev.uuid); + virCommandSetOutputBuffer(cmd, uuid_out); virCommandSetErrorBuffer(cmd, errmsg); @@ -816,7 +820,12 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn, return NULL; } - return nodeDeviceFindNewMediatedDevice(conn, uuid); + if (uuid && uuid[0]) { + g_free(def->caps->data.mdev.uuid); + def->caps->data.mdev.uuid = g_steal_pointer(&uuid); + } + + return nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); } @@ -1230,9 +1239,13 @@ nodeDeviceDefineXML(virConnect *conn, return NULL; } - def->caps->data.mdev.uuid = g_strdup(uuid); + if (uuid && uuid[0]) { + g_free(def->caps->data.mdev.uuid); + def->caps->data.mdev.uuid = g_steal_pointer(&uuid); + } + mdevGenerateDeviceName(def); - device = nodeDeviceFindNewMediatedDevice(conn, uuid); + device = nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); return device; } diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv index 773e98b963..118ec7a8da 100644 --- a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv @@ -1 +1,2 @@ -$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin +$MDEVCTL_BINARY$ define -p 0000:00:02.0 --jsonfile /dev/stdin \ +--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366 diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv index eb7262035e..129f438e4a 100644 --- a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv @@ -1 +1,2 @@ -$MDEVCTL_BINARY$ start -p 0000:00:02.0 --jsonfile /dev/stdin +$MDEVCTL_BINARY$ start -p 0000:00:02.0 --jsonfile /dev/stdin \ +--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366 diff --git a/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml index d6a2e99edc..605d8f63a1 100644 --- a/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml +++ b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml @@ -3,5 +3,6 @@ <parent>pci_0000_00_02_0</parent> <capability type='mdev'> <type id='i915-GVTg_V5_8'/> + <uuid>d069d019-36ea-4111-8f0a-8c9a70e21366</uuid> </capability> </device> -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:22AM -0500, Jonathon Jongsma wrote:
Use the new <uuid> element in the mdev caps to define and start devices with a specific UUID.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

Calling `mdevctl stop` for a mediated device that is in use by an active domain will block until that vm exits (or the vm closes the device). Since the nodedev driver cannot query the hypervisor driver to see whether any active domains are using the device, we resort to a workaround that relies on the fact that a vfio group can only be opened by one user at a time. If we get an EBUSY error when attempting to open the group file, we assume the device is in use and refuse to try to destroy that device. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index b1b3217afb..31322e3afb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1177,8 +1177,27 @@ nodeDeviceDestroy(virNodeDevicePtr device) ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + /* If this mediated device is in use by a vm, attempting to stop it + * will block until the vm closes the device. The nodedev driver + * cannot query the hypervisor driver to determine whether the device + * is in use by any active domains, since that would introduce circular + * dependencies between daemons and add a risk of deadlocks. So we need + * to resort to a workaround. vfio only allows the group for a device + * to be opened by one user at a time. So if we get EBUSY when opening + * the group, we infer that the device is in use and therefore we + * shouldn't try to remove the device. */ + g_autofree char *vfiogroup = + virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid); + VIR_AUTOCLOSE fd = open(vfiogroup, O_RDONLY); g_autofree char *errmsg = NULL; + if (fd < 0 && errno == EBUSY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': device in use"), + def->name); + goto cleanup; + } + if (virMdevctlStop(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to destroy '%s': %s"), def->name, -- 2.26.3

On 3/26/21 12:48 PM, Jonathon Jongsma wrote:
Calling `mdevctl stop` for a mediated device that is in use by an active domain will block until that vm exits (or the vm closes the device). Since the nodedev driver cannot query the hypervisor driver to see whether any active domains are using the device, we resort to a workaround that relies on the fact that a vfio group can only be opened by one user at a time. If we get an EBUSY error when attempting to open the group file, we assume the device is in use and refuse to try to destroy that device.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/node_device/node_device_driver.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index b1b3217afb..31322e3afb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1177,8 +1177,27 @@ nodeDeviceDestroy(virNodeDevicePtr device)
ret = 0; } else if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) { + /* If this mediated device is in use by a vm, attempting to stop it + * will block until the vm closes the device. The nodedev driver + * cannot query the hypervisor driver to determine whether the device + * is in use by any active domains, since that would introduce circular + * dependencies between daemons and add a risk of deadlocks. So we need + * to resort to a workaround. vfio only allows the group for a device + * to be opened by one user at a time. So if we get EBUSY when opening + * the group, we infer that the device is in use and therefore we + * shouldn't try to remove the device. */ + g_autofree char *vfiogroup = + virMediatedDeviceGetIOMMUGroupDev(def->caps->data.mdev.uuid);
FWIW: Coverity points out @vfiogroup could be returned as NULL here (w/ virReportError called) thus making the subsequent call possibly fail... John
+ VIR_AUTOCLOSE fd = open(vfiogroup, O_RDONLY); g_autofree char *errmsg = NULL;
+ if (fd < 0 && errno == EBUSY) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to destroy '%s': device in use"), + def->name); + goto cleanup; + } + if (virMdevctlStop(def, &errmsg) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to destroy '%s': %s"), def->name,

Mention that mdev attribute order is significant. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/formatnode.html.in | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 5c7286df8a..c58cd01395 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -379,7 +379,10 @@ This optional element can occur multiple times. It represents a vendor-specific attribute that is used to configure this mediated device. It has two required attributes: - <code>name</code> and <code>value</code>. + <code>name</code> and <code>value</code>. Note that the order + in which attributes are set may be important for some devices. + The order that they appear in the xml definition determines the + order that they will be written to the device. </dd> </dl> </dd> -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:24AM -0500, Jonathon Jongsma wrote:
Mention that mdev attribute order is significant.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

To accomodate re-use of this functionality in a following patch, split out the processing of an individual mdev definition into a separate function. --- src/node_device/node_device_driver.c | 103 +++++++++++++++------------ 1 file changed, 57 insertions(+), 46 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 31322e3afb..35db24817a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1506,6 +1506,60 @@ removeMissingPersistentMdevs(virNodeDeviceObj *obj, } +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) +{ + virNodeDeviceObj *obj; + virObjectEvent *event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDef *d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0; +} + + int nodeDeviceUpdateMediatedDevices(void) { @@ -1525,52 +1579,9 @@ nodeDeviceUpdateMediatedDevices(void) virNodeDeviceObjListForEachRemove(driver->devs, removeMissingPersistentMdevs, &data); - for (i = 0; i < data.ndefs; i++) { - virNodeDeviceObj *obj; - virObjectEvent *event; - g_autoptr(virNodeDeviceDef) def = defs[i]; - g_autofree char *name = g_strdup(def->name); - bool defined = false; - - def->driver = g_strdup("vfio_mdev"); - - if (!(obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { - virNodeDeviceDef *d = g_steal_pointer(&def); - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { - virNodeDeviceDefFree(d); - return -1; - } - } else { - bool changed; - virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); - - defined = virNodeDeviceObjIsPersistent(obj); - /* Active devices contain some additional information (e.g. sysfs - * path) that is not provided by mdevctl, so re-use the existing - * definition and copy over new mdev data */ - changed = nodeDeviceDefCopyFromMdevctl(olddef, def); - - if (defined && !changed) { - /* if this device was already defined and the definition - * hasn't changed, there's nothing to do for this device */ - virNodeDeviceObjEndAPI(&obj); - continue; - } - } - - /* all devices returned by virMdevctlListDefined() are persistent */ - virNodeDeviceObjSetPersistent(obj, true); - - if (!defined) - event = virNodeDeviceEventLifecycleNew(name, - VIR_NODE_DEVICE_EVENT_DEFINED, - 0); - else - event = virNodeDeviceEventUpdateNew(name); - - virNodeDeviceObjEndAPI(&obj); - virObjectEventStateQueue(driver->nodeDeviceEventState, event); - } + for (i = 0; i < data.ndefs; i++) + if (nodeDeviceUpdateMediatedDevice(defs[i]) < 0) + return -1; return 0; } -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:25AM -0500, Jonathon Jongsma wrote:
To accomodate re-use of this functionality in a following patch, split out the processing of an individual mdev definition into a separate function. --- src/node_device/node_device_driver.c | 103 +++++++++++++++------------ 1 file changed, 57 insertions(+), 46 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 31322e3afb..35db24817a 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1506,6 +1506,60 @@ removeMissingPersistentMdevs(virNodeDeviceObj *obj, }
If you move the function to the correct place as mentioned in 30/30: Reviewed-by: Erik Skultety <eskultet@redhat.com>

When calling virNodeDeviceDefineXML() to define a new mediated device, we call virMdevctlDefine() and then wait for the new device to appear in the driver's device list before returning. This caused long delays due to the behavior of nodeDeviceFindNewMediatedDevice(). This function checks to see if the device is in the list and then waits for 5s before checking again. Because mdevctl is relatively slow to query the list of defined devices[0], the newly-defined device was generally not in the device list when we first checked. This results in libvirt almost always taking at least 5s to complete this API call for mediated devices, which is unacceptable. In order to avoid this long delay, we resort to a workaround. If the call to virMdevctlDefine() was successful, we can assume that this new device will exist the next time we query mdevctl for new devices. So we simply add this provisional device definition directly to the nodedev driver's device list and return from the function. At some point in the future, the mdevctl handler will run and the "official" device will be processed, which will update the provisional device if any new details need to be added. The reason that this is not necessary for virNodeDeviceCreateXML() is because detecting newly-created (not defined) mdevs happens through udev instead of mdevctl. And nodeDeviceFindNewMediatedDevice() always calls 'udevadm settle' before checking to see whether the device is in the list. This allows us to wait just long enough for all udev events to be processed, so the device is almost always in the list the first time we check and so we almost never end up hitting the 5s sleep. [0] on my machine, 'mdevctl list --defined' took around 0.8s to complete for only 3 defined mdevs. --- src/node_device/node_device_driver.c | 126 +++++++++++++++------------ 1 file changed, 68 insertions(+), 58 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 35db24817a..a1b79d93f7 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1215,16 +1215,70 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; } + +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) +{ + virNodeDeviceObj *obj; + virObjectEvent *event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDef *d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0; +} + virNodeDevice* -nodeDeviceDefineXML(virConnect *conn, +nodeDeviceDefineXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags) { g_autoptr(virNodeDeviceDef) def = NULL; - virNodeDevice *device = NULL; const char *virt_type = NULL; g_autofree char *uuid = NULL; g_autofree char *errmsg = NULL; + g_autofree char *name = NULL; virCheckFlags(0, NULL); @@ -1264,9 +1318,19 @@ nodeDeviceDefineXML(virConnect *conn, } mdevGenerateDeviceName(def); - device = nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); + name = g_strdup(def->name); + + /* Normally we would call nodeDeviceFindNewMediatedDevice() here to wait + * for the new device to appear. But mdevctl can take a while to query + * devices, and if nodeDeviceFindNewMediatedDevice() doesn't find the new + * device immediately it will wait at 5s before checking again. Since we + * have already received the uuid from virMdevctlDefine(), we can simply + * add the provisional device to the list and return it immediately and + * avoid this long delay. */ + if (nodeDeviceUpdateMediatedDevice(g_steal_pointer(&def)) < 0) + return NULL; - return device; + return virGetNodeDevice(conn, name); } @@ -1506,60 +1570,6 @@ removeMissingPersistentMdevs(virNodeDeviceObj *obj, } -/* takes ownership of @def and potentially frees it. @def should not be used - * after returning from this function */ -static int -nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) -{ - virNodeDeviceObj *obj; - virObjectEvent *event; - bool defined = false; - g_autoptr(virNodeDeviceDef) owned = def; - g_autofree char *name = g_strdup(owned->name); - - owned->driver = g_strdup("vfio_mdev"); - - if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { - virNodeDeviceDef *d = g_steal_pointer(&owned); - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { - virNodeDeviceDefFree(d); - return -1; - } - } else { - bool changed; - virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); - - defined = virNodeDeviceObjIsPersistent(obj); - /* Active devices contain some additional information (e.g. sysfs - * path) that is not provided by mdevctl, so re-use the existing - * definition and copy over new mdev data */ - changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); - - if (defined && !changed) { - /* if this device was already defined and the definition - * hasn't changed, there's nothing to do for this device */ - virNodeDeviceObjEndAPI(&obj); - return 0; - } - } - - /* all devices returned by virMdevctlListDefined() are persistent */ - virNodeDeviceObjSetPersistent(obj, true); - - if (!defined) - event = virNodeDeviceEventLifecycleNew(name, - VIR_NODE_DEVICE_EVENT_DEFINED, - 0); - else - event = virNodeDeviceEventUpdateNew(name); - - virNodeDeviceObjEndAPI(&obj); - virObjectEventStateQueue(driver->nodeDeviceEventState, event); - - return 0; -} - - int nodeDeviceUpdateMediatedDevices(void) { -- 2.26.3

On Fri, Mar 26, 2021 at 11:48:26AM -0500, Jonathon Jongsma wrote:
When calling virNodeDeviceDefineXML() to define a new mediated device, we call virMdevctlDefine() and then wait for the new device to appear in the driver's device list before returning. This caused long delays due to the behavior of nodeDeviceFindNewMediatedDevice(). This function checks to see if the device is in the list and then waits for 5s before checking again.
Because mdevctl is relatively slow to query the list of defined devices[0], the newly-defined device was generally not in the device list when we first checked. This results in libvirt almost always taking at least 5s to complete this API call for mediated devices, which is unacceptable.
In order to avoid this long delay, we resort to a workaround. If the call to virMdevctlDefine() was successful, we can assume that this new device will exist the next time we query mdevctl for new devices. So we simply add this provisional device definition directly to the nodedev driver's device list and return from the function. At some point in the future, the mdevctl handler will run and the "official" device will be processed, which will update the provisional device if any new details need to be added.
The reason that this is not necessary for virNodeDeviceCreateXML() is because detecting newly-created (not defined) mdevs happens through udev instead of mdevctl. And nodeDeviceFindNewMediatedDevice() always calls 'udevadm settle' before checking to see whether the device is in the list. This allows us to wait just long enough for all udev events to be processed, so the device is almost always in the list the first time we check and so we almost never end up hitting the 5s sleep.
[0] on my machine, 'mdevctl list --defined' took around 0.8s to complete for only 3 defined mdevs. --- src/node_device/node_device_driver.c | 126 +++++++++++++++------------ 1 file changed, 68 insertions(+), 58 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 35db24817a..a1b79d93f7 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1215,16 +1215,70 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; }
+ +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) +{ + virNodeDeviceObj *obj; + virObjectEvent *event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDef *d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0;
In 29/30, you extracted ^this code into a separate function. If you'd moved it to the right place as well, the diff in this patch would be smaller.
+} + virNodeDevice* -nodeDeviceDefineXML(virConnect *conn, +nodeDeviceDefineXML(virConnectPtr conn,
I don't think you wanted ^this hunk
const char *xmlDesc, unsigned int flags) { g_autoptr(virNodeDeviceDef) def = NULL; - virNodeDevice *device = NULL; const char *virt_type = NULL; g_autofree char *uuid = NULL; g_autofree char *errmsg = NULL; + g_autofree char *name = NULL;
virCheckFlags(0, NULL);
@@ -1264,9 +1318,19 @@ nodeDeviceDefineXML(virConnect *conn, }
mdevGenerateDeviceName(def); - device = nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); + name = g_strdup(def->name); + + /* Normally we would call nodeDeviceFindNewMediatedDevice() here to wait + * for the new device to appear. But mdevctl can take a while to query + * devices, and if nodeDeviceFindNewMediatedDevice() doesn't find the new + * device immediately it will wait at 5s before checking again. Since we + * have already received the uuid from virMdevctlDefine(), we can simply + * add the provisional device to the list and return it immediately and + * avoid this long delay. */ + if (nodeDeviceUpdateMediatedDevice(g_steal_pointer(&def)) < 0) + return NULL;
Let's just hope this workaround will not bite us in the back in the future. That said, I really didn't like the 5s delay :). After decreasing the diff of this patch: Reviewed-by: Erik Skultety <eskultet@redhat.com>

On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
This patch series follows the previously-merged series which added support for transient mediated devices. This series expands mdev support to include persistent device definitions. Again, it relies on mdevctl as the backend.
It follows the common libvirt pattern of APIs by adding the following new APIs for node devices: - virNodeDeviceDefineXML() - defines a persistent device - virNodeDeviceUndefine() - undefines a persistent device - virNodeDeviceCreate() - starts a previously-defined device
It also adds virsh commands mapping to these new APIs: nodedev-define, nodedev-undefine, and nodedev-start.
Since we rely on mdevctl for the definition of mediated devices, we need a way to stay up-to-date with devices that are defined by mdevctl (outside of libvirt). The method for staying up-to-date is currently a little bit crude due to the fact that mdevctl does not emit any events when new devices are added or removed. As a workaround, we create a file monitor for the mdevctl config directory and re-query mdevctl when we detect changes within that directory. In the future, mdevctl may introduce a more elegant solution.
Changes in v6: - rebase to git master again - remove typedefs for various *Ptr types, since they're now discouraged in libvirt.
Okay, so I think I ACKed all of the patches now (if not, let me know which one I missed). I tested the patches, found 3 leaks, one of them I mentioned here, 2 were related to the code but not a direct result of this series I believe, so I'll tackle them some other time as a follow up. Overall, I think we're good to push this starting with the 7.3.0 cycle. Now, in v4 I promised to share my adjustments I made on top of your code. Some of them you already handled yourself in v6, so I rebased and performed a bunch of changes, so here they are: https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments Note, that I only split them into multiple patches so that they're easier to read, but I'm very well aware that probably none of them cannot be compiled on its own (I didn't bother to be that thorough), it's just to give you an idea what I've failed to express with words until we came to this v6. Please let me know your opinion on the changes so that: a) I can send it as a *proper* follow up series b) you can incorporate some of the changes into your series I don't care either way. Regards and thanks for being patient with the review process :), Erik

On Wed, 31 Mar 2021 16:00:48 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
This patch series follows the previously-merged series which added support for transient mediated devices. This series expands mdev support to include persistent device definitions. Again, it relies on mdevctl as the backend.
It follows the common libvirt pattern of APIs by adding the following new APIs for node devices: - virNodeDeviceDefineXML() - defines a persistent device - virNodeDeviceUndefine() - undefines a persistent device - virNodeDeviceCreate() - starts a previously-defined device
It also adds virsh commands mapping to these new APIs: nodedev-define, nodedev-undefine, and nodedev-start.
Since we rely on mdevctl for the definition of mediated devices, we need a way to stay up-to-date with devices that are defined by mdevctl (outside of libvirt). The method for staying up-to-date is currently a little bit crude due to the fact that mdevctl does not emit any events when new devices are added or removed. As a workaround, we create a file monitor for the mdevctl config directory and re-query mdevctl when we detect changes within that directory. In the future, mdevctl may introduce a more elegant solution.
Changes in v6: - rebase to git master again - remove typedefs for various *Ptr types, since they're now discouraged in libvirt.
Okay, so I think I ACKed all of the patches now (if not, let me know which one I missed). I tested the patches, found 3 leaks, one of them I mentioned here, 2 were related to the code but not a direct result of this series I believe, so I'll tackle them some other time as a follow up. Overall, I think we're good to push this starting with the 7.3.0 cycle.
I think everything else has been acked.
Now, in v4 I promised to share my adjustments I made on top of your code. Some of them you already handled yourself in v6, so I rebased and performed a bunch of changes, so here they are: https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments
Note, that I only split them into multiple patches so that they're easier to read, but I'm very well aware that probably none of them cannot be compiled on its own (I didn't bother to be that thorough), it's just to give you an idea what I've failed to express with words until we came to this v6. Please let me know your opinion on the changes so that: a) I can send it as a *proper* follow up series b) you can incorporate some of the changes into your series
Thanks for that. I think the changes look reasonable, and I think the benefits outweigh the drawbacks. I have a few changes I'd like to make to your commits. See the top 3 commits at [1] for details. Would you like me to just incorporate your stuff into my original series, or keep them as separate commits on top of my series? Jonathon [1] https://gitlab.com/jjongsma/libvirt/-/commits/mdevctl-adjustments/

On Thu, Apr 01, 2021 at 10:18:45AM -0500, Jonathon Jongsma wrote:
On Wed, 31 Mar 2021 16:00:48 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
This patch series follows the previously-merged series which added support for transient mediated devices. This series expands mdev support to include persistent device definitions. Again, it relies on mdevctl as the backend.
It follows the common libvirt pattern of APIs by adding the following new APIs for node devices: - virNodeDeviceDefineXML() - defines a persistent device - virNodeDeviceUndefine() - undefines a persistent device - virNodeDeviceCreate() - starts a previously-defined device
It also adds virsh commands mapping to these new APIs: nodedev-define, nodedev-undefine, and nodedev-start.
Since we rely on mdevctl for the definition of mediated devices, we need a way to stay up-to-date with devices that are defined by mdevctl (outside of libvirt). The method for staying up-to-date is currently a little bit crude due to the fact that mdevctl does not emit any events when new devices are added or removed. As a workaround, we create a file monitor for the mdevctl config directory and re-query mdevctl when we detect changes within that directory. In the future, mdevctl may introduce a more elegant solution.
Changes in v6: - rebase to git master again - remove typedefs for various *Ptr types, since they're now discouraged in libvirt.
Okay, so I think I ACKed all of the patches now (if not, let me know which one I missed). I tested the patches, found 3 leaks, one of them I mentioned here, 2 were related to the code but not a direct result of this series I believe, so I'll tackle them some other time as a follow up. Overall, I think we're good to push this starting with the 7.3.0 cycle.
I think everything else has been acked.
Now, in v4 I promised to share my adjustments I made on top of your code. Some of them you already handled yourself in v6, so I rebased and performed a bunch of changes, so here they are: https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments
Note, that I only split them into multiple patches so that they're easier to read, but I'm very well aware that probably none of them cannot be compiled on its own (I didn't bother to be that thorough), it's just to give you an idea what I've failed to express with words until we came to this v6. Please let me know your opinion on the changes so that: a) I can send it as a *proper* follow up series b) you can incorporate some of the changes into your series
Thanks for that. I think the changes look reasonable, and I think the benefits outweigh the drawbacks. I have a few changes I'd like to make to your commits. See the top 3 commits at [1] for details. Would you like me to just incorporate your stuff into my original series, or keep them as separate commits on top of my series?
I like your ^additional cleanup. As for the 30 patches - I'd like to avoid having to go through most of them again, so no, please go ahead an merge this series as is. The follow up changes that started to discuss are not functional anyway, so posting them separately is actually preferable. As for the follow up series - now that you see what my intentions wrt abstracting the mdevctl cmdline building code as much as possible were, feel free to take my commits, apply your 3 changes, shuffle them around as you need, and post it as a proper follow up series that can compile after every single patch :) and I'll review it. Regards, Erik

On Wed, 7 Apr 2021 08:05:17 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Thu, Apr 01, 2021 at 10:18:45AM -0500, Jonathon Jongsma wrote:
On Wed, 31 Mar 2021 16:00:48 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Fri, Mar 26, 2021 at 11:47:56AM -0500, Jonathon Jongsma wrote:
This patch series follows the previously-merged series which added support for transient mediated devices. This series expands mdev support to include persistent device definitions. Again, it relies on mdevctl as the backend.
It follows the common libvirt pattern of APIs by adding the following new APIs for node devices: - virNodeDeviceDefineXML() - defines a persistent device - virNodeDeviceUndefine() - undefines a persistent device - virNodeDeviceCreate() - starts a previously-defined device
It also adds virsh commands mapping to these new APIs: nodedev-define, nodedev-undefine, and nodedev-start.
Since we rely on mdevctl for the definition of mediated devices, we need a way to stay up-to-date with devices that are defined by mdevctl (outside of libvirt). The method for staying up-to-date is currently a little bit crude due to the fact that mdevctl does not emit any events when new devices are added or removed. As a workaround, we create a file monitor for the mdevctl config directory and re-query mdevctl when we detect changes within that directory. In the future, mdevctl may introduce a more elegant solution.
Changes in v6: - rebase to git master again - remove typedefs for various *Ptr types, since they're now discouraged in libvirt.
Okay, so I think I ACKed all of the patches now (if not, let me know which one I missed). I tested the patches, found 3 leaks, one of them I mentioned here, 2 were related to the code but not a direct result of this series I believe, so I'll tackle them some other time as a follow up. Overall, I think we're good to push this starting with the 7.3.0 cycle.
I think everything else has been acked.
Now, in v4 I promised to share my adjustments I made on top of your code. Some of them you already handled yourself in v6, so I rebased and performed a bunch of changes, so here they are: https://gitlab.com/eskultety/libvirt/-/commits/mdev-adjustments
Note, that I only split them into multiple patches so that they're easier to read, but I'm very well aware that probably none of them cannot be compiled on its own (I didn't bother to be that thorough), it's just to give you an idea what I've failed to express with words until we came to this v6. Please let me know your opinion on the changes so that: a) I can send it as a *proper* follow up series b) you can incorporate some of the changes into your series
Thanks for that. I think the changes look reasonable, and I think the benefits outweigh the drawbacks. I have a few changes I'd like to make to your commits. See the top 3 commits at [1] for details. Would you like me to just incorporate your stuff into my original series, or keep them as separate commits on top of my series?
I like your ^additional cleanup.
As for the 30 patches - I'd like to avoid having to go through most of them again, so no, please go ahead an merge this series as is. The follow up changes that started to discuss are not functional anyway, so posting them separately is actually preferable.
As for the follow up series - now that you see what my intentions wrt abstracting the mdevctl cmdline building code as much as possible were, feel free to take my commits, apply your 3 changes, shuffle them around as you need, and post it as a proper follow up series that can compile after every single patch :) and I'll review it.
Sounds good. Will send a followup series shortly. Thanks! Jonathon
participants (5)
-
Daniel P. Berrangé
-
Erik Skultety
-
John Ferlan
-
Jonathon Jongsma
-
Ján Tomko