[libvirt] [PATCH v4 2/4] qemu: guestinfo: handle unsupported agent commands

[Michal Privoznik]

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 at 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), &params,
> -                                 &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 at redhat.com>

Michal