Re: [Qemu-devel] [PATCH v2] qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

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