On Tue, 13 Dec 2011 14:27:56 -0600
Michael Roth <mdr...@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<lcapitul...@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)
> > {
>
