[libvirt] [PATCH] qemuDomainFSFreeze: report unless the agent supports mountpoints param

[Michal Privoznik]

On 13.08.2014 17:42, Tomoki Sekiyama wrote:
> This patch gives users a nicer error message when the QEMU guest agent is
> not new enough to support 'guest-fsfreeze-freeze-list' command, which is
> used by qemuDomainFSFreeze() to freeze specified filesystems only.
> 
> Before this patch, it was depending on the agent to complain about unknown
> command:
>    # virsh domfsfreeze domain --mountpoint /mnt/point
>    error: Unable to freeze filesystems
>    error: internal error: unable to execute QEMU agent command 'guest-
>    fsfreeze-freeze-list': The command guest-fsfreeze-freeze-list has not been
>    found
> 
> After:
>    # virsh domfsfreeze domain --mountpoint /mnt/point
>    error: Unable to freeze filesystems
>    error: argument unsupported: this version of guest agent doesn't support
>    specifying mountpoints
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama at hds.com>
> ---
>   src/qemu/qemu_agent.c |   79 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index a10954a..8102b36 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1279,6 +1279,75 @@ void qemuAgentNotifyEvent(qemuAgentPtr mon,
>       }
>   }
>   
> +static int qemuAgentCommandSupported(qemuAgentPtr mon,
> +                                     const char *cmdname)
> +{
> +    int ret = -1;
> +    size_t i;
> +    int ndata;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr data;
> +    virJSONValuePtr reply = NULL;
> +
> +    cmd = qemuAgentMakeCommand("guest-info", NULL);
> +    if (!cmd)
> +        return -1;
> +
> +    if (qemuAgentCommand(mon, cmd, &reply, false,
> +                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +        goto cleanup;
> +
> +    if (!(data = virJSONValueObjectGet(reply, "return"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("guest-info reply was missing return data"));
> +        goto cleanup;
> +    }
> +
> +    if (!(data = virJSONValueObjectGet(data, "supported_commands"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("guest-info reply was missing supported_commands"));
> +        goto cleanup;
> +    }
> +
> +    if (data->type != VIR_JSON_TYPE_ARRAY) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("guest-info supported_commands was not an array"));
> +        goto cleanup;
> +    }
> +
> +    ndata = virJSONValueArraySize(data);
> +
> +    for (i = 0; i < ndata; i++) {
> +        virJSONValuePtr entry = virJSONValueArrayGet(data, i);
> +        const char *name;
> +
> +        if (!entry) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("array element missing in guest-info "
> +                             "supported_commands"));
> +            goto cleanup;
> +        }
> +
> +        if (!(name = virJSONValueObjectGetString(entry, "name"))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("guest-info supported_commands was missing name"));
> +            goto cleanup;
> +        }
> +
> +        if (strcmp(name, cmdname) == 0) {
> +            ret = 1;
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}
> +
>   VIR_ENUM_DECL(qemuAgentShutdownMode);
>   
>   VIR_ENUM_IMPL(qemuAgentShutdownMode,
> @@ -1346,8 +1415,16 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
>           return -1;
>   
>       if (qemuAgentCommand(mon, cmd, &reply, true,
> -                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
> +                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
> +        if (mountpoints && nmountpoints) {
> +            if (qemuAgentCommandSupported(mon,
> +                                          "guest-fsfreeze-freeze-list") == 0)
> +                virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                               _("this version of guest agent doesn't support "
> +                                 "specifying mountpoints"));
> +        }
>           goto cleanup;
> +    }
>   
>       if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> 

It's not that I'm against this patch in general. I'm just against it now :) I mean, with current implementation libvirt doesn't know what qemu-ga is it talking to. The guest agent can come and go, without libvirt noticing anything [1]. So in this specific case, guest-info can be executed by say newer qemu-ga which does support guest-fsfreeze-freeze-list and thus direct libvirt to use it. But then the freeze command is executed by downgraded qemu-ga so user will still see the ugly error message.

At qemu monitor level it makes sense to query supported commands and capabilities because monitor disappears if and only if the qemu process dies. The assumptions is however not true with qemu-ga.

1: The long reasoning above is not entirely true. There were attempts to improve this. Previously, qemu was (and from libvirt's POV still is) a middle man, so it's impossible from outside to tell if somebody's listening on the guest side of a virtio channel. But things have changed since then and qemu-2.1 should emit an monitor event whenever qemu-ga (dis-)connects. And libvirt could then react to connect event by executing guest-info. However doing it without events doesn't make much sense to me. Sorry.

What we can do is, something like this (untested, just idea):

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index a10954a..8bc04e5 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1346,8 +1346,19 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
         return -1;
 
     if (qemuAgentCommand(mon, cmd, &reply, true,
-                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+                         VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0) {
+        if (mountpoints && nmountpoints &&
+            virJSONValueObjectHasKey(reply, "error")) {
+            virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
+            const char *klass = virJSONValueObjectGetString(error, "class");
+            if (STREQ(klass, "CommandNotFound")) {
+                virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                               _("this version of guest agent doesn't support "
+                                 "specifying mountpoints"));
+            }
+        }
         goto cleanup;
+    }
 
     if (virJSONValueObjectGetNumberInt(reply, "return", &ret) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

Although I'm not sure it's worth putting so much effort in to just printing nicer error message.

Michal