On 2/1/24 11:07 AM, Boris Fiuczynski wrote:
On 1/31/24 22:41, Jonathon Jongsma wrote:On 1/19/24 10:38 AM, Boris Fiuczynski wrote:Implement the API functions in the node device driver by using mdevctl modify with the options defined and live. Increase the minimum mdevctl version to 1.3.0 in spec file to ensure support exists in mdevctl.mdevctl 1.3.0 was only released about 3 weeks ago, so I don't think we can add an unconditional dependency on it. I think we'll have to make sure that we can degrade gracefully when the required version isn't available.What is your suggestion?1) Adding a mdevctl version check to the runtime code, which in general can cause trouble interpreting the verion number with regard to distro backports. 2) Before really calling modify find out if mdevctl supports it by issuing e.g. "mdevctl modify --live --help". If live is support the RC is 0 if it is not supported the RC is 2.3) Something else?
Hmm, I was thinking that we only needed the 1.3.0 version for the --live and --defined options, but I now realize that we also need the --jsonfile argument from 1.3.0. That makes it a bit harder to degrade gracefully.
However, even if 1.3.0 is installed, not all devices will be able to be live-updated. So we need to handle errors in command execution regardless. If the user has an older mdevctl, the command will return an error and the user will be unable to update an mdev via the new API, which is the same behavior as they had before this patch. If the user has a newer mdevctl, it will just work. Maybe that's good enough for now.
Jonathon
Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> --- libvirt.spec.in | 2 +- src/node_device/node_device_driver.c | 181 ++++++++++++++++++- src/node_device/node_device_driver.h | 11 ++ src/node_device/node_device_udev.c | 1 + tests/nodedevmdevctldata/mdevctl-modify.argv | 19 ++ tests/nodedevmdevctltest.c | 64 +++++++ 6 files changed, 275 insertions(+), 3 deletions(-) create mode 100644 tests/nodedevmdevctldata/mdevctl-modify.argv diff --git a/libvirt.spec.in b/libvirt.spec.in index 8413e3c19a..3ed14fa63c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -613,7 +613,7 @@ Requires: libvirt-libs = %{version}-%{release} # needed for device enumeration Requires: systemd >= 185 # For managing persistent mediated devices -Requires: mdevctl +Requires: mdevctl >= 1.3.0 # for modprobe of pci devices Requires: module-init-toolsdiff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.cindex d67c95d68d..dd57e9ca5b 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -54,7 +54,7 @@ virNodeDeviceDriverState *driver; VIR_ENUM_IMPL(virMdevctlCommand, MDEVCTL_CMD_LAST, - "start", "stop", "define", "undefine", "create" + "start", "stop", "define", "undefine", "create", "modify" ); @@ -754,6 +754,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, case MDEVCTL_CMD_START: case MDEVCTL_CMD_DEFINE: case MDEVCTL_CMD_UNDEFINE: + case MDEVCTL_CMD_MODIFY: cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL); break; case MDEVCTL_CMD_LAST: @@ -767,6 +768,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, switch (cmd_type) { case MDEVCTL_CMD_CREATE: case MDEVCTL_CMD_DEFINE: + case MDEVCTL_CMD_MODIFY: if (!def->caps->data.mdev.parent_addr) { virReportError(VIR_ERR_INTERNAL_ERROR,_("unable to find parent device '%1$s'"), def->parent);@@ -783,7 +785,8 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def, virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin"); virCommandSetInputBuffer(cmd, inbuf); - virCommandSetOutputBuffer(cmd, outbuf); + if (outbuf) + virCommandSetOutputBuffer(cmd, outbuf); break; case MDEVCTL_CMD_UNDEFINE:@@ -868,6 +871,61 @@ virMdevctlDefine(virNodeDeviceDef *def, char **uuid)} +/* gets a virCommand object that executes a mdevctl command to modify a + * a device to an updated version + */ +virCommand* +nodeDeviceGetMdevctlModifyCommand(virNodeDeviceDef *def, + bool defined, + bool live, + char **errmsg) +{ + virCommand *cmd = nodeDeviceGetMdevctlCommand(def, + MDEVCTL_CMD_MODIFY, + NULL, errmsg); + + if (!cmd) + return NULL; + + if (defined) + virCommandAddArg(cmd, "--defined"); + + if (live) + virCommandAddArg(cmd, "--live"); + + return cmd; +} + + +static int +virMdevctlModify(virNodeDeviceDef *def, + bool defined, + bool live) +{ + int status; + g_autofree char *errmsg = NULL; + g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlModifyCommand(def, + defined, + live, + &errmsg); + + if (!cmd) + return -1; + + if (virCommandRun(cmd, &status) < 0) + return -1; + + if (status != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to modify mediated device: %1$s"), + MDEVCTL_ERROR(errmsg)); + return -1; + } + + return 0; +} + + static virNodeDevicePtr nodeDeviceCreateXMLMdev(virConnectPtr conn, virNodeDeviceDef *def) @@ -2108,3 +2166,122 @@ nodeDeviceIsActive(virNodeDevice *device) virNodeDeviceObjEndAPI(&obj); return ret; } + + +static int +nodeDeviceDefCompareMdevs(virNodeDeviceDef *def_cur, + virNodeDeviceDef *def_upd, + bool live) +{ + virNodeDevCapsDef *caps = NULL; + virNodeDevCapMdev *cap_mdev_cur = NULL; + virNodeDevCapMdev *cap_mdev_upd = NULL; + + for (caps = def_cur->caps; caps != NULL; caps = caps->next) { + if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { + cap_mdev_cur = &caps->data.mdev; + } + } + if (!cap_mdev_cur) + return -1; + + for (caps = def_upd->caps; caps != NULL; caps = caps->next) { + if (caps->data.type == VIR_NODE_DEV_CAP_MDEV) { + cap_mdev_upd = &caps->data.mdev; + } + } + if (!cap_mdev_upd) + return -1; + + if (STRNEQ_NULLABLE(cap_mdev_cur->uuid, cap_mdev_upd->uuid)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("uuid mismatch (mdev:'%1$s' update:'%2$s')"),I'd suggest a more complete description of the error so the context is a bit more clear when scanning the log. Something like: "Cannot update device $NAME, uuid mismatch". I'm not sure if we need to log the actual values.+ cap_mdev_cur->uuid, + cap_mdev_upd->uuid); + return -1; + } + // for a live update the types of the active configs must match! + if (live &&+ STRNEQ_NULLABLE(cap_mdev_cur->active_config.type, cap_mdev_upd->active_config.type)) {+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("type mismatch (mdev:'%1$s' update:'%2$s')"),same+ cap_mdev_cur->active_config.type, + cap_mdev_upd->active_config.type); + return -1; + }+ if (STRNEQ_NULLABLE(cap_mdev_cur->parent_addr, cap_mdev_upd->parent_addr)) {+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,+ _("parent address mismatch (mdev:'%1$s' update:'%2$s')"),sameChanged the message as you suggested and added at the end the current value so that someone can change it in the XML used for the update.E.g. virReportError(VIR_ERR_CONFIG_UNSUPPORTED,_("Cannot update device '%1$s', parent address mismatch (current parent address '%2$s')"),dev_cur->name, cap_mdev_cur->parent_addr);+ cap_mdev_cur->parent_addr, + cap_mdev_upd->parent_addr); + return -1; + } + + return 0; +} + + +int +nodeDeviceUpdateXML(virNodeDevice *device, + const char *xmlDesc, + unsigned int flags) +{ + int ret = -1; + virNodeDeviceObj *obj = NULL; + virNodeDeviceDef *def_cur; + g_autoptr(virNodeDeviceDef) def_upd = NULL; + const char *virt_type = NULL; + bool updated = false; + + virCheckFlags(VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE | + VIR_NODE_DEVICE_UPDATE_XML_AFFECT_CONFIG, -1); + + if (nodeDeviceInitWait() < 0) + return -1; + + if (!(obj = nodeDeviceObjFindByName(device->name))) + return -1; + def_cur = virNodeDeviceObjGetDef(obj); + + virt_type = virConnectGetType(device->conn); ++ if (virNodeDeviceUpdateXMLEnsureACL(device->conn, def_cur, flags) < 0)+ goto cleanup; ++ if (!(def_upd = virNodeDeviceDefParse(xmlDesc, NULL, EXISTING_DEVICE, virt_type, + &driver->parserCallbacks, NULL, true)))+ goto cleanup; + + if (nodeDeviceHasCapability(def_cur, VIR_NODE_DEV_CAP_MDEV) && + nodeDeviceHasCapability(def_upd, VIR_NODE_DEV_CAP_MDEV)) {+ // Checks flags are valid for the state and sets flags for current if flags not set+ if (virNodeDeviceObjUpdateModificationImpact(obj, &flags) < 0) + goto cleanup; ++ // Compare def_cur and def_upd for compatibleness e.g. parent, type and uuid+ if (nodeDeviceDefCompareMdevs(def_cur, def_upd,+ (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE)) < 0)+ goto cleanup; + + // Update now+ VIR_DEBUG("Update node device '%s' with mdevctl", def_cur->name);+ if (virMdevctlModify(def_upd,+ (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_CONFIG), + (flags & VIR_NODE_DEVICE_UPDATE_XML_AFFECT_LIVE)) < 0) {+ goto cleanup; + }; + // Detect updates and also trigger events + updated = true; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported device type"));you need to goto cleanup here otherwise the return value will be set to 0.Ups, certainly correct.+ } + + ret = 0; + cleanup: + virNodeDeviceObjEndAPI(&obj); + if (updated) + nodeDeviceUpdateMediatedDevices(); + + return ret; +}diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.hindex 4dce7e6f17..5751625eda 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -39,6 +39,7 @@ typedef enum {* separation makes our code more readable in terms of knowing when we're * starting a defined device and when we're creating a transient one */MDEVCTL_CMD_CREATE, + MDEVCTL_CMD_MODIFY, MDEVCTL_CMD_LAST, } virMdevctlCommand; @@ -186,3 +187,13 @@ virCommand* nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def, bool autostart, char **errmsg); + +virCommand* +nodeDeviceGetMdevctlModifyCommand(virNodeDeviceDef *def, + bool defined, + bool live, + char **errmsg); +int +nodeDeviceUpdateXML(virNodeDevice *dev, + const char *xmlDesc, + unsigned int flags);diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.cindex 57368a96c3..4cd9d285fa 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c@@ -2403,6 +2403,7 @@ static virNodeDeviceDriver udevNodeDeviceDriver = {.nodeDeviceGetAutostart = nodeDeviceGetAutostart, /* 7.8.0 */ .nodeDeviceIsPersistent = nodeDeviceIsPersistent, /* 7.8.0 */ .nodeDeviceIsActive = nodeDeviceIsActive, /* 7.8.0 */ + .nodeDeviceUpdateXML = nodeDeviceUpdateXML, /* 10.1.0 */ };diff --git a/tests/nodedevmdevctldata/mdevctl-modify.argv b/tests/nodedevmdevctldata/mdevctl-modify.argvnew file mode 100644 index 0000000000..6acb9ca0bf --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-modify.argv @@ -0,0 +1,19 @@ +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--defined +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--live +mdevctl \ +modify \ +--parent=0.0.0052 \ +--jsonfile=/dev/stdin \ +--uuid=c60cc60c-c60c-c60c-c60c-c60cc60cc60c \ +--defined \ +--live diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 852d9ed6e7..37978c9a5c 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -63,6 +63,7 @@ testMdevctlCmd(virMdevctlCommand cmd_type, case MDEVCTL_CMD_START: case MDEVCTL_CMD_STOP: case MDEVCTL_CMD_UNDEFINE: + case MDEVCTL_CMD_MODIFY: create = EXISTING_DEVICE; break; case MDEVCTL_CMD_LAST:@@ -173,6 +174,64 @@ testMdevctlAutostart(const void *data G_GNUC_UNUSED)return ret; } + +static int +testMdevctlModify(const void *data G_GNUC_UNUSED) +{ + g_autoptr(virNodeDeviceDef) def = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) definedcmd = NULL; + g_autoptr(virCommand) livecmd = NULL; + g_autoptr(virCommand) bothcmd = NULL; + g_autofree char *errmsg = NULL; + /* just concatenate both calls into the same output file */ + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-modify.argv", + abs_srcdir); + g_autofree char *mdevxml =+ g_strdup_printf("%s/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml",+ abs_srcdir);+ g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew();++ if (!(def = virNodeDeviceDefParse(NULL, mdevxml, CREATE_DEVICE, VIRT_TYPE,+ &parser_callbacks, NULL, false))) + return -1; + + virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);You should also capture the stdin since that's really the majority of the command input.Ok, I agree and added the stdin check.++ if (!(definedcmd = nodeDeviceGetMdevctlModifyCommand(def, true, false, &errmsg)))+ goto cleanup; + + if (virCommandRun(definedcmd, NULL) < 0) + goto cleanup; ++ if (!(livecmd = nodeDeviceGetMdevctlModifyCommand(def, false, true, &errmsg)))+ goto cleanup; + + if (virCommandRun(livecmd, NULL) < 0) + goto cleanup; ++ if (!(bothcmd = nodeDeviceGetMdevctlModifyCommand(def, true, true, &errmsg)))+ goto cleanup; + + if (virCommandRun(bothcmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; ++ if (virTestCompareToFileFull(actualCmdline, cmdlinefile, false) < 0)+ goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} + static int testMdevctlListDefined(const void *data G_GNUC_UNUSED) { @@ -457,6 +516,9 @@ mymain(void) #define DO_TEST_AUTOSTART() \ DO_TEST_FULL("autostart mdevs", testMdevctlAutostart, NULL) +#define DO_TEST_MODIFY() \ + DO_TEST_FULL("modify mdevs", testMdevctlModify, NULL) + #define DO_TEST_PARSE_JSON(filename) \DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename)@@ -485,6 +547,8 @@ mymain(void) DO_TEST_AUTOSTART(); + DO_TEST_MODIFY(); + done: nodedevTestDriverFree(driver);Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxxThanks_______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
_______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
