On 8/27/19 10:35 PM, Jonathon Jongsma wrote:
When we're collecting guest information, older agents may not support all agent commands. In the case where the user requested all info types (i.e. types == 0), ignore unsupported command errors and gather as much information as possible. If the agent command failed for some other reason, or if the user explciitly requested a specific info type (i.e. types != 0), abort on the first error.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/qemu/qemu_agent.c | 70 ++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_driver.c | 33 ++++++++++---------- tests/qemuagenttest.c | 2 +- 3 files changed, 82 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index f2a8bb6263..c63db968c6 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -995,6 +995,26 @@ qemuAgentStringifyErrorClass(const char *klass) return "unknown QEMU command error"; }
+/* Checks whether the agent reply msg is an error caused by an unsupported + * command. + * + * Returns true when reply is CommandNotFound or CommandDisabled + * false otherwise + */ +static bool +qemuAgentErrorCommandUnsupported(virJSONValuePtr reply) +{ + const char *klass; + virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); + + if (!error) + return false; + + klass = virJSONValueObjectGetString(error, "class"); + return STREQ_NULLABLE(klass, "CommandNotFound") || + STREQ_NULLABLE(klass, "CommandDisabled");
Huh, so whilst reviewing this I found out that qemu-ga will no longer send us CommandDisabled class, because in 1.2.0 qemu dropped that error class. I'm reintroducing it back here: https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg06327.html
+} + /* Ignoring OOM in this method, since we're already reporting * a more important error * @@ -1708,8 +1728,11 @@ qemuAgentGetHostname(qemuAgentPtr mon, return ret;
if (qemuAgentCommand(mon, cmd, &reply, true, - VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) + VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) { + if (qemuAgentErrorCommandUnsupported(reply)) + ret = -2; goto cleanup; + }
if (!(data = virJSONValueObjectGet(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2005,6 +2028,10 @@ qemuAgentGetFSInfoInternalDisk(virJSONValuePtr jsondisks, return 0; }
+/* returns 0 on success + * -2 when agent command is not supported by the agent + * -1 otherwise + */
We prefer: "Returns: ..." instead of "returns ..."
static int qemuAgentGetFSInfoInternal(qemuAgentPtr mon, qemuAgentFSInfoPtr **info,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d0e67863ea..908460bc79 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -23220,6 +23220,7 @@ qemuDomainGetGuestInfo(virDomainPtr dom, int maxparams = 0; VIR_AUTOFREE(char *) hostname = NULL; unsigned int supportedTypes = types; + int status;
I prefer 'rc' or 'rv' because variables with that name are used widely accross our code base to hold intermediate retvals from functions. The @status name doesn't fall into that category.
virCheckFlags(0, -1); qemuDomainGetGuestInfoCheckSupport(&supportedTypes); @@ -23240,30 +23241,30 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
agent = qemuDomainObjEnterAgent(vm);
- /* Although the libvirt qemu driver supports all of these guest info types, - * some guest agents might be too old to support these commands. If these - * info categories were explicitly requested (i.e. 'types' is non-zero), - * abort and report an error on any failures, otherwise continue and return - * as much info as is supported by the guest agent. */ + /* The agent info commands will return -2 for any commands that are not + * supported by the agent, or -1 for all other errors. In the case where no + * categories were explicitly requrested (i.e. 'types' is 0), ignore
s/requrested/requested/
+ * 'unsupported' errors and gather as much information as we can. In all + * other cases, abort on error. */ if (supportedTypes & VIR_DOMAIN_GUEST_INFO_USERS) { - if (qemuAgentGetUsers(agent, params, nparams, &maxparams) < 0 && - types != 0) + status = qemuAgentGetUsers(agent, params, nparams, &maxparams); + if (status == -1 || (status == -2 && types != 0))
Here I'd rather change this to cover just every negative value (so that if we invent a new one, these lines don't have to be changed). Something like: if (rc < 0 && !(rc == -2 && types == 0)) should do.
goto exitagent; } if (supportedTypes & VIR_DOMAIN_GUEST_INFO_OS) { - if (qemuAgentGetOSInfo(agent, params, nparams, &maxparams) < 0 - && types != 0) + status = qemuAgentGetOSInfo(agent, params, nparams, &maxparams); + if (status == -1 || (status == -2 && types != 0)) goto exitagent; } if (supportedTypes & VIR_DOMAIN_GUEST_INFO_TIMEZONE) { - if (qemuAgentGetTimezone(agent, params, nparams, &maxparams) < 0 - && types != 0) + status = qemuAgentGetTimezone(agent, params, nparams, &maxparams); + if (status == -1 || (status == -2 && types != 0)) goto exitagent; } if (supportedTypes & VIR_DOMAIN_GUEST_INFO_HOSTNAME) { - if (qemuAgentGetHostname(agent, &hostname) < 0) { - if (types != 0) - goto exitagent; + status = qemuAgentGetHostname(agent, &hostname); + if (status == -1 || (status == -2 && types != 0)) { + goto exitagent; } else { if (virTypedParamsAddString(params, nparams, &maxparams, "hostname", hostname) < 0) @@ -23271,8 +23272,8 @@ qemuDomainGetGuestInfo(virDomainPtr dom, } } if (supportedTypes & VIR_DOMAIN_GUEST_INFO_FILESYSTEM) { - if (qemuAgentGetFSInfoParams(agent, params, nparams, &maxparams, vm->def) < 0 && - types != 0) + status = qemuAgentGetFSInfoParams(agent, params, nparams, &maxparams, vm->def); + if (status == -1 || (status == -2 && types != 0)) goto exitagent; }
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index ae57da5989..91f19644d5 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -462,7 +462,7 @@ testQemuAgentGetFSInfoParams(const void *data) goto cleanup;
if (qemuAgentGetFSInfoParams(qemuMonitorTestGetAgent(test), ¶ms, - &nparams, &maxparams, def) != -1) { + &nparams, &maxparams, def) != -2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "agent get-fsinfo command should have failed"); goto cleanup;
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal