[libvirt] [PATCH] qemu: use guest-fsfreeze-freeze-list command if mountpoints to freeze specified
A command to freeze a part of mounted file systems is implemented in upstream QEMU-guest-agent with a name of 'guest-fsfreeze-freeze-list'. This fixes the name of the command used to partial fsfreeze in qemu driver when 'mountpoints' option is specified to virDomainFSFreeze API. Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- src/qemu/qemu_agent.c | 2 +- tests/qemuagenttest.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 0421733..a10954a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1336,7 +1336,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, if (!arg) return -1; - cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list", "a:mountpoints", arg, NULL); } else { cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL); diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index be207e8..bc649b4 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -45,7 +45,7 @@ testQemuAgentFSFreeze(const void *data) if (qemuMonitorTestAddAgentSyncResponse(test) < 0) goto cleanup; - if (qemuMonitorTestAddItem(test, "guest-fsfreeze-freeze", + if (qemuMonitorTestAddItem(test, "guest-fsfreeze-freeze-list", "{ \"return\" : 5 }") < 0) goto cleanup;
On 08.08.2014 22:03, Tomoki Sekiyama wrote:
A command to freeze a part of mounted file systems is implemented in upstream QEMU-guest-agent with a name of 'guest-fsfreeze-freeze-list'. This fixes the name of the command used to partial fsfreeze in qemu driver when 'mountpoints' option is specified to virDomainFSFreeze API.
Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- src/qemu/qemu_agent.c | 2 +- tests/qemuagenttest.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 0421733..a10954a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1336,7 +1336,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, if (!arg) return -1;
- cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list", "a:mountpoints", arg, NULL); } else { cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL); diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index be207e8..bc649b4 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -45,7 +45,7 @@ testQemuAgentFSFreeze(const void *data) if (qemuMonitorTestAddAgentSyncResponse(test) < 0) goto cleanup;
- if (qemuMonitorTestAddItem(test, "guest-fsfreeze-freeze", + if (qemuMonitorTestAddItem(test, "guest-fsfreeze-freeze-list", "{ \"return\" : 5 }") < 0) goto cleanup;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACKed & pushed. Michal
On 08/08/2014 02:03 PM, Tomoki Sekiyama wrote:
A command to freeze a part of mounted file systems is implemented in upstream QEMU-guest-agent with a name of 'guest-fsfreeze-freeze-list'. This fixes the name of the command used to partial fsfreeze in qemu driver when 'mountpoints' option is specified to virDomainFSFreeze API.
Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- src/qemu/qemu_agent.c | 2 +- tests/qemuagenttest.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Michal already pushed this, but I wish he had waited a bit longer. You are trying to use a qemu-guest-agent command that has just barely been added for qemu 2.2 (merge commit 2d591ce in qemu.git). We tend to avoid doing patches to libvirt without guarantees that upstream qemu has committed to the interface, and I want to avoid even the slight risk that the interface could change in qemu prior to the release candidate freeze for qemu 2.2 in a couple months.
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 0421733..a10954a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1336,7 +1336,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, if (!arg) return -1;
- cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list",
Worse, this patch causes a regression for guests using qemu-guest-agent 2.1 (which lacks 'guest-fsfreeze-freeze-list', but has 'guest-fsfreeze-freeze'), for the case where the user provided NULL for the set of mountpoints. The only proper way to fix this is to do conditional calls: if the user supplies NULL for mountpoints, unconditionally use the old API; if the user supplies non-NULL mountpoints, then use the new command. Then, depending on whether the error message that gets generated when qga 2.1 doesn't support the command is gross looking (basically, if the error message includes "internal error"), then you also need libvirt capability probing so that libvirt can detect up front whether qga is new enough, and give a nicer error message up front instead of relying on the guest agent to complain. I'd like to see a followup patch before libvirt 1.2.8 goes into freeze; if we don't get one in time, then I plan on reverting this patch so that we don't cause regressions to users of older guest agents. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 8/11/14 16:36 , "Eric Blake" <eblake@redhat.com> wrote:
On 08/08/2014 02:03 PM, Tomoki Sekiyama wrote:
A command to freeze a part of mounted file systems is implemented in upstream QEMU-guest-agent with a name of 'guest-fsfreeze-freeze-list'. This fixes the name of the command used to partial fsfreeze in qemu driver when 'mountpoints' option is specified to virDomainFSFreeze API.
Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> --- src/qemu/qemu_agent.c | 2 +- tests/qemuagenttest.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Michal already pushed this, but I wish he had waited a bit longer. You are trying to use a qemu-guest-agent command that has just barely been added for qemu 2.2 (merge commit 2d591ce in qemu.git). We tend to avoid doing patches to libvirt without guarantees that upstream qemu has committed to the interface, and I want to avoid even the slight risk that the interface could change in qemu prior to the release candidate freeze for qemu 2.2 in a couple months.
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 0421733..a10954a 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1336,7 +1336,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, if (!arg) return -1;
- cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list",
Worse, this patch causes a regression for guests using qemu-guest-agent 2.1 (which lacks 'guest-fsfreeze-freeze-list', but has 'guest-fsfreeze-freeze'), for the case where the user provided NULL for the set of mountpoints. The only proper way to fix this is to do conditional calls: if the user supplies NULL for mountpoints, unconditionally use the old API; if the user supplies non-NULL mountpoints, then use the new command. Then, depending on whether the
This is what my patch does -- if mountpoints == NULL, it always uses 'guest-fsfreeze-freeze' command, so old qga (<=2.1) still works with patched libvirt.
error message that gets generated when qga 2.1 doesn't support the command is gross looking (basically, if the error message includes "internal error"), then you also need libvirt capability probing so that libvirt can detect up front whether qga is new enough, and give a nicer error message up front instead of relying on the guest agent to complain.
OK, for nicer error messages, I will post a follow up patch soon. That would query 'guest-info', and if qga doesn't support 'guest-fsfreeze-freeze-list', gives users an error message that says something like "this version of qemu guest agent doesn't support specifying mountpoints to freeze". Does it sound correct?
I'd like to see a followup patch before libvirt 1.2.8 goes into freeze; if we don't get one in time, then I plan on reverting this patch so that we don't cause regressions to users of older guest agents.
Thanks, Tomoki Sekiyama
On 08/11/2014 03:25 PM, Tomoki Sekiyama wrote:
On 8/11/14 16:36 , "Eric Blake" <eblake@redhat.com> wrote:
On 08/08/2014 02:03 PM, Tomoki Sekiyama wrote:
A command to freeze a part of mounted file systems is implemented in upstream QEMU-guest-agent with a name of 'guest-fsfreeze-freeze-list'. This fixes the name of the command used to partial fsfreeze in qemu driver when 'mountpoints' option is specified to virDomainFSFreeze API.
Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com> ---
@@ -1336,7 +1336,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, if (!arg) return -1;
- cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list",
Worse, this patch causes a regression for guests using qemu-guest-agent 2.1 (which lacks 'guest-fsfreeze-freeze-list', but has 'guest-fsfreeze-freeze'), for the case where the user provided NULL for the set of mountpoints. The only proper way to fix this is to do conditional calls: if the user supplies NULL for mountpoints, unconditionally use the old API; if the user supplies non-NULL mountpoints, then use the new command. Then, depending on whether the
This is what my patch does -- if mountpoints == NULL, it always uses 'guest-fsfreeze-freeze' command, so old qga (<=2.1) still works with patched libvirt.
Oh, I missed the bigger context: if (mountpoints && nmountpoints) { arg = qemuAgentMakeStringsArray(mountpoints, nmountpoints); if (!arg) return -1; cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list", "a:mountpoints", arg, NULL); } else { cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL); } so indeed, the command is already conditional, and the difference is now that a client of qga 2.1 use to report an error that 'mountpoints' is an unrecognized parameter to 'guest-fsfreeze-freeze', and now reports an error that 'guest-fsfreeze-freeze-list' is an unknown command. You've convinced me there is no regression of a command that used to pass now failing, so that leaves only the error message sanity in question.
error message that gets generated when qga 2.1 doesn't support the command is gross looking (basically, if the error message includes "internal error"), then you also need libvirt capability probing so that libvirt can detect up front whether qga is new enough, and give a nicer error message up front instead of relying on the guest agent to complain.
OK, for nicer error messages, I will post a follow up patch soon.
Thanks.
That would query 'guest-info', and if qga doesn't support 'guest-fsfreeze-freeze-list', gives users an error message that says something like "this version of qemu guest agent doesn't support specifying mountpoints to freeze". Does it sound correct?
Yes, that's the idiom we've used in several other places for unsupported commands present only in newer versions (although I'm not sure if we've done it for the agent in particular, or just for the QMP monitor).
I'd like to see a followup patch before libvirt 1.2.8 goes into freeze; if we don't get one in time, then I plan on reverting this patch so that we don't cause regressions to users of older guest agents.
And given that the command has already been conditional, and already produces not-so-nice error messages, I won't revert this patch after all (anything you do further will be an improvement, and not a regression fix; reverts should be reserved only for regressions). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Michal Privoznik -
Tomoki Sekiyama