10:42 a.m.
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>
---
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",
1:58 a.m.
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>
---
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
5 p.m.
Hi Michal,
On 8/14/14, 2:58 , "Michal Privoznik" <mprivozn(a)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(a)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
3752
days inactive
3754
days old
2 comments
2 participants
participants (2)
-
Michal Privoznik -
Tomoki Sekiyama