[2/2] qemu-ga: Add the guest-suspend command

For now it only supports the "hibernate" mode, which suspends the
guest to disk.

This command will try to execute the scripts provided by the pm-utils
package. If that fails, it will try to suspend manually by writing
to the "/sys/power/state" file.

To reap terminated children, a new signal handler is installed to
catch SIGCHLD signals and a non-blocking call to waitpid() is done to
collect their exit statuses.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema-guest.json     |   23 ++++++++++++++++++
 qemu-ga.c                  |   17 ++++++++++++-
 qga/guest-agent-commands.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 1 deletions(-)

On 01/04/2012 01:45 PM, Luiz Capitulino wrote:
> For now it only supports the "hibernate" mode, which suspends the
> guest to disk.
>
> This command will try to execute the scripts provided by the pm-utils
> package. If that fails, it will try to suspend manually by writing
> to the "/sys/power/state" file.
>
> To reap terminated children, a new signal handler is installed to
> catch SIGCHLD signals and a non-blocking call to waitpid() is done to
> collect their exit statuses.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>

Looks good.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>   qapi-schema-guest.json     |   23 ++++++++++++++++++
>   qemu-ga.c                  |   17 ++++++++++++-
>   qga/guest-agent-commands.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 94 insertions(+), 1 deletions(-)
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 5f8a18d..b151670 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,26 @@
>   ##
>   { 'command': 'guest-fsfreeze-thaw',
>     'returns': 'int' }
> +
> +##
> +# @guest-suspend
> +#
> +# Suspend guest execution by changing the guest's ACPI power state.
> +#
> +# This command tries to execute the scripts provided by the pm-utils
> +# package. If they are not available, it will perform the suspend
> +# operation by manually writing to a sysfs file.
> +#
> +# For the best results it's strongly recommended to have the pm-utils
> +# package installed in the guest.
> +#
> +# @mode: 'hibernate' RAM content is saved to the disk and the guest is
> +#                    powered off (this corresponds to ACPI S4)
> +#
> +# Notes: This is an asynchronous request. There's no guarantee a response
> +# will be sent. Errors will be logged to guest's syslog. More modes are
> +# expected in the future.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 98e4dfe..5b7a7a5 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -17,6 +17,7 @@
>   #include<getopt.h>
>   #include<termios.h>
>   #include<syslog.h>
> +#include<sys/wait.h>
>   #include "qemu_socket.h"
>   #include "json-streamer.h"
>   #include "json-parser.h"
> @@ -59,9 +60,15 @@ static void quit_handler(int sig)
>       }
>   }
>
> +static void child_handler(int sig)
> +{
> +    int status;
> +    waitpid(-1,&status, WNOHANG);
> +}
> +
>   static void register_signal_handlers(void)
>   {
> -    struct sigaction sigact;
> +    struct sigaction sigact, sigact_chld;
>       int ret;
>
>       memset(&sigact, 0, sizeof(struct sigaction));
> @@ -76,6 +83,14 @@ static void register_signal_handlers(void)
>       if (ret == -1) {
>           g_error("error configuring signal handler: %s", strerror(errno));
>       }
> +
> +    memset(&sigact_chld, 0, sizeof(struct sigaction));
> +    sigact_chld.sa_handler = child_handler;
> +    sigact_chld.sa_flags = SA_NOCLDSTOP;
> +    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..19f29c6 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -574,6 +574,61 @@ 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)
> +{
> +    pid_t pid;
> +    const char *pmutils_bin;
> +
> +    /* TODO implement 'sleep' and 'hybrid' modes once qemu is fixed to
> +       support them */
> +    if (strcmp(mode, "hibernate") == 0) {
> +        pmutils_bin = "pm-hibernate";
> +    } else {
> +        error_set(err, QERR_INVALID_PARAMETER, "mode");
> +        return;
> +    }
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        /* child */
> +        int fd;
> +
> +        setsid();
> +        fclose(stdin);
> +        fclose(stdout);
> +        fclose(stderr);
> +
> +        execlp(pmutils_bin, pmutils_bin, NULL);
> +
> +        /*
> +         * The exec call should not return, if it does something went wrong.
> +         * In this case we try to suspend manually if 'mode' is 'hibernate'
> +         */
> +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> +        slog("trying to suspend using the manual method...\n");
> +
> +        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> +        if (fd<  0) {
> +            slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE,
> +                    strerror(errno));
> +            exit(1);
> +        }
> +
> +        if (write(fd, "disk", 4)<  0) {
> +            slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> +                    strerror(errno));
> +            exit(1);
> +        }
> +
> +        exit(0);
> +    } else if (pid<  0) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +        return;
> +    }
> +}
> +
>   /* register init/cleanup routines for stateful command groups */
>   void ga_command_state_init(GAState *s, GACommandState *cs)
>   {

On 01/04/2012 12:45 PM, Luiz Capitulino wrote:
> +    if (pid == 0) {
> +        /* child */
> +        int fd;
> +
> +        setsid();
> +        fclose(stdin);
> +        fclose(stdout);
> +        fclose(stderr);
> +
> +        execlp(pmutils_bin, pmutils_bin, NULL);

It's generally a bad idea to exec a child process without fd 0, 1, and 2
open on something, even if that something is /dev/null.  POSIX says that
the system may, but not must, reopen fds on your behalf, and that the
child without open std descriptors is then executing in a non-conforming
environment and may misbehave in unexpected manners.

> +
> +        /* 
> +         * The exec call should not return, if it does something went wrong.
> +         * In this case we try to suspend manually if 'mode' is 'hibernate'
> +         */
> +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> +        slog("trying to suspend using the manual method...\n");
> +
> +        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);

Worse, since you _just_ closed stdin above, fd here will most likely be
0, but a O_WRONLY stdin is asking for problems.

> +        if (fd < 0) {
> +            slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE,
> +                    strerror(errno));

Also, I have no idea where slog() writes to, but since you closed
stderr, if slog() is trying to use stderr, your error messages would be
invisible.

On Wed, 04 Jan 2012 13:03:26 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 01/04/2012 12:45 PM, Luiz Capitulino wrote:
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +
> > +        setsid();
> > +        fclose(stdin);
> > +        fclose(stdout);
> > +        fclose(stderr);
> > +
> > +        execlp(pmutils_bin, pmutils_bin, NULL);
> 
> It's generally a bad idea to exec a child process without fd 0, 1, and 2
> open on something, even if that something is /dev/null.  POSIX says that
> the system may, but not must, reopen fds on your behalf, and that the
> child without open std descriptors is then executing in a non-conforming
> environment and may misbehave in unexpected manners.

You're right. I just copied what we do in qmp_guest_shutdown()... I think we
have to open /dev/null then, do you agree Michael?

I think I'm doing to use dup2(), like dup2(dev_null_fd, 0). Any better
recommendation?

> 
> > +
> > +        /* 
> > +         * The exec call should not return, if it does something went wrong.
> > +         * In this case we try to suspend manually if 'mode' is 'hibernate'
> > +         */
> > +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> > +        slog("trying to suspend using the manual method...\n");
> > +
> > +        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> 
> Worse, since you _just_ closed stdin above, fd here will most likely be
> 0, but a O_WRONLY stdin is asking for problems.
> 
> > +        if (fd < 0) {
> > +            slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE,
> > +                    strerror(errno));
> 
> Also, I have no idea where slog() writes to, but since you closed
> stderr, if slog() is trying to use stderr, your error messages would be
> invisible.
>

On Wed, Jan 04, 2012 at 05:45:13PM -0200, Luiz Capitulino wrote:
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..19f29c6 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -574,6 +574,61 @@ 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)
> +{
> +    pid_t pid;
> +    const char *pmutils_bin;
> +
> +    /* TODO implement 'sleep' and 'hybrid' modes once qemu is fixed to
> +       support them */
> +    if (strcmp(mode, "hibernate") == 0) {
> +        pmutils_bin = "pm-hibernate";
> +    } else {
> +        error_set(err, QERR_INVALID_PARAMETER, "mode");
> +        return;
> +    }
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        /* child */
> +        int fd;
> +
> +        setsid();
> +        fclose(stdin);
> +        fclose(stdout);
> +        fclose(stderr);
> +
> +        execlp(pmutils_bin, pmutils_bin, NULL);

Strictly speaking, in multi-threaded programs, between fork() and
exec() you are restricted to calling functions which are marked as
async-signal safe in POSIX spec - fclose() is not.

Also, if there was unflushed buffered output on stdout, calling
fclose() in the child will flush that output, but then the parent
process will also flush it some time later, causing duplicated
stdout data.

NB, you might not think qemu-ga is multi-threaded, but depending on
which GLib APIs you're calling, you might find you are in fact using
threads behind the scenes without realizing, so I think it is wise
to be conservative here & assume threads are possible.

Thus you really want to use a plain old 'close()' call, and then
re-open to /dev/null as Eric suggests, leaving stdin/out/err FILE*
alone.

> +
> +        /* 
> +         * The exec call should not return, if it does something went wrong.
> +         * In this case we try to suspend manually if 'mode' is 'hibernate'
> +         */
> +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> +        slog("trying to suspend using the manual method...\n");
> +
> +        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> +        if (fd < 0) {
> +            slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE,
> +                    strerror(errno));
> +            exit(1);
> +        }
> +
> +        if (write(fd, "disk", 4) < 0) {
> +            slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> +                    strerror(errno));
> +            exit(1);
> +        }
> +
> +        exit(0);

exit() is also not async-signal safe, because it calls into stdio
to flush  buffers. So you want to use _exit() instead for these.

The impl of slog() doesn't look too safe to me either.

Regards,
Daniel

On Thu, 5 Jan 2012 12:46:56 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jan 04, 2012 at 05:45:13PM -0200, Luiz Capitulino wrote:
> > diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> > index a09c8ca..19f29c6 100644
> > --- a/qga/guest-agent-commands.c
> > +++ b/qga/guest-agent-commands.c
> > @@ -574,6 +574,61 @@ 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)
> > +{
> > +    pid_t pid;
> > +    const char *pmutils_bin;
> > +
> > +    /* TODO implement 'sleep' and 'hybrid' modes once qemu is fixed to
> > +       support them */
> > +    if (strcmp(mode, "hibernate") == 0) {
> > +        pmutils_bin = "pm-hibernate";
> > +    } else {
> > +        error_set(err, QERR_INVALID_PARAMETER, "mode");
> > +        return;
> > +    }
> > +
> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +
> > +        setsid();
> > +        fclose(stdin);
> > +        fclose(stdout);
> > +        fclose(stderr);
> > +
> > +        execlp(pmutils_bin, pmutils_bin, NULL);
> 
> Strictly speaking, in multi-threaded programs, between fork() and
> exec() you are restricted to calling functions which are marked as
> async-signal safe in POSIX spec - fclose() is not.
> 
> Also, if there was unflushed buffered output on stdout, calling
> fclose() in the child will flush that output, but then the parent
> process will also flush it some time later, causing duplicated
> stdout data.
> 
> NB, you might not think qemu-ga is multi-threaded, but depending on
> which GLib APIs you're calling, you might find you are in fact using
> threads behind the scenes without realizing, so I think it is wise
> to be conservative here & assume threads are possible.

All good points.

> Thus you really want to use a plain old 'close()' call, and then
> re-open to /dev/null as Eric suggests, leaving stdin/out/err FILE*
> alone.

I'm going to use dup2(), which seems to be ok in that regard.

> 
> > +
> > +        /* 
> > +         * The exec call should not return, if it does something went wrong.
> > +         * In this case we try to suspend manually if 'mode' is 'hibernate'
> > +         */
> > +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> > +        slog("trying to suspend using the manual method...\n");
> > +
> > +        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
> > +        if (fd < 0) {
> > +            slog("can't open file %s: %s\n", LINUX_SYS_STATE_FILE,
> > +                    strerror(errno));
> > +            exit(1);
> > +        }
> > +
> > +        if (write(fd, "disk", 4) < 0) {
> > +            slog("can't write to %s: %s\n", LINUX_SYS_STATE_FILE,
> > +                    strerror(errno));
> > +            exit(1);
> > +        }
> > +
> > +        exit(0);
> 
> exit() is also not async-signal safe, because it calls into stdio
> to flush  buffers. So you want to use _exit() instead for these.

Ok, I'll change and will fix qmp_guest_shutdown() in a different patch.

> 
> The impl of slog() doesn't look too safe to me either.
> 
> Regards,
> Daniel