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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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)
>   {