[PATCH 0/3] qemu: Couple of qemuDomainGetGuestInfo() related cleanups
While reviewing Marc-André's patches I've noticed couple of cleanup possible. Michal Prívozník (3): qemuDomainGetGuestInfo: Exit early if getting info fails virJSONValueObjectGetStringArray: Report error if @key is not an array qemu: Use virJSONValueObjectGetStringArray() more src/qemu/qemu_agent.c | 30 +++---------------- src/qemu/qemu_driver.c | 3 ++ src/qemu/qemu_monitor_json.c | 56 +++++------------------------------- src/util/virjson.c | 6 +++- 4 files changed, 19 insertions(+), 76 deletions(-) -- 2.26.2
If there is an error getting info from guest agent, then the control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label and subsequently continues on 'endagentjob'. Both labels are hit also in success case, which is why there is a code that tries to match info obtained from the guest agent with domain definition. However, if we know that we've reached this area because of an error (ret = -1) there is no reason for us to attempt finding the match as the API as whole will end up with an error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2d4b5a8b99..338c609854 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20129,6 +20129,9 @@ qemuDomainGetGuestInfo(virDomainPtr dom, endagentjob: qemuDomainObjEndAgentJob(vm); + if (ret < 0) + goto cleanup; + if (nfs > 0 || ndisks > 0) { if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; -- 2.26.2
On a Tuesday in 2020, Michal Privoznik wrote:
If there is an error getting info from guest agent, then the control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label and subsequently continues on 'endagentjob'. Both labels are hit also in success case, which is why there is a code that tries to match info obtained from the guest agent with domain definition.
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)
However, if we know that we've reached this area because of an error (ret = -1) there is no reason for us to attempt finding the match as the API as whole will end up with an error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2d4b5a8b99..338c609854 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20129,6 +20129,9 @@ qemuDomainGetGuestInfo(virDomainPtr dom, endagentjob: qemuDomainObjEndAgentJob(vm);
+ if (ret < 0) + goto cleanup; + if (nfs > 0 || ndisks > 0) { if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; -- 2.26.2
On 12/1/20 4:39 PM, Ján Tomko wrote:
On a Tuesday in 2020, Michal Privoznik wrote:
If there is an error getting info from guest agent, then the control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label and subsequently continues on 'endagentjob'. Both labels are hit also in success case, which is why there is a code that tries to match info obtained from the guest agent with domain definition.
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)
Fair enough. Will post v2. Michal
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@redhat.com> --- 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
On a Tuesday in 2020, Michal Privoznik wrote:
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@redhat.com> --- 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"),
is missing or not an array Unless I'm missing something. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
+ 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@redhat.com> --- 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
On a Tuesday in 2020, Michal Privoznik wrote:
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@redhat.com> --- src/qemu/qemu_agent.c | 30 +++---------------- src/qemu/qemu_monitor_json.c | 56 +++++------------------------------- 2 files changed, 11 insertions(+), 75 deletions(-)
@@ -6788,9 +6764,6 @@ qemuMonitorJSONGetStringListProperty(qemuMonitorPtr mon, g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; VIR_AUTOSTRINGLIST list = NULL;
list is now unused: ../src/qemu/qemu_monitor_json.c:6766:24: error: unused variable 'list' [-Werror,-Wunused-variable] VIR_AUTOSTRINGLIST list = NULL; ^ 1 error generated.
- virJSONValuePtr data; - size_t n; - size_t i;
*strList = NULL;
with that fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik