* [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
@ 2011-12-13 18:28 Luiz Capitulino
2011-12-13 20:03 ` Michael Roth
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-13 18:28 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, aliguori, mdroth
It supports two modes: "hibernate" (which corresponds to S4) and
"sleep" (which corresponds to S3). It will try to execute the
pm-hibernate or pm-suspend scripts, if the scripts don't exist
the command will try to suspend by directly writing to the
"/sys/power/state" file.
An interesting implementation detail is how to cleanup the child's
status on termination, so that we don't create zombies. I've
choosen to ignore the SIGCHLD signal. This will cause the kernel to
automatically cleanup the child's status on its termination.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
I've tested this w/o any virtio driver, as they don't support S4 yet. For
S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
resume from it, but by checking the logs it seems to work fine.
changelog
---------
v2
o Rename the command to 'guest-suspend'
o Add 'mode' parameter
o Use pm-utils scripts
o Cleanup child termination status
qapi-schema-guest.json | 17 +++++++++++
qemu-ga.c | 11 +++++++-
qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 91 insertions(+), 1 deletions(-)
diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 29989fe..656bde9 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -219,3 +219,20 @@
##
{ 'command': 'guest-fsfreeze-thaw',
'returns': 'int' }
+
+##
+# @guest-suspend
+#
+# Suspend guest execution by entering ACPI power state S3 or S4.
+#
+# @mode: 'hibernate' RAM content is saved in the disk and the guest is
+# powered down (this corresponds to ACPI S4)
+# 'sleep' execution is suspended but the RAM retains its contents
+# (this corresponds to ACPI S3)
+#
+# Notes: This is an asynchronous request. There's no guarantee it will
+# succeed. Errors will be logged to guest's syslog.
+#
+# Since: 1.1
+##
+{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
diff --git a/qemu-ga.c b/qemu-ga.c
index 60d4972..b32e96c 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -61,7 +61,7 @@ static void quit_handler(int sig)
static void register_signal_handlers(void)
{
- struct sigaction sigact;
+ struct sigaction sigact, sigact_chld;
int ret;
memset(&sigact, 0, sizeof(struct sigaction));
@@ -76,6 +76,15 @@ static void register_signal_handlers(void)
if (ret == -1) {
g_error("error configuring signal handler: %s", strerror(errno));
}
+
+ /* This should cause the kernel to automatically cleanup child
+ termination status */
+ memset(&sigact_chld, 0, sizeof(struct sigaction));
+ sigact_chld.sa_handler = SIG_IGN;
+ ret = sigaction(SIGCHLD, &sigact_chld, NULL);
+ if (ret == -1) {
+ g_error("error configuring signal handler: %s", strerror(errno));
+ }
}
static void usage(const char *cmd)
diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
index a09c8ca..4799638 100644
--- a/qga/guest-agent-commands.c
+++ b/qga/guest-agent-commands.c
@@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
}
#endif
+#define LINUX_PM_UTILS_PATH "/usr/sbin"
+#define LINUX_SYS_STATE_FILE "/sys/power/state"
+
+void qmp_guest_suspend(const char *mode, Error **err)
+{
+ int ret, fd = -1;
+ const char *pmutils_bin;
+ char pmutils_bin_path[PATH_MAX];
+
+ if (strcmp(mode, "hibernate") == 0) {
+ pmutils_bin = "pm-hibernate";
+ } else if (strcmp(mode, "sleep") == 0) {
+ pmutils_bin = "pm-suspend";
+ } else {
+ error_set(err, QERR_INVALID_PARAMETER, "mode");
+ return;
+ }
+
+ snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
+ LINUX_PM_UTILS_PATH, pmutils_bin);
+
+ if (access(pmutils_bin_path, X_OK) != 0) {
+ pmutils_bin = NULL;
+ fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
+ if (fd < 0) {
+ error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
+ return;
+ }
+ }
+
+ ret = fork();
+ if (ret == 0) {
+ /* child */
+ setsid();
+ fclose(stdin);
+ fclose(stdout);
+ fclose(stderr);
+
+ if (pmutils_bin) {
+ ret = execl(pmutils_bin_path, pmutils_bin, NULL);
+ if (ret) {
+ slog("%s failed: %s", pmutils_bin_path, strerror(errno));
+ }
+ } else {
+ const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
+ ret = write(fd, cmd, strlen(cmd));
+ if (ret < 0) {
+ slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
+ strerror(errno));
+ }
+ close(fd);
+ }
+
+ exit(!!ret);
+ } else if (ret < 0) {
+ error_set(err, QERR_UNDEFINED_ERROR);
+ return;
+ }
+
+ if (!pmutils_bin) {
+ close(fd);
+ }
+}
+
/* register init/cleanup routines for stateful command groups */
void ga_command_state_init(GAState *s, GACommandState *cs)
{
--
1.7.8.167.g57526.dirty
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-13 18:28 [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command Luiz Capitulino
@ 2011-12-13 20:03 ` Michael Roth
2011-12-14 13:00 ` Luiz Capitulino
2011-12-13 20:27 ` Michael Roth
2011-12-13 23:13 ` Daniel P. Berrange
2 siblings, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-13 20:03 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
> It supports two modes: "hibernate" (which corresponds to S4) and
> "sleep" (which corresponds to S3). It will try to execute the
> pm-hibernate or pm-suspend scripts, if the scripts don't exist
> the command will try to suspend by directly writing to the
> "/sys/power/state" file.
>
> An interesting implementation detail is how to cleanup the child's
> status on termination, so that we don't create zombies. I've
> choosen to ignore the SIGCHLD signal. This will cause the kernel to
> automatically cleanup the child's status on its termination.
One downside to blocking SIGCHLD is it can screw with child processes
that utilize it. `sudo` for instance will just hang indefinitely because
it handles it's own cleanup via SIGCHLD, we might run into similar cases
with various pm-hibernate/pm-suspend implementations as well.
This will also screw with anything we launch via guest-exec as well,
once that goes in.
I wonder if we can tie the qemu_add_child_watch() stuff into qemu-ga's
main loop, or maybe just implement something similar...
Basically:
- add a qemu-ga.c:signal_channel_add() that creates a non-blocking
pipe and associate the read-side with a GIOChannel, then ties the
channel into the main loop via g_io_add_watch() on qemu-ga startup, with
an associated callback that reads everything off the pipe, then iterates
through a list of registered pids and does a non-blocking wait(pid, ...)
on each.
- add a SIGCHLD handler that writes a 1 to the write side of the pipe
whenever it gets called
Then, when creating a child, you just register the child by adding the
pid to the list that the signal channel callback checks.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>
> I've tested this w/o any virtio driver, as they don't support S4 yet. For
> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> resume from it, but by checking the logs it seems to work fine.
>
> changelog
> ---------
>
> v2
>
> o Rename the command to 'guest-suspend'
> o Add 'mode' parameter
> o Use pm-utils scripts
> o Cleanup child termination status
>
> qapi-schema-guest.json | 17 +++++++++++
> qemu-ga.c | 11 +++++++-
> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 91 insertions(+), 1 deletions(-)
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 29989fe..656bde9 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,20 @@
> ##
> { 'command': 'guest-fsfreeze-thaw',
> 'returns': 'int' }
> +
> +##
> +# @guest-suspend
> +#
> +# Suspend guest execution by entering ACPI power state S3 or S4.
> +#
> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> +# powered down (this corresponds to ACPI S4)
> +# 'sleep' execution is suspended but the RAM retains its contents
> +# (this corresponds to ACPI S3)
> +#
> +# Notes: This is an asynchronous request. There's no guarantee it will
> +# succeed. Errors will be logged to guest's syslog.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 60d4972..b32e96c 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
>
> static void register_signal_handlers(void)
> {
> - struct sigaction sigact;
> + struct sigaction sigact, sigact_chld;
> int ret;
>
> memset(&sigact, 0, sizeof(struct sigaction));
> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
> if (ret == -1) {
> g_error("error configuring signal handler: %s", strerror(errno));
> }
> +
> + /* This should cause the kernel to automatically cleanup child
> + termination status */
> + memset(&sigact_chld, 0, sizeof(struct sigaction));
> + sigact_chld.sa_handler = SIG_IGN;
> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
> + if (ret == -1) {
> + g_error("error configuring signal handler: %s", strerror(errno));
> + }
> }
>
> static void usage(const char *cmd)
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..4799638 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> }
> #endif
>
> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> +
> +void qmp_guest_suspend(const char *mode, Error **err)
> +{
> + int ret, fd = -1;
> + const char *pmutils_bin;
> + char pmutils_bin_path[PATH_MAX];
> +
> + if (strcmp(mode, "hibernate") == 0) {
> + pmutils_bin = "pm-hibernate";
> + } else if (strcmp(mode, "sleep") == 0) {
> + pmutils_bin = "pm-suspend";
> + } else {
> + error_set(err, QERR_INVALID_PARAMETER, "mode");
> + return;
> + }
> +
> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
> + LINUX_PM_UTILS_PATH, pmutils_bin);
> +
> + if (access(pmutils_bin_path, X_OK) != 0) {
> + pmutils_bin = NULL;
> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> + if (fd< 0) {
> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> + return;
> + }
> + }
> +
> + ret = fork();
> + if (ret == 0) {
> + /* child */
> + setsid();
> + fclose(stdin);
> + fclose(stdout);
> + fclose(stderr);
> +
> + if (pmutils_bin) {
> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
> + if (ret) {
> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
> + }
> + } else {
> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> + ret = write(fd, cmd, strlen(cmd));
> + if (ret< 0) {
> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> + strerror(errno));
> + }
> + close(fd);
> + }
> +
> + exit(!!ret);
> + } else if (ret< 0) {
> + error_set(err, QERR_UNDEFINED_ERROR);
> + return;
> + }
> +
> + if (!pmutils_bin) {
> + close(fd);
> + }
> +}
> +
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-13 18:28 [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command Luiz Capitulino
2011-12-13 20:03 ` Michael Roth
@ 2011-12-13 20:27 ` Michael Roth
2011-12-14 13:07 ` Luiz Capitulino
2011-12-13 23:13 ` Daniel P. Berrange
2 siblings, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-13 20:27 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
> It supports two modes: "hibernate" (which corresponds to S4) and
> "sleep" (which corresponds to S3). It will try to execute the
> pm-hibernate or pm-suspend scripts, if the scripts don't exist
> the command will try to suspend by directly writing to the
> "/sys/power/state" file.
>
> An interesting implementation detail is how to cleanup the child's
> status on termination, so that we don't create zombies. I've
> choosen to ignore the SIGCHLD signal. This will cause the kernel to
> automatically cleanup the child's status on its termination.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>
> I've tested this w/o any virtio driver, as they don't support S4 yet. For
> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> resume from it, but by checking the logs it seems to work fine.
>
> changelog
> ---------
>
> v2
>
> o Rename the command to 'guest-suspend'
> o Add 'mode' parameter
> o Use pm-utils scripts
> o Cleanup child termination status
>
> qapi-schema-guest.json | 17 +++++++++++
> qemu-ga.c | 11 +++++++-
> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 91 insertions(+), 1 deletions(-)
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 29989fe..656bde9 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,20 @@
> ##
> { 'command': 'guest-fsfreeze-thaw',
> 'returns': 'int' }
> +
> +##
> +# @guest-suspend
> +#
> +# Suspend guest execution by entering ACPI power state S3 or S4.
> +#
> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> +# powered down (this corresponds to ACPI S4)
> +# 'sleep' execution is suspended but the RAM retains its contents
> +# (this corresponds to ACPI S3)
'suspend' is generally associated with sleep, so maybe we should make
the mode optional and default to 'sleep'?
> +#
> +# Notes: This is an asynchronous request. There's no guarantee it will
> +# succeed. Errors will be logged to guest's syslog.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 60d4972..b32e96c 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
>
> static void register_signal_handlers(void)
> {
> - struct sigaction sigact;
> + struct sigaction sigact, sigact_chld;
> int ret;
>
> memset(&sigact, 0, sizeof(struct sigaction));
> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
> if (ret == -1) {
> g_error("error configuring signal handler: %s", strerror(errno));
> }
> +
> + /* This should cause the kernel to automatically cleanup child
> + termination status */
> + memset(&sigact_chld, 0, sizeof(struct sigaction));
> + sigact_chld.sa_handler = SIG_IGN;
> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
> + if (ret == -1) {
> + g_error("error configuring signal handler: %s", strerror(errno));
> + }
> }
>
> static void usage(const char *cmd)
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..4799638 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> }
> #endif
>
> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> +
> +void qmp_guest_suspend(const char *mode, Error **err)
> +{
> + int ret, fd = -1;
> + const char *pmutils_bin;
> + char pmutils_bin_path[PATH_MAX];
> +
> + if (strcmp(mode, "hibernate") == 0) {
> + pmutils_bin = "pm-hibernate";
> + } else if (strcmp(mode, "sleep") == 0) {
> + pmutils_bin = "pm-suspend";
> + } else {
> + error_set(err, QERR_INVALID_PARAMETER, "mode");
> + return;
> + }
> +
> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
> + LINUX_PM_UTILS_PATH, pmutils_bin);
I'd be surprised to find any distros where this isn't the case, but for
situations where the scripts aren't in /usr/sbin, maybe we'd be better
off just passing the command name to execlp() and letting it do the
search via PATH? The normal use case is that qemu-ga will get launched
as a service, so PATH should have the good stuff.
> +
> + if (access(pmutils_bin_path, X_OK) != 0) {
> + pmutils_bin = NULL;
> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> + if (fd< 0) {
> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> + return;
> + }
> + }
> +
> + ret = fork();
> + if (ret == 0) {
> + /* child */
> + setsid();
> + fclose(stdin);
> + fclose(stdout);
> + fclose(stderr);
> +
> + if (pmutils_bin) {
> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
> + if (ret) {
> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
> + }
> + } else {
> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> + ret = write(fd, cmd, strlen(cmd));
> + if (ret< 0) {
> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> + strerror(errno));
> + }
> + close(fd);
> + }
> +
> + exit(!!ret);
> + } else if (ret< 0) {
> + error_set(err, QERR_UNDEFINED_ERROR);
> + return;
> + }
> +
> + if (!pmutils_bin) {
> + close(fd);
> + }
> +}
> +
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-13 18:28 [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command Luiz Capitulino
2011-12-13 20:03 ` Michael Roth
2011-12-13 20:27 ` Michael Roth
@ 2011-12-13 23:13 ` Daniel P. Berrange
2011-12-14 13:08 ` Luiz Capitulino
2 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrange @ 2011-12-13 23:13 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel, mdroth
On Tue, Dec 13, 2011 at 04:28:50PM -0200, Luiz Capitulino wrote:
> It supports two modes: "hibernate" (which corresponds to S4) and
> "sleep" (which corresponds to S3). It will try to execute the
> pm-hibernate or pm-suspend scripts, if the scripts don't exist
> the command will try to suspend by directly writing to the
> "/sys/power/state" file.
>
> An interesting implementation detail is how to cleanup the child's
> status on termination, so that we don't create zombies. I've
> choosen to ignore the SIGCHLD signal. This will cause the kernel to
> automatically cleanup the child's status on its termination.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>
> I've tested this w/o any virtio driver, as they don't support S4 yet. For
> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> resume from it, but by checking the logs it seems to work fine.
>
> changelog
> ---------
>
> v2
>
> o Rename the command to 'guest-suspend'
> o Add 'mode' parameter
> o Use pm-utils scripts
> o Cleanup child termination status
>
> qapi-schema-guest.json | 17 +++++++++++
> qemu-ga.c | 11 +++++++-
> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 91 insertions(+), 1 deletions(-)
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 29989fe..656bde9 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,20 @@
> ##
> { 'command': 'guest-fsfreeze-thaw',
> 'returns': 'int' }
> +
> +##
> +# @guest-suspend
> +#
> +# Suspend guest execution by entering ACPI power state S3 or S4.
> +#
> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> +# powered down (this corresponds to ACPI S4)
> +# 'sleep' execution is suspended but the RAM retains its contents
> +# (this corresponds to ACPI S3)
Standard pm-utils in Linux supports three ways to suspend these
days, suspend, hibernate & suspend-hybrid. libvirt supports
all 3 modes in our recently added API for suspending the physical
host, so I think we'll want to also have all 3 for suspending
guests too.
[quote pm-suspend(8)]
pm-suspend
During suspend most devices are shutdown, and
system state is saved in RAM. The system still
requires power in this state. Most modern systems
require 3 to 5 seconds to enter and leave suspend,
and most laptops can stay in suspend mode for 1 to
3 days before exhausting their battery.
pm-hibernate
During hibernate the system is fully powered off,
and system state is saved to disk. The system does
not require power, and can stay in hibernate mode
indefinitely. Most modern systems require 15 to 45
seconds to enter and leave hibernate, and entering
and leaving hibernate takes longer when you have
more memory.
pm-suspend-hybrid
Hybrid-suspend is the process where the system does
everything it needs to hibernate, but suspends
instead of shutting down. This means that your
computer can wake up quicker than for normal
hibernation if you do not run out of power, and you
can resume even if you run out of power. s2both(8)
is an hybrid-suspend implementation.
[/quote]
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..4799638 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> }
> #endif
>
> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> +
> +void qmp_guest_suspend(const char *mode, Error **err)
> +{
> + int ret, fd = -1;
> + const char *pmutils_bin;
> + char pmutils_bin_path[PATH_MAX];
> +
> + if (strcmp(mode, "hibernate") == 0) {
> + pmutils_bin = "pm-hibernate";
> + } else if (strcmp(mode, "sleep") == 0) {
> + pmutils_bin = "pm-suspend";
Here you'd just add pm-suspend-hybrid too
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-13 20:03 ` Michael Roth
@ 2011-12-14 13:00 ` Luiz Capitulino
2011-12-14 15:54 ` Michael Roth
0 siblings, 1 reply; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 13:00 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Tue, 13 Dec 2011 14:03:08 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
> > It supports two modes: "hibernate" (which corresponds to S4) and
> > "sleep" (which corresponds to S3). It will try to execute the
> > pm-hibernate or pm-suspend scripts, if the scripts don't exist
> > the command will try to suspend by directly writing to the
> > "/sys/power/state" file.
> >
> > An interesting implementation detail is how to cleanup the child's
> > status on termination, so that we don't create zombies. I've
> > choosen to ignore the SIGCHLD signal. This will cause the kernel to
> > automatically cleanup the child's status on its termination.
>
> One downside to blocking SIGCHLD is it can screw with child processes
> that utilize it. `sudo` for instance will just hang indefinitely because
> it handles it's own cleanup via SIGCHLD, we might run into similar cases
> with various pm-hibernate/pm-suspend implementations as well.
>
> This will also screw with anything we launch via guest-exec as well,
> once that goes in.
>
> I wonder if we can tie the qemu_add_child_watch() stuff into qemu-ga's
> main loop, or maybe just implement something similar...
>
> Basically:
>
> - add a qemu-ga.c:signal_channel_add() that creates a non-blocking
> pipe and associate the read-side with a GIOChannel, then ties the
> channel into the main loop via g_io_add_watch() on qemu-ga startup, with
> an associated callback that reads everything off the pipe, then iterates
> through a list of registered pids and does a non-blocking wait(pid, ...)
> on each.
>
> - add a SIGCHLD handler that writes a 1 to the write side of the pipe
> whenever it gets called
Is the pipe really needed? Is there any side effect if we call waitpid()
from the signal handler considering it won't block?
I'm also wondering if we could use g_child_watch_add(), but it's not clear
to me if it works with processes not created with g_spawn_*() functions.
>
> Then, when creating a child, you just register the child by adding the
> pid to the list that the signal channel callback checks.
>
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> >
> > I've tested this w/o any virtio driver, as they don't support S4 yet. For
> > S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> > resume from it, but by checking the logs it seems to work fine.
> >
> > changelog
> > ---------
> >
> > v2
> >
> > o Rename the command to 'guest-suspend'
> > o Add 'mode' parameter
> > o Use pm-utils scripts
> > o Cleanup child termination status
> >
> > qapi-schema-guest.json | 17 +++++++++++
> > qemu-ga.c | 11 +++++++-
> > qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 91 insertions(+), 1 deletions(-)
> >
> > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> > index 29989fe..656bde9 100644
> > --- a/qapi-schema-guest.json
> > +++ b/qapi-schema-guest.json
> > @@ -219,3 +219,20 @@
> > ##
> > { 'command': 'guest-fsfreeze-thaw',
> > 'returns': 'int' }
> > +
> > +##
> > +# @guest-suspend
> > +#
> > +# Suspend guest execution by entering ACPI power state S3 or S4.
> > +#
> > +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> > +# powered down (this corresponds to ACPI S4)
> > +# 'sleep' execution is suspended but the RAM retains its contents
> > +# (this corresponds to ACPI S3)
> > +#
> > +# Notes: This is an asynchronous request. There's no guarantee it will
> > +# succeed. Errors will be logged to guest's syslog.
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> > diff --git a/qemu-ga.c b/qemu-ga.c
> > index 60d4972..b32e96c 100644
> > --- a/qemu-ga.c
> > +++ b/qemu-ga.c
> > @@ -61,7 +61,7 @@ static void quit_handler(int sig)
> >
> > static void register_signal_handlers(void)
> > {
> > - struct sigaction sigact;
> > + struct sigaction sigact, sigact_chld;
> > int ret;
> >
> > memset(&sigact, 0, sizeof(struct sigaction));
> > @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
> > if (ret == -1) {
> > g_error("error configuring signal handler: %s", strerror(errno));
> > }
> > +
> > + /* This should cause the kernel to automatically cleanup child
> > + termination status */
> > + memset(&sigact_chld, 0, sizeof(struct sigaction));
> > + sigact_chld.sa_handler = SIG_IGN;
> > + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
> > + if (ret == -1) {
> > + g_error("error configuring signal handler: %s", strerror(errno));
> > + }
> > }
> >
> > static void usage(const char *cmd)
> > diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> > index a09c8ca..4799638 100644
> > --- a/qga/guest-agent-commands.c
> > +++ b/qga/guest-agent-commands.c
> > @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> > }
> > #endif
> >
> > +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> > + int ret, fd = -1;
> > + const char *pmutils_bin;
> > + char pmutils_bin_path[PATH_MAX];
> > +
> > + if (strcmp(mode, "hibernate") == 0) {
> > + pmutils_bin = "pm-hibernate";
> > + } else if (strcmp(mode, "sleep") == 0) {
> > + pmutils_bin = "pm-suspend";
> > + } else {
> > + error_set(err, QERR_INVALID_PARAMETER, "mode");
> > + return;
> > + }
> > +
> > + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
> > + LINUX_PM_UTILS_PATH, pmutils_bin);
> > +
> > + if (access(pmutils_bin_path, X_OK) != 0) {
> > + pmutils_bin = NULL;
> > + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> > + if (fd< 0) {
> > + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> > + return;
> > + }
> > + }
> > +
> > + ret = fork();
> > + if (ret == 0) {
> > + /* child */
> > + setsid();
> > + fclose(stdin);
> > + fclose(stdout);
> > + fclose(stderr);
> > +
> > + if (pmutils_bin) {
> > + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
> > + if (ret) {
> > + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
> > + }
> > + } else {
> > + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> > + ret = write(fd, cmd, strlen(cmd));
> > + if (ret< 0) {
> > + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> > + strerror(errno));
> > + }
> > + close(fd);
> > + }
> > +
> > + exit(!!ret);
> > + } else if (ret< 0) {
> > + error_set(err, QERR_UNDEFINED_ERROR);
> > + return;
> > + }
> > +
> > + if (!pmutils_bin) {
> > + close(fd);
> > + }
> > +}
> > +
> > /* register init/cleanup routines for stateful command groups */
> > void ga_command_state_init(GAState *s, GACommandState *cs)
> > {
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-13 20:27 ` Michael Roth
@ 2011-12-14 13:07 ` Luiz Capitulino
2011-12-14 15:50 ` Michael Roth
0 siblings, 1 reply; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 13:07 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Tue, 13 Dec 2011 14:27:56 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
> > It supports two modes: "hibernate" (which corresponds to S4) and
> > "sleep" (which corresponds to S3). It will try to execute the
> > pm-hibernate or pm-suspend scripts, if the scripts don't exist
> > the command will try to suspend by directly writing to the
> > "/sys/power/state" file.
> >
> > An interesting implementation detail is how to cleanup the child's
> > status on termination, so that we don't create zombies. I've
> > choosen to ignore the SIGCHLD signal. This will cause the kernel to
> > automatically cleanup the child's status on its termination.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> >
> > I've tested this w/o any virtio driver, as they don't support S4 yet. For
> > S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> > resume from it, but by checking the logs it seems to work fine.
> >
> > changelog
> > ---------
> >
> > v2
> >
> > o Rename the command to 'guest-suspend'
> > o Add 'mode' parameter
> > o Use pm-utils scripts
> > o Cleanup child termination status
> >
> > qapi-schema-guest.json | 17 +++++++++++
> > qemu-ga.c | 11 +++++++-
> > qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 91 insertions(+), 1 deletions(-)
> >
> > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> > index 29989fe..656bde9 100644
> > --- a/qapi-schema-guest.json
> > +++ b/qapi-schema-guest.json
> > @@ -219,3 +219,20 @@
> > ##
> > { 'command': 'guest-fsfreeze-thaw',
> > 'returns': 'int' }
> > +
> > +##
> > +# @guest-suspend
> > +#
> > +# Suspend guest execution by entering ACPI power state S3 or S4.
> > +#
> > +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> > +# powered down (this corresponds to ACPI S4)
> > +# 'sleep' execution is suspended but the RAM retains its contents
> > +# (this corresponds to ACPI S3)
>
> 'suspend' is generally associated with sleep, so maybe we should make
> the mode optional and default to 'sleep'?
I'm not sure I like having defaults because we're choosing for the client.
If this were a human interface then it would make sense, but for a machine
one I don't think it does. Besides we don't seem to have a way to resume
from S3 so we're probably going to get reports about this command not working
(and that reminds me to add a note about that in the doc).
>
> > +#
> > +# Notes: This is an asynchronous request. There's no guarantee it will
> > +# succeed. Errors will be logged to guest's syslog.
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> > diff --git a/qemu-ga.c b/qemu-ga.c
> > index 60d4972..b32e96c 100644
> > --- a/qemu-ga.c
> > +++ b/qemu-ga.c
> > @@ -61,7 +61,7 @@ static void quit_handler(int sig)
> >
> > static void register_signal_handlers(void)
> > {
> > - struct sigaction sigact;
> > + struct sigaction sigact, sigact_chld;
> > int ret;
> >
> > memset(&sigact, 0, sizeof(struct sigaction));
> > @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
> > if (ret == -1) {
> > g_error("error configuring signal handler: %s", strerror(errno));
> > }
> > +
> > + /* This should cause the kernel to automatically cleanup child
> > + termination status */
> > + memset(&sigact_chld, 0, sizeof(struct sigaction));
> > + sigact_chld.sa_handler = SIG_IGN;
> > + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
> > + if (ret == -1) {
> > + g_error("error configuring signal handler: %s", strerror(errno));
> > + }
> > }
> >
> > static void usage(const char *cmd)
> > diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> > index a09c8ca..4799638 100644
> > --- a/qga/guest-agent-commands.c
> > +++ b/qga/guest-agent-commands.c
> > @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> > }
> > #endif
> >
> > +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> > + int ret, fd = -1;
> > + const char *pmutils_bin;
> > + char pmutils_bin_path[PATH_MAX];
> > +
> > + if (strcmp(mode, "hibernate") == 0) {
> > + pmutils_bin = "pm-hibernate";
> > + } else if (strcmp(mode, "sleep") == 0) {
> > + pmutils_bin = "pm-suspend";
> > + } else {
> > + error_set(err, QERR_INVALID_PARAMETER, "mode");
> > + return;
> > + }
> > +
> > + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
> > + LINUX_PM_UTILS_PATH, pmutils_bin);
>
> I'd be surprised to find any distros where this isn't the case, but for
> situations where the scripts aren't in /usr/sbin, maybe we'd be better
> off just passing the command name to execlp() and letting it do the
> search via PATH? The normal use case is that qemu-ga will get launched
> as a service, so PATH should have the good stuff.
Ok, I'll make that change.
>
> > +
> > + if (access(pmutils_bin_path, X_OK) != 0) {
> > + pmutils_bin = NULL;
> > + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> > + if (fd< 0) {
> > + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> > + return;
> > + }
> > + }
> > +
> > + ret = fork();
> > + if (ret == 0) {
> > + /* child */
> > + setsid();
> > + fclose(stdin);
> > + fclose(stdout);
> > + fclose(stderr);
> > +
> > + if (pmutils_bin) {
> > + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
> > + if (ret) {
> > + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
> > + }
> > + } else {
> > + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> > + ret = write(fd, cmd, strlen(cmd));
> > + if (ret< 0) {
> > + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> > + strerror(errno));
> > + }
> > + close(fd);
> > + }
> > +
> > + exit(!!ret);
> > + } else if (ret< 0) {
> > + error_set(err, QERR_UNDEFINED_ERROR);
> > + return;
> > + }
> > +
> > + if (!pmutils_bin) {
> > + close(fd);
> > + }
> > +}
> > +
> > /* register init/cleanup routines for stateful command groups */
> > void ga_command_state_init(GAState *s, GACommandState *cs)
> > {
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-13 23:13 ` Daniel P. Berrange
@ 2011-12-14 13:08 ` Luiz Capitulino
0 siblings, 0 replies; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 13:08 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Amit Shah, aliguori, qemu-devel, mdroth
On Tue, 13 Dec 2011 23:13:23 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 04:28:50PM -0200, Luiz Capitulino wrote:
> > It supports two modes: "hibernate" (which corresponds to S4) and
> > "sleep" (which corresponds to S3). It will try to execute the
> > pm-hibernate or pm-suspend scripts, if the scripts don't exist
> > the command will try to suspend by directly writing to the
> > "/sys/power/state" file.
> >
> > An interesting implementation detail is how to cleanup the child's
> > status on termination, so that we don't create zombies. I've
> > choosen to ignore the SIGCHLD signal. This will cause the kernel to
> > automatically cleanup the child's status on its termination.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >
> > I've tested this w/o any virtio driver, as they don't support S4 yet. For
> > S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> > resume from it, but by checking the logs it seems to work fine.
> >
> > changelog
> > ---------
> >
> > v2
> >
> > o Rename the command to 'guest-suspend'
> > o Add 'mode' parameter
> > o Use pm-utils scripts
> > o Cleanup child termination status
> >
> > qapi-schema-guest.json | 17 +++++++++++
> > qemu-ga.c | 11 +++++++-
> > qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 91 insertions(+), 1 deletions(-)
> >
> > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> > index 29989fe..656bde9 100644
> > --- a/qapi-schema-guest.json
> > +++ b/qapi-schema-guest.json
> > @@ -219,3 +219,20 @@
> > ##
> > { 'command': 'guest-fsfreeze-thaw',
> > 'returns': 'int' }
> > +
> > +##
> > +# @guest-suspend
> > +#
> > +# Suspend guest execution by entering ACPI power state S3 or S4.
> > +#
> > +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> > +# powered down (this corresponds to ACPI S4)
> > +# 'sleep' execution is suspended but the RAM retains its contents
> > +# (this corresponds to ACPI S3)
>
> Standard pm-utils in Linux supports three ways to suspend these
> days, suspend, hibernate & suspend-hybrid. libvirt supports
> all 3 modes in our recently added API for suspending the physical
> host, so I think we'll want to also have all 3 for suspending
> guests too.
Ok.
>
> [quote pm-suspend(8)]
> pm-suspend
> During suspend most devices are shutdown, and
> system state is saved in RAM. The system still
> requires power in this state. Most modern systems
> require 3 to 5 seconds to enter and leave suspend,
> and most laptops can stay in suspend mode for 1 to
> 3 days before exhausting their battery.
>
> pm-hibernate
> During hibernate the system is fully powered off,
> and system state is saved to disk. The system does
> not require power, and can stay in hibernate mode
> indefinitely. Most modern systems require 15 to 45
> seconds to enter and leave hibernate, and entering
> and leaving hibernate takes longer when you have
> more memory.
>
> pm-suspend-hybrid
> Hybrid-suspend is the process where the system does
> everything it needs to hibernate, but suspends
> instead of shutting down. This means that your
> computer can wake up quicker than for normal
> hibernation if you do not run out of power, and you
> can resume even if you run out of power. s2both(8)
> is an hybrid-suspend implementation.
> [/quote]
>
>
> > diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> > index a09c8ca..4799638 100644
> > --- a/qga/guest-agent-commands.c
> > +++ b/qga/guest-agent-commands.c
> > @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> > }
> > #endif
> >
> > +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> > + int ret, fd = -1;
> > + const char *pmutils_bin;
> > + char pmutils_bin_path[PATH_MAX];
> > +
> > + if (strcmp(mode, "hibernate") == 0) {
> > + pmutils_bin = "pm-hibernate";
> > + } else if (strcmp(mode, "sleep") == 0) {
> > + pmutils_bin = "pm-suspend";
>
> Here you'd just add pm-suspend-hybrid too
>
> Regards,
> Daniel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 13:07 ` Luiz Capitulino
@ 2011-12-14 15:50 ` Michael Roth
0 siblings, 0 replies; 19+ messages in thread
From: Michael Roth @ 2011-12-14 15:50 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 07:07 AM, Luiz Capitulino wrote:
> On Tue, 13 Dec 2011 14:27:56 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>
>> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
>>> It supports two modes: "hibernate" (which corresponds to S4) and
>>> "sleep" (which corresponds to S3). It will try to execute the
>>> pm-hibernate or pm-suspend scripts, if the scripts don't exist
>>> the command will try to suspend by directly writing to the
>>> "/sys/power/state" file.
>>>
>>> An interesting implementation detail is how to cleanup the child's
>>> status on termination, so that we don't create zombies. I've
>>> choosen to ignore the SIGCHLD signal. This will cause the kernel to
>>> automatically cleanup the child's status on its termination.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>>
>>> I've tested this w/o any virtio driver, as they don't support S4 yet. For
>>> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
>>> resume from it, but by checking the logs it seems to work fine.
>>>
>>> changelog
>>> ---------
>>>
>>> v2
>>>
>>> o Rename the command to 'guest-suspend'
>>> o Add 'mode' parameter
>>> o Use pm-utils scripts
>>> o Cleanup child termination status
>>>
>>> qapi-schema-guest.json | 17 +++++++++++
>>> qemu-ga.c | 11 +++++++-
>>> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 91 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>> index 29989fe..656bde9 100644
>>> --- a/qapi-schema-guest.json
>>> +++ b/qapi-schema-guest.json
>>> @@ -219,3 +219,20 @@
>>> ##
>>> { 'command': 'guest-fsfreeze-thaw',
>>> 'returns': 'int' }
>>> +
>>> +##
>>> +# @guest-suspend
>>> +#
>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>> +#
>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>> +# powered down (this corresponds to ACPI S4)
>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>> +# (this corresponds to ACPI S3)
>>
>> 'suspend' is generally associated with sleep, so maybe we should make
>> the mode optional and default to 'sleep'?
>
> I'm not sure I like having defaults because we're choosing for the client.
> If this were a human interface then it would make sense, but for a machine
> one I don't think it does. Besides we don't seem to have a way to resume
> from S3 so we're probably going to get reports about this command not working
> (and that reminds me to add a note about that in the doc).
True, defaulting to broken behavior probably isn't a good idea :)
>
>>
>>> +#
>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>> +# succeed. Errors will be logged to guest's syslog.
>>> +#
>>> +# Since: 1.1
>>> +##
>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>> index 60d4972..b32e96c 100644
>>> --- a/qemu-ga.c
>>> +++ b/qemu-ga.c
>>> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
>>>
>>> static void register_signal_handlers(void)
>>> {
>>> - struct sigaction sigact;
>>> + struct sigaction sigact, sigact_chld;
>>> int ret;
>>>
>>> memset(&sigact, 0, sizeof(struct sigaction));
>>> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
>>> if (ret == -1) {
>>> g_error("error configuring signal handler: %s", strerror(errno));
>>> }
>>> +
>>> + /* This should cause the kernel to automatically cleanup child
>>> + termination status */
>>> + memset(&sigact_chld, 0, sizeof(struct sigaction));
>>> + sigact_chld.sa_handler = SIG_IGN;
>>> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
>>> + if (ret == -1) {
>>> + g_error("error configuring signal handler: %s", strerror(errno));
>>> + }
>>> }
>>>
>>> static void usage(const char *cmd)
>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>> index a09c8ca..4799638 100644
>>> --- a/qga/guest-agent-commands.c
>>> +++ b/qga/guest-agent-commands.c
>>> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>> }
>>> #endif
>>>
>>> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>> +
>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>> +{
>>> + int ret, fd = -1;
>>> + const char *pmutils_bin;
>>> + char pmutils_bin_path[PATH_MAX];
>>> +
>>> + if (strcmp(mode, "hibernate") == 0) {
>>> + pmutils_bin = "pm-hibernate";
>>> + } else if (strcmp(mode, "sleep") == 0) {
>>> + pmutils_bin = "pm-suspend";
>>> + } else {
>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>> + return;
>>> + }
>>> +
>>> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
>>> + LINUX_PM_UTILS_PATH, pmutils_bin);
>>
>> I'd be surprised to find any distros where this isn't the case, but for
>> situations where the scripts aren't in /usr/sbin, maybe we'd be better
>> off just passing the command name to execlp() and letting it do the
>> search via PATH? The normal use case is that qemu-ga will get launched
>> as a service, so PATH should have the good stuff.
>
> Ok, I'll make that change.
>
>>
>>> +
>>> + if (access(pmutils_bin_path, X_OK) != 0) {
>>> + pmutils_bin = NULL;
>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>> + if (fd< 0) {
>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>> + return;
>>> + }
>>> + }
>>> +
>>> + ret = fork();
>>> + if (ret == 0) {
>>> + /* child */
>>> + setsid();
>>> + fclose(stdin);
>>> + fclose(stdout);
>>> + fclose(stderr);
>>> +
>>> + if (pmutils_bin) {
>>> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
>>> + if (ret) {
>>> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
>>> + }
>>> + } else {
>>> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>> + ret = write(fd, cmd, strlen(cmd));
>>> + if (ret< 0) {
>>> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
>>> + strerror(errno));
>>> + }
>>> + close(fd);
>>> + }
>>> +
>>> + exit(!!ret);
>>> + } else if (ret< 0) {
>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>> + return;
>>> + }
>>> +
>>> + if (!pmutils_bin) {
>>> + close(fd);
>>> + }
>>> +}
>>> +
>>> /* register init/cleanup routines for stateful command groups */
>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>> {
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 13:00 ` Luiz Capitulino
@ 2011-12-14 15:54 ` Michael Roth
2011-12-14 16:38 ` Luiz Capitulino
0 siblings, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-14 15:54 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 07:00 AM, Luiz Capitulino wrote:
> On Tue, 13 Dec 2011 14:03:08 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>
>> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
>>> It supports two modes: "hibernate" (which corresponds to S4) and
>>> "sleep" (which corresponds to S3). It will try to execute the
>>> pm-hibernate or pm-suspend scripts, if the scripts don't exist
>>> the command will try to suspend by directly writing to the
>>> "/sys/power/state" file.
>>>
>>> An interesting implementation detail is how to cleanup the child's
>>> status on termination, so that we don't create zombies. I've
>>> choosen to ignore the SIGCHLD signal. This will cause the kernel to
>>> automatically cleanup the child's status on its termination.
>>
>> One downside to blocking SIGCHLD is it can screw with child processes
>> that utilize it. `sudo` for instance will just hang indefinitely because
>> it handles it's own cleanup via SIGCHLD, we might run into similar cases
>> with various pm-hibernate/pm-suspend implementations as well.
>>
>> This will also screw with anything we launch via guest-exec as well,
>> once that goes in.
>>
>> I wonder if we can tie the qemu_add_child_watch() stuff into qemu-ga's
>> main loop, or maybe just implement something similar...
>>
>> Basically:
>>
>> - add a qemu-ga.c:signal_channel_add() that creates a non-blocking
>> pipe and associate the read-side with a GIOChannel, then ties the
>> channel into the main loop via g_io_add_watch() on qemu-ga startup, with
>> an associated callback that reads everything off the pipe, then iterates
>> through a list of registered pids and does a non-blocking wait(pid, ...)
>> on each.
>>
>> - add a SIGCHLD handler that writes a 1 to the write side of the pipe
>> whenever it gets called
>
> Is the pipe really needed? Is there any side effect if we call waitpid()
> from the signal handler considering it won't block?
In the current state of things, I believe that might actually be
sufficient. I was thinking of the eventual guest-exec case though: we
need to be able to poll for a guest-exec'd process' return status
asynchronously by calling a guest-exec-status command that does the
wait(), so if we use a waitpid(-1, ...) SIGCHLD handler, or block
SIGCHLD, the return status would be intercepted/lost.
>
> I'm also wondering if we could use g_child_watch_add(), but it's not clear
> to me if it works with processes not created with g_spawn_*() functions.
GPid's map to something other than PIDs on Windows, so I think we'd have
issues there. But our fork() approach probably wouldn't work at all on
Windows except maybe under cygwin, so at some point we'd probably want
to switch over to g_spawn for this kind of stuff anyway...
So this might be a good point to switch over to using the glib functions.
Would you mind trying to do the hibernate/zombie reaping stuff using
g_spawn+g_child_watch_add()? It might end up being the easiest route.
Otherwise I can take a look at it later today.
>
>>
>> Then, when creating a child, you just register the child by adding the
>> pid to the list that the signal channel callback checks.
>>
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>>
>>> I've tested this w/o any virtio driver, as they don't support S4 yet. For
>>> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
>>> resume from it, but by checking the logs it seems to work fine.
>>>
>>> changelog
>>> ---------
>>>
>>> v2
>>>
>>> o Rename the command to 'guest-suspend'
>>> o Add 'mode' parameter
>>> o Use pm-utils scripts
>>> o Cleanup child termination status
>>>
>>> qapi-schema-guest.json | 17 +++++++++++
>>> qemu-ga.c | 11 +++++++-
>>> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 91 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>> index 29989fe..656bde9 100644
>>> --- a/qapi-schema-guest.json
>>> +++ b/qapi-schema-guest.json
>>> @@ -219,3 +219,20 @@
>>> ##
>>> { 'command': 'guest-fsfreeze-thaw',
>>> 'returns': 'int' }
>>> +
>>> +##
>>> +# @guest-suspend
>>> +#
>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>> +#
>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>> +# powered down (this corresponds to ACPI S4)
>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>> +# (this corresponds to ACPI S3)
>>> +#
>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>> +# succeed. Errors will be logged to guest's syslog.
>>> +#
>>> +# Since: 1.1
>>> +##
>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>> index 60d4972..b32e96c 100644
>>> --- a/qemu-ga.c
>>> +++ b/qemu-ga.c
>>> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
>>>
>>> static void register_signal_handlers(void)
>>> {
>>> - struct sigaction sigact;
>>> + struct sigaction sigact, sigact_chld;
>>> int ret;
>>>
>>> memset(&sigact, 0, sizeof(struct sigaction));
>>> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
>>> if (ret == -1) {
>>> g_error("error configuring signal handler: %s", strerror(errno));
>>> }
>>> +
>>> + /* This should cause the kernel to automatically cleanup child
>>> + termination status */
>>> + memset(&sigact_chld, 0, sizeof(struct sigaction));
>>> + sigact_chld.sa_handler = SIG_IGN;
>>> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
>>> + if (ret == -1) {
>>> + g_error("error configuring signal handler: %s", strerror(errno));
>>> + }
>>> }
>>>
>>> static void usage(const char *cmd)
>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>> index a09c8ca..4799638 100644
>>> --- a/qga/guest-agent-commands.c
>>> +++ b/qga/guest-agent-commands.c
>>> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>> }
>>> #endif
>>>
>>> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>> +
>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>> +{
>>> + int ret, fd = -1;
>>> + const char *pmutils_bin;
>>> + char pmutils_bin_path[PATH_MAX];
>>> +
>>> + if (strcmp(mode, "hibernate") == 0) {
>>> + pmutils_bin = "pm-hibernate";
>>> + } else if (strcmp(mode, "sleep") == 0) {
>>> + pmutils_bin = "pm-suspend";
>>> + } else {
>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>> + return;
>>> + }
>>> +
>>> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
>>> + LINUX_PM_UTILS_PATH, pmutils_bin);
>>> +
>>> + if (access(pmutils_bin_path, X_OK) != 0) {
>>> + pmutils_bin = NULL;
>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>> + if (fd< 0) {
>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>> + return;
>>> + }
>>> + }
>>> +
>>> + ret = fork();
>>> + if (ret == 0) {
>>> + /* child */
>>> + setsid();
>>> + fclose(stdin);
>>> + fclose(stdout);
>>> + fclose(stderr);
>>> +
>>> + if (pmutils_bin) {
>>> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
>>> + if (ret) {
>>> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
>>> + }
>>> + } else {
>>> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>> + ret = write(fd, cmd, strlen(cmd));
>>> + if (ret< 0) {
>>> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
>>> + strerror(errno));
>>> + }
>>> + close(fd);
>>> + }
>>> +
>>> + exit(!!ret);
>>> + } else if (ret< 0) {
>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>> + return;
>>> + }
>>> +
>>> + if (!pmutils_bin) {
>>> + close(fd);
>>> + }
>>> +}
>>> +
>>> /* register init/cleanup routines for stateful command groups */
>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>> {
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 15:54 ` Michael Roth
@ 2011-12-14 16:38 ` Luiz Capitulino
2011-12-14 18:06 ` Michael Roth
2011-12-14 18:17 ` Luiz Capitulino
0 siblings, 2 replies; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 16:38 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Wed, 14 Dec 2011 09:54:29 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/14/2011 07:00 AM, Luiz Capitulino wrote:
> > On Tue, 13 Dec 2011 14:03:08 -0600
> > Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
> >
> >> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
> >>> It supports two modes: "hibernate" (which corresponds to S4) and
> >>> "sleep" (which corresponds to S3). It will try to execute the
> >>> pm-hibernate or pm-suspend scripts, if the scripts don't exist
> >>> the command will try to suspend by directly writing to the
> >>> "/sys/power/state" file.
> >>>
> >>> An interesting implementation detail is how to cleanup the child's
> >>> status on termination, so that we don't create zombies. I've
> >>> choosen to ignore the SIGCHLD signal. This will cause the kernel to
> >>> automatically cleanup the child's status on its termination.
> >>
> >> One downside to blocking SIGCHLD is it can screw with child processes
> >> that utilize it. `sudo` for instance will just hang indefinitely because
> >> it handles it's own cleanup via SIGCHLD, we might run into similar cases
> >> with various pm-hibernate/pm-suspend implementations as well.
> >>
> >> This will also screw with anything we launch via guest-exec as well,
> >> once that goes in.
> >>
> >> I wonder if we can tie the qemu_add_child_watch() stuff into qemu-ga's
> >> main loop, or maybe just implement something similar...
> >>
> >> Basically:
> >>
> >> - add a qemu-ga.c:signal_channel_add() that creates a non-blocking
> >> pipe and associate the read-side with a GIOChannel, then ties the
> >> channel into the main loop via g_io_add_watch() on qemu-ga startup, with
> >> an associated callback that reads everything off the pipe, then iterates
> >> through a list of registered pids and does a non-blocking wait(pid, ...)
> >> on each.
> >>
> >> - add a SIGCHLD handler that writes a 1 to the write side of the pipe
> >> whenever it gets called
> >
> > Is the pipe really needed? Is there any side effect if we call waitpid()
> > from the signal handler considering it won't block?
>
> In the current state of things, I believe that might actually be
> sufficient. I was thinking of the eventual guest-exec case though: we
> need to be able to poll for a guest-exec'd process' return status
> asynchronously by calling a guest-exec-status command that does the
> wait(), so if we use a waitpid(-1, ...) SIGCHLD handler, or block
> SIGCHLD, the return status would be intercepted/lost.
What I had in mind was to do what qemu_add_child_watch() does, ie. it
iterates through a list of child pids calling waitpid() on them instead
of doing waitpid(-1,...). The only difference is that we would do it from
a signal handler.
Maybe that could work for guest-exec too: we could let the signal handler
collect the exit status and store them. Then guest-status could retrieve
the status from there.
We have several options here. Maybe I should do the simplest solution for
the guest-suspend command and you work on improving it for guest-exec.
> >
> > I'm also wondering if we could use g_child_watch_add(), but it's not clear
> > to me if it works with processes not created with g_spawn_*() functions.
>
> GPid's map to something other than PIDs on Windows, so I think we'd have
> issues there. But our fork() approach probably wouldn't work at all on
> Windows except maybe under cygwin, so at some point we'd probably want
> to switch over to g_spawn for this kind of stuff anyway...
>
> So this might be a good point to switch over to using the glib functions.
>
> Would you mind trying to do the hibernate/zombie reaping stuff using
> g_spawn+g_child_watch_add()? It might end up being the easiest route.
> Otherwise I can take a look at it later today.
Well, there are two problems with g_spawn wrt to the manual method of
writing to the sysfs file. The first one is that I'm not sure if g_spawn()
reports the file not found error synchronously. The other problem is that,
I'd have to fork() anyway to write to the sysfs file (unless we decide that
it's ok to do this synchronously, which seems ok to me).
>
> >
> >>
> >> Then, when creating a child, you just register the child by adding the
> >> pid to the list that the signal channel callback checks.
> >>
> >>>
> >>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >>> ---
> >>>
> >>> I've tested this w/o any virtio driver, as they don't support S4 yet. For
> >>> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> >>> resume from it, but by checking the logs it seems to work fine.
> >>>
> >>> changelog
> >>> ---------
> >>>
> >>> v2
> >>>
> >>> o Rename the command to 'guest-suspend'
> >>> o Add 'mode' parameter
> >>> o Use pm-utils scripts
> >>> o Cleanup child termination status
> >>>
> >>> qapi-schema-guest.json | 17 +++++++++++
> >>> qemu-ga.c | 11 +++++++-
> >>> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 91 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> >>> index 29989fe..656bde9 100644
> >>> --- a/qapi-schema-guest.json
> >>> +++ b/qapi-schema-guest.json
> >>> @@ -219,3 +219,20 @@
> >>> ##
> >>> { 'command': 'guest-fsfreeze-thaw',
> >>> 'returns': 'int' }
> >>> +
> >>> +##
> >>> +# @guest-suspend
> >>> +#
> >>> +# Suspend guest execution by entering ACPI power state S3 or S4.
> >>> +#
> >>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> >>> +# powered down (this corresponds to ACPI S4)
> >>> +# 'sleep' execution is suspended but the RAM retains its contents
> >>> +# (this corresponds to ACPI S3)
> >>> +#
> >>> +# Notes: This is an asynchronous request. There's no guarantee it will
> >>> +# succeed. Errors will be logged to guest's syslog.
> >>> +#
> >>> +# Since: 1.1
> >>> +##
> >>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> >>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>> index 60d4972..b32e96c 100644
> >>> --- a/qemu-ga.c
> >>> +++ b/qemu-ga.c
> >>> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
> >>>
> >>> static void register_signal_handlers(void)
> >>> {
> >>> - struct sigaction sigact;
> >>> + struct sigaction sigact, sigact_chld;
> >>> int ret;
> >>>
> >>> memset(&sigact, 0, sizeof(struct sigaction));
> >>> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
> >>> if (ret == -1) {
> >>> g_error("error configuring signal handler: %s", strerror(errno));
> >>> }
> >>> +
> >>> + /* This should cause the kernel to automatically cleanup child
> >>> + termination status */
> >>> + memset(&sigact_chld, 0, sizeof(struct sigaction));
> >>> + sigact_chld.sa_handler = SIG_IGN;
> >>> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
> >>> + if (ret == -1) {
> >>> + g_error("error configuring signal handler: %s", strerror(errno));
> >>> + }
> >>> }
> >>>
> >>> static void usage(const char *cmd)
> >>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >>> index a09c8ca..4799638 100644
> >>> --- a/qga/guest-agent-commands.c
> >>> +++ b/qga/guest-agent-commands.c
> >>> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >>> }
> >>> #endif
> >>>
> >>> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> >>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> >>> +
> >>> +void qmp_guest_suspend(const char *mode, Error **err)
> >>> +{
> >>> + int ret, fd = -1;
> >>> + const char *pmutils_bin;
> >>> + char pmutils_bin_path[PATH_MAX];
> >>> +
> >>> + if (strcmp(mode, "hibernate") == 0) {
> >>> + pmutils_bin = "pm-hibernate";
> >>> + } else if (strcmp(mode, "sleep") == 0) {
> >>> + pmutils_bin = "pm-suspend";
> >>> + } else {
> >>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
> >>> + return;
> >>> + }
> >>> +
> >>> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
> >>> + LINUX_PM_UTILS_PATH, pmutils_bin);
> >>> +
> >>> + if (access(pmutils_bin_path, X_OK) != 0) {
> >>> + pmutils_bin = NULL;
> >>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> >>> + if (fd< 0) {
> >>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> >>> + return;
> >>> + }
> >>> + }
> >>> +
> >>> + ret = fork();
> >>> + if (ret == 0) {
> >>> + /* child */
> >>> + setsid();
> >>> + fclose(stdin);
> >>> + fclose(stdout);
> >>> + fclose(stderr);
> >>> +
> >>> + if (pmutils_bin) {
> >>> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
> >>> + if (ret) {
> >>> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
> >>> + }
> >>> + } else {
> >>> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> >>> + ret = write(fd, cmd, strlen(cmd));
> >>> + if (ret< 0) {
> >>> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> >>> + strerror(errno));
> >>> + }
> >>> + close(fd);
> >>> + }
> >>> +
> >>> + exit(!!ret);
> >>> + } else if (ret< 0) {
> >>> + error_set(err, QERR_UNDEFINED_ERROR);
> >>> + return;
> >>> + }
> >>> +
> >>> + if (!pmutils_bin) {
> >>> + close(fd);
> >>> + }
> >>> +}
> >>> +
> >>> /* register init/cleanup routines for stateful command groups */
> >>> void ga_command_state_init(GAState *s, GACommandState *cs)
> >>> {
> >>
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 16:38 ` Luiz Capitulino
@ 2011-12-14 18:06 ` Michael Roth
2011-12-14 23:44 ` Luiz Capitulino
2011-12-14 18:17 ` Luiz Capitulino
1 sibling, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-14 18:06 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 10:38 AM, Luiz Capitulino wrote:
> On Wed, 14 Dec 2011 09:54:29 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>
>> On 12/14/2011 07:00 AM, Luiz Capitulino wrote:
>>> On Tue, 13 Dec 2011 14:03:08 -0600
>>> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>>>
>>>> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
>>>>> It supports two modes: "hibernate" (which corresponds to S4) and
>>>>> "sleep" (which corresponds to S3). It will try to execute the
>>>>> pm-hibernate or pm-suspend scripts, if the scripts don't exist
>>>>> the command will try to suspend by directly writing to the
>>>>> "/sys/power/state" file.
>>>>>
>>>>> An interesting implementation detail is how to cleanup the child's
>>>>> status on termination, so that we don't create zombies. I've
>>>>> choosen to ignore the SIGCHLD signal. This will cause the kernel to
>>>>> automatically cleanup the child's status on its termination.
>>>>
>>>> One downside to blocking SIGCHLD is it can screw with child processes
>>>> that utilize it. `sudo` for instance will just hang indefinitely because
>>>> it handles it's own cleanup via SIGCHLD, we might run into similar cases
>>>> with various pm-hibernate/pm-suspend implementations as well.
>>>>
>>>> This will also screw with anything we launch via guest-exec as well,
>>>> once that goes in.
>>>>
>>>> I wonder if we can tie the qemu_add_child_watch() stuff into qemu-ga's
>>>> main loop, or maybe just implement something similar...
>>>>
>>>> Basically:
>>>>
>>>> - add a qemu-ga.c:signal_channel_add() that creates a non-blocking
>>>> pipe and associate the read-side with a GIOChannel, then ties the
>>>> channel into the main loop via g_io_add_watch() on qemu-ga startup, with
>>>> an associated callback that reads everything off the pipe, then iterates
>>>> through a list of registered pids and does a non-blocking wait(pid, ...)
>>>> on each.
>>>>
>>>> - add a SIGCHLD handler that writes a 1 to the write side of the pipe
>>>> whenever it gets called
>>>
>>> Is the pipe really needed? Is there any side effect if we call waitpid()
>>> from the signal handler considering it won't block?
>>
>> In the current state of things, I believe that might actually be
>> sufficient. I was thinking of the eventual guest-exec case though: we
>> need to be able to poll for a guest-exec'd process' return status
>> asynchronously by calling a guest-exec-status command that does the
>> wait(), so if we use a waitpid(-1, ...) SIGCHLD handler, or block
>> SIGCHLD, the return status would be intercepted/lost.
>
> What I had in mind was to do what qemu_add_child_watch() does, ie. it
> iterates through a list of child pids calling waitpid() on them instead
> of doing waitpid(-1,...). The only difference is that we would do it from
> a signal handler.
Ah, yah that might work. I'm not sure how well our list functions will
behave when accessed via an interrupt handler. We'll have race
conditions at least:
#define QTAILQ_INSERT_TAIL(head, elm, field) do { \
(elm)->field.tqe_next = NULL; \
(elm)->field.tqe_prev = (head)->tqh_last; \
/* interrupt */
*(head)->tqh_last = (elm); \
(head)->tqh_last = &(elm)->field.tqe_next;
So if we fork() and the process completes and triggers the interrupt
before we add the pid to the list, it will remain a zombie until
something else triggers the handler. It's not a huge deal...we at least
avoid accumulating zombies, but I'd be weary of other side-effects as
well, especially if we also remove entries from the list if they've been
waited on successfully.
>
> Maybe that could work for guest-exec too: we could let the signal handler
> collect the exit status and store them. Then guest-status could retrieve
> the status from there.
Yah, I think we can tie all this together rather nicely with a little work.
>
> We have several options here. Maybe I should do the simplest solution for
> the guest-suspend command and you work on improving it for guest-exec.
That's fine, I need to look at this more. But if we're gonna take the
simplest approach for now, let's not even bother with the pid
registration (and potential races), and just do a waitpid(-1, ...) in a
SIGCHLD handler like QEMU did before the child watch stuff was added,
and I'll make the changes necessary for the guest-exec stuff before that
gets added.
>
>>>
>>> I'm also wondering if we could use g_child_watch_add(), but it's not clear
>>> to me if it works with processes not created with g_spawn_*() functions.
>>
>> GPid's map to something other than PIDs on Windows, so I think we'd have
>> issues there. But our fork() approach probably wouldn't work at all on
>> Windows except maybe under cygwin, so at some point we'd probably want
>> to switch over to g_spawn for this kind of stuff anyway...
>>
>> So this might be a good point to switch over to using the glib functions.
>>
>> Would you mind trying to do the hibernate/zombie reaping stuff using
>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
>> Otherwise I can take a look at it later today.
>
> Well, there are two problems with g_spawn wrt to the manual method of
> writing to the sysfs file. The first one is that I'm not sure if g_spawn()
> reports the file not found error synchronously. The other problem is that,
"error can be NULL to ignore errors, or non-NULL to report errors. If an
error is set, the function returns FALSE. Errors are reported even if
they occur in the child (for example if the executable in argv[0] is not
found). Typically the message field of returned errors should be
displayed to users. Possible errors are those from the G_SPAWN_ERROR
domain."
So I'd think we'd be able to report the FNF synchronously even if we
launch into the background.
> I'd have to fork() anyway to write to the sysfs file (unless we decide that
> it's ok to do this synchronously, which seems ok to me).
I think what we'd have to do is open the file, then pass it in as stdout
for a g_spawn'd `echo "disk"` command. I'm not sure how we'd manage
cleaning up the FD though, if we're doing it asynchronously. And
synchronous means every guest-suspend call will eventually time out, so
I don't think that's doable right now.
So I'd say keep it the way it is for now. Probably not worth spending
too much time on until we see what the Windows stuff looks like anyway.
>
>>
>>>
>>>>
>>>> Then, when creating a child, you just register the child by adding the
>>>> pid to the list that the signal channel callback checks.
>>>>
>>>>>
>>>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>>> ---
>>>>>
>>>>> I've tested this w/o any virtio driver, as they don't support S4 yet. For
>>>>> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
>>>>> resume from it, but by checking the logs it seems to work fine.
>>>>>
>>>>> changelog
>>>>> ---------
>>>>>
>>>>> v2
>>>>>
>>>>> o Rename the command to 'guest-suspend'
>>>>> o Add 'mode' parameter
>>>>> o Use pm-utils scripts
>>>>> o Cleanup child termination status
>>>>>
>>>>> qapi-schema-guest.json | 17 +++++++++++
>>>>> qemu-ga.c | 11 +++++++-
>>>>> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 91 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>>>> index 29989fe..656bde9 100644
>>>>> --- a/qapi-schema-guest.json
>>>>> +++ b/qapi-schema-guest.json
>>>>> @@ -219,3 +219,20 @@
>>>>> ##
>>>>> { 'command': 'guest-fsfreeze-thaw',
>>>>> 'returns': 'int' }
>>>>> +
>>>>> +##
>>>>> +# @guest-suspend
>>>>> +#
>>>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>>>> +#
>>>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>>>> +# powered down (this corresponds to ACPI S4)
>>>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>>>> +# (this corresponds to ACPI S3)
>>>>> +#
>>>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>>>> +# succeed. Errors will be logged to guest's syslog.
>>>>> +#
>>>>> +# Since: 1.1
>>>>> +##
>>>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>>>> index 60d4972..b32e96c 100644
>>>>> --- a/qemu-ga.c
>>>>> +++ b/qemu-ga.c
>>>>> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
>>>>>
>>>>> static void register_signal_handlers(void)
>>>>> {
>>>>> - struct sigaction sigact;
>>>>> + struct sigaction sigact, sigact_chld;
>>>>> int ret;
>>>>>
>>>>> memset(&sigact, 0, sizeof(struct sigaction));
>>>>> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
>>>>> if (ret == -1) {
>>>>> g_error("error configuring signal handler: %s", strerror(errno));
>>>>> }
>>>>> +
>>>>> + /* This should cause the kernel to automatically cleanup child
>>>>> + termination status */
>>>>> + memset(&sigact_chld, 0, sizeof(struct sigaction));
>>>>> + sigact_chld.sa_handler = SIG_IGN;
>>>>> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
>>>>> + if (ret == -1) {
>>>>> + g_error("error configuring signal handler: %s", strerror(errno));
>>>>> + }
>>>>> }
>>>>>
>>>>> static void usage(const char *cmd)
>>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>>>> index a09c8ca..4799638 100644
>>>>> --- a/qga/guest-agent-commands.c
>>>>> +++ b/qga/guest-agent-commands.c
>>>>> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>>>> }
>>>>> #endif
>>>>>
>>>>> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
>>>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>>>> +
>>>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>>>> +{
>>>>> + int ret, fd = -1;
>>>>> + const char *pmutils_bin;
>>>>> + char pmutils_bin_path[PATH_MAX];
>>>>> +
>>>>> + if (strcmp(mode, "hibernate") == 0) {
>>>>> + pmutils_bin = "pm-hibernate";
>>>>> + } else if (strcmp(mode, "sleep") == 0) {
>>>>> + pmutils_bin = "pm-suspend";
>>>>> + } else {
>>>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
>>>>> + LINUX_PM_UTILS_PATH, pmutils_bin);
>>>>> +
>>>>> + if (access(pmutils_bin_path, X_OK) != 0) {
>>>>> + pmutils_bin = NULL;
>>>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>>>> + if (fd< 0) {
>>>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + ret = fork();
>>>>> + if (ret == 0) {
>>>>> + /* child */
>>>>> + setsid();
>>>>> + fclose(stdin);
>>>>> + fclose(stdout);
>>>>> + fclose(stderr);
>>>>> +
>>>>> + if (pmutils_bin) {
>>>>> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
>>>>> + if (ret) {
>>>>> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
>>>>> + }
>>>>> + } else {
>>>>> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>>>> + ret = write(fd, cmd, strlen(cmd));
>>>>> + if (ret< 0) {
>>>>> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
>>>>> + strerror(errno));
>>>>> + }
>>>>> + close(fd);
>>>>> + }
>>>>> +
>>>>> + exit(!!ret);
>>>>> + } else if (ret< 0) {
>>>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if (!pmutils_bin) {
>>>>> + close(fd);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> /* register init/cleanup routines for stateful command groups */
>>>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>>>> {
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 16:38 ` Luiz Capitulino
2011-12-14 18:06 ` Michael Roth
@ 2011-12-14 18:17 ` Luiz Capitulino
2011-12-14 19:43 ` Michael Roth
1 sibling, 1 reply; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 18:17 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Wed, 14 Dec 2011 14:38:55 -0200
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > I'm also wondering if we could use g_child_watch_add(), but it's not clear
> > > to me if it works with processes not created with g_spawn_*() functions.
> >
> > GPid's map to something other than PIDs on Windows, so I think we'd have
> > issues there. But our fork() approach probably wouldn't work at all on
> > Windows except maybe under cygwin, so at some point we'd probably want
> > to switch over to g_spawn for this kind of stuff anyway...
> >
> > So this might be a good point to switch over to using the glib functions.
> >
> > Would you mind trying to do the hibernate/zombie reaping stuff using
> > g_spawn+g_child_watch_add()? It might end up being the easiest route.
> > Otherwise I can take a look at it later today.
>
> Well, there are two problems with g_spawn wrt to the manual method of
> writing to the sysfs file. The first one is that I'm not sure if g_spawn()
> reports the file not found error synchronously. The other problem is that,
> I'd have to fork() anyway to write to the sysfs file (unless we decide that
> it's ok to do this synchronously, which seems ok to me).
The version below uses g_spawn_async(). The code is a bit simpler than previous
versions, but there are two important details about it:
1. I'm letting g_spawn_async() reap the child automatically. I don't know
how it does it though. I'd guess it uses g_child_watch_add(), worst
case it ignores SIGCHLD (although I think this would be awful)
2. The manual method of writing to the sysfs is done synchronously. This
means that the command response will only be sent when the guest resumes
If you think this approach is acceptable, I'll test it more, update its doc,
etc and post it again.
diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 5f8a18d..63f65a6 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -219,3 +219,20 @@
##
{ 'command': 'guest-fsfreeze-thaw',
'returns': 'int' }
+
+##
+# @guest-suspend
+#
+# Suspend guest execution by entering ACPI power state S3 or S4.
+#
+# @mode: 'hibernate' RAM content is saved in the disk and the guest is
+# powered down (this corresponds to ACPI S4)
+# 'sleep' execution is suspended but the RAM retains its contents
+# (this corresponds to ACPI S3)
+#
+# Notes: This is an asynchronous request. There's no guarantee it will
+# succeed. Errors will be logged to guest's syslog.
+#
+# Since: 1.1
+##
+{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
index a09c8ca..0c6b78e 100644
--- a/qga/guest-agent-commands.c
+++ b/qga/guest-agent-commands.c
@@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
}
#endif
+#define LINUX_SYS_STATE_FILE "/sys/power/state"
+
+void qmp_guest_suspend(const char *mode, Error **err)
+{
+ GError *error = NULL;
+ gchar *argv[2];
+
+ if (strcmp(mode, "hibernate") == 0) {
+ argv[0] = (gchar *) "pm-hibernate";
+ } else if (strcmp(mode, "sleep") == 0) {
+ argv[0] = (gchar *) "pm-suspend";
+ } else if (strcmp(mode, "hybrid") == 0) {
+ argv[0] = (gchar *) "pm-hybrid";
+ } else {
+ error_set(err, QERR_INVALID_PARAMETER, "mode");
+ return;
+ }
+
+ argv[1] = NULL;
+ if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
+ G_SPAWN_FILE_AND_ARGV_ZERO,
+ NULL, NULL, NULL, &error) < 0) {
+ int fd;
+ const char *cmd;
+
+ slog("%s\n", error->message);
+ g_error_free(error);
+
+ if (strcmp(mode, "hybrid") == 0) {
+ error_set(err, QERR_UNDEFINED_ERROR);
+ return;
+ }
+
+ slog("trying to suspend using the manual method...\n");
+
+ fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
+ if (fd < 0) {
+ error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
+ return;
+ }
+
+ cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
+ if (write(fd, cmd, strlen(cmd)) < 0) {
+ error_set(err, QERR_IO_ERROR);
+ }
+
+ close(fd);
+ }
+}
+
/* register init/cleanup routines for stateful command groups */
void ga_command_state_init(GAState *s, GACommandState *cs)
{
--
1.7.8.197.g73c6b.dirty
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 18:17 ` Luiz Capitulino
@ 2011-12-14 19:43 ` Michael Roth
2011-12-14 20:06 ` Luiz Capitulino
0 siblings, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-14 19:43 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
> On Wed, 14 Dec 2011 14:38:55 -0200
> Luiz Capitulino<lcapitulino@redhat.com> wrote:
>
>>>> I'm also wondering if we could use g_child_watch_add(), but it's not clear
>>>> to me if it works with processes not created with g_spawn_*() functions.
>>>
>>> GPid's map to something other than PIDs on Windows, so I think we'd have
>>> issues there. But our fork() approach probably wouldn't work at all on
>>> Windows except maybe under cygwin, so at some point we'd probably want
>>> to switch over to g_spawn for this kind of stuff anyway...
>>>
>>> So this might be a good point to switch over to using the glib functions.
>>>
>>> Would you mind trying to do the hibernate/zombie reaping stuff using
>>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
>>> Otherwise I can take a look at it later today.
>>
>> Well, there are two problems with g_spawn wrt to the manual method of
>> writing to the sysfs file. The first one is that I'm not sure if g_spawn()
>> reports the file not found error synchronously. The other problem is that,
>> I'd have to fork() anyway to write to the sysfs file (unless we decide that
>> it's ok to do this synchronously, which seems ok to me).
>
> The version below uses g_spawn_async(). The code is a bit simpler than previous
> versions, but there are two important details about it:
>
> 1. I'm letting g_spawn_async() reap the child automatically. I don't know
> how it does it though. I'd guess it uses g_child_watch_add(), worst
> case it ignores SIGCHLD (although I think this would be awful)
Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
not seeing an error that's causing it to reap it immediately after the fork?
>
> 2. The manual method of writing to the sysfs is done synchronously. This
> means that the command response will only be sent when the guest resumes
Want to avoid sync blocking calls as much as possible until timeouts are
sorted out, particularly in this case. Currently, with hibernate at
least, QEMU will exit (or is something being done to change that
behavior?), libvirt will restart the VM later via some other interface,
establish a connection to a new monitor and new agent channel, then gets
a spurious response from the agent for a command it issue to a different
QEMU process far in the past. So the common scenario would need nasty,
special handling in libvirt and they'd probably hate us for it.
So probably best to stick with your previous approach for now, and look
for a glib solution later.
>
> If you think this approach is acceptable, I'll test it more, update its doc,
> etc and post it again.
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 5f8a18d..63f65a6 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,20 @@
> ##
> { 'command': 'guest-fsfreeze-thaw',
> 'returns': 'int' }
> +
> +##
> +# @guest-suspend
> +#
> +# Suspend guest execution by entering ACPI power state S3 or S4.
> +#
> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> +# powered down (this corresponds to ACPI S4)
> +# 'sleep' execution is suspended but the RAM retains its contents
> +# (this corresponds to ACPI S3)
> +#
> +# Notes: This is an asynchronous request. There's no guarantee it will
> +# succeed. Errors will be logged to guest's syslog.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..0c6b78e 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> }
> #endif
>
> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> +
> +void qmp_guest_suspend(const char *mode, Error **err)
> +{
> + GError *error = NULL;
> + gchar *argv[2];
> +
> + if (strcmp(mode, "hibernate") == 0) {
> + argv[0] = (gchar *) "pm-hibernate";
> + } else if (strcmp(mode, "sleep") == 0) {
> + argv[0] = (gchar *) "pm-suspend";
> + } else if (strcmp(mode, "hybrid") == 0) {
> + argv[0] = (gchar *) "pm-hybrid";
> + } else {
> + error_set(err, QERR_INVALID_PARAMETER, "mode");
> + return;
> + }
> +
> + argv[1] = NULL;
> + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
> + G_SPAWN_FILE_AND_ARGV_ZERO,
> + NULL, NULL, NULL,&error)< 0) {
> + int fd;
> + const char *cmd;
> +
> + slog("%s\n", error->message);
> + g_error_free(error);
> +
> + if (strcmp(mode, "hybrid") == 0) {
> + error_set(err, QERR_UNDEFINED_ERROR);
> + return;
> + }
> +
> + slog("trying to suspend using the manual method...\n");
> +
> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> + if (fd< 0) {
> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> + return;
> + }
> +
> + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> + if (write(fd, cmd, strlen(cmd))< 0) {
> + error_set(err, QERR_IO_ERROR);
> + }
> +
> + close(fd);
> + }
> +}
> +
> /* register init/cleanup routines for stateful command groups */
> void ga_command_state_init(GAState *s, GACommandState *cs)
> {
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 19:43 ` Michael Roth
@ 2011-12-14 20:06 ` Luiz Capitulino
2011-12-14 20:56 ` Michael Roth
0 siblings, 1 reply; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 20:06 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Wed, 14 Dec 2011 13:43:23 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
> > On Wed, 14 Dec 2011 14:38:55 -0200
> > Luiz Capitulino<lcapitulino@redhat.com> wrote:
> >
> >>>> I'm also wondering if we could use g_child_watch_add(), but it's not clear
> >>>> to me if it works with processes not created with g_spawn_*() functions.
> >>>
> >>> GPid's map to something other than PIDs on Windows, so I think we'd have
> >>> issues there. But our fork() approach probably wouldn't work at all on
> >>> Windows except maybe under cygwin, so at some point we'd probably want
> >>> to switch over to g_spawn for this kind of stuff anyway...
> >>>
> >>> So this might be a good point to switch over to using the glib functions.
> >>>
> >>> Would you mind trying to do the hibernate/zombie reaping stuff using
> >>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
> >>> Otherwise I can take a look at it later today.
> >>
> >> Well, there are two problems with g_spawn wrt to the manual method of
> >> writing to the sysfs file. The first one is that I'm not sure if g_spawn()
> >> reports the file not found error synchronously. The other problem is that,
> >> I'd have to fork() anyway to write to the sysfs file (unless we decide that
> >> it's ok to do this synchronously, which seems ok to me).
> >
> > The version below uses g_spawn_async(). The code is a bit simpler than previous
> > versions, but there are two important details about it:
> >
> > 1. I'm letting g_spawn_async() reap the child automatically. I don't know
> > how it does it though. I'd guess it uses g_child_watch_add(), worst
> > case it ignores SIGCHLD (although I think this would be awful)
>
> Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
> not seeing an error that's causing it to reap it immediately after the fork?
No. I haven't tested it much though, but the basic use case seemed to work
fine.
> >
> > 2. The manual method of writing to the sysfs is done synchronously. This
> > means that the command response will only be sent when the guest resumes
>
> Want to avoid sync blocking calls as much as possible until timeouts are
> sorted out, particularly in this case. Currently, with hibernate at
> least, QEMU will exit (or is something being done to change that
> behavior?), libvirt will restart the VM later via some other interface,
> establish a connection to a new monitor and new agent channel, then gets
> a spurious response from the agent for a command it issue to a different
> QEMU process far in the past. So the common scenario would need nasty,
> special handling in libvirt and they'd probably hate us for it.
Problem is: even with the other approach there's no way to guarantee that
the response will arrive before the guest suspends or shuts down, as it
depends on which process runs first.
If we want to guarantee that the client will see a response before the
guest suspends, then we need some form of synchronization. Like, the child
should wait for the parent to send the response to the client before
exec()ing (or writing to the sysfs file).
>
> So probably best to stick with your previous approach for now, and look
> for a glib solution later.
>
> >
> > If you think this approach is acceptable, I'll test it more, update its doc,
> > etc and post it again.
> >
> > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> > index 5f8a18d..63f65a6 100644
> > --- a/qapi-schema-guest.json
> > +++ b/qapi-schema-guest.json
> > @@ -219,3 +219,20 @@
> > ##
> > { 'command': 'guest-fsfreeze-thaw',
> > 'returns': 'int' }
> > +
> > +##
> > +# @guest-suspend
> > +#
> > +# Suspend guest execution by entering ACPI power state S3 or S4.
> > +#
> > +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> > +# powered down (this corresponds to ACPI S4)
> > +# 'sleep' execution is suspended but the RAM retains its contents
> > +# (this corresponds to ACPI S3)
> > +#
> > +# Notes: This is an asynchronous request. There's no guarantee it will
> > +# succeed. Errors will be logged to guest's syslog.
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> > diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> > index a09c8ca..0c6b78e 100644
> > --- a/qga/guest-agent-commands.c
> > +++ b/qga/guest-agent-commands.c
> > @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> > }
> > #endif
> >
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> > + GError *error = NULL;
> > + gchar *argv[2];
> > +
> > + if (strcmp(mode, "hibernate") == 0) {
> > + argv[0] = (gchar *) "pm-hibernate";
> > + } else if (strcmp(mode, "sleep") == 0) {
> > + argv[0] = (gchar *) "pm-suspend";
> > + } else if (strcmp(mode, "hybrid") == 0) {
> > + argv[0] = (gchar *) "pm-hybrid";
> > + } else {
> > + error_set(err, QERR_INVALID_PARAMETER, "mode");
> > + return;
> > + }
> > +
> > + argv[1] = NULL;
> > + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
> > + G_SPAWN_FILE_AND_ARGV_ZERO,
> > + NULL, NULL, NULL,&error)< 0) {
> > + int fd;
> > + const char *cmd;
> > +
> > + slog("%s\n", error->message);
> > + g_error_free(error);
> > +
> > + if (strcmp(mode, "hybrid") == 0) {
> > + error_set(err, QERR_UNDEFINED_ERROR);
> > + return;
> > + }
> > +
> > + slog("trying to suspend using the manual method...\n");
> > +
> > + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> > + if (fd< 0) {
> > + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> > + return;
> > + }
> > +
> > + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> > + if (write(fd, cmd, strlen(cmd))< 0) {
> > + error_set(err, QERR_IO_ERROR);
> > + }
> > +
> > + close(fd);
> > + }
> > +}
> > +
> > /* register init/cleanup routines for stateful command groups */
> > void ga_command_state_init(GAState *s, GACommandState *cs)
> > {
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 20:06 ` Luiz Capitulino
@ 2011-12-14 20:56 ` Michael Roth
2011-12-14 21:14 ` Michael Roth
0 siblings, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-14 20:56 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 02:06 PM, Luiz Capitulino wrote:
> On Wed, 14 Dec 2011 13:43:23 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>
>> On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
>>> On Wed, 14 Dec 2011 14:38:55 -0200
>>> Luiz Capitulino<lcapitulino@redhat.com> wrote:
>>>
>>>>>> I'm also wondering if we could use g_child_watch_add(), but it's not clear
>>>>>> to me if it works with processes not created with g_spawn_*() functions.
>>>>>
>>>>> GPid's map to something other than PIDs on Windows, so I think we'd have
>>>>> issues there. But our fork() approach probably wouldn't work at all on
>>>>> Windows except maybe under cygwin, so at some point we'd probably want
>>>>> to switch over to g_spawn for this kind of stuff anyway...
>>>>>
>>>>> So this might be a good point to switch over to using the glib functions.
>>>>>
>>>>> Would you mind trying to do the hibernate/zombie reaping stuff using
>>>>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
>>>>> Otherwise I can take a look at it later today.
>>>>
>>>> Well, there are two problems with g_spawn wrt to the manual method of
>>>> writing to the sysfs file. The first one is that I'm not sure if g_spawn()
>>>> reports the file not found error synchronously. The other problem is that,
>>>> I'd have to fork() anyway to write to the sysfs file (unless we decide that
>>>> it's ok to do this synchronously, which seems ok to me).
>>>
>>> The version below uses g_spawn_async(). The code is a bit simpler than previous
>>> versions, but there are two important details about it:
>>>
>>> 1. I'm letting g_spawn_async() reap the child automatically. I don't know
>>> how it does it though. I'd guess it uses g_child_watch_add(), worst
>>> case it ignores SIGCHLD (although I think this would be awful)
>>
>> Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
>> not seeing an error that's causing it to reap it immediately after the fork?
>
> No. I haven't tested it much though, but the basic use case seemed to work
> fine.
>
>>>
>>> 2. The manual method of writing to the sysfs is done synchronously. This
>>> means that the command response will only be sent when the guest resumes
>>
>> Want to avoid sync blocking calls as much as possible until timeouts are
>> sorted out, particularly in this case. Currently, with hibernate at
>> least, QEMU will exit (or is something being done to change that
>> behavior?), libvirt will restart the VM later via some other interface,
>> establish a connection to a new monitor and new agent channel, then gets
>> a spurious response from the agent for a command it issue to a different
>> QEMU process far in the past. So the common scenario would need nasty,
>> special handling in libvirt and they'd probably hate us for it.
>
> Problem is: even with the other approach there's no way to guarantee that
> the response will arrive before the guest suspends or shuts down, as it
> depends on which process runs first.
> If we want to guarantee that the client will see a response before the
> guest suspends, then we need some form of synchronization. Like, the child
> should wait for the parent to send the response to the client before
> exec()ing (or writing to the sysfs file).
>
There's no way to guarantee it though, even if we remove the window of
opportunity for a child to complete a shutdown/suspend/etc request
before we write out a response, there's still the potential scenario
where an admin kills off/restarts qemu-ga while it's processing a request.
So relying on client-side timeouts to handle these corner cases is
acceptable, since being able to handle that is a requirement of a robust
client, especially since we're talking to a potentially malicious
guest/agent. We just need to do our best to make these situations corner
cases rather than common ones.
>>
>> So probably best to stick with your previous approach for now, and look
>> for a glib solution later.
>>
>>>
>>> If you think this approach is acceptable, I'll test it more, update its doc,
>>> etc and post it again.
>>>
>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>> index 5f8a18d..63f65a6 100644
>>> --- a/qapi-schema-guest.json
>>> +++ b/qapi-schema-guest.json
>>> @@ -219,3 +219,20 @@
>>> ##
>>> { 'command': 'guest-fsfreeze-thaw',
>>> 'returns': 'int' }
>>> +
>>> +##
>>> +# @guest-suspend
>>> +#
>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>> +#
>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>> +# powered down (this corresponds to ACPI S4)
>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>> +# (this corresponds to ACPI S3)
>>> +#
>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>> +# succeed. Errors will be logged to guest's syslog.
>>> +#
>>> +# Since: 1.1
>>> +##
>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>> index a09c8ca..0c6b78e 100644
>>> --- a/qga/guest-agent-commands.c
>>> +++ b/qga/guest-agent-commands.c
>>> @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>> }
>>> #endif
>>>
>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>> +
>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>> +{
>>> + GError *error = NULL;
>>> + gchar *argv[2];
>>> +
>>> + if (strcmp(mode, "hibernate") == 0) {
>>> + argv[0] = (gchar *) "pm-hibernate";
>>> + } else if (strcmp(mode, "sleep") == 0) {
>>> + argv[0] = (gchar *) "pm-suspend";
>>> + } else if (strcmp(mode, "hybrid") == 0) {
>>> + argv[0] = (gchar *) "pm-hybrid";
>>> + } else {
>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>> + return;
>>> + }
>>> +
>>> + argv[1] = NULL;
>>> + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
>>> + G_SPAWN_FILE_AND_ARGV_ZERO,
>>> + NULL, NULL, NULL,&error)< 0) {
>>> + int fd;
>>> + const char *cmd;
>>> +
>>> + slog("%s\n", error->message);
>>> + g_error_free(error);
>>> +
>>> + if (strcmp(mode, "hybrid") == 0) {
>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>> + return;
>>> + }
>>> +
>>> + slog("trying to suspend using the manual method...\n");
>>> +
>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>> + if (fd< 0) {
>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>> + return;
>>> + }
>>> +
>>> + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>> + if (write(fd, cmd, strlen(cmd))< 0) {
>>> + error_set(err, QERR_IO_ERROR);
>>> + }
>>> +
>>> + close(fd);
>>> + }
>>> +}
>>> +
>>> /* register init/cleanup routines for stateful command groups */
>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>> {
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 20:56 ` Michael Roth
@ 2011-12-14 21:14 ` Michael Roth
2011-12-14 23:56 ` Luiz Capitulino
0 siblings, 1 reply; 19+ messages in thread
From: Michael Roth @ 2011-12-14 21:14 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 02:56 PM, Michael Roth wrote:
> On 12/14/2011 02:06 PM, Luiz Capitulino wrote:
>> On Wed, 14 Dec 2011 13:43:23 -0600
>> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>>
>>> On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
>>>> On Wed, 14 Dec 2011 14:38:55 -0200
>>>> Luiz Capitulino<lcapitulino@redhat.com> wrote:
>>>>
>>>>>>> I'm also wondering if we could use g_child_watch_add(), but it's
>>>>>>> not clear
>>>>>>> to me if it works with processes not created with g_spawn_*()
>>>>>>> functions.
>>>>>>
>>>>>> GPid's map to something other than PIDs on Windows, so I think
>>>>>> we'd have
>>>>>> issues there. But our fork() approach probably wouldn't work at
>>>>>> all on
>>>>>> Windows except maybe under cygwin, so at some point we'd probably
>>>>>> want
>>>>>> to switch over to g_spawn for this kind of stuff anyway...
>>>>>>
>>>>>> So this might be a good point to switch over to using the glib
>>>>>> functions.
>>>>>>
>>>>>> Would you mind trying to do the hibernate/zombie reaping stuff using
>>>>>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
>>>>>> Otherwise I can take a look at it later today.
>>>>>
>>>>> Well, there are two problems with g_spawn wrt to the manual method of
>>>>> writing to the sysfs file. The first one is that I'm not sure if
>>>>> g_spawn()
>>>>> reports the file not found error synchronously. The other problem
>>>>> is that,
>>>>> I'd have to fork() anyway to write to the sysfs file (unless we
>>>>> decide that
>>>>> it's ok to do this synchronously, which seems ok to me).
>>>>
>>>> The version below uses g_spawn_async(). The code is a bit simpler
>>>> than previous
>>>> versions, but there are two important details about it:
>>>>
>>>> 1. I'm letting g_spawn_async() reap the child automatically. I don't
>>>> know
>>>> how it does it though. I'd guess it uses g_child_watch_add(), worst
>>>> case it ignores SIGCHLD (although I think this would be awful)
>>>
>>> Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
>>> not seeing an error that's causing it to reap it immediately after
>>> the fork?
>>
>> No. I haven't tested it much though, but the basic use case seemed to
>> work
>> fine.
>>
>>>>
>>>> 2. The manual method of writing to the sysfs is done synchronously.
>>>> This
>>>> means that the command response will only be sent when the guest
>>>> resumes
>>>
>>> Want to avoid sync blocking calls as much as possible until timeouts are
>>> sorted out, particularly in this case. Currently, with hibernate at
>>> least, QEMU will exit (or is something being done to change that
>>> behavior?), libvirt will restart the VM later via some other interface,
>>> establish a connection to a new monitor and new agent channel, then gets
>>> a spurious response from the agent for a command it issue to a different
>>> QEMU process far in the past. So the common scenario would need nasty,
>>> special handling in libvirt and they'd probably hate us for it.
>>
>> Problem is: even with the other approach there's no way to guarantee that
>> the response will arrive before the guest suspends or shuts down, as it
>> depends on which process runs first.
>
>> If we want to guarantee that the client will see a response before the
>> guest suspends, then we need some form of synchronization. Like, the
>> child
>> should wait for the parent to send the response to the client before
>> exec()ing (or writing to the sysfs file).
>>
>
> There's no way to guarantee it though, even if we remove the window of
Rather, there's no way to guarantee the client will get a response (or
that, given a response, the async command will succeed).
But you're that we could make it so that time-out/lack of response at
least implies that the command *won't* be carried out...
That might be a nice guarantee, but your call on whether you want to do
the signalling approach for hibernate. Otherwise, I think async will
work well enough in the common case, and response timeout would imply
neither success nor failure of the request, which I think is the norm
for most things.
> opportunity for a child to complete a shutdown/suspend/etc request
> before we write out a response, there's still the potential scenario
> where an admin kills off/restarts qemu-ga while it's processing a request.
>
> So relying on client-side timeouts to handle these corner cases is
> acceptable, since being able to handle that is a requirement of a robust
> client, especially since we're talking to a potentially malicious
> guest/agent. We just need to do our best to make these situations corner
> cases rather than common ones.
>
>>>
>>> So probably best to stick with your previous approach for now, and look
>>> for a glib solution later.
>>>
>>>>
>>>> If you think this approach is acceptable, I'll test it more, update
>>>> its doc,
>>>> etc and post it again.
>>>>
>>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>>> index 5f8a18d..63f65a6 100644
>>>> --- a/qapi-schema-guest.json
>>>> +++ b/qapi-schema-guest.json
>>>> @@ -219,3 +219,20 @@
>>>> ##
>>>> { 'command': 'guest-fsfreeze-thaw',
>>>> 'returns': 'int' }
>>>> +
>>>> +##
>>>> +# @guest-suspend
>>>> +#
>>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>>> +#
>>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>>> +# powered down (this corresponds to ACPI S4)
>>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>>> +# (this corresponds to ACPI S3)
>>>> +#
>>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>>> +# succeed. Errors will be logged to guest's syslog.
>>>> +#
>>>> +# Since: 1.1
>>>> +##
>>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>>> index a09c8ca..0c6b78e 100644
>>>> --- a/qga/guest-agent-commands.c
>>>> +++ b/qga/guest-agent-commands.c
>>>> @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>>> }
>>>> #endif
>>>>
>>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>>> +
>>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>>> +{
>>>> + GError *error = NULL;
>>>> + gchar *argv[2];
>>>> +
>>>> + if (strcmp(mode, "hibernate") == 0) {
>>>> + argv[0] = (gchar *) "pm-hibernate";
>>>> + } else if (strcmp(mode, "sleep") == 0) {
>>>> + argv[0] = (gchar *) "pm-suspend";
>>>> + } else if (strcmp(mode, "hybrid") == 0) {
>>>> + argv[0] = (gchar *) "pm-hybrid";
>>>> + } else {
>>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>>> + return;
>>>> + }
>>>> +
>>>> + argv[1] = NULL;
>>>> + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
>>>> + G_SPAWN_FILE_AND_ARGV_ZERO,
>>>> + NULL, NULL, NULL,&error)< 0) {
>>>> + int fd;
>>>> + const char *cmd;
>>>> +
>>>> + slog("%s\n", error->message);
>>>> + g_error_free(error);
>>>> +
>>>> + if (strcmp(mode, "hybrid") == 0) {
>>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>>> + return;
>>>> + }
>>>> +
>>>> + slog("trying to suspend using the manual method...\n");
>>>> +
>>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>>> + if (fd< 0) {
>>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>>> + return;
>>>> + }
>>>> +
>>>> + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>>> + if (write(fd, cmd, strlen(cmd))< 0) {
>>>> + error_set(err, QERR_IO_ERROR);
>>>> + }
>>>> +
>>>> + close(fd);
>>>> + }
>>>> +}
>>>> +
>>>> /* register init/cleanup routines for stateful command groups */
>>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>>> {
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 18:06 ` Michael Roth
@ 2011-12-14 23:44 ` Luiz Capitulino
0 siblings, 0 replies; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 23:44 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Wed, 14 Dec 2011 12:06:13 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/14/2011 10:38 AM, Luiz Capitulino wrote:
> > On Wed, 14 Dec 2011 09:54:29 -0600
> > Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
> >
> >> On 12/14/2011 07:00 AM, Luiz Capitulino wrote:
> >>> On Tue, 13 Dec 2011 14:03:08 -0600
> >>> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
> >>>
> >>>> On 12/13/2011 12:28 PM, Luiz Capitulino wrote:
> >>>>> It supports two modes: "hibernate" (which corresponds to S4) and
> >>>>> "sleep" (which corresponds to S3). It will try to execute the
> >>>>> pm-hibernate or pm-suspend scripts, if the scripts don't exist
> >>>>> the command will try to suspend by directly writing to the
> >>>>> "/sys/power/state" file.
> >>>>>
> >>>>> An interesting implementation detail is how to cleanup the child's
> >>>>> status on termination, so that we don't create zombies. I've
> >>>>> choosen to ignore the SIGCHLD signal. This will cause the kernel to
> >>>>> automatically cleanup the child's status on its termination.
> >>>>
> >>>> One downside to blocking SIGCHLD is it can screw with child processes
> >>>> that utilize it. `sudo` for instance will just hang indefinitely because
> >>>> it handles it's own cleanup via SIGCHLD, we might run into similar cases
> >>>> with various pm-hibernate/pm-suspend implementations as well.
> >>>>
> >>>> This will also screw with anything we launch via guest-exec as well,
> >>>> once that goes in.
> >>>>
> >>>> I wonder if we can tie the qemu_add_child_watch() stuff into qemu-ga's
> >>>> main loop, or maybe just implement something similar...
> >>>>
> >>>> Basically:
> >>>>
> >>>> - add a qemu-ga.c:signal_channel_add() that creates a non-blocking
> >>>> pipe and associate the read-side with a GIOChannel, then ties the
> >>>> channel into the main loop via g_io_add_watch() on qemu-ga startup, with
> >>>> an associated callback that reads everything off the pipe, then iterates
> >>>> through a list of registered pids and does a non-blocking wait(pid, ...)
> >>>> on each.
> >>>>
> >>>> - add a SIGCHLD handler that writes a 1 to the write side of the pipe
> >>>> whenever it gets called
> >>>
> >>> Is the pipe really needed? Is there any side effect if we call waitpid()
> >>> from the signal handler considering it won't block?
> >>
> >> In the current state of things, I believe that might actually be
> >> sufficient. I was thinking of the eventual guest-exec case though: we
> >> need to be able to poll for a guest-exec'd process' return status
> >> asynchronously by calling a guest-exec-status command that does the
> >> wait(), so if we use a waitpid(-1, ...) SIGCHLD handler, or block
> >> SIGCHLD, the return status would be intercepted/lost.
> >
> > What I had in mind was to do what qemu_add_child_watch() does, ie. it
> > iterates through a list of child pids calling waitpid() on them instead
> > of doing waitpid(-1,...). The only difference is that we would do it from
> > a signal handler.
>
> Ah, yah that might work. I'm not sure how well our list functions will
> behave when accessed via an interrupt handler. We'll have race
> conditions at least:
>
> #define QTAILQ_INSERT_TAIL(head, elm, field) do { \
> (elm)->field.tqe_next = NULL; \
> (elm)->field.tqe_prev = (head)->tqh_last; \
> /* interrupt */
> *(head)->tqh_last = (elm); \
> (head)->tqh_last = &(elm)->field.tqe_next;
>
> So if we fork() and the process completes and triggers the interrupt
> before we add the pid to the list, it will remain a zombie until
> something else triggers the handler. It's not a huge deal...we at least
> avoid accumulating zombies, but I'd be weary of other side-effects as
> well, especially if we also remove entries from the list if they've been
> waited on successfully.
We can block SIGCHLD temporarily while inserting. If a new signal arrives
while it's blocked, it will be queued and will be delivered when unblocked.
> > Maybe that could work for guest-exec too: we could let the signal handler
> > collect the exit status and store them. Then guest-status could retrieve
> > the status from there.
>
> Yah, I think we can tie all this together rather nicely with a little work.
>
> >
> > We have several options here. Maybe I should do the simplest solution for
> > the guest-suspend command and you work on improving it for guest-exec.
>
> That's fine, I need to look at this more. But if we're gonna take the
> simplest approach for now, let's not even bother with the pid
> registration (and potential races), and just do a waitpid(-1, ...) in a
> SIGCHLD handler like QEMU did before the child watch stuff was added,
> and I'll make the changes necessary for the guest-exec stuff before that
> gets added.
Okay, that works for me.
>
> >
> >>>
> >>> I'm also wondering if we could use g_child_watch_add(), but it's not clear
> >>> to me if it works with processes not created with g_spawn_*() functions.
> >>
> >> GPid's map to something other than PIDs on Windows, so I think we'd have
> >> issues there. But our fork() approach probably wouldn't work at all on
> >> Windows except maybe under cygwin, so at some point we'd probably want
> >> to switch over to g_spawn for this kind of stuff anyway...
> >>
> >> So this might be a good point to switch over to using the glib functions.
> >>
> >> Would you mind trying to do the hibernate/zombie reaping stuff using
> >> g_spawn+g_child_watch_add()? It might end up being the easiest route.
> >> Otherwise I can take a look at it later today.
> >
> > Well, there are two problems with g_spawn wrt to the manual method of
> > writing to the sysfs file. The first one is that I'm not sure if g_spawn()
> > reports the file not found error synchronously. The other problem is that,
>
> "error can be NULL to ignore errors, or non-NULL to report errors. If an
> error is set, the function returns FALSE. Errors are reported even if
> they occur in the child (for example if the executable in argv[0] is not
> found). Typically the message field of returned errors should be
> displayed to users. Possible errors are those from the G_SPAWN_ERROR
> domain."
>
> So I'd think we'd be able to report the FNF synchronously even if we
> launch into the background.
Yeah, from my testing looks like it does.
>
> > I'd have to fork() anyway to write to the sysfs file (unless we decide that
> > it's ok to do this synchronously, which seems ok to me).
>
> I think what we'd have to do is open the file, then pass it in as stdout
> for a g_spawn'd `echo "disk"` command. I'm not sure how we'd manage
> cleaning up the FD though, if we're doing it asynchronously. And
> synchronous means every guest-suspend call will eventually time out, so
> I don't think that's doable right now.
>
> So I'd say keep it the way it is for now. Probably not worth spending
> too much time on until we see what the Windows stuff looks like anyway.
Ok.
>
> >
> >>
> >>>
> >>>>
> >>>> Then, when creating a child, you just register the child by adding the
> >>>> pid to the list that the signal channel callback checks.
> >>>>
> >>>>>
> >>>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >>>>> ---
> >>>>>
> >>>>> I've tested this w/o any virtio driver, as they don't support S4 yet. For
> >>>>> S4 it seems to work ok. I couldn't fully test S3 because we lack a way to
> >>>>> resume from it, but by checking the logs it seems to work fine.
> >>>>>
> >>>>> changelog
> >>>>> ---------
> >>>>>
> >>>>> v2
> >>>>>
> >>>>> o Rename the command to 'guest-suspend'
> >>>>> o Add 'mode' parameter
> >>>>> o Use pm-utils scripts
> >>>>> o Cleanup child termination status
> >>>>>
> >>>>> qapi-schema-guest.json | 17 +++++++++++
> >>>>> qemu-ga.c | 11 +++++++-
> >>>>> qga/guest-agent-commands.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>> 3 files changed, 91 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> >>>>> index 29989fe..656bde9 100644
> >>>>> --- a/qapi-schema-guest.json
> >>>>> +++ b/qapi-schema-guest.json
> >>>>> @@ -219,3 +219,20 @@
> >>>>> ##
> >>>>> { 'command': 'guest-fsfreeze-thaw',
> >>>>> 'returns': 'int' }
> >>>>> +
> >>>>> +##
> >>>>> +# @guest-suspend
> >>>>> +#
> >>>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
> >>>>> +#
> >>>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> >>>>> +# powered down (this corresponds to ACPI S4)
> >>>>> +# 'sleep' execution is suspended but the RAM retains its contents
> >>>>> +# (this corresponds to ACPI S3)
> >>>>> +#
> >>>>> +# Notes: This is an asynchronous request. There's no guarantee it will
> >>>>> +# succeed. Errors will be logged to guest's syslog.
> >>>>> +#
> >>>>> +# Since: 1.1
> >>>>> +##
> >>>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> >>>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>>> index 60d4972..b32e96c 100644
> >>>>> --- a/qemu-ga.c
> >>>>> +++ b/qemu-ga.c
> >>>>> @@ -61,7 +61,7 @@ static void quit_handler(int sig)
> >>>>>
> >>>>> static void register_signal_handlers(void)
> >>>>> {
> >>>>> - struct sigaction sigact;
> >>>>> + struct sigaction sigact, sigact_chld;
> >>>>> int ret;
> >>>>>
> >>>>> memset(&sigact, 0, sizeof(struct sigaction));
> >>>>> @@ -76,6 +76,15 @@ static void register_signal_handlers(void)
> >>>>> if (ret == -1) {
> >>>>> g_error("error configuring signal handler: %s", strerror(errno));
> >>>>> }
> >>>>> +
> >>>>> + /* This should cause the kernel to automatically cleanup child
> >>>>> + termination status */
> >>>>> + memset(&sigact_chld, 0, sizeof(struct sigaction));
> >>>>> + sigact_chld.sa_handler = SIG_IGN;
> >>>>> + ret = sigaction(SIGCHLD,&sigact_chld, NULL);
> >>>>> + if (ret == -1) {
> >>>>> + g_error("error configuring signal handler: %s", strerror(errno));
> >>>>> + }
> >>>>> }
> >>>>>
> >>>>> static void usage(const char *cmd)
> >>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >>>>> index a09c8ca..4799638 100644
> >>>>> --- a/qga/guest-agent-commands.c
> >>>>> +++ b/qga/guest-agent-commands.c
> >>>>> @@ -574,6 +574,70 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >>>>> }
> >>>>> #endif
> >>>>>
> >>>>> +#define LINUX_PM_UTILS_PATH "/usr/sbin"
> >>>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> >>>>> +
> >>>>> +void qmp_guest_suspend(const char *mode, Error **err)
> >>>>> +{
> >>>>> + int ret, fd = -1;
> >>>>> + const char *pmutils_bin;
> >>>>> + char pmutils_bin_path[PATH_MAX];
> >>>>> +
> >>>>> + if (strcmp(mode, "hibernate") == 0) {
> >>>>> + pmutils_bin = "pm-hibernate";
> >>>>> + } else if (strcmp(mode, "sleep") == 0) {
> >>>>> + pmutils_bin = "pm-suspend";
> >>>>> + } else {
> >>>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
> >>>>> + return;
> >>>>> + }
> >>>>> +
> >>>>> + snprintf(pmutils_bin_path, sizeof(pmutils_bin_path), "%s/%s",
> >>>>> + LINUX_PM_UTILS_PATH, pmutils_bin);
> >>>>> +
> >>>>> + if (access(pmutils_bin_path, X_OK) != 0) {
> >>>>> + pmutils_bin = NULL;
> >>>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> >>>>> + if (fd< 0) {
> >>>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> >>>>> + return;
> >>>>> + }
> >>>>> + }
> >>>>> +
> >>>>> + ret = fork();
> >>>>> + if (ret == 0) {
> >>>>> + /* child */
> >>>>> + setsid();
> >>>>> + fclose(stdin);
> >>>>> + fclose(stdout);
> >>>>> + fclose(stderr);
> >>>>> +
> >>>>> + if (pmutils_bin) {
> >>>>> + ret = execl(pmutils_bin_path, pmutils_bin, NULL);
> >>>>> + if (ret) {
> >>>>> + slog("%s failed: %s", pmutils_bin_path, strerror(errno));
> >>>>> + }
> >>>>> + } else {
> >>>>> + const char *cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> >>>>> + ret = write(fd, cmd, strlen(cmd));
> >>>>> + if (ret< 0) {
> >>>>> + slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> >>>>> + strerror(errno));
> >>>>> + }
> >>>>> + close(fd);
> >>>>> + }
> >>>>> +
> >>>>> + exit(!!ret);
> >>>>> + } else if (ret< 0) {
> >>>>> + error_set(err, QERR_UNDEFINED_ERROR);
> >>>>> + return;
> >>>>> + }
> >>>>> +
> >>>>> + if (!pmutils_bin) {
> >>>>> + close(fd);
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> /* register init/cleanup routines for stateful command groups */
> >>>>> void ga_command_state_init(GAState *s, GACommandState *cs)
> >>>>> {
> >>>>
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 21:14 ` Michael Roth
@ 2011-12-14 23:56 ` Luiz Capitulino
2011-12-15 1:27 ` Michael Roth
0 siblings, 1 reply; 19+ messages in thread
From: Luiz Capitulino @ 2011-12-14 23:56 UTC (permalink / raw)
To: Michael Roth; +Cc: Amit Shah, aliguori, qemu-devel
On Wed, 14 Dec 2011 15:14:40 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 12/14/2011 02:56 PM, Michael Roth wrote:
> > On 12/14/2011 02:06 PM, Luiz Capitulino wrote:
> >> On Wed, 14 Dec 2011 13:43:23 -0600
> >> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
> >>
> >>> On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
> >>>> On Wed, 14 Dec 2011 14:38:55 -0200
> >>>> Luiz Capitulino<lcapitulino@redhat.com> wrote:
> >>>>
> >>>>>>> I'm also wondering if we could use g_child_watch_add(), but it's
> >>>>>>> not clear
> >>>>>>> to me if it works with processes not created with g_spawn_*()
> >>>>>>> functions.
> >>>>>>
> >>>>>> GPid's map to something other than PIDs on Windows, so I think
> >>>>>> we'd have
> >>>>>> issues there. But our fork() approach probably wouldn't work at
> >>>>>> all on
> >>>>>> Windows except maybe under cygwin, so at some point we'd probably
> >>>>>> want
> >>>>>> to switch over to g_spawn for this kind of stuff anyway...
> >>>>>>
> >>>>>> So this might be a good point to switch over to using the glib
> >>>>>> functions.
> >>>>>>
> >>>>>> Would you mind trying to do the hibernate/zombie reaping stuff using
> >>>>>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
> >>>>>> Otherwise I can take a look at it later today.
> >>>>>
> >>>>> Well, there are two problems with g_spawn wrt to the manual method of
> >>>>> writing to the sysfs file. The first one is that I'm not sure if
> >>>>> g_spawn()
> >>>>> reports the file not found error synchronously. The other problem
> >>>>> is that,
> >>>>> I'd have to fork() anyway to write to the sysfs file (unless we
> >>>>> decide that
> >>>>> it's ok to do this synchronously, which seems ok to me).
> >>>>
> >>>> The version below uses g_spawn_async(). The code is a bit simpler
> >>>> than previous
> >>>> versions, but there are two important details about it:
> >>>>
> >>>> 1. I'm letting g_spawn_async() reap the child automatically. I don't
> >>>> know
> >>>> how it does it though. I'd guess it uses g_child_watch_add(), worst
> >>>> case it ignores SIGCHLD (although I think this would be awful)
> >>>
> >>> Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
> >>> not seeing an error that's causing it to reap it immediately after
> >>> the fork?
> >>
> >> No. I haven't tested it much though, but the basic use case seemed to
> >> work
> >> fine.
> >>
> >>>>
> >>>> 2. The manual method of writing to the sysfs is done synchronously.
> >>>> This
> >>>> means that the command response will only be sent when the guest
> >>>> resumes
> >>>
> >>> Want to avoid sync blocking calls as much as possible until timeouts are
> >>> sorted out, particularly in this case. Currently, with hibernate at
> >>> least, QEMU will exit (or is something being done to change that
> >>> behavior?), libvirt will restart the VM later via some other interface,
> >>> establish a connection to a new monitor and new agent channel, then gets
> >>> a spurious response from the agent for a command it issue to a different
> >>> QEMU process far in the past. So the common scenario would need nasty,
> >>> special handling in libvirt and they'd probably hate us for it.
> >>
> >> Problem is: even with the other approach there's no way to guarantee that
> >> the response will arrive before the guest suspends or shuts down, as it
> >> depends on which process runs first.
> >
> >> If we want to guarantee that the client will see a response before the
> >> guest suspends, then we need some form of synchronization. Like, the
> >> child
> >> should wait for the parent to send the response to the client before
> >> exec()ing (or writing to the sysfs file).
> >>
> >
> > There's no way to guarantee it though, even if we remove the window of
>
> Rather, there's no way to guarantee the client will get a response (or
> that, given a response, the async command will succeed).
Well, it's the best we can do, ie. only allow the child to run when the
parent is done sending the response. For the error case, we don't report
errors in the child back to the client anyway, we do it via syslog.
> But you're that we could make it so that time-out/lack of response at
> least implies that the command *won't* be carried out...
>
> That might be a nice guarantee, but your call on whether you want to do
> the signalling approach for hibernate. Otherwise, I think async will
> work well enough in the common case, and response timeout would imply
> neither success nor failure of the request, which I think is the norm
> for most things.
Yes, I think the signaling approach is complex and I'm not sure we really
we have a problem here. Maybe I should do it async for now, if this turns
out to be a problem for libvirt we can try more complex approaches.
>
> > opportunity for a child to complete a shutdown/suspend/etc request
> > before we write out a response, there's still the potential scenario
> > where an admin kills off/restarts qemu-ga while it's processing a request.
> >
> > So relying on client-side timeouts to handle these corner cases is
> > acceptable, since being able to handle that is a requirement of a robust
> > client, especially since we're talking to a potentially malicious
> > guest/agent. We just need to do our best to make these situations corner
> > cases rather than common ones.
> >
> >>>
> >>> So probably best to stick with your previous approach for now, and look
> >>> for a glib solution later.
> >>>
> >>>>
> >>>> If you think this approach is acceptable, I'll test it more, update
> >>>> its doc,
> >>>> etc and post it again.
> >>>>
> >>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> >>>> index 5f8a18d..63f65a6 100644
> >>>> --- a/qapi-schema-guest.json
> >>>> +++ b/qapi-schema-guest.json
> >>>> @@ -219,3 +219,20 @@
> >>>> ##
> >>>> { 'command': 'guest-fsfreeze-thaw',
> >>>> 'returns': 'int' }
> >>>> +
> >>>> +##
> >>>> +# @guest-suspend
> >>>> +#
> >>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
> >>>> +#
> >>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
> >>>> +# powered down (this corresponds to ACPI S4)
> >>>> +# 'sleep' execution is suspended but the RAM retains its contents
> >>>> +# (this corresponds to ACPI S3)
> >>>> +#
> >>>> +# Notes: This is an asynchronous request. There's no guarantee it will
> >>>> +# succeed. Errors will be logged to guest's syslog.
> >>>> +#
> >>>> +# Since: 1.1
> >>>> +##
> >>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> >>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >>>> index a09c8ca..0c6b78e 100644
> >>>> --- a/qga/guest-agent-commands.c
> >>>> +++ b/qga/guest-agent-commands.c
> >>>> @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >>>> }
> >>>> #endif
> >>>>
> >>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> >>>> +
> >>>> +void qmp_guest_suspend(const char *mode, Error **err)
> >>>> +{
> >>>> + GError *error = NULL;
> >>>> + gchar *argv[2];
> >>>> +
> >>>> + if (strcmp(mode, "hibernate") == 0) {
> >>>> + argv[0] = (gchar *) "pm-hibernate";
> >>>> + } else if (strcmp(mode, "sleep") == 0) {
> >>>> + argv[0] = (gchar *) "pm-suspend";
> >>>> + } else if (strcmp(mode, "hybrid") == 0) {
> >>>> + argv[0] = (gchar *) "pm-hybrid";
> >>>> + } else {
> >>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
> >>>> + return;
> >>>> + }
> >>>> +
> >>>> + argv[1] = NULL;
> >>>> + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
> >>>> + G_SPAWN_FILE_AND_ARGV_ZERO,
> >>>> + NULL, NULL, NULL,&error)< 0) {
> >>>> + int fd;
> >>>> + const char *cmd;
> >>>> +
> >>>> + slog("%s\n", error->message);
> >>>> + g_error_free(error);
> >>>> +
> >>>> + if (strcmp(mode, "hybrid") == 0) {
> >>>> + error_set(err, QERR_UNDEFINED_ERROR);
> >>>> + return;
> >>>> + }
> >>>> +
> >>>> + slog("trying to suspend using the manual method...\n");
> >>>> +
> >>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> >>>> + if (fd< 0) {
> >>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
> >>>> + return;
> >>>> + }
> >>>> +
> >>>> + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> >>>> + if (write(fd, cmd, strlen(cmd))< 0) {
> >>>> + error_set(err, QERR_IO_ERROR);
> >>>> + }
> >>>> +
> >>>> + close(fd);
> >>>> + }
> >>>> +}
> >>>> +
> >>>> /* register init/cleanup routines for stateful command groups */
> >>>> void ga_command_state_init(GAState *s, GACommandState *cs)
> >>>> {
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command
2011-12-14 23:56 ` Luiz Capitulino
@ 2011-12-15 1:27 ` Michael Roth
0 siblings, 0 replies; 19+ messages in thread
From: Michael Roth @ 2011-12-15 1:27 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Amit Shah, aliguori, qemu-devel
On 12/14/2011 05:56 PM, Luiz Capitulino wrote:
> On Wed, 14 Dec 2011 15:14:40 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>
>> On 12/14/2011 02:56 PM, Michael Roth wrote:
>>> On 12/14/2011 02:06 PM, Luiz Capitulino wrote:
>>>> On Wed, 14 Dec 2011 13:43:23 -0600
>>>> Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>>>>
>>>>> On 12/14/2011 12:17 PM, Luiz Capitulino wrote:
>>>>>> On Wed, 14 Dec 2011 14:38:55 -0200
>>>>>> Luiz Capitulino<lcapitulino@redhat.com> wrote:
>>>>>>
>>>>>>>>> I'm also wondering if we could use g_child_watch_add(), but it's
>>>>>>>>> not clear
>>>>>>>>> to me if it works with processes not created with g_spawn_*()
>>>>>>>>> functions.
>>>>>>>>
>>>>>>>> GPid's map to something other than PIDs on Windows, so I think
>>>>>>>> we'd have
>>>>>>>> issues there. But our fork() approach probably wouldn't work at
>>>>>>>> all on
>>>>>>>> Windows except maybe under cygwin, so at some point we'd probably
>>>>>>>> want
>>>>>>>> to switch over to g_spawn for this kind of stuff anyway...
>>>>>>>>
>>>>>>>> So this might be a good point to switch over to using the glib
>>>>>>>> functions.
>>>>>>>>
>>>>>>>> Would you mind trying to do the hibernate/zombie reaping stuff using
>>>>>>>> g_spawn+g_child_watch_add()? It might end up being the easiest route.
>>>>>>>> Otherwise I can take a look at it later today.
>>>>>>>
>>>>>>> Well, there are two problems with g_spawn wrt to the manual method of
>>>>>>> writing to the sysfs file. The first one is that I'm not sure if
>>>>>>> g_spawn()
>>>>>>> reports the file not found error synchronously. The other problem
>>>>>>> is that,
>>>>>>> I'd have to fork() anyway to write to the sysfs file (unless we
>>>>>>> decide that
>>>>>>> it's ok to do this synchronously, which seems ok to me).
>>>>>>
>>>>>> The version below uses g_spawn_async(). The code is a bit simpler
>>>>>> than previous
>>>>>> versions, but there are two important details about it:
>>>>>>
>>>>>> 1. I'm letting g_spawn_async() reap the child automatically. I don't
>>>>>> know
>>>>>> how it does it though. I'd guess it uses g_child_watch_add(), worst
>>>>>> case it ignores SIGCHLD (although I think this would be awful)
>>>>>
>>>>> Weird, doesn't seem to add a watcher or do anything with SIGCHLD. You're
>>>>> not seeing an error that's causing it to reap it immediately after
>>>>> the fork?
>>>>
>>>> No. I haven't tested it much though, but the basic use case seemed to
>>>> work
>>>> fine.
>>>>
>>>>>>
>>>>>> 2. The manual method of writing to the sysfs is done synchronously.
>>>>>> This
>>>>>> means that the command response will only be sent when the guest
>>>>>> resumes
>>>>>
>>>>> Want to avoid sync blocking calls as much as possible until timeouts are
>>>>> sorted out, particularly in this case. Currently, with hibernate at
>>>>> least, QEMU will exit (or is something being done to change that
>>>>> behavior?), libvirt will restart the VM later via some other interface,
>>>>> establish a connection to a new monitor and new agent channel, then gets
>>>>> a spurious response from the agent for a command it issue to a different
>>>>> QEMU process far in the past. So the common scenario would need nasty,
>>>>> special handling in libvirt and they'd probably hate us for it.
>>>>
>>>> Problem is: even with the other approach there's no way to guarantee that
>>>> the response will arrive before the guest suspends or shuts down, as it
>>>> depends on which process runs first.
>>>
>>>> If we want to guarantee that the client will see a response before the
>>>> guest suspends, then we need some form of synchronization. Like, the
>>>> child
>>>> should wait for the parent to send the response to the client before
>>>> exec()ing (or writing to the sysfs file).
>>>>
>>>
>>> There's no way to guarantee it though, even if we remove the window of
>>
>> Rather, there's no way to guarantee the client will get a response (or
>> that, given a response, the async command will succeed).
>
> Well, it's the best we can do, ie. only allow the child to run when the
> parent is done sending the response. For the error case, we don't report
> errors in the child back to the client anyway, we do it via syslog.
>
>> But you're that we could make it so that time-out/lack of response at
>> least implies that the command *won't* be carried out...
>>
>> That might be a nice guarantee, but your call on whether you want to do
>> the signalling approach for hibernate. Otherwise, I think async will
>> work well enough in the common case, and response timeout would imply
>> neither success nor failure of the request, which I think is the norm
>> for most things.
>
> Yes, I think the signaling approach is complex and I'm not sure we really
> we have a problem here. Maybe I should do it async for now, if this turns
> out to be a problem for libvirt we can try more complex approaches.
>
Agreed, it reduces corner-cases, but still requires similar
error-handling in place, so it's a good candidate for treating as a
future optimization. Although...
>>
>>> opportunity for a child to complete a shutdown/suspend/etc request
>>> before we write out a response, there's still the potential scenario
>>> where an admin kills off/restarts qemu-ga while it's processing a request.
>>>
>>> So relying on client-side timeouts to handle these corner cases is
>>> acceptable, since being able to handle that is a requirement of a robust
>>> client, especially since we're talking to a potentially malicious
>>> guest/agent. We just need to do our best to make these situations corner
>>> cases rather than common ones.
Anthony rightly pointed out on IRC that punting the problem to a
client-side timeout still requires a reset mechanism in the case of
suspend/hibernate, since, although it won't happen every single time as
it would with a synchronous call, we can still send a spurious response
when the guest wakes up (if we slept before delivery).
So the libvirt implementation will need to do a reset every every time
they reconnect after waking up a suspended/hibernated guest. It should
also be part of the general timeout logic, since there's no guarantee
the timeout wasn't premature and a spurious response won't still arrive.
I'll get the logic for doing qemu-ga resets added to the wiki... it's
not pretty, and I was hoping to hide it away in the QMP integration
since QEMU's JSON parser is already prepped to handle the reserved 0xFF
flush token that's required...but it works at least.
>>>
>>>>>
>>>>> So probably best to stick with your previous approach for now, and look
>>>>> for a glib solution later.
>>>>>
>>>>>>
>>>>>> If you think this approach is acceptable, I'll test it more, update
>>>>>> its doc,
>>>>>> etc and post it again.
>>>>>>
>>>>>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>>>>>> index 5f8a18d..63f65a6 100644
>>>>>> --- a/qapi-schema-guest.json
>>>>>> +++ b/qapi-schema-guest.json
>>>>>> @@ -219,3 +219,20 @@
>>>>>> ##
>>>>>> { 'command': 'guest-fsfreeze-thaw',
>>>>>> 'returns': 'int' }
>>>>>> +
>>>>>> +##
>>>>>> +# @guest-suspend
>>>>>> +#
>>>>>> +# Suspend guest execution by entering ACPI power state S3 or S4.
>>>>>> +#
>>>>>> +# @mode: 'hibernate' RAM content is saved in the disk and the guest is
>>>>>> +# powered down (this corresponds to ACPI S4)
>>>>>> +# 'sleep' execution is suspended but the RAM retains its contents
>>>>>> +# (this corresponds to ACPI S3)
>>>>>> +#
>>>>>> +# Notes: This is an asynchronous request. There's no guarantee it will
>>>>>> +# succeed. Errors will be logged to guest's syslog.
>>>>>> +#
>>>>>> +# Since: 1.1
>>>>>> +##
>>>>>> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
>>>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>>>>> index a09c8ca..0c6b78e 100644
>>>>>> --- a/qga/guest-agent-commands.c
>>>>>> +++ b/qga/guest-agent-commands.c
>>>>>> @@ -574,6 +574,56 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>>>>> }
>>>>>> #endif
>>>>>>
>>>>>> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
>>>>>> +
>>>>>> +void qmp_guest_suspend(const char *mode, Error **err)
>>>>>> +{
>>>>>> + GError *error = NULL;
>>>>>> + gchar *argv[2];
>>>>>> +
>>>>>> + if (strcmp(mode, "hibernate") == 0) {
>>>>>> + argv[0] = (gchar *) "pm-hibernate";
>>>>>> + } else if (strcmp(mode, "sleep") == 0) {
>>>>>> + argv[0] = (gchar *) "pm-suspend";
>>>>>> + } else if (strcmp(mode, "hybrid") == 0) {
>>>>>> + argv[0] = (gchar *) "pm-hybrid";
>>>>>> + } else {
>>>>>> + error_set(err, QERR_INVALID_PARAMETER, "mode");
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + argv[1] = NULL;
>>>>>> + if (g_spawn_async(NULL, argv, NULL,G_SPAWN_SEARCH_PATH |
>>>>>> + G_SPAWN_FILE_AND_ARGV_ZERO,
>>>>>> + NULL, NULL, NULL,&error)< 0) {
>>>>>> + int fd;
>>>>>> + const char *cmd;
>>>>>> +
>>>>>> + slog("%s\n", error->message);
>>>>>> + g_error_free(error);
>>>>>> +
>>>>>> + if (strcmp(mode, "hybrid") == 0) {
>>>>>> + error_set(err, QERR_UNDEFINED_ERROR);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + slog("trying to suspend using the manual method...\n");
>>>>>> +
>>>>>> + fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
>>>>>> + if (fd< 0) {
>>>>>> + error_set(err, QERR_OPEN_FILE_FAILED, LINUX_SYS_STATE_FILE);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
>>>>>> + if (write(fd, cmd, strlen(cmd))< 0) {
>>>>>> + error_set(err, QERR_IO_ERROR);
>>>>>> + }
>>>>>> +
>>>>>> + close(fd);
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> /* register init/cleanup routines for stateful command groups */
>>>>>> void ga_command_state_init(GAState *s, GACommandState *cs)
>>>>>> {
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-12-15 1:27 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-13 18:28 [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command Luiz Capitulino
2011-12-13 20:03 ` Michael Roth
2011-12-14 13:00 ` Luiz Capitulino
2011-12-14 15:54 ` Michael Roth
2011-12-14 16:38 ` Luiz Capitulino
2011-12-14 18:06 ` Michael Roth
2011-12-14 23:44 ` Luiz Capitulino
2011-12-14 18:17 ` Luiz Capitulino
2011-12-14 19:43 ` Michael Roth
2011-12-14 20:06 ` Luiz Capitulino
2011-12-14 20:56 ` Michael Roth
2011-12-14 21:14 ` Michael Roth
2011-12-14 23:56 ` Luiz Capitulino
2011-12-15 1:27 ` Michael Roth
2011-12-13 20:27 ` Michael Roth
2011-12-14 13:07 ` Luiz Capitulino
2011-12-14 15:50 ` Michael Roth
2011-12-13 23:13 ` Daniel P. Berrange
2011-12-14 13:08 ` Luiz Capitulino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.