[PATCH 00/18] QMP schema validation fixes
Turns out we still had wired up use of 'device' argument of block_set_io_throttle which got deprecated and our valiator didn't catch it due to multiple reasons. Address all of that. Peter Krempa (18): qemuMonitorTestAddItemVerbatim: Simplify cleanup qemuMonitorTestAddHandler: Remove return value virRaiseErrorLog: Don't skip error printing when enabling debug logging env variable testQemuMonitorJSONAttachChardev: Move all setup code under virTestRun qemuMonitorJSONTestAttachOneChardev: Rewrite using qemuMonitorTestAddItemVerbatim qemuMonitorTestAddItemExpect: Remove unused helper testQemuAgentCPU: Rewrite using qemuMonitorTestAddItemVerbatim testQemuAgentFSTrim: Rewrite using qemuMonitorTestAddItemVerbatim testQemuMonitorJSONqemuMonitorJSONSendKeyHoldtime: Rewrite using qemuMonitorTestAddItemVerbatim testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle: Rewrite using qemuMonitorTestAddItemVerbatim Drop unused qemuMonitorTestAddItemParams qemumonitorjsontest: Drop 'schema-meta' case qemuDiskConfigBlkdeviotuneEnabled: Make 'disk' argument const qemu: Refuse setting <iotune> for 'SD' disks qemumonitorjsontest: Use 'id' instead of deprecated 'device' argument of 'block_set_io_throttle' qemuMonitorGetBlockIoThrottle: Drop 'diskalias' argument qemuMonitorSetBlockIoThrottle: Drop 'diskalias' argument testQEMUSchemaValidateObjectMember: validate QMP object member deprecation src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 2 +- src/qemu/qemu_driver.c | 40 ++---- src/qemu/qemu_hotplug.c | 4 +- src/qemu/qemu_monitor.c | 12 +- src/qemu/qemu_monitor.h | 2 - src/qemu/qemu_monitor_json.c | 14 +- src/qemu/qemu_monitor_json.h | 2 - src/qemu/qemu_process.c | 12 +- src/qemu/qemu_validate.c | 8 ++ src/util/virerror.c | 4 +- tests/qemuagenttest.c | 92 ++++++------ tests/qemumonitorjsontest.c | 153 ++++++++++---------- tests/qemumonitortestutils.c | 266 +++-------------------------------- tests/qemumonitortestutils.h | 16 +-- tests/testutilsqemuschema.c | 4 + 16 files changed, 191 insertions(+), 442 deletions(-) -- 2.40.1
Reformat the JSON string before allocating the test data structure so that we don't have to free it if the reformatting fails. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 4e99c4b54e..cfe78b258a 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -673,24 +673,21 @@ qemuMonitorTestAddItemVerbatim(qemuMonitorTest *test, const char *response) { struct qemuMonitorTestHandlerData *data; + char *reformatted = NULL; + + if (!(reformatted = virJSONStringReformat(command, false))) + return -1; data = g_new0(struct qemuMonitorTestHandlerData, 1); data->response = g_strdup(response); data->cmderr = g_strdup(cmderr); - - data->command_name = virJSONStringReformat(command, false); - if (!data->command_name) - goto error; + data->command_name = g_steal_pointer(&reformatted); return qemuMonitorTestAddHandler(test, command, qemuMonitorTestProcessCommandVerbatim, data, qemuMonitorTestHandlerDataFree); - - error: - qemuMonitorTestHandlerDataFree(data); - return -1; } -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Reformat the JSON string before allocating the test data structure so that we don't have to free it if the reformatting fails.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function always returns 0. Remove the return value and fix callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 39 ++++++++++++-------------- tests/qemumonitortestutils.c | 54 +++++++++++++++++++++--------------- tests/qemumonitortestutils.h | 2 +- 3 files changed, 49 insertions(+), 46 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 7724df2742..7d8c66707d 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -475,10 +475,9 @@ testQemuAgentShutdown(const void *data) priv.event = QEMU_AGENT_EVENT_SHUTDOWN; priv.mode = "halt"; - if (qemuMonitorTestAddHandler(test, "guest-shutdown", - qemuAgentShutdownTestMonitorHandler, - &priv, NULL) < 0) - return -1; + qemuMonitorTestAddHandler(test, "guest-shutdown", + qemuAgentShutdownTestMonitorHandler, + &priv, NULL); if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), QEMU_AGENT_SHUTDOWN_HALT) < 0) @@ -487,10 +486,9 @@ testQemuAgentShutdown(const void *data) priv.event = QEMU_AGENT_EVENT_SHUTDOWN; priv.mode = "powerdown"; - if (qemuMonitorTestAddHandler(test, "guest-shutdown", - qemuAgentShutdownTestMonitorHandler, - &priv, NULL) < 0) - return -1; + qemuMonitorTestAddHandler(test, "guest-shutdown", + qemuAgentShutdownTestMonitorHandler, + &priv, NULL); if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), QEMU_AGENT_SHUTDOWN_POWERDOWN) < 0) @@ -499,11 +497,10 @@ testQemuAgentShutdown(const void *data) priv.event = QEMU_AGENT_EVENT_RESET; priv.mode = "reboot"; - if (qemuMonitorTestAddHandler(test, - "guest-shutdown", - qemuAgentShutdownTestMonitorHandler, - &priv, NULL) < 0) - return -1; + qemuMonitorTestAddHandler(test, + "guest-shutdown", + qemuAgentShutdownTestMonitorHandler, + &priv, NULL); if (qemuAgentShutdown(qemuMonitorTestGetAgent(test), QEMU_AGENT_SHUTDOWN_REBOOT) < 0) @@ -691,10 +688,9 @@ testQemuAgentTimeout(const void *data) if (virTestGetExpensive() == 0) return EXIT_AM_SKIP; - if (qemuMonitorTestAddHandler(test, NULL, - qemuAgentTimeoutTestMonitorHandler, - NULL, NULL) < 0) - return -1; + qemuMonitorTestAddHandler(test, NULL, + qemuAgentTimeoutTestMonitorHandler, + NULL, NULL); if (qemuAgentFSFreeze(qemuMonitorTestGetAgent(test), NULL, 0) != -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -706,11 +702,10 @@ testQemuAgentTimeout(const void *data) if (qemuMonitorTestAddAgentSyncResponse(test) < 0) return -1; - if (qemuMonitorTestAddHandler(test, - NULL, - qemuAgentTimeoutTestMonitorHandler, - NULL, NULL) < 0) - return -1; + qemuMonitorTestAddHandler(test, + NULL, + qemuAgentTimeoutTestMonitorHandler, + NULL, NULL); if (qemuAgentArbitraryCommand(qemuMonitorTestGetAgent(test), "{\"execute\":\"ble\"}", diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index cfe78b258a..40ef057271 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -441,7 +441,7 @@ qemuMonitorTestFree(qemuMonitorTest *test) } -int +void qemuMonitorTestAddHandler(qemuMonitorTest *test, const char *identifier, qemuMonitorTestResponseCallback cb, @@ -460,8 +460,6 @@ qemuMonitorTestAddHandler(qemuMonitorTest *test, VIR_WITH_MUTEX_LOCK_GUARD(&test->lock) { VIR_APPEND_ELEMENT(test->items, test->nitems, item); } - - return 0; } void * @@ -604,10 +602,12 @@ qemuMonitorTestAddItem(qemuMonitorTest *test, data->command_name = g_strdup(command_name); data->response = g_strdup(response); - return qemuMonitorTestAddHandler(test, - command_name, - qemuMonitorTestProcessCommandDefault, - data, qemuMonitorTestHandlerDataFree); + qemuMonitorTestAddHandler(test, + command_name, + qemuMonitorTestProcessCommandDefault, + data, qemuMonitorTestHandlerDataFree); + + return 0; } @@ -684,10 +684,12 @@ qemuMonitorTestAddItemVerbatim(qemuMonitorTest *test, data->cmderr = g_strdup(cmderr); data->command_name = g_steal_pointer(&reformatted); - return qemuMonitorTestAddHandler(test, - command, - qemuMonitorTestProcessCommandVerbatim, - data, qemuMonitorTestHandlerDataFree); + qemuMonitorTestAddHandler(test, + command, + qemuMonitorTestProcessCommandVerbatim, + data, qemuMonitorTestHandlerDataFree); + + return 0; } @@ -732,10 +734,12 @@ qemuMonitorTestAddAgentSyncResponse(qemuMonitorTest *test) return -1; } - return qemuMonitorTestAddHandler(test, - "agent-sync", - qemuMonitorTestProcessGuestAgentSync, - NULL, NULL); + qemuMonitorTestAddHandler(test, + "agent-sync", + qemuMonitorTestProcessGuestAgentSync, + NULL, NULL); + + return 0; } @@ -841,10 +845,12 @@ qemuMonitorTestAddItemParams(qemuMonitorTest *test, va_end(args); - return qemuMonitorTestAddHandler(test, - cmdname, - qemuMonitorTestProcessCommandWithArgs, - data, qemuMonitorTestHandlerDataFree); + qemuMonitorTestAddHandler(test, + cmdname, + qemuMonitorTestProcessCommandWithArgs, + data, qemuMonitorTestHandlerDataFree); + + return 0; error: va_end(args); @@ -939,10 +945,12 @@ qemuMonitorTestAddItemExpect(qemuMonitorTest *test, } } - return qemuMonitorTestAddHandler(test, - cmdname, - qemuMonitorTestProcessCommandWithArgStr, - data, qemuMonitorTestHandlerDataFree); + qemuMonitorTestAddHandler(test, + cmdname, + qemuMonitorTestProcessCommandWithArgStr, + data, qemuMonitorTestHandlerDataFree); + + return 0; } diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index ffcb32a5cc..df0d544720 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -30,7 +30,7 @@ typedef int (*qemuMonitorTestResponseCallback)(qemuMonitorTest *test, qemuMonitorTestItem *item, const char *message); -int +void qemuMonitorTestAddHandler(qemuMonitorTest *test, const char *identifier, qemuMonitorTestResponseCallback cb, -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
The function always returns 0. Remove the return value and fix callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 39 ++++++++++++-------------- tests/qemumonitortestutils.c | 54 +++++++++++++++++++++--------------- tests/qemumonitortestutils.h | 2 +- 3 files changed, 49 insertions(+), 46 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When a user requests debug logging by setting the environment variable: LIBVIRT_DEBUG=1 we should log any errors regardless of the setting of e.g. 'LIBVIRT_LOG_OUTPUTS' as the code will log every 'debug' and 'info' level message to stderr but will skip 'error' level messages. This obviously makes debugging things very complicated as you can get to a situation when the error itself is missing. This can happen e.g. in tests. Fix the issue by probing the default log level and calling the logger if it's set for VIR_LOG_DEBUG. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virerror.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index 0bfa803b1f..453f19514e 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -775,9 +775,11 @@ void virRaiseErrorLog(const char *filename, * hate & thus disable that too. If the daemon has set * a priority filter though, we should always forward * all errors to the logging code. + * Similarly when debug priority is the default we want to log the error. */ if (virLogGetNbOutputs() > 0 || - virErrorLogPriorityFilter) + virErrorLogPriorityFilter || + virLogGetDefaultPriority() == VIR_LOG_DEBUG) virLogMessage(&virLogSelf, priority, filename, linenr, funcname, -- 2.40.1
On 5/25/23 10:13 AM, Peter Krempa wrote:
When a user requests debug logging by setting the environment variable:
LIBVIRT_DEBUG=1
we should log any errors regardless of the setting of e.g. 'LIBVIRT_LOG_OUTPUTS' as the code will log every 'debug' and 'info' level message to stderr but will skip 'error' level messages.
This obviously makes debugging things very complicated as you can get to a situation when the error itself is missing.
This can happen e.g. in tests.
Fix the issue by probing the default log level and calling the logger if it's set for VIR_LOG_DEBUG.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virerror.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virerror.c b/src/util/virerror.c index 0bfa803b1f..453f19514e 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -775,9 +775,11 @@ void virRaiseErrorLog(const char *filename, * hate & thus disable that too. If the daemon has set * a priority filter though, we should always forward * all errors to the logging code. + * Similarly when debug priority is the default we want to log the error. */ if (virLogGetNbOutputs() > 0 || - virErrorLogPriorityFilter) + virErrorLogPriorityFilter || + virLogGetDefaultPriority() == VIR_LOG_DEBUG) virLogMessage(&virLogSelf, priority, filename, linenr, funcname,
LIBVIRT_DEBUG accepts the following values: (1 or "debug"), (2 or "info"), (3 or "warning"), and (4 or "error"). In addition, the --verbose option sets the default log priority to VIR_LOG_INFO. It seems a little bit odd to add a workaround only for the "debug" case. Jonathon
Any failure which happens outside is hard to debug as errors will be reset and not raised. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 60 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1db1f2b949..3dabd5c00d 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -596,19 +596,35 @@ testQemuMonitorJSONGetTPMModels(const void *opaque) struct qemuMonitorJSONTestAttachChardevData { - qemuMonitorTest *test; virDomainChrSourceDef *chr; const char *expectPty; bool fail; + + virDomainXMLOption *xmlopt; + GHashTable *schema; + const char *expectargs; + const char *reply; }; static int testQemuMonitorJSONAttachChardev(const void *opaque) { const struct qemuMonitorJSONTestAttachChardevData *data = opaque; + g_autoptr(qemuMonitorTest) test = qemuMonitorTestNewSchema(data->xmlopt, data->schema); int rc; - if ((rc = qemuMonitorAttachCharDev(qemuMonitorTestGetMonitor(data->test), + if (!test) + return -1; + + if (data->expectargs) { + g_autofree char *jsonreply = g_strdup_printf("{\"return\": {%s}}", NULLSTR_EMPTY(data->reply)); + + if (qemuMonitorTestAddItemExpect(test, "chardev-add", + data->expectargs, true, jsonreply) < 0) + return -1; + } + + if ((rc = qemuMonitorAttachCharDev(qemuMonitorTestGetMonitor(test), "alias", data->chr)) < 0) goto cleanup; @@ -643,37 +659,19 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOption *xmlopt, bool fail) { - struct qemuMonitorJSONTestAttachChardevData data = {0}; - g_autoptr(qemuMonitorTest) test = qemuMonitorTestNewSchema(xmlopt, schema); - g_autofree char *jsonreply = NULL; - g_autofree char *fulllabel = NULL; - - if (!test) - return -1; - - if (!reply) - reply = ""; - - jsonreply = g_strdup_printf("{\"return\": {%s}}", reply); - - fulllabel = g_strdup_printf("qemuMonitorJSONTestAttachChardev(%s)", label); - - if (expectargs && - qemuMonitorTestAddItemExpect(test, "chardev-add", - expectargs, true, jsonreply) < 0) - return -1; - - data.chr = chr; - data.fail = fail; - data.expectPty = expectPty; - data.test = test; - - if (virTestRun(fulllabel, &testQemuMonitorJSONAttachChardev, &data) < 0) - return -1; - - return 0; + struct qemuMonitorJSONTestAttachChardevData data = { .chr = chr, + .fail = fail, + .expectPty = expectPty, + .expectargs = expectargs, + .reply = reply, + .xmlopt = xmlopt, + .schema = schema }; + g_autofree char *fulllabel = g_strdup_printf("qemuMonitorJSONTestAttachChardev(%s)", label); + + return virTestRun(fulllabel, &testQemuMonitorJSONAttachChardev, &data); } + static int qemuMonitorJSONTestAttachChardev(virDomainXMLOption *xmlopt, GHashTable *schema) -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Any failure which happens outside is hard to debug as errors will be reset and not raised.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 60 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
'qemuMonitorTestAddItemExpect' doesn't do QMP schema validation. Since it's the only use we can reimplement it using 'qemuMonitorTestAddItemVerbatim' which does schema validation and remove the old code instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 3dabd5c00d..f6b6da372c 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -618,9 +618,18 @@ testQemuMonitorJSONAttachChardev(const void *opaque) if (data->expectargs) { g_autofree char *jsonreply = g_strdup_printf("{\"return\": {%s}}", NULLSTR_EMPTY(data->reply)); + g_autofree char *jsoncommand = NULL; + char *n; - if (qemuMonitorTestAddItemExpect(test, "chardev-add", - data->expectargs, true, jsonreply) < 0) + jsoncommand = g_strdup_printf("{\"execute\": \"chardev-add\", \"arguments\": %s, \"id\" : \"libvirt-1\"}", data->expectargs); + + /* data->expectargs has ' instead of " */ + for (n = jsoncommand; *n; n++) { + if (*n == '\'') + *n = '"'; + } + + if (qemuMonitorTestAddItemVerbatim(test, jsoncommand, NULL, jsonreply) < 0) return -1; } -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
'qemuMonitorTestAddItemExpect' doesn't do QMP schema validation. Since it's the only use we can reimplement it using 'qemuMonitorTestAddItemVerbatim' which does schema validation and remove the old code instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 95 ------------------------------------ tests/qemumonitortestutils.h | 7 --- 2 files changed, 102 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 40ef057271..9e6da78a8b 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -859,101 +859,6 @@ qemuMonitorTestAddItemParams(qemuMonitorTest *test, } -static int -qemuMonitorTestProcessCommandWithArgStr(qemuMonitorTest *test, - qemuMonitorTestItem *item, - const char *cmdstr) -{ - struct qemuMonitorTestHandlerData *data = item->opaque; - g_autoptr(virJSONValue) val = NULL; - virJSONValue *args; - g_autofree char *argstr = NULL; - const char *cmdname; - - if (!(val = virJSONValueFromString(cmdstr))) - return -1; - - if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - qemuMonitorTestError("Missing command name in %s", cmdstr); - return -1; - } - - if (STRNEQ(data->command_name, cmdname)) { - qemuMonitorTestErrorInvalidCommand(data->command_name, cmdname); - return -1; - } - - if (!(args = virJSONValueObjectGet(val, "arguments"))) { - qemuMonitorTestError("Missing arguments section for command '%s'", - data->command_name); - return -1; - } - - /* convert the arguments to string */ - if (!(argstr = virJSONValueToString(args, false))) - return -1; - - /* verify that the argument value is expected */ - if (STRNEQ(argstr, data->expectArgs)) { - qemuMonitorTestError("%s: expected arguments: '%s', got: '%s'", - data->command_name, - data->expectArgs, argstr); - return -1; - } - - /* arguments checked out, return the response */ - return qemuMonitorTestAddResponse(test, data->response); -} - - -/** - * qemuMonitorTestAddItemExpect: - * - * @test: test monitor object - * @cmdname: command name - * @cmdargs: expected arguments of the command - * @apostrophe: convert apostrophes (') in @cmdargs to quotes (") - * @response: simulated response of the command - * - * Simulates a qemu monitor command. Checks that the 'arguments' of the qmp - * command are expected. If @apostrophe is true apostrophes are converted to - * quotes for simplification of writing the strings into code. - */ -int -qemuMonitorTestAddItemExpect(qemuMonitorTest *test, - const char *cmdname, - const char *cmdargs, - bool apostrophe, - const char *response) -{ - struct qemuMonitorTestHandlerData *data; - - data = g_new0(struct qemuMonitorTestHandlerData, 1); - - data->command_name = g_strdup(cmdname); - data->response = g_strdup(response); - data->expectArgs = g_strdup(cmdargs); - - if (apostrophe) { - char *tmp = data->expectArgs; - - while (*tmp != '\0') { - if (*tmp == '\'') - *tmp = '"'; - - tmp++; - } - } - - qemuMonitorTestAddHandler(test, - cmdname, - qemuMonitorTestProcessCommandWithArgStr, - data, qemuMonitorTestHandlerDataFree); - - return 0; -} - - static void qemuMonitorTestEOFNotify(qemuMonitor *mon G_GNUC_UNUSED, virDomainObj *vm G_GNUC_UNUSED) diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index df0d544720..5de55172be 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -81,13 +81,6 @@ qemuMonitorTestAddItemParams(qemuMonitorTest *test, ...) G_GNUC_NULL_TERMINATED; -int -qemuMonitorTestAddItemExpect(qemuMonitorTest *test, - const char *cmdname, - const char *cmdargs, - bool apostrophe, - const char *response); - #define qemuMonitorTestNewSimple(xmlopt) \ qemuMonitorTestNew(xmlopt, NULL, NULL, NULL) #define qemuMonitorTestNewSchema(xmlopt, schema) \ -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 95 ------------------------------------ tests/qemumonitortestutils.h | 7 --- 2 files changed, 102 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 7d8c66707d..04faf7b83a 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -552,16 +552,6 @@ static const char testQemuAgentCPUResponse[] = " ]" "}"; -static const char testQemuAgentCPUArguments1[] = - "[{\"logical-id\":1,\"online\":false}]"; - -static const char testQemuAgentCPUArguments2[] = - "[{\"logical-id\":1,\"online\":true}," - "{\"logical-id\":3,\"online\":true}]"; - -static const char testQemuAgentCPUArguments3[] = - "[{\"logical-id\":3,\"online\":true}]"; - static int testQemuAgentCPU(const void *data) { @@ -595,26 +585,36 @@ testQemuAgentCPU(const void *data) if (qemuAgentUpdateCPUInfo(2, cpuinfo, nvcpus) < 0) return -1; - if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus", - "{ \"return\" : 1 }", - "vcpus", testQemuAgentCPUArguments1, - NULL) < 0) + if (qemuMonitorTestAddItemVerbatim(test, + "{\"execute\":\"guest-set-vcpus\"," + " \"arguments\": {" + " \"vcpus\":[{\"logical-id\":1,\"online\":false}]" + "}}", + NULL, + "{ \"return\" : 1 }") < 0) return -1; if (qemuAgentSetVCPUs(qemuMonitorTestGetAgent(test), cpuinfo, nvcpus) < 0) return -1; /* try to hotplug two, second one will fail */ - if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus", - "{ \"return\" : 1 }", - "vcpus", testQemuAgentCPUArguments2, - NULL) < 0) - return -1; - - if (qemuMonitorTestAddItemParams(test, "guest-set-vcpus", - "{ \"error\" : \"random error\" }", - "vcpus", testQemuAgentCPUArguments3, - NULL) < 0) + if (qemuMonitorTestAddItemVerbatim(test, + "{\"execute\":\"guest-set-vcpus\"," + " \"arguments\": {" + " \"vcpus\":[{\"logical-id\":1,\"online\":true}," + " {\"logical-id\":3,\"online\":true}]" + "}}", + NULL, + "{ \"return\" : 1 }") < 0) + return -1; + + if (qemuMonitorTestAddItemVerbatim(test, + "{\"execute\":\"guest-set-vcpus\"," + " \"arguments\": {" + " \"vcpus\":[{\"logical-id\":3,\"online\":true}]" + "}}", + NULL, + "{ \"error\" : \"random error\" }") < 0) return -1; if (qemuAgentUpdateCPUInfo(4, cpuinfo, nvcpus) < 0) -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 04faf7b83a..3c24a3139f 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -197,10 +197,11 @@ testQemuAgentFSTrim(const void *data) if (qemuMonitorTestAddAgentSyncResponse(test) < 0) return -1; - if (qemuMonitorTestAddItemParams(test, "guest-fstrim", - "{ \"return\" : {} }", - "minimum", "1337", - NULL) < 0) + if (qemuMonitorTestAddItemVerbatim(test, + "{\"execute\":\"guest-fstrim\"," + " \"arguments\": {\"minimum\":1337}}", + NULL, + "{ \"return\" : {}}") < 0) return -1; if (qemuAgentFSTrim(qemuMonitorTestGetAgent(test), 1337) < 0) -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuagenttest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f6b6da372c..ceb7108ac0 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2036,14 +2036,16 @@ testQemuMonitorJSONqemuMonitorJSONSendKeyHoldtime(const void *opaque) if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) return -1; - if (qemuMonitorTestAddItemParams(test, "send-key", - "{\"return\":{}}", - "hold-time", "31337", - "keys", "[{\"type\":\"number\",\"data\":43}," - "{\"type\":\"number\",\"data\":26}," - "{\"type\":\"number\",\"data\":46}," - "{\"type\":\"number\",\"data\":32}]", - NULL, NULL) < 0) + if (qemuMonitorTestAddItemVerbatim(test, + "{\"execute\":\"send-key\"," + " \"arguments\":{\"keys\":[{\"type\":\"number\",\"data\":43}," + " {\"type\":\"number\",\"data\":26}," + " {\"type\":\"number\",\"data\":46}," + " {\"type\":\"number\",\"data\":32}]," + " \"hold-time\":31337}," + " \"id\":\"libvirt-1\"}", + NULL, + "{ \"return\" : {}}") < 0) return -1; if (qemuMonitorJSONSendKey(qemuMonitorTestGetMonitor(test), -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 49 +++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index ceb7108ac0..d9323d94c3 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1865,25 +1865,36 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *opaque) expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20}; expectedInfo.group_name = g_strdup("group14"); - if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0 || - qemuMonitorTestAddItemParams(test, "block_set_io_throttle", - "{\"return\":{}}", - "device", "\"drive-virtio-disk1\"", - "bps", "1", "bps_rd", "2", "bps_wr", "3", - "iops", "4", "iops_rd", "5", "iops_wr", "6", - "bps_max", "7", "bps_rd_max", "8", - "bps_wr_max", "9", - "iops_max", "10", "iops_rd_max", "11", - "iops_wr_max", "12", "iops_size", "13", - "group", "\"group14\"", - "bps_max_length", "15", - "bps_rd_max_length", "16", - "bps_wr_max_length", "17", - "iops_max_length", "18", - "iops_rd_max_length", "19", - "iops_wr_max_length", "20", - NULL, NULL) < 0) - goto cleanup; + if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < 0) + return -1; + + if (qemuMonitorTestAddItemVerbatim(test, + "{\"execute\":\"block_set_io_throttle\"," + " \"arguments\":{\"device\": \"drive-virtio-disk1\"," + " \"bps\": 1," + " \"bps_rd\": 2," + " \"bps_wr\": 3," + " \"iops\": 4," + " \"iops_rd\": 5," + " \"iops_wr\": 6," + " \"bps_max\": 7," + " \"bps_rd_max\": 8," + " \"bps_wr_max\": 9," + " \"iops_max\": 10," + " \"iops_rd_max\": 11," + " \"iops_wr_max\": 12," + " \"iops_size\": 13," + " \"group\": \"group14\"," + " \"bps_max_length\": 15," + " \"bps_rd_max_length\": 16," + " \"bps_wr_max_length\": 17," + " \"iops_max_length\": 18," + " \"iops_rd_max_length\": 19," + " \"iops_wr_max_length\": 20}," + " \"id\":\"libvirt-2\"}", + NULL, + "{ \"return\" : {}}") < 0) + return -1; if (qemuMonitorJSONGetBlockIoThrottle(qemuMonitorTestGetMonitor(test), "drive-virtio-disk0", NULL, &info) < 0) -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Replace qemuMonitorTestAddItemParams by qemuMonitorTestAddItemVerbatim
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 49 +++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Using qemuMonitorTestAddItemVerbatim is more universal and that helper also does QMP schema validation. Remove the now unused helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 132 ----------------------------------- tests/qemumonitortestutils.h | 7 -- 2 files changed, 139 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 9e6da78a8b..8b8b02a790 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -469,19 +469,10 @@ qemuMonitorTestItemGetPrivateData(qemuMonitorTestItem *item) } -typedef struct _qemuMonitorTestCommandArgs qemuMonitorTestCommandArgs; -struct _qemuMonitorTestCommandArgs { - char *argname; - char *argval; -}; - - struct qemuMonitorTestHandlerData { char *command_name; char *cmderr; char *response; - size_t nargs; - qemuMonitorTestCommandArgs *args; char *expectArgs; }; @@ -489,20 +480,13 @@ static void qemuMonitorTestHandlerDataFree(void *opaque) { struct qemuMonitorTestHandlerData *data = opaque; - size_t i; if (!data) return; - for (i = 0; i < data->nargs; i++) { - g_free(data->args[i].argname); - g_free(data->args[i].argval); - } - g_free(data->command_name); g_free(data->cmderr); g_free(data->response); - g_free(data->args); g_free(data->expectArgs); g_free(data); } @@ -743,122 +727,6 @@ qemuMonitorTestAddAgentSyncResponse(qemuMonitorTest *test) } -static int -qemuMonitorTestProcessCommandWithArgs(qemuMonitorTest *test, - qemuMonitorTestItem *item, - const char *cmdstr) -{ - struct qemuMonitorTestHandlerData *data = item->opaque; - g_autoptr(virJSONValue) val = NULL; - virJSONValue *args; - virJSONValue *argobj; - const char *cmdname; - size_t i; - - if (!(val = virJSONValueFromString(cmdstr))) - return -1; - - if (!(cmdname = virJSONValueObjectGetString(val, "execute"))) { - qemuMonitorTestError("Missing command name in %s", cmdstr); - return -1; - } - - if (data->command_name && - STRNEQ(data->command_name, cmdname)) { - qemuMonitorTestErrorInvalidCommand(data->command_name, cmdname); - return -1; - } - - if (!(args = virJSONValueObjectGet(val, "arguments"))) { - qemuMonitorTestError("Missing arguments section for command '%s'", - NULLSTR(data->command_name)); - return -1; - } - - /* validate the args */ - for (i = 0; i < data->nargs; i++) { - qemuMonitorTestCommandArgs *arg = &data->args[i]; - g_autofree char *argstr = NULL; - - if (!(argobj = virJSONValueObjectGet(args, arg->argname))) { - qemuMonitorTestError("Missing argument '%s' for command '%s'", - arg->argname, - NULLSTR(data->command_name)); - return -1; - } - - /* convert the argument to string */ - if (!(argstr = virJSONValueToString(argobj, false))) - return -1; - - /* verify that the argument value is expected */ - if (STRNEQ(argstr, arg->argval)) { - qemuMonitorTestError("Invalid value of argument '%s' of command '%s': " - "expected '%s' got '%s'", - arg->argname, - NULLSTR(data->command_name), - arg->argval, argstr); - return -1; - } - } - - /* arguments checked out, return the response */ - return qemuMonitorTestAddResponse(test, data->response); -} - - - -/* this allows to add a responder that is able to check - * a (shallow) structure of arguments for a command */ -int -qemuMonitorTestAddItemParams(qemuMonitorTest *test, - const char *cmdname, - const char *response, - ...) -{ - struct qemuMonitorTestHandlerData *data; - const char *argname; - const char *argval; - va_list args; - - va_start(args, response); - - data = g_new0(struct qemuMonitorTestHandlerData, 1); - - data->command_name = g_strdup(cmdname); - data->response = g_strdup(response); - - while ((argname = va_arg(args, char *))) { - size_t i; - if (!(argval = va_arg(args, char *))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Missing argument value for argument '%s'", - argname); - goto error; - } - - i = data->nargs; - VIR_EXPAND_N(data->args, data->nargs, 1); - data->args[i].argname = g_strdup(argname); - data->args[i].argval = g_strdup(argval); - } - - va_end(args); - - qemuMonitorTestAddHandler(test, - cmdname, - qemuMonitorTestProcessCommandWithArgs, - data, qemuMonitorTestHandlerDataFree); - - return 0; - - error: - va_end(args); - qemuMonitorTestHandlerDataFree(data); - return -1; -} - - static void qemuMonitorTestEOFNotify(qemuMonitor *mon G_GNUC_UNUSED, virDomainObj *vm G_GNUC_UNUSED) diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index 5de55172be..eddd8294bb 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -74,13 +74,6 @@ qemuMonitorTestAddItemVerbatim(qemuMonitorTest *test, int qemuMonitorTestAddAgentSyncResponse(qemuMonitorTest *test); -int -qemuMonitorTestAddItemParams(qemuMonitorTest *test, - const char *cmdname, - const char *response, - ...) - G_GNUC_NULL_TERMINATED; - #define qemuMonitorTestNewSimple(xmlopt) \ qemuMonitorTestNew(xmlopt, NULL, NULL, NULL) #define qemuMonitorTestNewSchema(xmlopt, schema) \ -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Using qemuMonitorTestAddItemVerbatim is more universal and that helper also does QMP schema validation. Remove the now unused helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 132 ----------------------------------- tests/qemumonitortestutils.h | 7 -- 2 files changed, 139 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The test case is validating the QMP schema against itself. This was useful when I was developing the validator but at this point it's no longer needed. Additionally the QMP schema has few deprecated members now, which our validator doesn't catch yet, so this test would start failing once I fix the validator. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d9323d94c3..c3963050de 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2825,8 +2825,6 @@ mymain(void) g_autoptr(GHashTable) qapischema_x86_64 = NULL; g_autoptr(GHashTable) qapischema_s390x = NULL; struct testQAPISchemaData qapiData; - g_autoptr(virJSONValue) metaschema = NULL; - g_autofree char *metaschemastr = NULL; if (qemuTestDriverInit(&driver) < 0) return EXIT_FAILURE; @@ -3047,17 +3045,6 @@ mymain(void) DO_TEST_QAPI_VALIDATE("alternate 2", "blockdev-add/arg-type", false, "{\"driver\":\"qcow2\",\"file\": 1234}"); - if (!(metaschema = testQEMUSchemaGetLatest("x86_64")) || - !(metaschemastr = virJSONValueToString(metaschema, false))) { - VIR_TEST_VERBOSE("failed to load latest qapi schema"); - ret = -1; - goto cleanup; - } - - DO_TEST_QAPI_VALIDATE("schema-meta", "query-qmp-schema/ret-type", true, - metaschemastr); - - #undef DO_TEST_QAPI_VALIDATE if (virTestRun("validate that object-add and device_add don't have schema", -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
The test case is validating the QMP schema against itself. This was useful when I was developing the validator but at this point it's no longer needed.
Additionally the QMP schema has few deprecated members now, which our validator doesn't catch yet, so this test would start failing once I fix the validator.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 ------------- 1 file changed, 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function doesn't modify it. Fix the argument declaration so that the function can be used in a context where we have a 'const' disk definition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 15383f646a..cdaa77dcbc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1550,7 +1550,7 @@ qemuBuildChardevCommand(virCommand *cmd, bool -qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDef *disk) +qemuDiskConfigBlkdeviotuneEnabled(const virDomainDiskDef *disk) { return !!disk->blkdeviotune.group_name || virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 5fdb138030..55efa45601 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -208,7 +208,7 @@ qemuBuildZPCIDevProps(virDomainDeviceInfo *dev); int qemuNetworkPrepareDevices(virDomainDef *def); bool -qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDef *disk); +qemuDiskConfigBlkdeviotuneEnabled(const virDomainDiskDef *disk); virJSONValue *qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu) ATTRIBUTE_NONNULL(1); -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
The function doesn't modify it. Fix the argument declaration so that the function can be used in a context where we have a 'const' disk definition.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_command.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Historically this didn't work with any supported qemu version as we don't set the alias of the device, and thus qemu uses a different alias resulting in a failure to startup the VM: internal error: unable to execute QEMU command 'block_set_io_throttle': Device 'drive-sd-disk0' not found Refuse setting throttling as this is unlikely to be needed and proper fix requires using -device instead of -drive if=sd. Note that this was broken when I moved the setup of throttling as a command at startup for blockdev integration quite a while ago. Until then throttling was passed as arguments for -drive. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 +++++++++-------- src/qemu/qemu_validate.c | 8 ++++++++ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 00b58e0424..d3b37486c3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14764,11 +14764,12 @@ typedef enum { static bool -qemuDomainDiskBlockIoTuneIsSupported(virStorageSource *src) +qemuDomainDiskBlockIoTuneIsSupported(virDomainDiskDef *disk) { - if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_VHOST_USER) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("a block I/O throttling is not supported for vhostuser disk")); + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER || + disk->bus == VIR_DOMAIN_DISK_BUS_SD) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("block I/O throttling is not supported for disk '%1$s'"), disk->dst); return false; } @@ -15107,7 +15108,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (!(disk = qemuDomainDiskByName(def, path))) goto endjob; - if (!qemuDomainDiskBlockIoTuneIsSupported(disk->src)) + if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob; if (QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) { @@ -15193,7 +15194,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (!qemuDomainDiskBlockIoTuneIsSupported(conf_disk->src)) + if (!qemuDomainDiskBlockIoTuneIsSupported(conf_disk)) goto endjob; conf_cur_info = qemuDomainFindGroupBlockIoTune(persistentDef, conf_disk, &info); @@ -15284,7 +15285,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!(disk = qemuDomainDiskByName(def, path))) goto endjob; - if (!qemuDomainDiskBlockIoTuneIsSupported(disk->src)) + if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob; if (QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) { @@ -15309,7 +15310,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, goto endjob; } - if (!qemuDomainDiskBlockIoTuneIsSupported(disk->src)) + if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob; reply = disk->blkdeviotune; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index da4b9a3b35..04d0c9df73 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3094,6 +3094,14 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const virDomainDiskDef *disk, return -1; } + /* setting throttling for the SD card didn't work, refuse it explicitly */ + if (disk->bus == VIR_DOMAIN_DISK_BUS_SD && + qemuDiskConfigBlkdeviotuneEnabled(disk)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("<iotune> is not supported with bus='sd'")); + return -1; + } + /* checking def here is only for calling from tests */ if (disk->blkdeviotune.group_name) { size_t i; -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Historically this didn't work with any supported qemu version as we don't set the alias of the device, and thus qemu uses a different alias resulting in a failure to startup the VM:
internal error: unable to execute QEMU command 'block_set_io_throttle': Device 'drive-sd-disk0' not found
Refuse setting throttling as this is unlikely to be needed and proper fix requires using -device instead of -drive if=sd.
Note that this was broken when I moved the setup of throttling as a command at startup for blockdev integration quite a while ago. Until then throttling was passed as arguments for -drive.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 +++++++++-------- src/qemu/qemu_validate.c | 8 ++++++++ 2 files changed, 17 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The 'device' argument is deprecated. All real usage in the qemu driver already uses 'id' as we populate the 'qomName' for everything except for SD cards where throttling didn't work with libvirt for a very long time. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index c3963050de..5c05669280 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1870,7 +1870,7 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *opaque) if (qemuMonitorTestAddItemVerbatim(test, "{\"execute\":\"block_set_io_throttle\"," - " \"arguments\":{\"device\": \"drive-virtio-disk1\"," + " \"arguments\":{\"id\": \"drive-virtio-disk1\"," " \"bps\": 1," " \"bps_rd\": 2," " \"bps_wr\": 3," @@ -1904,7 +1904,7 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *opaque) goto cleanup; if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), - "drive-virtio-disk1", NULL, &info) < 0) + NULL, "drive-virtio-disk1", &info) < 0) goto cleanup; ret = 0; -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
The 'device' argument is deprecated. All real usage in the qemu driver already uses 'id' as we populate the 'qomName' for everything except for SD cards where throttling didn't work with libvirt for a very long time.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Every caller will pass 'qdevid' as it's populated in the data madatorily with qemu-4.2 and onwards due to mandatory -blockdev use. Thus we can drop compatibility with the old way of matching the disk via alias. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 10 +--------- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_monitor_json.c | 10 ++++------ src/qemu/qemu_monitor_json.h | 1 - tests/qemumonitorjsontest.c | 2 +- 6 files changed, 8 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3b37486c3..430b75880b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15240,8 +15240,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, virDomainDef *def = NULL; virDomainDef *persistentDef = NULL; virDomainBlockIoTuneInfo reply = {0}; - g_autofree char *drivealias = NULL; - const char *qdevid = NULL; int ret = -1; int maxparams; @@ -15288,14 +15286,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob; - if (QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) { - qdevid = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName; - } else { - if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) - goto endjob; - } qemuDomainObjEnterMonitor(vm); - rc = qemuMonitorGetBlockIoThrottle(priv->mon, drivealias, qdevid, &reply); + rc = qemuMonitorGetBlockIoThrottle(priv->mon, QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName, &reply); qemuDomainObjExitMonitor(vm); if (rc < 0) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dacf161971..5faf64a4ec 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2988,16 +2988,14 @@ qemuMonitorSetBlockIoThrottle(qemuMonitor *mon, int qemuMonitorGetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qdevid, virDomainBlockIoTuneInfo *reply) { - VIR_DEBUG("drivealias=%s, qdevid=%s, reply=%p", - NULLSTR(drivealias), NULLSTR(qdevid), reply); + VIR_DEBUG("qdevid=%s, reply=%p", NULLSTR(qdevid), reply); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONGetBlockIoThrottle(mon, drivealias, qdevid, reply); + return qemuMonitorJSONGetBlockIoThrottle(mon, qdevid, reply); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 09f22f2328..6006a451a3 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1057,7 +1057,6 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitor *mon, virDomainBlockIoTuneInfo *info); int qemuMonitorGetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qdevid, virDomainBlockIoTuneInfo *reply); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 745d83e2b6..af7649f8b0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4517,7 +4517,6 @@ int qemuMonitorJSONOpenGraphics(qemuMonitor *mon, } static int qemuMonitorJSONBlockIoThrottleInfo(virJSONValue *io_throttle, - const char *drivealias, const char *qdevid, virDomainBlockIoTuneInfo *reply) { @@ -4547,8 +4546,8 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValue *io_throttle, return -1; } - if ((drivealias && current_drive && STRNEQ(current_drive, drivealias)) || - (qdevid && current_qdev && STRNEQ(current_qdev, qdevid))) + if (STRNEQ_NULLABLE(current_qdev, qdevid) && + STRNEQ_NULLABLE(current_drive, qdevid)) continue; found = true; @@ -4587,7 +4586,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValue *io_throttle, if (!found) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find throttling info for device '%1$s'"), - drivealias ? drivealias : qdevid); + qdevid); return -1; } @@ -4640,7 +4639,6 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitor *mon, } int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qdevid, virDomainBlockIoTuneInfo *reply) { @@ -4649,7 +4647,7 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon, if (!(devices = qemuMonitorJSONQueryBlock(mon))) return -1; - return qemuMonitorJSONBlockIoThrottleInfo(devices, drivealias, qdevid, reply); + return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply); } int qemuMonitorJSONSystemWakeup(qemuMonitor *mon) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 6f376cf9b7..3f77ab0ab3 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -407,7 +407,6 @@ qemuMonitorJSONSetBlockIoThrottle(qemuMonitor *mon, int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qdevid, virDomainBlockIoTuneInfo *reply); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 5c05669280..ba12aae5b7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1897,7 +1897,7 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *opaque) return -1; if (qemuMonitorJSONGetBlockIoThrottle(qemuMonitorTestGetMonitor(test), - "drive-virtio-disk0", NULL, &info) < 0) + "drive-virtio-disk0", &info) < 0) goto cleanup; if (testValidateGetBlockIoThrottle(&info, &expectedInfo) < 0) -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Every caller will pass 'qdevid' as it's populated in the data madatorily
mandatorily?
with qemu-4.2 and onwards due to mandatory -blockdev use. Thus we can drop compatibility with the old way of matching the disk via alias.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 10 +--------- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_monitor_json.c | 10 ++++------ src/qemu/qemu_monitor_json.h | 1 - tests/qemumonitorjsontest.c | 2 +- 6 files changed, 8 insertions(+), 22 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Every caller will pass 'qdevid' as it's populated in the data madatorily with qemu-4.2 and onwards due to mandatory -blockdev use. Thus we can drop compatibility with the old way of matching the disk via alias. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 +++---------- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_monitor_json.c | 4 +--- src/qemu/qemu_monitor_json.h | 1 - src/qemu/qemu_process.c | 12 +++--------- tests/qemumonitorjsontest.c | 2 +- 8 files changed, 12 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 430b75880b..56f4cd6197 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14914,8 +14914,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, virDomainDef *persistentDef = NULL; virDomainBlockIoTuneInfo info; virDomainBlockIoTuneInfo conf_info; - g_autofree char *drivealias = NULL; - const char *qdevid = NULL; int ret = -1; size_t i; virDomainDiskDef *conf_disk = NULL; @@ -15111,13 +15109,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) goto endjob; - if (QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) { - qdevid = QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName; - } else { - if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) - goto endjob; - } - cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info); if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, @@ -15167,7 +15158,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, int rc = 0; qemuDomainObjEnterMonitor(vm); - rc = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid, &info); + rc = qemuMonitorSetBlockIoThrottle(priv->mon, + QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName, + &info); qemuDomainObjExitMonitor(vm); if (rc < 0) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 972df572a7..806aecb29d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -517,7 +517,7 @@ qemuDomainChangeMediaBlockdev(virDomainObj *vm, if (rc == 0 && !virStorageSourceIsEmpty(newsrc) && qemuDiskConfigBlkdeviotuneEnabled(disk)) { - rc = qemuMonitorSetBlockIoThrottle(priv->mon, NULL, + rc = qemuMonitorSetBlockIoThrottle(priv->mon, diskPriv->qomName, &disk->blkdeviotune); } @@ -734,7 +734,7 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm, g_autoptr(GHashTable) blockinfo = NULL; if (qemuDiskConfigBlkdeviotuneEnabled(disk)) { - if (qemuMonitorSetBlockIoThrottle(priv->mon, NULL, diskPriv->qomName, + if (qemuMonitorSetBlockIoThrottle(priv->mon, diskPriv->qomName, &disk->blkdeviotune) < 0) VIR_WARN("failed to set blkdeviotune for '%s' of '%s'", disk->dst, vm->def->name); } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5faf64a4ec..73a019e732 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2973,16 +2973,14 @@ qemuMonitorJobComplete(qemuMonitor *mon, int qemuMonitorSetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qomid, virDomainBlockIoTuneInfo *info) { - VIR_DEBUG("drivealias=%s, qomid=%s, info=%p", - NULLSTR(drivealias), NULLSTR(qomid), info); + VIR_DEBUG("qomid=%s, info=%p", NULLSTR(qomid), info); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONSetBlockIoThrottle(mon, drivealias, qomid, info); + return qemuMonitorJSONSetBlockIoThrottle(mon, qomid, info); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6006a451a3..8f1bfc702a 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1052,7 +1052,6 @@ int qemuMonitorOpenGraphics(qemuMonitor *mon, bool skipauth); int qemuMonitorSetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qomid, virDomainBlockIoTuneInfo *info); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index af7649f8b0..360b6423cf 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4596,7 +4596,6 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValue *io_throttle, #undef GET_THROTTLE_STATS_OPTIONAL int qemuMonitorJSONSetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qomid, virDomainBlockIoTuneInfo *info) { @@ -4604,8 +4603,7 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitor *mon, g_autoptr(virJSONValue) result = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", - "S:device", drivealias, - "S:id", qomid, + "s:id", qomid, "U:bps", info->total_bytes_sec, "U:bps_rd", info->read_bytes_sec, "U:bps_wr", info->write_bytes_sec, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 3f77ab0ab3..802f6e7b44 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -401,7 +401,6 @@ qemuMonitorJSONOpenGraphics(qemuMonitor *mon, int qemuMonitorJSONSetBlockIoThrottle(qemuMonitor *mon, - const char *drivealias, const char *qomid, virDomainBlockIoTuneInfo *info); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 730e59eb7e..73cecde1f5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7300,13 +7300,6 @@ qemuProcessSetupDiskThrottling(virDomainObj *vm, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDef *disk = vm->def->disks[i]; - qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - g_autofree char *drivealias = NULL; - - if (!QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName) { - if (!(drivealias = qemuAliasDiskDriveFromDisk(disk))) - goto cleanup; - } /* Setting throttling for empty drives fails */ if (virStorageSourceIsEmpty(disk->src)) @@ -7315,8 +7308,9 @@ qemuProcessSetupDiskThrottling(virDomainObj *vm, if (!qemuDiskConfigBlkdeviotuneEnabled(disk)) continue; - if (qemuMonitorSetBlockIoThrottle(qemuDomainGetMonitor(vm), drivealias, - diskPriv->qomName, &disk->blkdeviotune) < 0) + if (qemuMonitorSetBlockIoThrottle(qemuDomainGetMonitor(vm), + QEMU_DOMAIN_DISK_PRIVATE(disk)->qomName, + &disk->blkdeviotune) < 0) goto cleanup; } diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index ba12aae5b7..7e79b2e757 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1904,7 +1904,7 @@ testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *opaque) goto cleanup; if (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), - NULL, "drive-virtio-disk1", &info) < 0) + "drive-virtio-disk1", &info) < 0) goto cleanup; ret = 0; -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
Every caller will pass 'qdevid' as it's populated in the data madatorily
mandatorily?
with qemu-4.2 and onwards due to mandatory -blockdev use. Thus we can drop compatibility with the old way of matching the disk via alias.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 +++---------- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_monitor.c | 6 ++---- src/qemu/qemu_monitor.h | 1 - src/qemu/qemu_monitor_json.c | 4 +--- src/qemu/qemu_monitor_json.h | 1 - src/qemu/qemu_process.c | 12 +++--------- tests/qemumonitorjsontest.c | 2 +- 8 files changed, 12 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The QMP schema validator wasn't adapted to consider features of 'object' members and thus we didn't catch the deprecation of 'device' in 'block_set_io_throttle'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index d431e7b3fc..f7b2e122bd 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -187,6 +187,10 @@ testQEMUSchemaValidateObjectMember(const char *key, return -1; } + /* validate that the member is not deprecated */ + if ((rc = testQEMUSchemaValidateDeprecated(keymember, key, data->ctxt)) < 0) + return rc; + /* lookup schema entry for keytype */ if (!(keytype = virJSONValueObjectGetString(keymember, "type")) || !(keyschema = virHashLookup(data->ctxt->schema, keytype))) { -- 2.40.1
On a Thursday in 2023, Peter Krempa wrote:
The QMP schema validator wasn't adapted to consider features of 'object' members and thus we didn't catch the deprecation of 'device' in 'block_set_io_throttle'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 4 ++++ 1 file changed, 4 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Jonathon Jongsma -
Ján Tomko -
Peter Krempa