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

[Tomoki Sekiyama]

Hi Michal,

On 8/14/14, 2:58 , "Michal Privoznik" <mprivozn at redhat.com> wrote:

>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>
>> ---
<snip>
>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.

I understood. This requires to distinguish version of qemu-ga older than
2.1 too, so using guest-info (outside the connect event) for capabilities
is impossible.

>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",

I¹ve tested this and confirmed that it works as expected.
I¹m okey with this patch instead of mine.

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

And now, even I think it is reasonable to rely on qemu-ga¹s error message,
until we have capability detection using the agent (dis-)connect events.

Thanks,
Tomoki Sekiyama