[PATCH 00/12 RESEND] qemu: Preparation for 'object-add' qapification
This is a resend of the patches from: https://www.redhat.com/archives/libvir-list/2020-November/msg01625.html which can be justified without the rest of the series. This series cleans up some monitor code and few related bits to testing. The reset of the series will be posted once there's progress on the qapification of object-add in qemu. Peter Krempa (12): qemuMonitorJSONSetMigrationParams: Take double pointer for @params qemuMonitorSetMigrationCapabilities: Take double pointer for @caps qemuMonitorJSONSetMigrationCapabilities: Refactor cleanup testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities: refactor cleanup qemuMonitorJSONAddObject: Take double pointer for @props qemuMonitorJSONMakeCommandInternal: Clear @arguments when stolen qemuMonitorAddObject: Fix semantics of @alias qemuMonitorAddObject: Refactor cleanup util: json: Replace virJSONValueObjectSteal by virJSONValueObjectRemoveKey tests: qemuxml2argv: Don't check whether -netdev was QAPIfied repeatedly qemuBuildChrChardevStr: Rename 'flags' to 'cdevflags' testCompareXMLToArgvValidateSchema: Populate autoNodeset src/qemu/qemu_command.c | 10 +++--- src/qemu/qemu_migration_params.c | 22 ++++-------- src/qemu/qemu_monitor.c | 45 +++++++++---------------- src/qemu/qemu_monitor.h | 4 +-- src/qemu/qemu_monitor_json.c | 57 ++++++++++++-------------------- src/qemu/qemu_monitor_json.h | 6 ++-- src/util/virjson.c | 29 +++------------- tests/qemumonitorjsontest.c | 26 +++++---------- tests/qemuxml2argvtest.c | 18 +++++++--- 9 files changed, 81 insertions(+), 136 deletions(-) -- 2.29.2
This allows simplification of the caller as well as will enable a later refactor of qemuMonitorJSONMakeCommandInternal. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 9 +++------ src/qemu/qemu_monitor.c | 11 +++-------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 5 +++-- src/qemu/qemu_monitor_json.h | 2 +- 5 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index df5560d39f..d1d59aeb01 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -843,12 +843,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, if (!(params = qemuMigrationParamsToJSON(migParams))) goto cleanup; - if (virJSONValueObjectKeysNumber(params) > 0) { - rc = qemuMonitorSetMigrationParams(priv->mon, params); - params = NULL; - if (rc < 0) - goto cleanup; - } + if (virJSONValueObjectKeysNumber(params) > 0 && + qemuMonitorSetMigrationParams(priv->mon, ¶ms) < 0) + goto cleanup; ret = 0; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 40f2997cb6..a81cd5fff5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2464,22 +2464,17 @@ qemuMonitorGetMigrationParams(qemuMonitorPtr mon, * @mon: Pointer to the monitor object. * @params: Migration parameters. * - * The @params object is consumed and should not be referenced by the caller - * after this function returns. + * The @params object is consumed and cleared. * * Returns 0 on success, -1 on error. */ int qemuMonitorSetMigrationParams(qemuMonitorPtr mon, - virJSONValuePtr params) + virJSONValuePtr *params) { - QEMU_CHECK_MONITOR_GOTO(mon, error); + QEMU_CHECK_MONITOR(mon); return qemuMonitorJSONSetMigrationParams(mon, params); - - error: - virJSONValueFree(params); - return -1; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 3a09b995ce..c543515cdc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -793,7 +793,7 @@ int qemuMonitorSetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorGetMigrationParams(qemuMonitorPtr mon, virJSONValuePtr *params); int qemuMonitorSetMigrationParams(qemuMonitorPtr mon, - virJSONValuePtr params); + virJSONValuePtr *params); typedef enum { QEMU_MONITOR_MIGRATION_STATUS_INACTIVE, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 936254f0ec..ad517a99b8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3461,12 +3461,13 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon, int qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, - virJSONValuePtr params) + virJSONValuePtr *params) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr par = g_steal_pointer(params); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters", params))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters", par))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4eb0f667a2..f3d7d204d6 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -140,7 +140,7 @@ int qemuMonitorJSONSetMigrationCacheSize(qemuMonitorPtr mon, int qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon, virJSONValuePtr *params); int qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, - virJSONValuePtr params); + virJSONValuePtr *params); int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon, qemuMonitorMigrationStatsPtr stats, -- 2.29.2
On 1/6/21 3:03 PM, Peter Krempa wrote:
This allows simplification of the caller as well as will enable a later refactor of qemuMonitorJSONMakeCommandInternal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 9 +++------ src/qemu/qemu_monitor.c | 11 +++-------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 5 +++-- src/qemu/qemu_monitor_json.h | 2 +- 5 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index df5560d39f..d1d59aeb01 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -843,12 +843,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, if (!(params = qemuMigrationParamsToJSON(migParams))) goto cleanup;
- if (virJSONValueObjectKeysNumber(params) > 0) { - rc = qemuMonitorSetMigrationParams(priv->mon, params); - params = NULL; - if (rc < 0) - goto cleanup; - } + if (virJSONValueObjectKeysNumber(params) > 0 && + qemuMonitorSetMigrationParams(priv->mon, ¶ms) < 0) + goto cleanup;
ret = 0;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 40f2997cb6..a81cd5fff5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2464,22 +2464,17 @@ qemuMonitorGetMigrationParams(qemuMonitorPtr mon, * @mon: Pointer to the monitor object. * @params: Migration parameters. * - * The @params object is consumed and should not be referenced by the caller - * after this function returns. + * The @params object is consumed and cleared.
This is not entirely true. If QEMU_CHECK_MONITOR() fails, then @params is not touched at all. However, since callers will almost certainly us g_auto() (because they need to construct virJSONValue object) I guess it's okay. Maybe change it to "consumed and cleared on success and potentially on some error cases too"?
* * Returns 0 on success, -1 on error. */ int qemuMonitorSetMigrationParams(qemuMonitorPtr mon, - virJSONValuePtr params) + virJSONValuePtr *params) { - QEMU_CHECK_MONITOR_GOTO(mon, error); + QEMU_CHECK_MONITOR(mon);
return qemuMonitorJSONSetMigrationParams(mon, params); - - error: - virJSONValueFree(params); - return -1; }
Michal
This allows simplification of the callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 13 ++++--------- src/qemu/qemu_monitor.c | 11 +++-------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 5 ++--- src/qemu/qemu_monitor_json.h | 2 +- tests/qemumonitorjsontest.c | 3 +-- 6 files changed, 12 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index d1d59aeb01..8c019bf2ce 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -803,7 +803,6 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, g_autoptr(virJSONValue) caps = NULL; qemuMigrationParam xbzrle = QEMU_MIGRATION_PARAM_XBZRLE_CACHE_SIZE; int ret = -1; - int rc; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; @@ -819,12 +818,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps, migParams->caps))) goto cleanup; - if (virJSONValueArraySize(caps) > 0) { - rc = qemuMonitorSetMigrationCapabilities(priv->mon, caps); - caps = NULL; - if (rc < 0) - goto cleanup; - } + if (virJSONValueArraySize(caps) > 0 && + qemuMonitorSetMigrationCapabilities(priv->mon, &caps) < 0) + goto cleanup; } /* If QEMU is too old to support xbzrle-cache-size migration parameter, @@ -1389,8 +1385,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; - rc = qemuMonitorSetMigrationCapabilities(priv->mon, json); - json = NULL; + rc = qemuMonitorSetMigrationCapabilities(priv->mon, &json); if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a81cd5fff5..8b1f90b363 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3877,22 +3877,17 @@ qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon, * @mon: Pointer to the monitor object. * @caps: Migration capabilities. * - * The @caps object is consumed and should not be referenced by the caller - * after this function returns. + * The @caps object is consumed cleared. * * Returns 0 on success, -1 on error. */ int qemuMonitorSetMigrationCapabilities(qemuMonitorPtr mon, - virJSONValuePtr caps) + virJSONValuePtr *caps) { - QEMU_CHECK_MONITOR_GOTO(mon, error); + QEMU_CHECK_MONITOR(mon); return qemuMonitorJSONSetMigrationCapabilities(mon, caps); - - error: - virJSONValueFree(caps); - return -1; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c543515cdc..a07617ec28 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -864,7 +864,7 @@ int qemuMonitorGetMigrationStats(qemuMonitorPtr mon, int qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon, char ***capabilities); int qemuMonitorSetMigrationCapabilities(qemuMonitorPtr mon, - virJSONValuePtr caps); + virJSONValuePtr *caps); int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, virGICCapability **capabilities); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ad517a99b8..0660c37e1e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6972,14 +6972,14 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon, int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, - virJSONValuePtr caps) + virJSONValuePtr *caps) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities", - "a:capabilities", &caps, + "a:capabilities", caps, NULL); if (!cmd) goto cleanup; @@ -6992,7 +6992,6 @@ qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, ret = 0; cleanup: - virJSONValueFree(caps); virJSONValueFree(cmd); virJSONValueFree(reply); return ret; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f3d7d204d6..4a5292a69c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -149,7 +149,7 @@ int qemuMonitorJSONGetMigrationStats(qemuMonitorPtr mon, int qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon, char ***capabilities); int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, - virJSONValuePtr caps); + virJSONValuePtr *caps); int qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, virGICCapability **capabilities); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index d0c37967d5..13794c2886 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2092,8 +2092,7 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque) goto cleanup; ret = qemuMonitorJSONSetMigrationCapabilities(qemuMonitorTestGetMonitor(test), - json); - json = NULL; + &json); cleanup: virJSONValueFree(json); -- 2.29.2
On 1/6/21 3:03 PM, Peter Krempa wrote:
This allows simplification of the callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_params.c | 13 ++++--------- src/qemu/qemu_monitor.c | 11 +++-------- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 5 ++--- src/qemu/qemu_monitor_json.h | 2 +- tests/qemumonitorjsontest.c | 3 +-- 6 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index d1d59aeb01..8c019bf2ce 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -803,7 +803,6 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, g_autoptr(virJSONValue) caps = NULL; qemuMigrationParam xbzrle = QEMU_MIGRATION_PARAM_XBZRLE_CACHE_SIZE; int ret = -1; - int rc;
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; @@ -819,12 +818,9 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps, migParams->caps))) goto cleanup;
- if (virJSONValueArraySize(caps) > 0) { - rc = qemuMonitorSetMigrationCapabilities(priv->mon, caps); - caps = NULL; - if (rc < 0) - goto cleanup; - } + if (virJSONValueArraySize(caps) > 0 && + qemuMonitorSetMigrationCapabilities(priv->mon, &caps) < 0) + goto cleanup; }
/* If QEMU is too old to support xbzrle-cache-size migration parameter, @@ -1389,8 +1385,7 @@ qemuMigrationCapsCheck(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1;
- rc = qemuMonitorSetMigrationCapabilities(priv->mon, json); - json = NULL; + rc = qemuMonitorSetMigrationCapabilities(priv->mon, &json);
if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a81cd5fff5..8b1f90b363 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3877,22 +3877,17 @@ qemuMonitorGetMigrationCapabilities(qemuMonitorPtr mon, * @mon: Pointer to the monitor object. * @caps: Migration capabilities. * - * The @caps object is consumed and should not be referenced by the caller - * after this function returns. + * The @caps object is consumed cleared.
Same here.
* * Returns 0 on success, -1 on error. */ int qemuMonitorSetMigrationCapabilities(qemuMonitorPtr mon, - virJSONValuePtr caps) + virJSONValuePtr *caps) { - QEMU_CHECK_MONITOR_GOTO(mon, error); + QEMU_CHECK_MONITOR(mon);
return qemuMonitorJSONSetMigrationCapabilities(mon, caps); - - error: - virJSONValueFree(caps); - return -1; }
Michal
Use automatic memory freeing and remove the 'cleanup' label and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 0660c37e1e..cb31df3019 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6974,27 +6974,21 @@ int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, virJSONValuePtr *caps) { - int ret = -1; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; - cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities", - "a:capabilities", caps, - NULL); - if (!cmd) - goto cleanup; + if (!(cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities", + "a:capabilities", caps, + NULL))) + return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.29.2
On 1/6/21 3:03 PM, Peter Krempa wrote:
Use automatic memory freeing and remove the 'cleanup' label and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 0660c37e1e..cb31df3019 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6974,27 +6974,21 @@ int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, virJSONValuePtr *caps) { - int ret = -1; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL;
- cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities", - "a:capabilities", caps, - NULL); - if (!cmd) - goto cleanup; + if (!(cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities", + "a:capabilities", caps, + NULL))) + return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1;
if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; + return -1;
- ret = 0; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; }
Or simply return qemuMonitorJSONCheckError(); We have some such occurrences already. Michal
Use automatic memory freeing to remove the 'cleanup:' label and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 13794c2886..29c396891b 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2050,11 +2050,10 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque) { const testGenericData *data = opaque; virDomainXMLOptionPtr xmlopt = data->xmlopt; - int ret = -1; const char *cap; - char **caps = NULL; - virBitmapPtr bitmap = NULL; - virJSONValuePtr json = NULL; + g_auto(GStrv) caps = NULL; + g_autoptr(virBitmap) bitmap = NULL; + g_autoptr(virJSONValue) json = NULL; const char *reply = "{" " \"return\": [" @@ -2073,32 +2072,26 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque) if (qemuMonitorTestAddItem(test, "query-migrate-capabilities", reply) < 0 || qemuMonitorTestAddItem(test, "migrate-set-capabilities", "{\"return\":{}}") < 0) - goto cleanup; + return -1; if (qemuMonitorGetMigrationCapabilities(qemuMonitorTestGetMonitor(test), &caps) < 0) - goto cleanup; + return -1; cap = qemuMigrationCapabilityTypeToString(QEMU_MIGRATION_CAP_XBZRLE); if (!virStringListHasString((const char **) caps, cap)) { virReportError(VIR_ERR_INTERNAL_ERROR, "Expected capability %s is missing", cap); - goto cleanup; + return -1; } bitmap = virBitmapNew(QEMU_MIGRATION_CAP_LAST); ignore_value(virBitmapSetBit(bitmap, QEMU_MIGRATION_CAP_XBZRLE)); if (!(json = qemuMigrationCapsToJSON(bitmap, bitmap))) - goto cleanup; - - ret = qemuMonitorJSONSetMigrationCapabilities(qemuMonitorTestGetMonitor(test), - &json); + return -1; - cleanup: - virJSONValueFree(json); - g_strfreev(caps); - virBitmapFree(bitmap); - return ret; + return qemuMonitorJSONSetMigrationCapabilities(qemuMonitorTestGetMonitor(test), + &json); } static int -- 2.29.2
Prepare for a refactor of qemuMonitorJSONMakeCommandInternal. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 3 +-- src/qemu/qemu_monitor_json.c | 5 +++-- src/qemu/qemu_monitor_json.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 8b1f90b363..a55c4e19dd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3076,8 +3076,7 @@ qemuMonitorAddObject(qemuMonitorPtr mon, if (alias) tmp = g_strdup(id); - ret = qemuMonitorJSONAddObject(mon, *props); - *props = NULL; + ret = qemuMonitorJSONAddObject(mon, props); if (alias) *alias = g_steal_pointer(&tmp); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index cb31df3019..9116a8a8cb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4593,12 +4593,13 @@ qemuMonitorJSONAddDevice(qemuMonitorPtr mon, int qemuMonitorJSONAddObject(qemuMonitorPtr mon, - virJSONValuePtr props) + virJSONValuePtr *props) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", pr))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4a5292a69c..ba1531fee8 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -244,7 +244,7 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon, const char *devalias); int qemuMonitorJSONAddObject(qemuMonitorPtr mon, - virJSONValuePtr props); + virJSONValuePtr *props); int qemuMonitorJSONDelObject(qemuMonitorPtr mon, const char *objalias, -- 2.29.2
All callers of qemuMonitorJSONMakeCommandInternal will benefit from making @arguments a double pointer and passing it to virJSONValueObjectCreate directly which will clear it if it steals the value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9116a8a8cb..8a75a2734e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -546,20 +546,18 @@ qemuMonitorJSONTransactionAdd(virJSONValuePtr actions, * * Create a JSON object used on the QMP monitor to call a command. * - * Note that @arguments is always consumed and should not be referenced after - * the call to this function. + * Note that @arguments is consumed and cleared. */ static virJSONValuePtr qemuMonitorJSONMakeCommandInternal(const char *cmdname, - virJSONValuePtr arguments) + virJSONValuePtr *arguments) { virJSONValuePtr ret = NULL; ignore_value(virJSONValueObjectCreate(&ret, "s:execute", cmdname, - "A:arguments", &arguments, NULL)); + "A:arguments", arguments, NULL)); - virJSONValueFree(arguments); return ret; } @@ -569,7 +567,7 @@ qemuMonitorJSONMakeCommand(const char *cmdname, ...) { virJSONValuePtr obj = NULL; - virJSONValuePtr jargs = NULL; + g_autoptr(virJSONValue) jargs = NULL; va_list args; va_start(args, cmdname); @@ -577,7 +575,7 @@ qemuMonitorJSONMakeCommand(const char *cmdname, if (virJSONValueObjectCreateVArgs(&jargs, args) < 0) goto cleanup; - obj = qemuMonitorJSONMakeCommandInternal(cmdname, jargs); + obj = qemuMonitorJSONMakeCommandInternal(cmdname, &jargs); cleanup: va_end(args); @@ -3465,9 +3463,8 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virJSONValuePtr par = g_steal_pointer(params); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters", par))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("migrate-set-parameters", params))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -3984,7 +3981,7 @@ int qemuMonitorJSONAddFileHandleToSet(qemuMonitorPtr mon, return -1; } - if (!(cmd = qemuMonitorJSONMakeCommandInternal("add-fd", g_steal_pointer(&args)))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("add-fd", &args))) return -1; if (qemuMonitorJSONCommandWithFd(mon, cmd, fd, &reply) < 0) @@ -4168,9 +4165,8 @@ qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("netdev_add", pr))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("netdev_add", props))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -4597,9 +4593,8 @@ qemuMonitorJSONAddObject(qemuMonitorPtr mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", pr))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -7347,9 +7342,8 @@ qemuMonitorJSONBlockExportAdd(qemuMonitorPtr mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("block-export-add", pr))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("block-export-add", props))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -8866,9 +8860,8 @@ qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", pr))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", props))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) @@ -8887,9 +8880,8 @@ qemuMonitorJSONBlockdevReopen(qemuMonitorPtr mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virJSONValuePtr pr = g_steal_pointer(props); - if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-reopen", pr))) + if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-reopen", props))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) -- 2.29.2
The callers of qemuMonitorAddObject rely on the fact that @alias is filled only when the object is added successfully. This is documented but the code didn't behave like that. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a55c4e19dd..7919eb3a04 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3076,11 +3076,14 @@ qemuMonitorAddObject(qemuMonitorPtr mon, if (alias) tmp = g_strdup(id); - ret = qemuMonitorJSONAddObject(mon, props); + if (qemuMonitorJSONAddObject(mon, props) < 0) + goto cleanup; if (alias) *alias = g_steal_pointer(&tmp); + ret = 0; + cleanup: VIR_FREE(tmp); virJSONValueFree(*props); -- 2.29.2
Remove freeing/clearing of @props as the function doesn't guarantee that it happens on success, rename the variable hodling copy of the alias and use g_autofree to automatically free it and remove the cleanup label as well as 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7919eb3a04..55ec032b5f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3050,13 +3050,12 @@ qemuMonitorAddObject(qemuMonitorPtr mon, { const char *type = NULL; const char *id = NULL; - char *tmp = NULL; - int ret = -1; + g_autofree char *aliasCopy = NULL; if (!*props) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("object props can't be NULL")); - goto cleanup; + return -1; } type = virJSONValueObjectGetString(*props, "qom-type"); @@ -3064,31 +3063,25 @@ qemuMonitorAddObject(qemuMonitorPtr mon, VIR_DEBUG("type=%s id=%s", NULLSTR(type), NULLSTR(id)); - QEMU_CHECK_MONITOR_GOTO(mon, cleanup); + QEMU_CHECK_MONITOR(mon); if (!id || !type) { virReportError(VIR_ERR_INTERNAL_ERROR, _("missing alias or qom-type for qemu object '%s'"), NULLSTR(type)); - goto cleanup; + return -1; } if (alias) - tmp = g_strdup(id); + aliasCopy = g_strdup(id); if (qemuMonitorJSONAddObject(mon, props) < 0) - goto cleanup; + return -1; if (alias) - *alias = g_steal_pointer(&tmp); - - ret = 0; + *alias = g_steal_pointer(&aliasCopy); - cleanup: - VIR_FREE(tmp); - virJSONValueFree(*props); - *props = NULL; - return ret; + return 0; } -- 2.29.2
virJSONValueObjectRemoveKey can be used as direct replacement. Fix the one caller and remove the duplicate function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virjson.c | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index c80c2f1ecb..f85b61957c 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -883,30 +883,6 @@ virJSONValueObjectGet(virJSONValuePtr object, } -static virJSONValuePtr -virJSONValueObjectSteal(virJSONValuePtr object, - const char *key) -{ - size_t i; - virJSONValuePtr obj = NULL; - - if (object->type != VIR_JSON_TYPE_OBJECT) - return NULL; - - for (i = 0; i < object->data.object.npairs; i++) { - if (STREQ(object->data.object.pairs[i].key, key)) { - obj = g_steal_pointer(&object->data.object.pairs[i].value); - VIR_FREE(object->data.object.pairs[i].key); - VIR_DELETE_ELEMENT(object->data.object.pairs, i, - object->data.object.npairs); - break; - } - } - - return obj; -} - - /* Return the value associated with KEY within OBJECT, but return NULL * if the key is missing or if value is not the correct TYPE. */ virJSONValuePtr @@ -929,7 +905,10 @@ virJSONValueObjectStealByType(virJSONValuePtr object, const char *key, virJSONType type) { - virJSONValuePtr value = virJSONValueObjectSteal(object, key); + virJSONValuePtr value; + + if (virJSONValueObjectRemoveKey(object, key, &value) <= 0) + return NULL; if (value && value->type == type) return value; -- 2.29.2
Check once before looping through the args. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d2712e0dce..662d0d5df6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -524,6 +524,7 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv, g_autoptr(GHashTable) schema = NULL; g_autoptr(virCommand) cmd = NULL; unsigned int parseFlags = info->parseFlags; + bool netdevQAPIfied = false; if (info->schemafile) schema = testQEMUSchemaLoad(info->schemafile); @@ -552,6 +553,8 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv, if (virCommandGetArgList(cmd, &args, &nargs) < 0) return -1; + netdevQAPIfied = !virQEMUQAPISchemaPathExists("netdev_add/arg-type/type/!string", schema); + for (i = 0; i < nargs; i++) { g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER; g_autoptr(virJSONValue) jsonargs = NULL; @@ -569,13 +572,14 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv, i++; } else if (STREQ(args[i], "-netdev")) { + if (!netdevQAPIfied) { + i++; + continue; + } + if (!(jsonargs = virJSONValueFromString(args[i + 1]))) return -1; - /* skip the validation for pre-QAPIfication cases */ - if (virQEMUQAPISchemaPathExists("netdev_add/arg-type/type/!string", schema)) - continue; - if (testQEMUSchemaValidateCommand("netdev_add", jsonargs, schema, false, false, &debug) < 0) { VIR_TEST_VERBOSE("failed to validate -netdev '%s' against QAPI schema: %s", -- 2.29.2
The monitor code uses 'flags' for the flags of the monitor builder, while in this function it's a different set of flags. All callers pass a variable named 'cdevflags', so rename the argument to suit. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 02f956ce48..3e6518b96d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4756,7 +4756,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, const virDomainChrSourceDef *dev, const char *alias, virQEMUCapsPtr qemuCaps, - unsigned int flags) + unsigned int cdevflags) { qemuDomainChrSourcePrivatePtr chrSourcePriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(dev); g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -4789,7 +4789,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, case VIR_DOMAIN_CHR_TYPE_FILE: virBufferAsprintf(&buf, "file,id=%s", charAlias); - if (qemuBuildChrChardevFileStr(flags & QEMU_BUILD_CHARDEV_FILE_LOGD ? + if (qemuBuildChrChardevFileStr(cdevflags & QEMU_BUILD_CHARDEV_FILE_LOGD ? logManager : NULL, cmd, def, &buf, "path", dev->data.file.path, @@ -4838,7 +4838,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (dev->data.tcp.listen) { virBufferAddLit(&buf, ",server"); - if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) + if (cdevflags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) virBufferAddLit(&buf, ",nowait"); } @@ -4878,7 +4878,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, case VIR_DOMAIN_CHR_TYPE_UNIX: virBufferAsprintf(&buf, "socket,id=%s", charAlias); if (dev->data.nix.listen && - (flags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) && + (cdevflags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) { int fd; @@ -4901,7 +4901,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, } if (dev->data.nix.listen) { virBufferAddLit(&buf, ",server"); - if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) + if (cdevflags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) virBufferAddLit(&buf, ",nowait"); } -- 2.29.2
We create a new 'vm' so we must also fake the nodeset. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 662d0d5df6..8b2df07448 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -519,6 +519,7 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv, { g_auto(GStrv) args = NULL; g_autoptr(virDomainObj) vm = NULL; + qemuDomainObjPrivatePtr priv = NULL; size_t nargs = 0; size_t i; g_autoptr(GHashTable) schema = NULL; @@ -546,6 +547,11 @@ testCompareXMLToArgvValidateSchema(virQEMUDriverPtr drv, NULL, parseFlags))) return -1; + priv = vm->privateData; + + if (virBitmapParse("0-3", &priv->autoNodeset, 4) < 0) + return -1; + if (!(cmd = testCompareXMLToArgvCreateArgs(drv, vm, migrateURI, info, flags, true))) return -1; -- 2.29.2
On 1/6/21 3:03 PM, Peter Krempa wrote:
This is a resend of the patches from:
https://www.redhat.com/archives/libvir-list/2020-November/msg01625.html
which can be justified without the rest of the series. This series cleans up some monitor code and few related bits to testing.
The reset of the series will be posted once there's progress on the qapification of object-add in qemu.
Peter Krempa (12): qemuMonitorJSONSetMigrationParams: Take double pointer for @params qemuMonitorSetMigrationCapabilities: Take double pointer for @caps qemuMonitorJSONSetMigrationCapabilities: Refactor cleanup testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities: refactor cleanup qemuMonitorJSONAddObject: Take double pointer for @props qemuMonitorJSONMakeCommandInternal: Clear @arguments when stolen qemuMonitorAddObject: Fix semantics of @alias qemuMonitorAddObject: Refactor cleanup util: json: Replace virJSONValueObjectSteal by virJSONValueObjectRemoveKey tests: qemuxml2argv: Don't check whether -netdev was QAPIfied repeatedly qemuBuildChrChardevStr: Rename 'flags' to 'cdevflags' testCompareXMLToArgvValidateSchema: Populate autoNodeset
src/qemu/qemu_command.c | 10 +++--- src/qemu/qemu_migration_params.c | 22 ++++-------- src/qemu/qemu_monitor.c | 45 +++++++++---------------- src/qemu/qemu_monitor.h | 4 +-- src/qemu/qemu_monitor_json.c | 57 ++++++++++++-------------------- src/qemu/qemu_monitor_json.h | 6 ++-- src/util/virjson.c | 29 +++------------- tests/qemumonitorjsontest.c | 26 +++++---------- tests/qemuxml2argvtest.c | 18 +++++++--- 9 files changed, 81 insertions(+), 136 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Peter Krempa