Hi Michal, On 8/14/14, 2:58 , "Michal Privoznik" <mprivozn@redhat.com> 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@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
On 13.08.2014 17:42, Tomoki Sekiyama wrote: 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