[PATCH 00/12] JSON monitor handling cleanups and spurious error bug fix
First half introduces qemuMonitorJSONGetReply and uses it in our code base, second half introduces virJSONValueArrayToStringList, uses it to fix a bug and refactor the rest of the usage. Peter Krempa (12): qemu: monitor: Introduce qemuMonitorJSONGetReply, a better qemuMonitorJSONCheckReply qemu: monitor: Use qemuMonitorJSONGetReply for VIR_JSON_TYPE_OBJECT qemu: monitor: Use qemuMonitorJSONGetReply for VIR_JSON_TYPE_ARRAY qemu: monitor: Use qemuMonitorJSONGetReply when the value is extracted directly qemu: monitor: Unify and refactor 'PTY' case in qemuMonitorJSONAttachCharDev util: json: Split out array->strinlist conversion from virJSONValueObjectGetStringArray qemuAgentGetDisks: Don't use virJSONValueObjectGetStringArray for optional data qemuMonitorJSONGetCPUDefinitions: Rework lookup of 'unavailable-features' qemuMonitorJSONGetCPUDefinitions: Avoid double lookup of object qemu: monitor: Use qemuMonitorJSONGetReply in conjunction with virJSONValueArrayToStringList qemuAgentSSHGetAuthorizedKeys: Convert last use ofvirJSONValueObjectGetStringArray util: json: Remove unused virJSONValueObjectGetStringArray wrapper src/libvirt_private.syms | 2 +- src/qemu/qemu_agent.c | 13 ++- src/qemu/qemu_monitor_json.c | 189 +++++++++++++++-------------------- src/util/virjson.c | 44 +++----- src/util/virjson.h | 3 +- 5 files changed, 106 insertions(+), 145 deletions(-) -- 2.38.1
Rather than simply checking that the 'return' field is of the expected type we can directly return it as the caller is very likely going to use it. Extract the code into the new function and add a wrapper to preserve old functionality. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f4e942a32f..e6cfaaf0e9 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -396,15 +396,15 @@ qemuMonitorJSONCheckError(virJSONValue *cmd, } -static int -qemuMonitorJSONCheckReply(virJSONValue *cmd, - virJSONValue *reply, - virJSONType type) +static virJSONValue * +qemuMonitorJSONGetReply(virJSONValue *cmd, + virJSONValue *reply, + virJSONType type) { virJSONValue *data; if (qemuMonitorJSONCheckError(cmd, reply) < 0) - return -1; + return NULL; data = virJSONValueObjectGet(reply, "return"); if (virJSONValueGetType(data) != type) { @@ -417,9 +417,21 @@ qemuMonitorJSONCheckReply(virJSONValue *cmd, _("unexpected type returned by QEMU command '%s'"), qemuMonitorJSONCommandName(cmd)); - return -1; + return NULL; } + return data; +} + + +static int +qemuMonitorJSONCheckReply(virJSONValue *cmd, + virJSONValue *reply, + virJSONType type) +{ + if (!qemuMonitorJSONGetReply(cmd, reply, type)) + return -1; + return 0; } -- 2.38.1
Replace usage of the following pattern with the new helper: if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) return -1; data = virJSONValueObjectGetObject(reply, "return"); Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 52 +++++++++--------------------------- 1 file changed, 13 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e6cfaaf0e9..e3fe068c1b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1481,11 +1481,9 @@ qemuMonitorJSONGetStatus(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (virJSONValueObjectGetBoolean(data, "running", running) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-status reply was missing running state")); @@ -1866,11 +1864,9 @@ qemuMonitorJSONGetBalloonInfo(qemuMonitor *mon, } /* See if any other fatal error occurred */ - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (virJSONValueObjectGetNumberUlong(data, "actual", &mem) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("info balloon reply was missing balloon data")); @@ -1964,11 +1960,9 @@ qemuMonitorJSONGetMemoryStats(qemuMonitor *mon, } } - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return got; - data = virJSONValueObjectGetObject(reply, "return"); - if (!(statsdata = virJSONValueObjectGet(data, "stats"))) { VIR_DEBUG("data does not include 'stats'"); return got; @@ -3165,11 +3159,9 @@ qemuMonitorJSONGetMigrationBlockers(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons"))) return 0; @@ -3244,11 +3236,9 @@ qemuMonitorJSONQueryDump(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(result = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - result = virJSONValueObjectGetObject(reply, "return"); - return qemuMonitorJSONExtractDumpStats(result, stats); } @@ -3270,11 +3260,9 @@ qemuMonitorJSONGetDumpGuestMemoryCapability(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(caps = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - caps = virJSONValueObjectGetObject(reply, "return"); - if (!(formats = virJSONValueObjectGetArray(caps, "formats"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing supported dump formats")); @@ -4656,11 +4644,9 @@ int qemuMonitorJSONGetVersion(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (!(qemu = virJSONValueObjectGetObject(data, "qemu"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-version reply was missing 'qemu' data")); @@ -5155,11 +5141,9 @@ qemuMonitorJSONGetCPUModelBaseline(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (qemuMonitorJSONParseCPUModelData(data, "query-cpu-model-baseline", false, &cpu_model, &cpu_props, &cpu_name) < 0) @@ -5322,11 +5306,9 @@ int qemuMonitorJSONGetKVMState(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (virJSONValueObjectGetBoolean(data, "enabled", enabled) < 0 || virJSONValueObjectGetBoolean(data, "present", present) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -5761,11 +5743,9 @@ qemuMonitorJSONGetTargetArch(qemuMonitor *mon) if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return NULL; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return NULL; - data = virJSONValueObjectGetObject(reply, "return"); - if (!(arch = virJSONValueObjectGetString(data, "arch"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-target reply was missing arch data")); @@ -7521,11 +7501,9 @@ qemuMonitorJSONGetRTCTime(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGet(reply, "return"); - if (virJSONValueObjectGetNumberInt(data, "tm_year", &tm->tm_year) < 0 || virJSONValueObjectGetNumberInt(data, "tm_mon", &tm->tm_mon) < 0 || virJSONValueObjectGetNumberInt(data, "tm_mday", &tm->tm_mday) < 0 || @@ -7973,11 +7951,9 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitor *mon) if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return NULL; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return NULL; - data = virJSONValueObjectGetObject(reply, "return"); - if (!(tmp = virJSONValueObjectGetString(data, "data"))) return NULL; @@ -8016,11 +7992,9 @@ qemuMonitorJSONGetSEVInfo(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) return -1; - data = virJSONValueObjectGetObject(reply, "return"); - if (virJSONValueObjectGetNumberUint(data, "api-major", apiMajor) < 0 || virJSONValueObjectGetNumberUint(data, "api-minor", apiMinor) < 0 || virJSONValueObjectGetNumberUint(data, "build-id", buildID) < 0 || -- 2.38.1
Replace usage of the following pattern with the new helper: if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) return -1; data = virJSONValueObjectGetArray(reply, "return"); Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 48 +++++++++++------------------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e3fe068c1b..79729e0a9a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4702,10 +4702,9 @@ int qemuMonitorJSONGetMachines(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) goto cleanup; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); /* null-terminated list */ @@ -4830,10 +4829,9 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitor *mon, if (qemuMonitorJSONHasError(reply, "GenericError")) return 0; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); ncpus = virJSONValueArraySize(data); if (!(defs = qemuMonitorCPUDefsNew(ncpus))) @@ -5214,10 +5212,9 @@ int qemuMonitorJSONGetCommands(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); /* null-terminated list */ @@ -5339,10 +5336,9 @@ qemuMonitorJSONGetObjectTypes(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); /* null-terminated list */ @@ -5388,10 +5384,9 @@ int qemuMonitorJSONGetObjectListPaths(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) goto cleanup; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); /* null-terminated list */ @@ -5620,10 +5615,9 @@ qemuMonitorJSONParsePropsList(virJSONValue *cmd, size_t count = 0; size_t i; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); /* null-terminated list */ @@ -5776,10 +5770,9 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(caps = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - caps = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(caps); list = g_new0(char *, n + 1); @@ -5871,10 +5864,9 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitor *mon, if (qemuMonitorJSONHasError(reply, "CommandNotFound")) return 0; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(caps = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - caps = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(caps); /* If the returned array was empty we have to return successfully */ @@ -6754,10 +6746,9 @@ qemuMonitorJSONGetCPUx86Data(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); if (!(*cpudata = qemuMonitorJSONParseCPUx86Features(data))) return -1; @@ -6795,10 +6786,9 @@ qemuMonitorJSONCheckCPUx86(qemuMonitor *mon, } } - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); for (i = 0; i < n; i++) { @@ -7031,10 +7021,9 @@ qemuMonitorJSONGetIOThreads(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) goto cleanup; - data = virJSONValueObjectGetArray(reply, "return"); n = virJSONValueArraySize(data); /* null-terminated list */ @@ -7174,11 +7163,9 @@ qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); - for (i = 0; i < virJSONValueArraySize(data); i++) { virJSONValue *elem = virJSONValueArrayGet(data, i); g_autofree qemuMonitorMemoryDeviceInfo *meminfo = NULL; @@ -7659,10 +7646,9 @@ qemuMonitorJSONGetHotpluggableCPUs(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) goto cleanup; - data = virJSONValueObjectGet(reply, "return"); ninfo = virJSONValueArraySize(data); info = g_new0(struct qemuMonitorQueryHotpluggableCpusEntry, ninfo); @@ -8372,11 +8358,9 @@ qemuMonitorJSONGetJobInfo(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - data = virJSONValueObjectGetArray(reply, "return"); - for (i = 0; i < virJSONValueArraySize(data); i++) { qemuMonitorJobInfo *job = NULL; @@ -8783,11 +8767,9 @@ qemuMonitorJSONQueryStatsSchema(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return NULL; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(ret = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return NULL; - ret = virJSONValueObjectGetArray(reply, "return"); - return qemuMonitorJSONExtractQueryStatsSchema(ret); } -- 2.38.1
Use qemuMonitorJSONGetReply in cases where qemuMonitorJSONCheckReply is followed by virJSONValueObjectGet*(reply, "return"). Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 79729e0a9a..47df4a5160 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5267,6 +5267,7 @@ qemuMonitorJSONGetCommandLineOptions(qemuMonitor *mon) g_autoptr(GHashTable) ret = virHashNew(virJSONValueHashFree); g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValue *data; if (!(cmd = qemuMonitorJSONMakeCommand("query-command-line-options", NULL))) return NULL; @@ -5274,10 +5275,10 @@ qemuMonitorJSONGetCommandLineOptions(qemuMonitor *mon) if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return NULL; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return NULL; - if (virJSONValueArrayForeachSteal(virJSONValueObjectGetArray(reply, "return"), + if (virJSONValueArrayForeachSteal(data, qemuMonitorJSONGetCommandLineOptionsWorker, ret) < 0) return NULL; @@ -5673,6 +5674,7 @@ qemuMonitorJSONGetDeviceProps(qemuMonitor *mon, g_autoptr(GHashTable) props = virHashNew(virJSONValueHashFree); g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValue *data; if (!(cmd = qemuMonitorJSONMakeCommand("device-list-properties", "s:typename", device, @@ -5686,10 +5688,10 @@ qemuMonitorJSONGetDeviceProps(qemuMonitor *mon, if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) return g_steal_pointer(&props); - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return NULL; - if (virJSONValueArrayForeachSteal(virJSONValueObjectGetArray(reply, "return"), + if (virJSONValueArrayForeachSteal(data, qemuMonitorJSONGetDevicePropsWorker, props) < 0) return NULL; @@ -8381,6 +8383,7 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitor *mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValue *data; if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", "s:path", cpuQOMPath, @@ -8394,11 +8397,10 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitor *mon, if (qemuMonitorJSONHasError(reply, "GenericError")) return 1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_BOOLEAN) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_BOOLEAN))) return -1; - return virJSONValueGetBoolean(virJSONValueObjectGet(reply, "return"), - migratable); + return virJSONValueGetBoolean(data, migratable); } -- 2.38.1
Use qemuMonitorJSONGetReply and unify the two blocks with the same condition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 47df4a5160..a5800e182c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6578,24 +6578,19 @@ qemuMonitorJSONAttachCharDev(qemuMonitor *mon, return -1; if (chr->type == VIR_DOMAIN_CHR_TYPE_PTY) { - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) - return -1; - } else { - if (qemuMonitorJSONCheckError(cmd, reply) < 0) - return -1; - } + virJSONValue *data; - if (chr->type == VIR_DOMAIN_CHR_TYPE_PTY) { - virJSONValue *data = virJSONValueObjectGetObject(reply, "return"); - const char *path; + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_OBJECT))) + return -1; - if (!(path = virJSONValueObjectGetString(data, "pty"))) { + if (!(chr->data.file.path = g_strdup(virJSONValueObjectGetString(data, "pty")))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("chardev-add reply was missing pty path")); return -1; } - - chr->data.file.path = g_strdup(path); + } else { + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; } return 0; -- 2.38.1
Introduce virJSONValueArrayToStringList which does only the conversion from an array to a stringlist. This will allow refactoring the callers to be more careful in case when they want to handle the existance of the member in the parent object differently. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 43 ++++++++++++++++++++++------------------ src/util/virjson.h | 2 ++ 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ebd7bc61a8..a5bb44a9f8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2564,6 +2564,7 @@ virJSONValueArrayForeachSteal; virJSONValueArrayGet; virJSONValueArraySize; virJSONValueArraySteal; +virJSONValueArrayToStringList; virJSONValueCopy; virJSONValueFree; virJSONValueFromString; diff --git a/src/util/virjson.c b/src/util/virjson.c index ef59b5deb4..5701c09bfd 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1316,10 +1316,7 @@ virJSONValueObjectStealObject(virJSONValue *object, char ** virJSONValueObjectGetStringArray(virJSONValue *object, const char *key) { - g_auto(GStrv) ret = NULL; virJSONValue *data; - size_t n; - size_t i; data = virJSONValueObjectGetArray(object, key); if (!data) { @@ -1329,32 +1326,40 @@ virJSONValueObjectGetStringArray(virJSONValue *object, const char *key) return NULL; } - n = virJSONValueArraySize(data); - ret = g_new0(char *, n + 1); + return virJSONValueArrayToStringList(data); +} + + +/** + * virJSONValueArrayToStringList: + * @data: a JSON array containing strings to convert + * + * Converts @data a JSON array containing strings to a NULL-terminated string + * list. @data must be a JSON array. In case @data is doesn't contain only + * strings an error is reported. + */ +char ** +virJSONValueArrayToStringList(virJSONValue *data) +{ + size_t n = virJSONValueArraySize(data); + g_auto(GStrv) ret = g_new0(char *, n + 1); + size_t i; + for (i = 0; i < n; i++) { virJSONValue *child = virJSONValueArrayGet(data, i); - const char *tmp; - if (!child) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s array element is missing item %zu"), - key, i); + if (!child || + !(ret[i] = g_strdup(virJSONValueGetString(child)))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("JSON string array contains non-string element")); return NULL; } - - if (!(tmp = virJSONValueGetString(child))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s array element does not contain a string"), - key); - return NULL; - } - - ret[i] = g_strdup(tmp); } return g_steal_pointer(&ret); } + /** * virJSONValueObjectForeachKeyValue: * @object: JSON object to iterate diff --git a/src/util/virjson.h b/src/util/virjson.h index ce181dcb82..49a89e0910 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -172,6 +172,8 @@ virJSONValueObjectGetString(virJSONValue *object, char ** virJSONValueObjectGetStringArray(virJSONValue *object, const char *key); +char ** +virJSONValueArrayToStringList(virJSONValue *data); const char * virJSONValueObjectGetStringOrNumber(virJSONValue *object, const char *key); -- 2.38.1
The 'dependencies' field in the return data may be missing in some cases. Historically 'virJSONValueObjectGetStringArray' didn't report error in such case, but later refactor (commit 043b50b948ef3c2 ) added an error in order to use it in other places too. Unfortunately this results in the error log being spammed with an irrelevant error in case when qemuAgentGetDisks is invoked on a VM running windows. Replace the use of virJSONValueObjectGetStringArray by fetching the array first and calling virJSONValueArrayToStringList only when we have an array. Fixes: 043b50b948ef3c2a4adf5fa32a93ec2589851ac6 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2149752 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_agent.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index d420b2b901..70d45955b2 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2545,6 +2545,7 @@ int qemuAgentGetDisks(qemuAgent *agent, for (i = 0; i < ndata; i++) { virJSONValue *addr; virJSONValue *entry = virJSONValueArrayGet(data, i); + virJSONValue *dependencies; qemuAgentDiskInfo *disk; if (!entry) { @@ -2570,7 +2571,11 @@ int qemuAgentGetDisks(qemuAgent *agent, goto error; } - disk->dependencies = virJSONValueObjectGetStringArray(entry, "dependencies"); + if ((dependencies = virJSONValueObjectGetArray(entry, "dependencies"))) { + if (!(disk->dependencies = virJSONValueArrayToStringList(dependencies))) + goto error; + } + disk->alias = g_strdup(virJSONValueObjectGetString(entry, "alias")); addr = virJSONValueObjectGetObject(entry, "address"); if (addr) { -- 2.38.1
Rather than checking that the object has the correct key and then fetching it again use fetch the array first and then use virJSONValueArrayToStringList to directly convert it. Additionally we can avoid the conversion if there are no members simplifying the surrounding logic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a5800e182c..cbd1eb6eec 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4841,6 +4841,7 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitor *mon, virJSONValue *child = virJSONValueArrayGet(data, i); const char *tmp; qemuMonitorCPUDefInfo *cpu = defs->cpus + i; + virJSONValue *feat; if (!(tmp = virJSONValueObjectGetString(child, "name"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4853,16 +4854,14 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitor *mon, if ((tmp = virJSONValueObjectGetString(child, "typename")) && *tmp) cpu->type = g_strdup(tmp); - if (virJSONValueObjectHasKey(child, "unavailable-features")) { - if (!(cpu->blockers = virJSONValueObjectGetStringArray(child, - "unavailable-features"))) - return -1; + if ((feat = virJSONValueObjectGetArray(child, "unavailable-features"))) { + if (virJSONValueArraySize(feat) > 0) { + if (!(cpu->blockers = virJSONValueArrayToStringList(feat))) + return -1; - if (g_strv_length(cpu->blockers) == 0) { - cpu->usable = VIR_DOMCAPS_CPU_USABLE_YES; - g_clear_pointer(&cpu->blockers, g_strfreev); - } else { cpu->usable = VIR_DOMCAPS_CPU_USABLE_NO; + } else { + cpu->usable = VIR_DOMCAPS_CPU_USABLE_YES; } } -- 2.38.1
Using 'virJSONValueObjectHasKey' when we want to access the value afterwards is wasteful. Fetch the JSON value right away. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index cbd1eb6eec..1deb755130 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4842,6 +4842,7 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitor *mon, const char *tmp; qemuMonitorCPUDefInfo *cpu = defs->cpus + i; virJSONValue *feat; + virJSONValue *deprecated; if (!(tmp = virJSONValueObjectGetString(child, "name"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4865,8 +4866,8 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitor *mon, } } - if (virJSONValueObjectHasKey(child, "deprecated") && - virJSONValueObjectGetBoolean(child, "deprecated", &cpu->deprecated) < 0) + if ((deprecated = virJSONValueObjectGet(child, "deprecated")) && + virJSONValueGetBoolean(deprecated, &cpu->deprecated) < 0) return -1; } -- 2.38.1
In two instances (qemuMonitorJSONGetStringListProperty, qemuMonitorJSONGetStringArray) the return value is checked by qemuMonitorJSONCheckReply and extracted by virJSONValueObjectGetStringArray. We can use qemuMonitorJSONGetReply which returns it directly and then virJSONValueArrayToStringList to convert it without the additional lookup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 1deb755130..39f313c2af 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5522,6 +5522,7 @@ qemuMonitorJSONGetStringListProperty(qemuMonitor *mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValue *data; *strList = NULL; @@ -5534,10 +5535,10 @@ qemuMonitorJSONGetStringListProperty(qemuMonitor *mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) return -1; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - if (!(*strList = virJSONValueObjectGetStringArray(reply, "return"))) + if (!(*strList = virJSONValueArrayToStringList(data))) return -1; return 0; @@ -6297,6 +6298,7 @@ qemuMonitorJSONGetStringArray(qemuMonitor *mon, { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; + virJSONValue *data; *array = NULL; @@ -6309,10 +6311,10 @@ qemuMonitorJSONGetStringArray(qemuMonitor *mon, if (qemuMonitorJSONHasError(reply, "CommandNotFound")) return 0; - if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) + if (!(data = qemuMonitorJSONGetReply(cmd, reply, VIR_JSON_TYPE_ARRAY))) return -1; - if (!(*array = virJSONValueObjectGetStringArray(reply, "return"))) + if (!(*array = virJSONValueArrayToStringList(data))) return -1; return 0; -- 2.38.1
Use virJSONValueObjectGetArray + virJSONValueArrayToStringList instead so that the ofvirJSONValueObjectGetStringArray wrapper can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_agent.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 70d45955b2..fa2c0bf915 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2413,6 +2413,7 @@ qemuAgentSSHGetAuthorizedKeys(qemuAgent *agent, g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; virJSONValue *data = NULL; + virJSONValue *arr = NULL; if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys", "s:username", user, @@ -2422,13 +2423,14 @@ qemuAgentSSHGetAuthorizedKeys(qemuAgent *agent, if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 0) return -1; - if (!(data = virJSONValueObjectGetObject(reply, "return"))) { + if (!(data = virJSONValueObjectGetObject(reply, "return")) || + !(arr = virJSONValueObjectGetArray(data, "keys"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("qemu agent didn't return an array of keys")); return -1; } - if (!(*keys = virJSONValueObjectGetStringArray(data, "keys"))) + if (!(*keys = virJSONValueArrayToStringList(arr))) return -1; return g_strv_length(*keys); -- 2.38.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virjson.c | 17 ----------------- src/util/virjson.h | 3 --- 3 files changed, 21 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a5bb44a9f8..9389778db7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2616,7 +2616,6 @@ virJSONValueObjectGetNumberUint; virJSONValueObjectGetNumberUlong; virJSONValueObjectGetObject; virJSONValueObjectGetString; -virJSONValueObjectGetStringArray; virJSONValueObjectGetValue; virJSONValueObjectHasKey; virJSONValueObjectKeysNumber; diff --git a/src/util/virjson.c b/src/util/virjson.c index 5701c09bfd..1c3a50440e 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1313,23 +1313,6 @@ virJSONValueObjectStealObject(virJSONValue *object, } -char ** -virJSONValueObjectGetStringArray(virJSONValue *object, const char *key) -{ - virJSONValue *data; - - data = virJSONValueObjectGetArray(object, key); - if (!data) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s is missing or not an array"), - key); - return NULL; - } - - return virJSONValueArrayToStringList(data); -} - - /** * virJSONValueArrayToStringList: * @data: a JSON array containing strings to convert diff --git a/src/util/virjson.h b/src/util/virjson.h index 49a89e0910..0ea7caad23 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -170,9 +170,6 @@ const char * virJSONValueObjectGetString(virJSONValue *object, const char *key); char ** -virJSONValueObjectGetStringArray(virJSONValue *object, - const char *key); -char ** virJSONValueArrayToStringList(virJSONValue *data); const char * virJSONValueObjectGetStringOrNumber(virJSONValue *object, -- 2.38.1
On 12/1/22 17:31, Peter Krempa wrote:
First half introduces qemuMonitorJSONGetReply and uses it in our code base, second half introduces virJSONValueArrayToStringList, uses it to fix a bug and refactor the rest of the usage.
Peter Krempa (12): qemu: monitor: Introduce qemuMonitorJSONGetReply, a better qemuMonitorJSONCheckReply qemu: monitor: Use qemuMonitorJSONGetReply for VIR_JSON_TYPE_OBJECT qemu: monitor: Use qemuMonitorJSONGetReply for VIR_JSON_TYPE_ARRAY qemu: monitor: Use qemuMonitorJSONGetReply when the value is extracted directly qemu: monitor: Unify and refactor 'PTY' case in qemuMonitorJSONAttachCharDev util: json: Split out array->strinlist conversion from virJSONValueObjectGetStringArray qemuAgentGetDisks: Don't use virJSONValueObjectGetStringArray for optional data qemuMonitorJSONGetCPUDefinitions: Rework lookup of 'unavailable-features' qemuMonitorJSONGetCPUDefinitions: Avoid double lookup of object qemu: monitor: Use qemuMonitorJSONGetReply in conjunction with virJSONValueArrayToStringList qemuAgentSSHGetAuthorizedKeys: Convert last use ofvirJSONValueObjectGetStringArray util: json: Remove unused virJSONValueObjectGetStringArray wrapper
src/libvirt_private.syms | 2 +- src/qemu/qemu_agent.c | 13 ++- src/qemu/qemu_monitor_json.c | 189 +++++++++++++++-------------------- src/util/virjson.c | 44 +++----- src/util/virjson.h | 3 +- 5 files changed, 106 insertions(+), 145 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa