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

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

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