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 <jjong...@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 <mpriv...@redhat.com>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
