[PATCH 00/15] tests: qemu: Detect deprecation in the QMP schema (deprecation part 1)
Recent qemu versions started adding the 'deprecated' feature to commands and fields. Use this data to validate that our code doesn't rely on deprecated commands. It turns out that we still use some old migration commands (See 14/15) and add validator to catch anything deprecated early. Peter Krempa (15): testQemuHotplugCpuPrepare: Allow unused monitor commands only on failure testutilsqemuschema: Introduce testQEMUSchemaValidateCommand qemuMonitorTestProcessCommandDefaultValidate: Use testQEMUSchemaValidateCommand qemuMonitorTestProcessCommandDefaultValidate: Clean up return value use qemumonitortestutils: Introduce qemuMonitorTestSkipDeprecatedValidation testutilsqemuschema: Use automatic variable clearing where possible testutilsqemuschema: Pass in 'schema' and 'debug' variables to workers in a struct testQEMUSchemaValidate(Command): Allow skipping validation of deprecated fields testQemuHotplugCpuPrepare: Allow deprecated commands for non-modern cpu hotplug test qemumonitorjsontest: Add infrastructure for generated tests of deprecated commands testQemuMonitorJSONqemuMonitorJSONQueryCPUs: Split off test for query-cpus-fast qemumonitorjsontest: Allow use of deprecated 'query-cpus' qemumonitorjsontest: Allow use of deprecated 'cpu-add' and 'change' command qemumonitorjsontest: Mark recently deprecated migration command in our tests testQEMUSchemaValidate*: Reject usage of fields with 'deprecated' set tests/qemublocktest.c | 14 +- tests/qemuhotplugtest.c | 11 +- tests/qemumonitorjsontest.c | 67 +++++-- tests/qemumonitortestutils.c | 66 ++++--- tests/qemumonitortestutils.h | 2 + tests/testutilsqemuschema.c | 334 ++++++++++++++++++++++------------- tests/testutilsqemuschema.h | 9 + 7 files changed, 325 insertions(+), 178 deletions(-) -- 2.26.2
Only tests expected to fail should allow unused commads as the normal tests will consume all of them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 9a215ab303..849e7e7636 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -409,6 +409,7 @@ testQemuHotplugCpuDataFree(struct testQemuHotplugCpuData *data) static struct testQemuHotplugCpuData * testQemuHotplugCpuPrepare(const char *test, bool modern, + bool fail, virHashTablePtr qmpschema) { qemuDomainObjPrivatePtr priv = NULL; @@ -453,7 +454,8 @@ testQemuHotplugCpuPrepare(const char *test, &driver, data->vm, qmpschema))) goto error; - qemuMonitorTestAllowUnusedCommands(data->mon); + if (fail) + qemuMonitorTestAllowUnusedCommands(data->mon); priv->mon = qemuMonitorTestGetMonitor(data->mon); virObjectUnlock(priv->mon); @@ -528,7 +530,7 @@ testQemuHotplugCpuGroup(const void *opaque) int rc; if (!(data = testQemuHotplugCpuPrepare(params->test, params->modern, - params->schema))) + params->fail, params->schema))) return -1; rc = qemuDomainSetVcpusInternal(&driver, data->vm, data->vm->def, @@ -565,7 +567,7 @@ testQemuHotplugCpuIndividual(const void *opaque) int rc; if (!(data = testQemuHotplugCpuPrepare(params->test, params->modern, - params->schema))) + params->fail, params->schema))) return -1; if (virBitmapParse(params->cpumap, &map, 128) < 0) -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Only tests expected to fail should allow unused commads as the normal tests will consume all of them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The new helper splits out all steps necessary to validate a QMP command against the schema. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 44 +++++++++++++++++++++++++++++++++++++ tests/testutilsqemuschema.h | 6 +++++ 2 files changed, 50 insertions(+) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 7b82ff27b2..60409a0f91 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -517,6 +517,50 @@ testQEMUSchemaValidate(virJSONValuePtr obj, } +/** + * testQEMUSchemaValidateCommand: + * @command: command to validate + * @arguments: arguments of @command to validate + * @schema: hash table containing schema entries + * @debug: a virBuffer which will be filled with debug information if provided + * + * Validates whether @command and it's @arguments conforms to the QAPI schema + * passed in via @schema. Returns 0, if the command and args matches @schema, + * -1 if it does not and -2 if there is a problem with the schema or with + * internals. + * + * @debug is filled with information regarding the validation process + */ +int +testQEMUSchemaValidateCommand(const char *command, + virJSONValuePtr arguments, + virHashTablePtr schema, + virBufferPtr debug) +{ + g_autofree char *schemapatharguments = g_strdup_printf("%s/arg-type", command); + g_autoptr(virJSONValue) emptyargs = NULL; + virJSONValuePtr schemarootcommand; + virJSONValuePtr schemarootarguments; + + if (virQEMUQAPISchemaPathGet(command, schema, &schemarootcommand) < 0 || + !schemarootcommand) { + virBufferAsprintf(debug, "ERROR: command '%s' not found in the schema", command); + return -1; + } + + if (!arguments) + arguments = emptyargs = virJSONValueNewObject(); + + if (virQEMUQAPISchemaPathGet(schemapatharguments, schema, &schemarootarguments) < 0 || + !schemarootarguments) { + virBufferAsprintf(debug, "ERROR: failed to look up 'arg-type' of '%s'", command); + return -1; + } + + return testQEMUSchemaValidateRecurse(arguments, schemarootarguments, schema, debug); +} + + /** * testQEMUSchemaGetLatest: * diff --git a/tests/testutilsqemuschema.h b/tests/testutilsqemuschema.h index 84ee9a9670..e3a375b038 100644 --- a/tests/testutilsqemuschema.h +++ b/tests/testutilsqemuschema.h @@ -28,6 +28,12 @@ testQEMUSchemaValidate(virJSONValuePtr obj, virHashTablePtr schema, virBufferPtr debug); +int +testQEMUSchemaValidateCommand(const char *command, + virJSONValuePtr arguments, + virHashTablePtr schema, + virBufferPtr debug); + virJSONValuePtr testQEMUSchemaGetLatest(const char* arch); -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
The new helper splits out all steps necessary to validate a QMP command against the schema.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 44 +++++++++++++++++++++++++++++++++++++ tests/testutilsqemuschema.h | 6 +++++ 2 files changed, 50 insertions(+)
diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 7b82ff27b2..60409a0f91 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -517,6 +517,50 @@ testQEMUSchemaValidate(virJSONValuePtr obj, }
+/** + * testQEMUSchemaValidateCommand: + * @command: command to validate + * @arguments: arguments of @command to validate + * @schema: hash table containing schema entries + * @debug: a virBuffer which will be filled with debug information if provided + * + * Validates whether @command and it's @arguments conforms to the QAPI schema
*its *conform
+ * passed in via @schema. Returns 0, if the command and args matches @schema,
*match
+ * -1 if it does not and -2 if there is a problem with the schema or with + * internals. + * + * @debug is filled with information regarding the validation process + */
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove the ad-hoc command validation in favor of the new helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 4ca29ab061..df780643dd 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -537,9 +537,6 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, virJSONValuePtr args) { g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER; - virJSONValuePtr schemaroot; - g_autoptr(virJSONValue) emptyargs = NULL; - g_autofree char *schemapath = NULL; if (!test->qapischema) return 0; @@ -555,20 +552,13 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, if (STREQ(cmdname, "device_add")) return 0; - schemapath = g_strdup_printf("%s/arg-type", cmdname); - - if (virQEMUQAPISchemaPathGet(schemapath, test->qapischema, &schemaroot) < 0 || - !schemaroot) { - qemuMonitorTestError("command '%s' not found in QAPI schema", cmdname); - return -1; - } + if (testQEMUSchemaValidateCommand(cmdname, args, test->qapischema, &debug) < 0) { + if (virTestGetDebug() == 2) { + g_autofree char *argstr = NULL; - if (!args) - args = emptyargs = virJSONValueNewObject(); + if (args) + argstr = virJSONValueToString(args, true); - if (testQEMUSchemaValidate(args, schemaroot, test->qapischema, &debug) < 0) { - if (virTestGetDebug() == 2) { - g_autofree char *argstr = virJSONValueToString(args, true); fprintf(stderr, "\nfailed to validate arguments of '%s' against QAPI schema\n" "args:\n%s\nvalidator output:\n %s\n", -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Remove the ad-hoc command validation in favor of the new helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We no longer return the error via the monitor, so the function no longer returns '1'. Remove the mention from comment and fix callers to stop looking for the return value of '1'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index df780643dd..3dc4b674f3 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -529,8 +529,7 @@ qemuMonitorTestHandlerDataFree(void *opaque) } -/* Returns -1 on error, 0 if validation was successful/not necessary, 1 if - * the validation has failed, and the reply was properly constructed */ +/* Returns -1 on error, 0 if validation was successful/not necessary */ static int qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, const char *cmdname, @@ -585,7 +584,6 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, g_autoptr(virJSONValue) val = NULL; virJSONValuePtr cmdargs = NULL; const char *cmdname; - int rc; if (!(val = virJSONValueFromString(cmdstr))) return -1; @@ -596,10 +594,8 @@ qemuMonitorTestProcessCommandDefault(qemuMonitorTestPtr test, } cmdargs = virJSONValueObjectGet(val, "arguments"); - if ((rc = qemuMonitorTestProcessCommandDefaultValidate(test, cmdname, cmdargs)) < 0) + if (qemuMonitorTestProcessCommandDefaultValidate(test, cmdname, cmdargs) < 0) return -1; - if (rc == 1) - return 0; if (data->command_name && STRNEQ(data->command_name, cmdname)) { qemuMonitorTestErrorInvalidCommand(data->command_name, cmdname); @@ -641,7 +637,6 @@ qemuMonitorTestProcessCommandVerbatim(qemuMonitorTestPtr test, virJSONValuePtr cmdargs; const char *cmdname; int ret = -1; - int rc; /* JSON strings will be reformatted to simplify checking */ if (!(json = virJSONValueFromString(cmdstr)) || @@ -653,12 +648,8 @@ qemuMonitorTestProcessCommandVerbatim(qemuMonitorTestPtr test, /* in this case we do a best-effort schema check if we can find the command */ if ((cmdname = virJSONValueObjectGetString(json, "execute"))) { cmdargs = virJSONValueObjectGet(json, "arguments"); - - if ((rc = qemuMonitorTestProcessCommandDefaultValidate(test, cmdname, cmdargs)) < 0) + if (qemuMonitorTestProcessCommandDefaultValidate(test, cmdname, cmdargs) < 0) return -1; - - if (rc == 1) - return 0; } if (STREQ(data->command_name, cmdstr)) { -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
We no longer return the error via the monitor, so the function no longer returns '1'. Remove the mention from comment and fix callers to stop looking for the return value of '1'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Upcoming patches will add validation which rejects objects with the 'deprecated' feature in the QMP schema. To support tests which deal with legacy properties in case when a command or argument is marked as deprecated or removed by qemu qemuMonitorTestSkipDeprecatedValidation will allow configuring the tests to ignore such errors. In case of commands/features which are not yet replaced, the 'allowRemoved' bool should not be set to provide a hard notification once qemu drops the command. Note that at this point 'allowRemoved' only includes whole commands, but not specific properties. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 28 ++++++++++++++++++++++++++++ tests/qemumonitortestutils.h | 2 ++ 2 files changed, 30 insertions(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 3dc4b674f3..0d9427f1d1 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -59,6 +59,9 @@ struct _qemuMonitorTest { bool allowUnusedCommands; + bool skipValidationDeprecated; + bool skipValidationRemoved; + char *incoming; size_t incomingLength; size_t incomingCapacity; @@ -1298,6 +1301,31 @@ qemuMonitorTestAllowUnusedCommands(qemuMonitorTestPtr test) } +/** + * qemuMonitorTestSkipDeprecatedValidation: + * @test: test monitor object + * @allowRemoved: don't produce errors if command was removed from QMP schema + * + * By default if the QMP schema is provided all test items/commands are + * validated against the schema. This function allows to override the validation + * and additionally if @allowRemoved is true and if such a command is no longer + * present in the QMP, only a warning is printed. + * + * '@allowRemoved' must be used only if a suitable replacement is already in + * use and the code tests legacy interactions. + * + * Note that currently '@allowRemoved' influences only removed commands. If an + * argument is removed it will still fail validation. + */ +void +qemuMonitorTestSkipDeprecatedValidation(qemuMonitorTestPtr test, + bool allowRemoved) +{ + test->skipValidationDeprecated = true; + test->skipValidationRemoved = allowRemoved; +} + + static int qemuMonitorTestFullAddItem(qemuMonitorTestPtr test, const char *filename, diff --git a/tests/qemumonitortestutils.h b/tests/qemumonitortestutils.h index f45e850000..1073ef4100 100644 --- a/tests/qemumonitortestutils.h +++ b/tests/qemumonitortestutils.h @@ -51,6 +51,8 @@ void *qemuMonitorTestItemGetPrivateData(qemuMonitorTestItemPtr item); int qemuMonitorTestAddErrorResponse(qemuMonitorTestPtr test, const char *errmsg, ...); void qemuMonitorTestAllowUnusedCommands(qemuMonitorTestPtr test); +void qemuMonitorTestSkipDeprecatedValidation(qemuMonitorTestPtr test, + bool allowRemoved); int qemuMonitorTestAddItem(qemuMonitorTestPtr test, const char *command_name, -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Upcoming patches will add validation which rejects objects with the 'deprecated' feature in the QMP schema. To support tests which deal with legacy properties in case when a command or argument is marked as deprecated or removed by qemu qemuMonitorTestSkipDeprecatedValidation will allow configuring the tests to ignore such errors.
In case of commands/features which are not yet replaced, the 'allowRemoved' bool should not be set to provide a hard notification once qemu drops the command.
Note that at this point 'allowRemoved' only includes whole commands, but not specific properties.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitortestutils.c | 28 ++++++++++++++++++++++++++++ tests/qemumonitortestutils.h | 2 ++ 2 files changed, 30 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Refactor all cleanup to avoid manual clearing, unnecessary labels and return value variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 81 +++++++++++++------------------------ 1 file changed, 29 insertions(+), 52 deletions(-) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 60409a0f91..a43cbe2f91 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -123,36 +123,33 @@ testQEMUSchemaValidateObjectMember(const char *key, void *opaque) { struct testQEMUSchemaValidateObjectMemberData *data = opaque; - virJSONValuePtr keymember = NULL; + g_autoptr(virJSONValue) keymember = NULL; const char *keytype; virJSONValuePtr keyschema = NULL; - int ret = -1; + int rc; virBufferStrcat(data->debug, key, ": ", NULL); /* lookup 'member' entry for key */ if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers))) { - virBufferAddLit(data->debug, "ERROR: attribute not in schema"); - goto cleanup; + virBufferAddLit(data->debug, "ERROR: attribute not in schema\n"); + return -1; } /* lookup schema entry for keytype */ if (!(keytype = virJSONValueObjectGetString(keymember, "type")) || !(keyschema = virHashLookup(data->schema, keytype))) { - virBufferAsprintf(data->debug, "ERROR: can't find schema for type '%s'", + virBufferAsprintf(data->debug, "ERROR: can't find schema for type '%s'\n", NULLSTR(keytype)); - ret = -2; - goto cleanup; + return -2; } /* recurse */ - ret = testQEMUSchemaValidateRecurse(value, keyschema, data->schema, + rc = testQEMUSchemaValidateRecurse(value, keyschema, data->schema, data->debug); - cleanup: virBufferAddLit(data->debug, "\n"); - virJSONValueFree(keymember); - return ret; + return rc; } @@ -188,13 +185,12 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, virBufferPtr debug) { size_t i; - virJSONValuePtr variants = NULL; + g_autoptr(virJSONValue) variants = NULL; virJSONValuePtr variant; virJSONValuePtr variantschema; virJSONValuePtr variantschemamembers; virJSONValuePtr rootmembers; const char *varianttype = NULL; - int ret = -1; if (!(variants = virJSONValueObjectStealArray(root, "variants"))) { virBufferAddLit(debug, "ERROR: missing 'variants' in schema\n"); @@ -214,8 +210,7 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, if (!varianttype) { virBufferAsprintf(debug, "ERROR: variant '%s' for discriminator '%s' not found\n", variantname, variantfield); - goto cleanup; - + return -1; } if (!(variantschema = virHashLookup(schema, varianttype)) || @@ -223,8 +218,7 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, virBufferAsprintf(debug, "ERROR: missing schema or schema members for variant '%s'(%s)\n", variantname, varianttype); - ret = -2; - goto cleanup; + return -2; } rootmembers = virJSONValueObjectGetArray(root, "members"); @@ -232,15 +226,10 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, if (virJSONValueArrayForeachSteal(variantschemamembers, testQEMUSchemaValidateObjectMergeVariantMember, rootmembers) < 0) { - ret = -2; - goto cleanup; + return -2; } - ret = 0; - - cleanup: - virJSONValueFree(variants); - return ret; + return 0; } @@ -269,10 +258,9 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, { struct testQEMUSchemaValidateObjectMemberData data = { NULL, schema, debug, false }; - virJSONValuePtr localroot = NULL; + g_autoptr(virJSONValue) localroot = NULL; const char *variantfield; const char *variantname; - int ret = -1; if (virJSONValueGetType(obj) != VIR_JSON_TYPE_OBJECT) { virBufferAddLit(debug, "ERROR: not an object"); @@ -283,23 +271,21 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, virBufferAdjustIndent(debug, 3); /* copy schema */ - if (!(localroot = virJSONValueCopy(root))) { - ret = -2; - goto cleanup; - } + if (!(localroot = virJSONValueCopy(root))) + return -2; /* remove variant */ if ((variantfield = virJSONValueObjectGetString(localroot, "tag"))) { if (!(variantname = virJSONValueObjectGetString(obj, variantfield))) { virBufferAsprintf(debug, "ERROR: missing variant discriminator attribute '%s'\n", variantfield); - goto cleanup; + return -1; } if (testQEMUSchemaValidateObjectMergeVariant(localroot, variantfield, variantname, schema, debug) < 0) - goto cleanup; + return -1; } @@ -308,26 +294,21 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, if (virJSONValueObjectForeachKeyValue(obj, testQEMUSchemaValidateObjectMember, &data) < 0) - goto cleanup; + return -1; /* check missing mandatory values */ if (virJSONValueArrayForeachSteal(data.rootmembers, testQEMUSchemaValidateObjectMandatoryMember, &data) < 0) { - ret = -2; - goto cleanup; + return -2; } if (data.missingMandatory) - goto cleanup; + return -1; virBufferAdjustIndent(debug, -3); virBufferAddLit(debug, "} OK"); - ret = 0; - - cleanup: - virJSONValueFree(localroot); - return ret; + return 0; } @@ -570,11 +551,11 @@ testQEMUSchemaValidateCommand(const char *command, virJSONValuePtr testQEMUSchemaGetLatest(const char* arch) { - char *capsLatestFile = NULL; - char *capsLatest = NULL; + g_autofree char *capsLatestFile = NULL; + g_autofree char *capsLatest = NULL; char *schemaReply; char *end; - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) reply = NULL; virJSONValuePtr schema = NULL; if (!(capsLatestFile = testQemuGetLatestCapsForArch(arch, "replies"))) { @@ -585,14 +566,14 @@ testQEMUSchemaGetLatest(const char* arch) VIR_TEST_DEBUG("replies file: '%s'", capsLatestFile); if (virTestLoadFile(capsLatestFile, &capsLatest) < 0) - goto cleanup; + return NULL; if (!(schemaReply = strstr(capsLatest, "\"execute\": \"query-qmp-schema\"")) || !(schemaReply = strstr(schemaReply, "\n\n")) || !(end = strstr(schemaReply + 2, "\n\n"))) { VIR_TEST_VERBOSE("failed to find reply to 'query-qmp-schema' in '%s'", capsLatestFile); - goto cleanup; + return NULL; } schemaReply += 2; @@ -601,19 +582,15 @@ testQEMUSchemaGetLatest(const char* arch) if (!(reply = virJSONValueFromString(schemaReply))) { VIR_TEST_VERBOSE("failed to parse 'query-qmp-schema' reply from '%s'", capsLatestFile); - goto cleanup; + return NULL; } if (!(schema = virJSONValueObjectStealArray(reply, "return"))) { VIR_TEST_VERBOSE("missing qapi schema data in reply in '%s'", capsLatestFile); - goto cleanup; + return NULL; } - cleanup: - VIR_FREE(capsLatestFile); - VIR_FREE(capsLatest); - virJSONValueFree(reply); return schema; } -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Refactor all cleanup to avoid manual clearing, unnecessary labels and return value variables.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 81 +++++++++++++------------------------ 1 file changed, 29 insertions(+), 52 deletions(-)
diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 60409a0f91..a43cbe2f91 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -123,36 +123,33 @@ testQEMUSchemaValidateObjectMember(const char *key, void *opaque) { struct testQEMUSchemaValidateObjectMemberData *data = opaque; - virJSONValuePtr keymember = NULL; + g_autoptr(virJSONValue) keymember = NULL; const char *keytype; virJSONValuePtr keyschema = NULL; - int ret = -1; + int rc;
virBufferStrcat(data->debug, key, ": ", NULL);
/* lookup 'member' entry for key */ if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers))) { - virBufferAddLit(data->debug, "ERROR: attribute not in schema"); - goto cleanup; + virBufferAddLit(data->debug, "ERROR: attribute not in schema\n"); + return -1; }
/* lookup schema entry for keytype */ if (!(keytype = virJSONValueObjectGetString(keymember, "type")) || !(keyschema = virHashLookup(data->schema, keytype))) { - virBufferAsprintf(data->debug, "ERROR: can't find schema for type '%s'", + virBufferAsprintf(data->debug, "ERROR: can't find schema for type '%s'\n", NULLSTR(keytype)); - ret = -2; - goto cleanup; + return -2; }
/* recurse */ - ret = testQEMUSchemaValidateRecurse(value, keyschema, data->schema, + rc = testQEMUSchemaValidateRecurse(value, keyschema, data->schema, data->debug);
Indentation is off.
- cleanup: virBufferAddLit(data->debug, "\n"); - virJSONValueFree(keymember); - return ret; + return rc; }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Refactor the code so that we pass in the repeated 'schema' and 'debug' arguments via a new struct testQEMUSchemaValidateCtxt. This will simplify adding new parameters in the future. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 152 ++++++++++++++++++------------------ 1 file changed, 76 insertions(+), 76 deletions(-) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index a43cbe2f91..b449171d15 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -21,16 +21,21 @@ #include "testutilsqemuschema.h" #include "qemu/qemu_qapi.h" +struct testQEMUSchemaValidateCtxt { + virHashTablePtr schema; + virBufferPtr debug; +}; + + static int testQEMUSchemaValidateRecurse(virJSONValuePtr obj, virJSONValuePtr root, - virHashTablePtr schema, - virBufferPtr debug); + struct testQEMUSchemaValidateCtxt *ctxt); static int testQEMUSchemaValidateBuiltin(virJSONValuePtr obj, virJSONValuePtr root, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { const char *t = virJSONValueObjectGetString(root, "json-type"); const char *s = NULL; @@ -81,17 +86,16 @@ testQEMUSchemaValidateBuiltin(virJSONValuePtr obj, cleanup: if (ret == 0) - virBufferAsprintf(debug, "'%s': OK", s); + virBufferAsprintf(ctxt->debug, "'%s': OK", s); else - virBufferAsprintf(debug, "ERROR: expected type '%s', actual type %d", + virBufferAsprintf(ctxt->debug, "ERROR: expected type '%s', actual type %d", t, virJSONValueGetType(obj)); return ret; } struct testQEMUSchemaValidateObjectMemberData { virJSONValuePtr rootmembers; - virHashTablePtr schema; - virBufferPtr debug; + struct testQEMUSchemaValidateCtxt *ctxt; bool missingMandatory; }; @@ -128,27 +132,26 @@ testQEMUSchemaValidateObjectMember(const char *key, virJSONValuePtr keyschema = NULL; int rc; - virBufferStrcat(data->debug, key, ": ", NULL); + virBufferStrcat(data->ctxt->debug, key, ": ", NULL); /* lookup 'member' entry for key */ if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers))) { - virBufferAddLit(data->debug, "ERROR: attribute not in schema\n"); + virBufferAddLit(data->ctxt->debug, "ERROR: attribute not in schema\n"); return -1; } /* lookup schema entry for keytype */ if (!(keytype = virJSONValueObjectGetString(keymember, "type")) || - !(keyschema = virHashLookup(data->schema, keytype))) { - virBufferAsprintf(data->debug, "ERROR: can't find schema for type '%s'\n", + !(keyschema = virHashLookup(data->ctxt->schema, keytype))) { + virBufferAsprintf(data->ctxt->debug, "ERROR: can't find schema for type '%s'\n", NULLSTR(keytype)); return -2; } /* recurse */ - rc = testQEMUSchemaValidateRecurse(value, keyschema, data->schema, - data->debug); + rc = testQEMUSchemaValidateRecurse(value, keyschema, data->ctxt); - virBufferAddLit(data->debug, "\n"); + virBufferAddLit(data->ctxt->debug, "\n"); return rc; } @@ -181,8 +184,7 @@ static int testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, const char *variantfield, const char *variantname, - virHashTablePtr schema, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { size_t i; g_autoptr(virJSONValue) variants = NULL; @@ -193,7 +195,7 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, const char *varianttype = NULL; if (!(variants = virJSONValueObjectStealArray(root, "variants"))) { - virBufferAddLit(debug, "ERROR: missing 'variants' in schema\n"); + virBufferAddLit(ctxt->debug, "ERROR: missing 'variants' in schema\n"); return -2; } @@ -208,14 +210,14 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, } if (!varianttype) { - virBufferAsprintf(debug, "ERROR: variant '%s' for discriminator '%s' not found\n", + virBufferAsprintf(ctxt->debug, "ERROR: variant '%s' for discriminator '%s' not found\n", variantname, variantfield); return -1; } - if (!(variantschema = virHashLookup(schema, varianttype)) || + if (!(variantschema = virHashLookup(ctxt->schema, varianttype)) || !(variantschemamembers = virJSONValueObjectGetArray(variantschema, "members"))) { - virBufferAsprintf(debug, + virBufferAsprintf(ctxt->debug, "ERROR: missing schema or schema members for variant '%s'(%s)\n", variantname, varianttype); return -2; @@ -241,7 +243,7 @@ testQEMUSchemaValidateObjectMandatoryMember(size_t pos G_GNUC_UNUSED, struct testQEMUSchemaValidateObjectMemberData *data = opaque; if (virJSONValueObjectHasKey(item, "default") != 1) { - virBufferAsprintf(data->debug, "ERROR: missing mandatory attribute '%s'\n", + virBufferAsprintf(data->ctxt->debug, "ERROR: missing mandatory attribute '%s'\n", NULLSTR(virJSONValueObjectGetString(item, "name"))); data->missingMandatory = true; } @@ -253,22 +255,20 @@ testQEMUSchemaValidateObjectMandatoryMember(size_t pos G_GNUC_UNUSED, static int testQEMUSchemaValidateObject(virJSONValuePtr obj, virJSONValuePtr root, - virHashTablePtr schema, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { - struct testQEMUSchemaValidateObjectMemberData data = { NULL, schema, - debug, false }; + struct testQEMUSchemaValidateObjectMemberData data = { NULL, ctxt, false }; g_autoptr(virJSONValue) localroot = NULL; const char *variantfield; const char *variantname; if (virJSONValueGetType(obj) != VIR_JSON_TYPE_OBJECT) { - virBufferAddLit(debug, "ERROR: not an object"); + virBufferAddLit(ctxt->debug, "ERROR: not an object"); return -1; } - virBufferAddLit(debug, "{\n"); - virBufferAdjustIndent(debug, 3); + virBufferAddLit(ctxt->debug, "{\n"); + virBufferAdjustIndent(ctxt->debug, 3); /* copy schema */ if (!(localroot = virJSONValueCopy(root))) @@ -277,14 +277,13 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, /* remove variant */ if ((variantfield = virJSONValueObjectGetString(localroot, "tag"))) { if (!(variantname = virJSONValueObjectGetString(obj, variantfield))) { - virBufferAsprintf(debug, "ERROR: missing variant discriminator attribute '%s'\n", + virBufferAsprintf(ctxt->debug, "ERROR: missing variant discriminator attribute '%s'\n", variantfield); return -1; } if (testQEMUSchemaValidateObjectMergeVariant(localroot, variantfield, - variantname, - schema, debug) < 0) + variantname, ctxt) < 0) return -1; } @@ -306,8 +305,8 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, if (data.missingMandatory) return -1; - virBufferAdjustIndent(debug, -3); - virBufferAddLit(debug, "} OK"); + virBufferAdjustIndent(ctxt->debug, -3); + virBufferAddLit(ctxt->debug, "} OK"); return 0; } @@ -315,7 +314,7 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, static int testQEMUSchemaValidateEnum(virJSONValuePtr obj, virJSONValuePtr root, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { const char *objstr; virJSONValuePtr values = NULL; @@ -323,14 +322,14 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj, size_t i; if (virJSONValueGetType(obj) != VIR_JSON_TYPE_STRING) { - virBufferAddLit(debug, "ERROR: not a string"); + virBufferAddLit(ctxt->debug, "ERROR: not a string"); return -1; } objstr = virJSONValueGetString(obj); if (!(values = virJSONValueObjectGetArray(root, "values"))) { - virBufferAsprintf(debug, "ERROR: missing enum values in schema '%s'", + virBufferAsprintf(ctxt->debug, "ERROR: missing enum values in schema '%s'", NULLSTR(virJSONValueObjectGetString(root, "name"))); return -2; } @@ -339,12 +338,12 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj, value = virJSONValueArrayGet(values, i); if (STREQ_NULLABLE(objstr, virJSONValueGetString(value))) { - virBufferAsprintf(debug, "'%s' OK", NULLSTR(objstr)); + virBufferAsprintf(ctxt->debug, "'%s' OK", NULLSTR(objstr)); return 0; } } - virBufferAsprintf(debug, "ERROR: enum value '%s' is not in schema", + virBufferAsprintf(ctxt->debug, "ERROR: enum value '%s' is not in schema", NULLSTR(objstr)); return -1; } @@ -353,8 +352,7 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj, static int testQEMUSchemaValidateArray(virJSONValuePtr objs, virJSONValuePtr root, - virHashTablePtr schema, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { const char *elemtypename = virJSONValueObjectGetString(root, "element-type"); virJSONValuePtr elementschema; @@ -362,29 +360,29 @@ testQEMUSchemaValidateArray(virJSONValuePtr objs, size_t i; if (virJSONValueGetType(objs) != VIR_JSON_TYPE_ARRAY) { - virBufferAddLit(debug, "ERROR: not an array\n"); + virBufferAddLit(ctxt->debug, "ERROR: not an array\n"); return -1; } if (!elemtypename || - !(elementschema = virHashLookup(schema, elemtypename))) { - virBufferAsprintf(debug, "ERROR: missing schema for array element type '%s'", + !(elementschema = virHashLookup(ctxt->schema, elemtypename))) { + virBufferAsprintf(ctxt->debug, "ERROR: missing schema for array element type '%s'", NULLSTR(elemtypename)); return -2; } - virBufferAddLit(debug, "[\n"); - virBufferAdjustIndent(debug, 3); + virBufferAddLit(ctxt->debug, "[\n"); + virBufferAdjustIndent(ctxt->debug, 3); for (i = 0; i < virJSONValueArraySize(objs); i++) { obj = virJSONValueArrayGet(objs, i); - if (testQEMUSchemaValidateRecurse(obj, elementschema, schema, debug) < 0) + if (testQEMUSchemaValidateRecurse(obj, elementschema, ctxt) < 0) return -1; - virBufferAddLit(debug, ",\n"); + virBufferAddLit(ctxt->debug, ",\n"); } - virBufferAddLit(debug, "] OK"); - virBufferAdjustIndent(debug, -3); + virBufferAddLit(ctxt->debug, "] OK"); + virBufferAdjustIndent(ctxt->debug, -3); return 0; } @@ -392,8 +390,7 @@ testQEMUSchemaValidateArray(virJSONValuePtr objs, static int testQEMUSchemaValidateAlternate(virJSONValuePtr obj, virJSONValuePtr root, - virHashTablePtr schema, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { virJSONValuePtr members; virJSONValuePtr member; @@ -405,44 +402,44 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj, int rc; if (!(members = virJSONValueObjectGetArray(root, "members"))) { - virBufferAddLit(debug, "ERROR: missing 'members' for alternate schema"); + virBufferAddLit(ctxt->debug, "ERROR: missing 'members' for alternate schema"); return -2; } - virBufferAddLit(debug, "(\n"); - virBufferAdjustIndent(debug, 3); - indent = virBufferGetIndent(debug); + virBufferAddLit(ctxt->debug, "(\n"); + virBufferAdjustIndent(ctxt->debug, 3); + indent = virBufferGetIndent(ctxt->debug); n = virJSONValueArraySize(members); for (i = 0; i < n; i++) { membertype = NULL; /* P != NP */ - virBufferAsprintf(debug, "(alternate %zu/%zu)\n", i + 1, n); - virBufferAdjustIndent(debug, 3); + virBufferAsprintf(ctxt->debug, "(alternate %zu/%zu)\n", i + 1, n); + virBufferAdjustIndent(ctxt->debug, 3); if (!(member = virJSONValueArrayGet(members, i)) || !(membertype = virJSONValueObjectGetString(member, "type")) || - !(memberschema = virHashLookup(schema, membertype))) { - virBufferAsprintf(debug, "ERROR: missing schema for alternate type '%s'", + !(memberschema = virHashLookup(ctxt->schema, membertype))) { + virBufferAsprintf(ctxt->debug, "ERROR: missing schema for alternate type '%s'", NULLSTR(membertype)); return -2; } - rc = testQEMUSchemaValidateRecurse(obj, memberschema, schema, debug); + rc = testQEMUSchemaValidateRecurse(obj, memberschema, ctxt); - virBufferAddLit(debug, "\n"); - virBufferSetIndent(debug, indent); - virBufferAsprintf(debug, "(/alternate %zu/%zu)\n", i + 1, n); + virBufferAddLit(ctxt->debug, "\n"); + virBufferSetIndent(ctxt->debug, indent); + virBufferAsprintf(ctxt->debug, "(/alternate %zu/%zu)\n", i + 1, n); if (rc == 0) { - virBufferAdjustIndent(debug, -3); - virBufferAddLit(debug, ") OK"); + virBufferAdjustIndent(ctxt->debug, -3); + virBufferAddLit(ctxt->debug, ") OK"); return 0; } } - virBufferAddLit(debug, "ERROR: no alternate type was matched"); + virBufferAddLit(ctxt->debug, "ERROR: no alternate type was matched"); return -1; } @@ -450,25 +447,24 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj, static int testQEMUSchemaValidateRecurse(virJSONValuePtr obj, virJSONValuePtr root, - virHashTablePtr schema, - virBufferPtr debug) + struct testQEMUSchemaValidateCtxt *ctxt) { const char *n = virJSONValueObjectGetString(root, "name"); const char *t = virJSONValueObjectGetString(root, "meta-type"); if (STREQ_NULLABLE(t, "builtin")) { - return testQEMUSchemaValidateBuiltin(obj, root, debug); + return testQEMUSchemaValidateBuiltin(obj, root, ctxt); } else if (STREQ_NULLABLE(t, "object")) { - return testQEMUSchemaValidateObject(obj, root, schema, debug); + return testQEMUSchemaValidateObject(obj, root, ctxt); } else if (STREQ_NULLABLE(t, "enum")) { - return testQEMUSchemaValidateEnum(obj, root, debug); + return testQEMUSchemaValidateEnum(obj, root, ctxt); } else if (STREQ_NULLABLE(t, "array")) { - return testQEMUSchemaValidateArray(obj, root, schema, debug); + return testQEMUSchemaValidateArray(obj, root, ctxt); } else if (STREQ_NULLABLE(t, "alternate")) { - return testQEMUSchemaValidateAlternate(obj, root, schema, debug); + return testQEMUSchemaValidateAlternate(obj, root, ctxt); } - virBufferAsprintf(debug, + virBufferAsprintf(ctxt->debug, "qapi schema meta-type '%s' of type '%s' not handled\n", NULLSTR(t), NULLSTR(n)); return -2; @@ -494,7 +490,9 @@ testQEMUSchemaValidate(virJSONValuePtr obj, virHashTablePtr schema, virBufferPtr debug) { - return testQEMUSchemaValidateRecurse(obj, root, schema, debug); + struct testQEMUSchemaValidateCtxt ctxt = { .schema = schema, + .debug = debug }; + return testQEMUSchemaValidateRecurse(obj, root, &ctxt); } @@ -518,6 +516,8 @@ testQEMUSchemaValidateCommand(const char *command, virHashTablePtr schema, virBufferPtr debug) { + struct testQEMUSchemaValidateCtxt ctxt = { .schema = schema, + .debug = debug }; g_autofree char *schemapatharguments = g_strdup_printf("%s/arg-type", command); g_autoptr(virJSONValue) emptyargs = NULL; virJSONValuePtr schemarootcommand; @@ -538,7 +538,7 @@ testQEMUSchemaValidateCommand(const char *command, return -1; } - return testQEMUSchemaValidateRecurse(arguments, schemarootarguments, schema, debug); + return testQEMUSchemaValidateRecurse(arguments, schemarootarguments, &ctxt); } -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Refactor the code so that we pass in the repeated 'schema' and 'debug' arguments via a new struct testQEMUSchemaValidateCtxt. This will simplify adding new parameters in the future.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 152 ++++++++++++++++++------------------ 1 file changed, 76 insertions(+), 76 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Some test cases are used to validate interactions with old qemu. We need to skip validation of deprecation with those once it will be added. In case of commands which were already replaced by code based on capabilities we can skip the full validation once the command is removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 14 +++++++------- tests/qemumonitorjsontest.c | 3 ++- tests/qemumonitortestutils.c | 5 ++++- tests/testutilsqemuschema.c | 17 +++++++++++++++-- tests/testutilsqemuschema.h | 3 +++ 5 files changed, 31 insertions(+), 11 deletions(-) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 8001807552..604e71bba7 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -86,7 +86,7 @@ testBackingXMLjsonXML(const void *args) if (!data->legacy) { if (testQEMUSchemaValidate(backendprops, data->schemaroot, - data->schema, &debug) < 0) { + data->schema, false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); g_autofree char *debugprops = virJSONValueToString(backendprops, true); @@ -168,7 +168,7 @@ testJSONtoJSON(const void *args) return -1; if (testQEMUSchemaValidate(jsonsrcout, data->schemaroot, - data->schema, &debug) < 0) { + data->schema, false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); VIR_TEST_VERBOSE("json does not conform to QAPI schema"); @@ -342,7 +342,7 @@ testQemuDiskXMLToPropsValidateSchema(const void *opaque) g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER; if (testQEMUSchemaValidate(data->images[i].formatprops, data->schemaroot, - data->schema, &debug) < 0) { + data->schema, false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); g_autofree char *propsstr = virJSONValueToString(data->images[i].formatprops, true); VIR_TEST_VERBOSE("json does not conform to QAPI schema"); @@ -354,7 +354,7 @@ testQemuDiskXMLToPropsValidateSchema(const void *opaque) virBufferFreeAndReset(&debug); if (testQEMUSchemaValidate(data->images[i].storageprops, data->schemaroot, - data->schema, &debug) < 0) { + data->schema, false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); g_autofree char *propsstr = virJSONValueToString(data->images[i].storageprops, true); VIR_TEST_VERBOSE("json does not conform to QAPI schema"); @@ -366,7 +366,7 @@ testQemuDiskXMLToPropsValidateSchema(const void *opaque) virBufferFreeAndReset(&debug); if (testQEMUSchemaValidate(data->images[i].storagepropssrc, data->schemaroot, - data->schema, &debug) < 0) { + data->schema, false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); g_autofree char *propsstr = virJSONValueToString(data->images[i].storagepropssrc, true); VIR_TEST_VERBOSE("json does not conform to QAPI schema"); @@ -544,7 +544,7 @@ testQemuImageCreate(const void *opaque) return -1; if (testQEMUSchemaValidate(formatprops, data->schemaroot, data->schema, - &debug) < 0) { + false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); VIR_TEST_VERBOSE("blockdev-create format json does not conform to QAPI schema"); VIR_TEST_DEBUG("json:\n%s\ndoes not match schema. Debug output:\n %s", @@ -559,7 +559,7 @@ testQemuImageCreate(const void *opaque) return -1; if (testQEMUSchemaValidate(protocolprops, data->schemaroot, data->schema, - &debug) < 0) { + false, &debug) < 0) { g_autofree char *debugmsg = virBufferContentAndReset(&debug); VIR_TEST_VERBOSE("blockdev-create protocol json does not conform to QAPI schema"); VIR_TEST_DEBUG("json:\n%s\ndoes not match schema. Debug output:\n %s", diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 60c816d1d1..fce88083b9 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2842,7 +2842,8 @@ testQAPISchemaValidate(const void *opaque) if (!(json = virJSONValueFromString(data->json))) goto cleanup; - if ((testQEMUSchemaValidate(json, schemaroot, data->schema, &debug) == 0) != data->success) { + if ((testQEMUSchemaValidate(json, schemaroot, data->schema, false, + &debug) == 0) != data->success) { if (!data->success) VIR_TEST_VERBOSE("\nschema validation should have failed"); } else { diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 0d9427f1d1..6be7555dc0 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -554,7 +554,10 @@ qemuMonitorTestProcessCommandDefaultValidate(qemuMonitorTestPtr test, if (STREQ(cmdname, "device_add")) return 0; - if (testQEMUSchemaValidateCommand(cmdname, args, test->qapischema, &debug) < 0) { + if (testQEMUSchemaValidateCommand(cmdname, args, test->qapischema, + test->skipValidationDeprecated, + test->skipValidationRemoved, + &debug) < 0) { if (virTestGetDebug() == 2) { g_autofree char *argstr = NULL; diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index b449171d15..f94a415b18 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -24,6 +24,7 @@ struct testQEMUSchemaValidateCtxt { virHashTablePtr schema; virBufferPtr debug; + bool allowDeprecated; }; @@ -488,10 +489,13 @@ int testQEMUSchemaValidate(virJSONValuePtr obj, virJSONValuePtr root, virHashTablePtr schema, + bool allowDeprecated, virBufferPtr debug) { struct testQEMUSchemaValidateCtxt ctxt = { .schema = schema, - .debug = debug }; + .debug = debug, + .allowDeprecated = allowDeprecated }; + return testQEMUSchemaValidateRecurse(obj, root, &ctxt); } @@ -501,6 +505,9 @@ testQEMUSchemaValidate(virJSONValuePtr obj, * @command: command to validate * @arguments: arguments of @command to validate * @schema: hash table containing schema entries + * @allowDeprecated: don't fails schema validation if @command or one of @arguments + * is deprecated + * @allowRemoved: skip validation fully if @command was not found * @debug: a virBuffer which will be filled with debug information if provided * * Validates whether @command and it's @arguments conforms to the QAPI schema @@ -508,16 +515,22 @@ testQEMUSchemaValidate(virJSONValuePtr obj, * -1 if it does not and -2 if there is a problem with the schema or with * internals. * + * @alllowRemoved should generally be used only if it's certain that there's a + * replacement of @command in place. + * * @debug is filled with information regarding the validation process */ int testQEMUSchemaValidateCommand(const char *command, virJSONValuePtr arguments, virHashTablePtr schema, + bool allowDeprecated, + bool allowRemoved G_GNUC_UNUSED, virBufferPtr debug) { struct testQEMUSchemaValidateCtxt ctxt = { .schema = schema, - .debug = debug }; + .debug = debug, + .allowDeprecated = allowDeprecated }; g_autofree char *schemapatharguments = g_strdup_printf("%s/arg-type", command); g_autoptr(virJSONValue) emptyargs = NULL; virJSONValuePtr schemarootcommand; diff --git a/tests/testutilsqemuschema.h b/tests/testutilsqemuschema.h index e3a375b038..8b6803afda 100644 --- a/tests/testutilsqemuschema.h +++ b/tests/testutilsqemuschema.h @@ -26,12 +26,15 @@ int testQEMUSchemaValidate(virJSONValuePtr obj, virJSONValuePtr root, virHashTablePtr schema, + bool allowDeprecated, virBufferPtr debug); int testQEMUSchemaValidateCommand(const char *command, virJSONValuePtr arguments, virHashTablePtr schema, + bool allowDeprecated, + bool allowRemoved, virBufferPtr debug); virJSONValuePtr -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Some test cases are used to validate interactions with old qemu. We need to skip validation of deprecation with those once it will be added.
In case of commands which were already replaced by code based on capabilities we can skip the full validation once the command is removed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 14 +++++++------- tests/qemumonitorjsontest.c | 3 ++- tests/qemumonitortestutils.c | 5 ++++- tests/testutilsqemuschema.c | 17 +++++++++++++++-- tests/testutilsqemuschema.h | 3 +++ 5 files changed, 31 insertions(+), 11 deletions(-)
@@ -508,16 +515,22 @@ testQEMUSchemaValidate(virJSONValuePtr obj, * -1 if it does not and -2 if there is a problem with the schema or with * internals. * + * @alllowRemoved should generally be used only if it's certain that there's a
*allow
+ * replacement of @command in place. + * * @debug is filled with information regarding the validation process */ int testQEMUSchemaValidateCommand(const char *command, virJSONValuePtr arguments, virHashTablePtr schema,
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We have a few cases validating that the code behaves correctly in pre-modern hotplug era. This is controled by the 'modern' flag for the test. Since 'cpu-add' command is now deprecated in qemu, there is a modern replacement for it, and the test output is checked against expected commands we can skip schema validation for the legacy command. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 849e7e7636..724f640aef 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -457,6 +457,9 @@ testQemuHotplugCpuPrepare(const char *test, if (fail) qemuMonitorTestAllowUnusedCommands(data->mon); + if (!data->modern) + qemuMonitorTestSkipDeprecatedValidation(data->mon, true); + priv->mon = qemuMonitorTestGetMonitor(data->mon); virObjectUnlock(priv->mon); -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
We have a few cases validating that the code behaves correctly in pre-modern hotplug era. This is controled by the 'modern' flag for the test. Since 'cpu-add' command is now deprecated in qemu, there is a modern replacement for it, and the test output is checked against expected commands we can skip schema validation for the legacy command.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuhotplugtest.c | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
For sanity-chcecking of deprecated commands which are still used on some old code paths which used the simple generated test cases add a mechanism to mark them as deprecated so schema checking can be skipped. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index fce88083b9..33bad45b96 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -46,6 +46,8 @@ struct _testQemuMonitorJSONSimpleFuncData { virDomainXMLOptionPtr xmlopt; const char *reply; virHashTablePtr schema; + bool allowDeprecated; + bool allowRemoved; }; typedef struct _testGenericData testGenericData; @@ -1278,6 +1280,9 @@ testQemuMonitorJSON ## funcName(const void *opaque) \ \ if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) \ return -1; \ + \ + if (data->allowDeprecated) \ + qemuMonitorTestSkipDeprecatedValidation(test, data->allowRemoved); \ \ if (!reply) \ reply = "{\"return\":{}}"; \ @@ -3121,13 +3126,19 @@ mymain(void) if (virTestRun(# FNC, testQemuMonitorJSONSimpleFunc, &simpleFunc) < 0) \ ret = -1 -#define DO_TEST_GEN(name, ...) \ +#define DO_TEST_GEN_FULL(name, dpr, rmvd, ...) \ simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = driver.xmlopt, \ + .allowDeprecated = dpr, \ + .allowRemoved = rmvd, \ .schema = qapiData.schema \ __VA_ARGS__ }; \ if (virTestRun(# name, testQemuMonitorJSON ## name, &simpleFunc) < 0) \ ret = -1 +#define DO_TEST_GEN(name, ...) DO_TEST_GEN_FULL(name, false, false, __VA_ARGS__) +#define DO_TEST_GEN_DEPRECATED(name, removed, ...) \ + DO_TEST_GEN_FULL(name, true, removed, __VA_ARGS__) + #define DO_TEST_CPU_DATA(name) \ do { \ struct testCPUData data = { name, driver.xmlopt, qapiData.schema }; \ -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
For sanity-chcecking of deprecated commands which are still used on some old code paths which used the simple generated test cases add a mechanism to mark them as deprecated so schema checking can be skipped.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Separate the test for the newer command into a new function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 33bad45b96..7e0ab4609c 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1435,10 +1435,6 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *opaque) {2, 17626, (char *) "/machine/unattached/device[2]", true}, {3, 17628, NULL, true}, }; - struct qemuMonitorQueryCpusEntry expect_fast[] = { - {0, 17629, (char *) "/machine/unattached/device[0]", false}, - {1, 17630, (char *) "/machine/unattached/device[1]", false}, - }; g_autoptr(qemuMonitorTest) test = NULL; if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) @@ -1483,6 +1479,29 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *opaque) "}") < 0) return -1; + /* query-cpus */ + if (testQEMUMonitorJSONqemuMonitorJSONQueryCPUsHelper(test, expect_slow, + false, 4)) + return -1; + + return 0; +} + + +static int +testQemuMonitorJSONqemuMonitorJSONQueryCPUsFast(const void *opaque) +{ + const testGenericData *data = opaque; + virDomainXMLOptionPtr xmlopt = data->xmlopt; + struct qemuMonitorQueryCpusEntry expect_fast[] = { + {0, 17629, (char *) "/machine/unattached/device[0]", false}, + {1, 17630, (char *) "/machine/unattached/device[1]", false}, + }; + g_autoptr(qemuMonitorTest) test = NULL; + + if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) + return -1; + if (qemuMonitorTestAddItem(test, "query-cpus-fast", "{" " \"return\": [" @@ -1501,11 +1520,6 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *opaque) "}") < 0) return -1; - /* query-cpus */ - if (testQEMUMonitorJSONqemuMonitorJSONQueryCPUsHelper(test, expect_slow, - false, 4)) - return -1; - /* query-cpus-fast */ if (testQEMUMonitorJSONqemuMonitorJSONQueryCPUsHelper(test, expect_fast, true, 2)) @@ -3236,6 +3250,7 @@ mymain(void) DO_TEST(qemuMonitorJSONGetTargetArch); DO_TEST(qemuMonitorJSONGetMigrationCapabilities); DO_TEST(qemuMonitorJSONQueryCPUs); + DO_TEST(qemuMonitorJSONQueryCPUsFast); DO_TEST(qemuMonitorJSONGetVirtType); DO_TEST(qemuMonitorJSONSendKey); DO_TEST(qemuMonitorJSONGetDumpGuestMemoryCapability); -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Separate the test for the newer command into a new function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The command was replaced with 'query-cpus-fast' which is always used when detected by the capabilities so we can allow our test usage of the deprecated command even if it will be removed from the schema. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 7e0ab4609c..eaaabe9a47 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1440,6 +1440,8 @@ testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *opaque) if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) return -1; + qemuMonitorTestSkipDeprecatedValidation(test, true); + if (qemuMonitorTestAddItem(test, "query-cpus", "{" " \"return\": [" @@ -2696,10 +2698,12 @@ testQemuMonitorCPUInfo(const void *opaque) queryHotpluggableStr) < 0) goto cleanup; - if (data->fast) + if (data->fast) { queryCpusFunction = "query-cpus-fast"; - else + } else { queryCpusFunction = "query-cpus"; + qemuMonitorTestSkipDeprecatedValidation(test, true); + } if (qemuMonitorTestAddItem(test, queryCpusFunction, queryCpusStr) < 0) goto cleanup; -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
The command was replaced with 'query-cpus-fast' which is always used when detected by the capabilities so we can allow our test usage of the deprecated command even if it will be removed from the schema.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Modify the generated test cases for the 'cpu-add' and 'change' command which are deprecated by qemu. We now use device-add and blockdev-change-media instead so we are okay if they will be removed. 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 eaaabe9a47..82a8122f29 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -3214,9 +3214,9 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONSetPassword); DO_TEST_GEN(qemuMonitorJSONExpirePassword); DO_TEST_GEN(qemuMonitorJSONSetBalloon); - DO_TEST_GEN(qemuMonitorJSONSetCPU); + DO_TEST_GEN_DEPRECATED(qemuMonitorJSONSetCPU, true); DO_TEST_GEN(qemuMonitorJSONEjectMedia); - DO_TEST_GEN(qemuMonitorJSONChangeMedia); + DO_TEST_GEN_DEPRECATED(qemuMonitorJSONChangeMedia, true); DO_TEST_GEN(qemuMonitorJSONSaveVirtualMemory); DO_TEST_GEN(qemuMonitorJSONSavePhysicalMemory); DO_TEST_GEN(qemuMonitorJSONSetMigrationSpeed); -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Modify the generated test cases for the 'cpu-add' and 'change' command which are deprecated by qemu. We now use device-add and blockdev-change-media instead so we are okay if they will be removed.
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
"migrate_set_downtime", "migrate_set_speed", and "query-migrate-cache-size" were marked as deprecated in the QMP schema in qemu 5.0. Since libvirt still actively uses them we must not mark them as okay to be missing, but still mark them as deprecated, so that we can add tests for deprecated commands. The replacement of the command usage in libvirt is tracked by: https://bugzilla.redhat.com/show_bug.cgi?id=1829543 https://bugzilla.redhat.com/show_bug.cgi?id=1829544 https://bugzilla.redhat.com/show_bug.cgi?id=1829545 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 82a8122f29..f58b18a110 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1889,6 +1889,8 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationCacheSize(const void *opaque) if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) return -1; + qemuMonitorTestSkipDeprecatedValidation(test, false); + if (qemuMonitorTestAddItem(test, "query-migrate-cache-size", "{" " \"return\": 67108864," @@ -3219,8 +3221,8 @@ mymain(void) DO_TEST_GEN_DEPRECATED(qemuMonitorJSONChangeMedia, true); DO_TEST_GEN(qemuMonitorJSONSaveVirtualMemory); DO_TEST_GEN(qemuMonitorJSONSavePhysicalMemory); - DO_TEST_GEN(qemuMonitorJSONSetMigrationSpeed); - DO_TEST_GEN(qemuMonitorJSONSetMigrationDowntime); + DO_TEST_GEN_DEPRECATED(qemuMonitorJSONSetMigrationSpeed, false); + DO_TEST_GEN_DEPRECATED(qemuMonitorJSONSetMigrationDowntime, false); DO_TEST_GEN(qemuMonitorJSONMigrate); DO_TEST_GEN(qemuMonitorJSONDump); DO_TEST_GEN(qemuMonitorJSONGraphicsRelocate); -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
"migrate_set_downtime", "migrate_set_speed", and "query-migrate-cache-size" were marked as deprecated in the QMP schema in qemu 5.0. Since libvirt still actively uses them we must not mark them as okay to be missing, but still mark them as deprecated, so that we can add tests for deprecated commands.
The replacement of the command usage in libvirt is tracked by: https://bugzilla.redhat.com/show_bug.cgi?id=1829543 https://bugzilla.redhat.com/show_bug.cgi?id=1829544 https://bugzilla.redhat.com/show_bug.cgi?id=1829545
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Make our QMP schema validator reject any use of schema entries which were deprecated by QEMU except for those whitelisted. This will allow us to catch this before qemu actually removed what we'd still use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 54 ++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index f94a415b18..898be68b0a 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -445,6 +445,47 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj, } +static int +testQEMUSchemaValidateDeprecated(virJSONValuePtr root, + const char *name, + struct testQEMUSchemaValidateCtxt *ctxt) +{ + virJSONValuePtr features = virJSONValueObjectGetArray(root, "features"); + size_t nfeatures; + size_t i; + + if (!features) + return 0; + + nfeatures = virJSONValueArraySize(features); + + for (i = 0; i < nfeatures; i++) { + virJSONValuePtr cur = virJSONValueArrayGet(features, i); + const char *curstr; + + if (!cur || + !(curstr = virJSONValueGetString(cur))) { + virBufferAsprintf(ctxt->debug, "ERROR: features of '%s' are malformed", name); + return -2; + } + + if (STREQ(curstr, "deprecated")) { + if (ctxt->allowDeprecated) { + virBufferAsprintf(ctxt->debug, "WARNING: '%s' is deprecated", name); + if (virTestGetVerbose()) + g_fprintf(stderr, "\nWARNING: '%s' is deprecated\n", name); + return 0; + } else { + virBufferAsprintf(ctxt->debug, "ERROR: '%s' is deprecated", name); + return -1; + } + } + } + + return 0; +} + + static int testQEMUSchemaValidateRecurse(virJSONValuePtr obj, virJSONValuePtr root, @@ -452,6 +493,10 @@ testQEMUSchemaValidateRecurse(virJSONValuePtr obj, { const char *n = virJSONValueObjectGetString(root, "name"); const char *t = virJSONValueObjectGetString(root, "meta-type"); + int rc; + + if ((rc = testQEMUSchemaValidateDeprecated(root, n, ctxt)) < 0) + return rc; if (STREQ_NULLABLE(t, "builtin")) { return testQEMUSchemaValidateBuiltin(obj, root, ctxt); @@ -525,7 +570,7 @@ testQEMUSchemaValidateCommand(const char *command, virJSONValuePtr arguments, virHashTablePtr schema, bool allowDeprecated, - bool allowRemoved G_GNUC_UNUSED, + bool allowRemoved, virBufferPtr debug) { struct testQEMUSchemaValidateCtxt ctxt = { .schema = schema, @@ -535,13 +580,20 @@ testQEMUSchemaValidateCommand(const char *command, g_autoptr(virJSONValue) emptyargs = NULL; virJSONValuePtr schemarootcommand; virJSONValuePtr schemarootarguments; + int rc; if (virQEMUQAPISchemaPathGet(command, schema, &schemarootcommand) < 0 || !schemarootcommand) { + if (allowRemoved) + return 0; + virBufferAsprintf(debug, "ERROR: command '%s' not found in the schema", command); return -1; } + if ((rc = testQEMUSchemaValidateDeprecated(schemarootcommand, command, &ctxt)) < 0) + return rc; + if (!arguments) arguments = emptyargs = virJSONValueNewObject(); -- 2.26.2
On a Wednesday in 2020, Peter Krempa wrote:
Make our QMP schema validator reject any use of schema entries which were deprecated by QEMU except for those whitelisted.
This will allow us to catch this before qemu actually removed what we'd still use.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/testutilsqemuschema.c | 54 ++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa