On a Tuesday in 2020, Michal Privoznik wrote: I'm confused by 'exitagent' and 'exitagentjob' being above code that is only done (or only makes sense) on success. And ret being set to zero so early - I guess that's due to the nature of the best-effort information gathering here. But I think it would be perfectly fine to error out if we fail to get a query job or the domain dies in the meantime. Moving the exitagent and endagentjob labels after the cleanup block would remove the need to check ret. (i.e. duplicating ExitAgent and EndAgentJob calls - one pair that would be exectued on success and one pair only on failure)

The virJSONValueObjectGetStringArray() function is given a @key which is supposed to be an array inside given @object. Well, if it's not then an error state is returned (NULL), but no error message is set. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virjson.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index d471923732..160f6172d2 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1472,8 +1472,12 @@ virJSONValueObjectGetStringArray(virJSONValuePtr object, const char *key) size_t i; data = virJSONValueObjectGetArray(object, key); - if (!data) + if (!data) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s is missing not an array"), + key); return NULL; + } n = virJSONValueArraySize(data); ret = g_new0(char *, n + 1); -- 2.26.2

In a few commit back (v6.10.0-5-gb3dad96972) a new helper for obtaining string arrays from a virJSONObject was introduced: virJSONValueObjectGetStringArray(). I've identified three places where it can be used instead of open coding it: qemuAgentSSHGetAuthorizedKeys(), qemuMonitorJSONGetStringListProperty() and qemuMonitorJSONGetCPUDefinitions(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_agent.c | 30 +++---------------- src/qemu/qemu_monitor_json.c | 56 +++++------------------------------- 2 files changed, 11 insertions(+), 75 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 1796ade6e2..d4530dcd7a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2533,9 +2533,6 @@ qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent, g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; virJSONValuePtr data = NULL; - size_t ndata; - size_t i; - char **keys_ret = NULL; if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys", "s:username", user, @@ -2545,35 +2542,16 @@ qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent, if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 0) return -1; - if (!(data = virJSONValueObjectGetObject(reply, "return")) || - !(data = virJSONValueObjectGetArray(data, "keys"))) { + if (!(data = virJSONValueObjectGetObject(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("qemu agent didn't return an array of keys")); return -1; } - ndata = virJSONValueArraySize(data); + if (!(*keys = virJSONValueObjectGetStringArray(data, "keys"))) + return -1; - keys_ret = g_new0(char *, ndata + 1); - - for (i = 0; i < ndata; i++) { - virJSONValuePtr entry = virJSONValueArrayGet(data, i); - - if (!entry) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("array element missing in guest-ssh-get-authorized-keys return value")); - goto error; - } - - keys_ret[i] = g_strdup(virJSONValueGetString(entry)); - } - - *keys = g_steal_pointer(&keys_ret); - return ndata; - - error: - virStringListFreeCount(keys_ret, ndata); - return -1; + return g_strv_length(*keys); } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e7aa6b6ffd..ea65d6bee7 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5931,41 +5931,17 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, cpu->type = g_strdup(tmp); if (virJSONValueObjectHasKey(child, "unavailable-features")) { - virJSONValuePtr blockers; - size_t j; - size_t len; - - blockers = virJSONValueObjectGetArray(child, - "unavailable-features"); - if (!blockers) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unavailable-features in query-cpu-definitions " - "reply data was not an array")); + if (!(cpu->blockers = virJSONValueObjectGetStringArray(child, + "unavailable-features"))) return -1; - } - len = virJSONValueArraySize(blockers); - - if (len == 0) { + if (g_strv_length(cpu->blockers) == 0) { cpu->usable = VIR_DOMCAPS_CPU_USABLE_YES; + g_clear_pointer(&cpu->blockers, g_strfreev); continue; } cpu->usable = VIR_DOMCAPS_CPU_USABLE_NO; - cpu->blockers = g_new0(char *, len + 1); - - for (j = 0; j < len; j++) { - virJSONValuePtr blocker = virJSONValueArrayGet(blockers, j); - - if (virJSONValueGetType(blocker) != VIR_JSON_TYPE_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("unexpected value in unavailable-features " - "array")); - return -1; - } - - cpu->blockers[j] = g_strdup(virJSONValueGetString(blocker)); - } } } @@ -6788,9 +6764,6 @@ qemuMonitorJSONGetStringListProperty(qemuMonitorPtr mon, g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; VIR_AUTOSTRINGLIST list = NULL; - virJSONValuePtr data; - size_t n; - size_t i; *strList = NULL; @@ -6806,25 +6779,10 @@ qemuMonitorJSONGetStringListProperty(qemuMonitorPtr mon, if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0) return -1; - data = virJSONValueObjectGetArray(reply, "return"); - n = virJSONValueArraySize(data); + if (!(*strList = virJSONValueObjectGetStringArray(reply, "return"))) + return -1; - list = g_new0(char *, n + 1); - - for (i = 0; i < n; i++) { - virJSONValuePtr item = virJSONValueArrayGet(data, i); - - if (virJSONValueGetType(item) != VIR_JSON_TYPE_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected value in %s array"), property); - return -1; - } - - list[i] = g_strdup(virJSONValueGetString(item)); - } - - *strList = g_steal_pointer(&list); - return n; + return g_strv_length(*strList); } -- 2.26.2