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

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(a)hds.com&gt; > ---

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.