[Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

This version drops modes 'sleep' and 'hybrid' because they don't work
properly due to issues in qemu. Only the 'hibernate' mode is supported
for now.

Also note that virtio doesn't currently support ACPI S4. There are
patches flying on lkml to fix that though.

Please refer to patch 2/2 for more details on the implementation.

v4

o Drop 'sleep' and 'hybrid' modes
o pull in a fix from Michael Roth (patch 1/2)

 qapi-schema-guest.json     |   23 ++++++++++++++++++
 qemu-ga.c                  |   19 +++++++++++++-
 qga/guest-agent-commands.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH 1/2] qemu-ga: set O_NONBLOCK for serial channels

[Luiz Capitulino]

This fixes a bug when using -m isa-serial where qemu-ga will
hang on a read()'s when communicating to the host via isa-serial.

Original fix by Michael Roth.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-ga.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 200bb15..98e4dfe 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -504,7 +504,7 @@ static void init_guest_agent(GAState *s)
             exit(EXIT_FAILURE);
         }
     } else if (strcmp(s->method, "isa-serial") == 0) {
-        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
+        fd = qemu_open(s->path, O_RDWR | O_NOCTTY | O_NONBLOCK);
         if (fd == -1) {
             g_critical("error opening channel: %s", strerror(errno));
             exit(EXIT_FAILURE);
-- 
1.7.8.2.321.g4570a.dirty

Re: [Qemu-devel] [PATCH 1/2] qemu-ga: set O_NONBLOCK for serial channels

[Michael Roth]

On 01/04/2012 01:45 PM, Luiz Capitulino wrote:
> This fixes a bug when using -m isa-serial where qemu-ga will
> hang on a read()'s when communicating to the host via isa-serial.
>
> Original fix by Michael Roth.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   qemu-ga.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 200bb15..98e4dfe 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -504,7 +504,7 @@ static void init_guest_agent(GAState *s)
>               exit(EXIT_FAILURE);
>           }
>       } else if (strcmp(s->method, "isa-serial") == 0) {
> -        fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
> +        fd = qemu_open(s->path, O_RDWR | O_NOCTTY | O_NONBLOCK);
>           if (fd == -1) {
>               g_critical("error opening channel: %s", strerror(errno));
>               exit(EXIT_FAILURE);

Thanks for sending this.

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

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

[Luiz Capitulino]

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

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)
 {
-- 
1.7.8.2.321.g4570a.dirty

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

[Michael Roth]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Luiz Capitulino]

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.
> 

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

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Luiz Capitulino]

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

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Daniel P. Berrange]

On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> This version drops modes 'sleep' and 'hybrid' because they don't work
> properly due to issues in qemu. Only the 'hibernate' mode is supported
> for now.

IMHO this is short-sighted. When the bugs QEMU in are fixed so that
these modes work, you have needlessly put users in the situation where
they have to now upgrade the guest agent everywhere to take advantage
of the bugfix.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

On Thu, 5 Jan 2012 10:16:30 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> > This version drops modes 'sleep' and 'hybrid' because they don't work
> > properly due to issues in qemu. Only the 'hibernate' mode is supported
> > for now.
> 
> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
> these modes work, you have needlessly put users in the situation where
> they have to now upgrade the guest agent everywhere to take advantage
> of the bugfix.

That was my thinking until v4. But after discussing with Michael the issues
we have with S3 I concluded that it doesn't make sense to offer an API to
something that doesn't work, this will just generate bug reports. Also,
updating to get new features is normal and expected.

I'm willing to change my mind if I'm the only one thinking like this though.

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Daniel P. Berrange]

On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
> On Thu, 5 Jan 2012 10:16:30 +0000
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> > > This version drops modes 'sleep' and 'hybrid' because they don't work
> > > properly due to issues in qemu. Only the 'hibernate' mode is supported
> > > for now.
> > 
> > IMHO this is short-sighted. When the bugs QEMU in are fixed so that
> > these modes work, you have needlessly put users in the situation where
> > they have to now upgrade the guest agent everywhere to take advantage
> > of the bugfix.
> 
> That was my thinking until v4. But after discussing with Michael the issues
> we have with S3 I concluded that it doesn't make sense to offer an API to
> something that doesn't work, this will just generate bug reports. Also,
> updating to get new features is normal and expected.

This is assuming that users will always upgrade their VMs & hosts in
lock step, which I rather doubt they will in practice. eg imagine a
deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
and QEMU 1.2 (working S3). If they build VM disk images they will likely
use the QEMU GA from 1.2 for all their builds, even if many of them
will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
'hybrid' commands available in the guest agent, even though the host
QEMU doesn't work properly.

So you *will* ultimately need to make sure that QEMU GA from 1.2, has
sensible behaviour when run on a QEMU 1.1 host.  If you don't address
this during 1.1, you may well find yourself in an un-winnable situation
for 1.2, where it is impossible to provide good behaviour on old hosts.

So IMHO we are better off in the long run, if we include all commands
right now, even though some don't work yet, and work to ensure we have
good error reporting behaviour for those that don't work.

As an example, if S3 is broken in current QEMU, then we should not be
advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
to return false, at which point the guest agent can send back a nice error
message 'Suspend is not supported on this host', instead of just having the
guest try to suspend & hang or worse.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

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

> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
> > On Thu, 5 Jan 2012 10:16:30 +0000
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> > > > This version drops modes 'sleep' and 'hybrid' because they don't work
> > > > properly due to issues in qemu. Only the 'hibernate' mode is supported
> > > > for now.
> > > 
> > > IMHO this is short-sighted. When the bugs QEMU in are fixed so that
> > > these modes work, you have needlessly put users in the situation where
> > > they have to now upgrade the guest agent everywhere to take advantage
> > > of the bugfix.
> > 
> > That was my thinking until v4. But after discussing with Michael the issues
> > we have with S3 I concluded that it doesn't make sense to offer an API to
> > something that doesn't work, this will just generate bug reports. Also,
> > updating to get new features is normal and expected.
> 
> This is assuming that users will always upgrade their VMs & hosts in
> lock step, which I rather doubt they will in practice. eg imagine a
> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
> and QEMU 1.2 (working S3). If they build VM disk images they will likely
> use the QEMU GA from 1.2 for all their builds, even if many of them
> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
> 'hybrid' commands available in the guest agent, even though the host
> QEMU doesn't work properly.
> 
> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
> this during 1.1, you may well find yourself in an un-winnable situation
> for 1.2, where it is impossible to provide good behaviour on old hosts.
> 
> So IMHO we are better off in the long run, if we include all commands
> right now, even though some don't work yet, and work to ensure we have
> good error reporting behaviour for those that don't work.

Yes, I agree. As a side note: if we add error reporting it will only work
on 1.1 and later.  Ie, the problem you describe above will still happen
with 1.0.

But what you're suggesting seems to be the right thing to do. Do you agree
Michael?

> As an example, if S3 is broken in current QEMU, then we should not be
> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
> to return false, at which point the guest agent can send back a nice error
> message 'Suspend is not supported on this host', instead of just having the
> guest try to suspend & hang or worse.

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Michael Roth]

On 01/05/2012 08:42 AM, Luiz Capitulino wrote:
> On Thu, 5 Jan 2012 12:59:27 +0000
> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>
>> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
>>> On Thu, 5 Jan 2012 10:16:30 +0000
>>> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>>>
>>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
>>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
>>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
>>>>> for now.
>>>>
>>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
>>>> these modes work, you have needlessly put users in the situation where
>>>> they have to now upgrade the guest agent everywhere to take advantage
>>>> of the bugfix.
>>>
>>> That was my thinking until v4. But after discussing with Michael the issues
>>> we have with S3 I concluded that it doesn't make sense to offer an API to
>>> something that doesn't work, this will just generate bug reports. Also,
>>> updating to get new features is normal and expected.
>>
>> This is assuming that users will always upgrade their VMs&  hosts in
>> lock step, which I rather doubt they will in practice. eg imagine a
>> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
>> and QEMU 1.2 (working S3). If they build VM disk images they will likely
>> use the QEMU GA from 1.2 for all their builds, even if many of them
>> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
>> 'hybrid' commands available in the guest agent, even though the host
>> QEMU doesn't work properly.
>>
>> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
>> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
>> this during 1.1, you may well find yourself in an un-winnable situation
>> for 1.2, where it is impossible to provide good behaviour on old hosts.
>>
>> So IMHO we are better off in the long run, if we include all commands
>> right now, even though some don't work yet, and work to ensure we have
>> good error reporting behaviour for those that don't work.
>
> Yes, I agree. As a side note: if we add error reporting it will only work
> on 1.1 and later.  Ie, the problem you describe above will still happen
> with 1.0.
>
> But what you're suggesting seems to be the right thing to do. Do you agree
> Michael?

Agree, but unless we add an RPC that QEMU uses to advertise 
capabilities, I'm really not sure it's possible to detect whether or not 
the host will support it. And if we can't detect that reliably, we're 
better off leaving it out for now, because sleeping guests is not 
obscure functionality, and accidentally nuking guests when a user sleeps 
them (presumably because they want to retain their working state) is 
much worse than telling a user to upgrade their agent, or not supported 
or whatever.

>
>> As an example, if S3 is broken in current QEMU, then we should not be
>> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
>> to return false, at which point the guest agent can send back a nice error
>> message 'Suspend is not supported on this host', instead of just having the
>> guest try to suspend&  hang or worse.
>

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

On Thu, 05 Jan 2012 09:10:50 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 01/05/2012 08:42 AM, Luiz Capitulino wrote:
> > On Thu, 5 Jan 2012 12:59:27 +0000
> > "Daniel P. Berrange"<berrange@redhat.com>  wrote:
> >
> >> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
> >>> On Thu, 5 Jan 2012 10:16:30 +0000
> >>> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
> >>>
> >>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> >>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
> >>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
> >>>>> for now.
> >>>>
> >>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
> >>>> these modes work, you have needlessly put users in the situation where
> >>>> they have to now upgrade the guest agent everywhere to take advantage
> >>>> of the bugfix.
> >>>
> >>> That was my thinking until v4. But after discussing with Michael the issues
> >>> we have with S3 I concluded that it doesn't make sense to offer an API to
> >>> something that doesn't work, this will just generate bug reports. Also,
> >>> updating to get new features is normal and expected.
> >>
> >> This is assuming that users will always upgrade their VMs&  hosts in
> >> lock step, which I rather doubt they will in practice. eg imagine a
> >> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
> >> and QEMU 1.2 (working S3). If they build VM disk images they will likely
> >> use the QEMU GA from 1.2 for all their builds, even if many of them
> >> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
> >> 'hybrid' commands available in the guest agent, even though the host
> >> QEMU doesn't work properly.
> >>
> >> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
> >> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
> >> this during 1.1, you may well find yourself in an un-winnable situation
> >> for 1.2, where it is impossible to provide good behaviour on old hosts.
> >>
> >> So IMHO we are better off in the long run, if we include all commands
> >> right now, even though some don't work yet, and work to ensure we have
> >> good error reporting behaviour for those that don't work.
> >
> > Yes, I agree. As a side note: if we add error reporting it will only work
> > on 1.1 and later.  Ie, the problem you describe above will still happen
> > with 1.0.
> >
> > But what you're suggesting seems to be the right thing to do. Do you agree
> > Michael?
> 
> Agree, but unless we add an RPC that QEMU uses to advertise 
> capabilities, I'm really not sure it's possible to detect whether or not 
> the host will support it.

You mean an RPC to advertise if 'sleep' is supported? I think this is best done
by making guest-suspend return an error as suggested by Daniel, otherwise a
client that doesn't query for capabilities might run in trouble.

There's an important detail though: we need to make qemu not advertise S3 for
this to work. However, we might be able to fix S3 for 1.1 (and bugs, like the
S4 ones, can't be detected, limiting the scope of the 'unsupported' error).

So, we could merge all modes and commit to get S3 fixed for 1.1 :)

> And if we can't detect that reliably, we're 
> better off leaving it out for now, because sleeping guests is not 
> obscure functionality, and accidentally nuking guests when a user sleeps 
> them (presumably because they want to retain their working state) is 
> much worse than telling a user to upgrade their agent, or not supported 
> or whatever.
> 
> >
> >> As an example, if S3 is broken in current QEMU, then we should not be
> >> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
> >> to return false, at which point the guest agent can send back a nice error
> >> message 'Suspend is not supported on this host', instead of just having the
> >> guest try to suspend&  hang or worse.
> >
> 

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Michael Roth]

On 01/05/2012 02:25 PM, Luiz Capitulino wrote:
> On Thu, 05 Jan 2012 09:10:50 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 01/05/2012 08:42 AM, Luiz Capitulino wrote:
>>> On Thu, 5 Jan 2012 12:59:27 +0000
>>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
>>>
>>>> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
>>>>> On Thu, 5 Jan 2012 10:16:30 +0000
>>>>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
>>>>>
>>>>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
>>>>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
>>>>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
>>>>>>> for now.
>>>>>>
>>>>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
>>>>>> these modes work, you have needlessly put users in the situation where
>>>>>> they have to now upgrade the guest agent everywhere to take advantage
>>>>>> of the bugfix.
>>>>>
>>>>> That was my thinking until v4. But after discussing with Michael the issues
>>>>> we have with S3 I concluded that it doesn't make sense to offer an API to
>>>>> something that doesn't work, this will just generate bug reports. Also,
>>>>> updating to get new features is normal and expected.
>>>>
>>>> This is assuming that users will always upgrade their VMs&   hosts in
>>>> lock step, which I rather doubt they will in practice. eg imagine a
>>>> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
>>>> and QEMU 1.2 (working S3). If they build VM disk images they will likely
>>>> use the QEMU GA from 1.2 for all their builds, even if many of them
>>>> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
>>>> 'hybrid' commands available in the guest agent, even though the host
>>>> QEMU doesn't work properly.
>>>>
>>>> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
>>>> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
>>>> this during 1.1, you may well find yourself in an un-winnable situation
>>>> for 1.2, where it is impossible to provide good behaviour on old hosts.
>>>>
>>>> So IMHO we are better off in the long run, if we include all commands
>>>> right now, even though some don't work yet, and work to ensure we have
>>>> good error reporting behaviour for those that don't work.
>>>
>>> Yes, I agree. As a side note: if we add error reporting it will only work
>>> on 1.1 and later.  Ie, the problem you describe above will still happen
>>> with 1.0.
>>>
>>> But what you're suggesting seems to be the right thing to do. Do you agree
>>> Michael?
>>
>> Agree, but unless we add an RPC that QEMU uses to advertise
>> capabilities, I'm really not sure it's possible to detect whether or not
>> the host will support it.
>
> You mean an RPC to advertise if 'sleep' is supported? I think this is best done
> by making guest-suspend return an error as suggested by Daniel, otherwise a
> client that doesn't query for capabilities might run in trouble.

Agreed, but what I mean is that if the user executes the suspend using 
on up-level agent running on a down-level 1.0 host, the agent will still 
see s3 advertised and issue the buggy suspend. That's why I suggested 
the host->agent capabilities reporting as a possible (but somewhat ugly) 
way to just simply tell the agent it can handle it (and, lacking that, 
assume that it can't).

>
> There's an important detail though: we need to make qemu not advertise S3 for
> this to work. However, we might be able to fix S3 for 1.1 (and bugs, like the
> S4 ones, can't be detected, limiting the scope of the 'unsupported' error).
>
> So, we could merge all modes and commit to get S3 fixed for 1.1 :)

No disagreement there, if we can commit to making qemu-ga/qemu 1.1 
releases interoperable in this manner (whether by fixing s3 or not 
advertising it), I think that approach is perfectly fine, ideal even. 
Doing a 1.1 release where qemu and qemu-ga are not interoperable (qemu 
missing s3 support, qemu-ga using s3) was my main objection.

But there is a 2nd topic here I'm trying to mull over: what is qemu-ga's 
support policy for down-level hosts? backward-compatible? incompatible?

The above approach to this problem suggests the latter (qemu-ga 1.1 has 
RPCs that will knowingly break 1.0 qemu instances). We could solve this 
by introducing the capabilities negotiation I mentioned early. It 
actually wouldn't need to be anything other than qemu telling qemu-ga 
what qemu-ga version-level it supports. By default we assume 1.0, and 
limit qemu-ga to that until qemu-ga is told otherwise (so, no 
sleep/hybrid suspend modes). For new RPCs we may be able to handle this 
version automatically, since we include qemu version levels for the RPCs 
in the schema. For functionality within an RPC (like sleep/hybrid 
suspend modes) we could use conditional code.

If we take that approach (maintaining backward-compatibility), we'd need 
to introduce that code in the agent now, and require qemu/libvirt 
execute the guest-set-support-level RPC or whatever to access these 1.1 
features.

Technically, there's a required RPC qemu-ga clients need to execute 
already: guest-sync. It's required because we have no way to reliably 
detect EOF over virtio-serial, and thus an agent may send stale data to 
a newly-connected qemu-ga client, so the client needs to do the 
guest-sync command to find the expected response and re-sync the 
streams. We could roll the guest-set-support-level functionality into 
that. Basically just add another field.

>
>> And if we can't detect that reliably, we're
>> better off leaving it out for now, because sleeping guests is not
>> obscure functionality, and accidentally nuking guests when a user sleeps
>> them (presumably because they want to retain their working state) is
>> much worse than telling a user to upgrade their agent, or not supported
>> or whatever.
>>
>>>
>>>> As an example, if S3 is broken in current QEMU, then we should not be
>>>> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
>>>> to return false, at which point the guest agent can send back a nice error
>>>> message 'Suspend is not supported on this host', instead of just having the
>>>> guest try to suspend&   hang or worse.
>>>
>>
>

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

On Thu, 05 Jan 2012 15:41:33 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 01/05/2012 02:25 PM, Luiz Capitulino wrote:
> > On Thu, 05 Jan 2012 09:10:50 -0600
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> On 01/05/2012 08:42 AM, Luiz Capitulino wrote:
> >>> On Thu, 5 Jan 2012 12:59:27 +0000
> >>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
> >>>
> >>>> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
> >>>>> On Thu, 5 Jan 2012 10:16:30 +0000
> >>>>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
> >>>>>
> >>>>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> >>>>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
> >>>>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
> >>>>>>> for now.
> >>>>>>
> >>>>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
> >>>>>> these modes work, you have needlessly put users in the situation where
> >>>>>> they have to now upgrade the guest agent everywhere to take advantage
> >>>>>> of the bugfix.
> >>>>>
> >>>>> That was my thinking until v4. But after discussing with Michael the issues
> >>>>> we have with S3 I concluded that it doesn't make sense to offer an API to
> >>>>> something that doesn't work, this will just generate bug reports. Also,
> >>>>> updating to get new features is normal and expected.
> >>>>
> >>>> This is assuming that users will always upgrade their VMs&   hosts in
> >>>> lock step, which I rather doubt they will in practice. eg imagine a
> >>>> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
> >>>> and QEMU 1.2 (working S3). If they build VM disk images they will likely
> >>>> use the QEMU GA from 1.2 for all their builds, even if many of them
> >>>> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
> >>>> 'hybrid' commands available in the guest agent, even though the host
> >>>> QEMU doesn't work properly.
> >>>>
> >>>> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
> >>>> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
> >>>> this during 1.1, you may well find yourself in an un-winnable situation
> >>>> for 1.2, where it is impossible to provide good behaviour on old hosts.
> >>>>
> >>>> So IMHO we are better off in the long run, if we include all commands
> >>>> right now, even though some don't work yet, and work to ensure we have
> >>>> good error reporting behaviour for those that don't work.
> >>>
> >>> Yes, I agree. As a side note: if we add error reporting it will only work
> >>> on 1.1 and later.  Ie, the problem you describe above will still happen
> >>> with 1.0.
> >>>
> >>> But what you're suggesting seems to be the right thing to do. Do you agree
> >>> Michael?
> >>
> >> Agree, but unless we add an RPC that QEMU uses to advertise
> >> capabilities, I'm really not sure it's possible to detect whether or not
> >> the host will support it.
> >
> > You mean an RPC to advertise if 'sleep' is supported? I think this is best done
> > by making guest-suspend return an error as suggested by Daniel, otherwise a
> > client that doesn't query for capabilities might run in trouble.
> 
> Agreed, but what I mean is that if the user executes the suspend using 
> on up-level agent running on a down-level 1.0 host, the agent will still 
> see s3 advertised and issue the buggy suspend. That's why I suggested 
> the host->agent capabilities reporting as a possible (but somewhat ugly) 
> way to just simply tell the agent it can handle it (and, lacking that, 
> assume that it can't).

That makes sense.

> 
> >
> > There's an important detail though: we need to make qemu not advertise S3 for
> > this to work. However, we might be able to fix S3 for 1.1 (and bugs, like the
> > S4 ones, can't be detected, limiting the scope of the 'unsupported' error).
> >
> > So, we could merge all modes and commit to get S3 fixed for 1.1 :)
> 
> No disagreement there, if we can commit to making qemu-ga/qemu 1.1 
> releases interoperable in this manner (whether by fixing s3 or not 
> advertising it), I think that approach is perfectly fine, ideal even. 
> Doing a 1.1 release where qemu and qemu-ga are not interoperable (qemu 
> missing s3 support, qemu-ga using s3) was my main objection.

I see.

> But there is a 2nd topic here I'm trying to mull over: what is qemu-ga's 
> support policy for down-level hosts? backward-compatible? incompatible?

That's a good question, I think we should be backward-compatible, but I think
that's not going to be trivial.

> The above approach to this problem suggests the latter (qemu-ga 1.1 has 
> RPCs that will knowingly break 1.0 qemu instances). We could solve this 
> by introducing the capabilities negotiation I mentioned early. It 
> actually wouldn't need to be anything other than qemu telling qemu-ga 
> what qemu-ga version-level it supports. By default we assume 1.0, and 
> limit qemu-ga to that until qemu-ga is told otherwise (so, no 
> sleep/hybrid suspend modes). For new RPCs we may be able to handle this 
> version automatically, since we include qemu version levels for the RPCs 
> in the schema. For functionality within an RPC (like sleep/hybrid 
> suspend modes) we could use conditional code.
> 
> If we take that approach (maintaining backward-compatibility), we'd need 
> to introduce that code in the agent now, and require qemu/libvirt 
> execute the guest-set-support-level RPC or whatever to access these 1.1 
> features.

What does guest-set-support-level do? It enables all 1.1 post features?

A different approach would be to add a new field in the command dict in
the schema file, say 'broken-in-qemu-version', and change qemu-ga to check
that field in its main loop before executing a command. If
'broken-in-qemu-version' <= qemu version qemu-ga returns an not supported
error.

For commands like the guest-suspend which is partially supported, we'd have
to do a manual check for the qemu version as you suggested above.

That's just an idea though, I'm not sure what's the best way to do this.

> 
> Technically, there's a required RPC qemu-ga clients need to execute 
> already: guest-sync. It's required because we have no way to reliably 
> detect EOF over virtio-serial, and thus an agent may send stale data to 
> a newly-connected qemu-ga client, so the client needs to do the 
> guest-sync command to find the expected response and re-sync the 
> streams. We could roll the guest-set-support-level functionality into 
> that. Basically just add another field.
> 
> >
> >> And if we can't detect that reliably, we're
> >> better off leaving it out for now, because sleeping guests is not
> >> obscure functionality, and accidentally nuking guests when a user sleeps
> >> them (presumably because they want to retain their working state) is
> >> much worse than telling a user to upgrade their agent, or not supported
> >> or whatever.
> >>
> >>>
> >>>> As an example, if S3 is broken in current QEMU, then we should not be
> >>>> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
> >>>> to return false, at which point the guest agent can send back a nice error
> >>>> message 'Suspend is not supported on this host', instead of just having the
> >>>> guest try to suspend&   hang or worse.
> >>>
> >>
> >
> 

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Michael Roth]

On 01/06/2012 01:04 PM, Luiz Capitulino wrote:
> On Thu, 05 Jan 2012 15:41:33 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 01/05/2012 02:25 PM, Luiz Capitulino wrote:
>>> On Thu, 05 Jan 2012 09:10:50 -0600
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
>>>
>>>> On 01/05/2012 08:42 AM, Luiz Capitulino wrote:
>>>>> On Thu, 5 Jan 2012 12:59:27 +0000
>>>>> "Daniel P. Berrange"<berrange@redhat.com>    wrote:
>>>>>
>>>>>> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
>>>>>>> On Thu, 5 Jan 2012 10:16:30 +0000
>>>>>>> "Daniel P. Berrange"<berrange@redhat.com>    wrote:
>>>>>>>
>>>>>>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
>>>>>>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
>>>>>>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
>>>>>>>>> for now.
>>>>>>>>
>>>>>>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
>>>>>>>> these modes work, you have needlessly put users in the situation where
>>>>>>>> they have to now upgrade the guest agent everywhere to take advantage
>>>>>>>> of the bugfix.
>>>>>>>
>>>>>>> That was my thinking until v4. But after discussing with Michael the issues
>>>>>>> we have with S3 I concluded that it doesn't make sense to offer an API to
>>>>>>> something that doesn't work, this will just generate bug reports. Also,
>>>>>>> updating to get new features is normal and expected.
>>>>>>
>>>>>> This is assuming that users will always upgrade their VMs&    hosts in
>>>>>> lock step, which I rather doubt they will in practice. eg imagine a
>>>>>> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
>>>>>> and QEMU 1.2 (working S3). If they build VM disk images they will likely
>>>>>> use the QEMU GA from 1.2 for all their builds, even if many of them
>>>>>> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
>>>>>> 'hybrid' commands available in the guest agent, even though the host
>>>>>> QEMU doesn't work properly.
>>>>>>
>>>>>> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
>>>>>> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
>>>>>> this during 1.1, you may well find yourself in an un-winnable situation
>>>>>> for 1.2, where it is impossible to provide good behaviour on old hosts.
>>>>>>
>>>>>> So IMHO we are better off in the long run, if we include all commands
>>>>>> right now, even though some don't work yet, and work to ensure we have
>>>>>> good error reporting behaviour for those that don't work.
>>>>>
>>>>> Yes, I agree. As a side note: if we add error reporting it will only work
>>>>> on 1.1 and later.  Ie, the problem you describe above will still happen
>>>>> with 1.0.
>>>>>
>>>>> But what you're suggesting seems to be the right thing to do. Do you agree
>>>>> Michael?
>>>>
>>>> Agree, but unless we add an RPC that QEMU uses to advertise
>>>> capabilities, I'm really not sure it's possible to detect whether or not
>>>> the host will support it.
>>>
>>> You mean an RPC to advertise if 'sleep' is supported? I think this is best done
>>> by making guest-suspend return an error as suggested by Daniel, otherwise a
>>> client that doesn't query for capabilities might run in trouble.
>>
>> Agreed, but what I mean is that if the user executes the suspend using
>> on up-level agent running on a down-level 1.0 host, the agent will still
>> see s3 advertised and issue the buggy suspend. That's why I suggested
>> the host->agent capabilities reporting as a possible (but somewhat ugly)
>> way to just simply tell the agent it can handle it (and, lacking that,
>> assume that it can't).
>
> That makes sense.
>
>>
>>>
>>> There's an important detail though: we need to make qemu not advertise S3 for
>>> this to work. However, we might be able to fix S3 for 1.1 (and bugs, like the
>>> S4 ones, can't be detected, limiting the scope of the 'unsupported' error).
>>>
>>> So, we could merge all modes and commit to get S3 fixed for 1.1 :)
>>
>> No disagreement there, if we can commit to making qemu-ga/qemu 1.1
>> releases interoperable in this manner (whether by fixing s3 or not
>> advertising it), I think that approach is perfectly fine, ideal even.
>> Doing a 1.1 release where qemu and qemu-ga are not interoperable (qemu
>> missing s3 support, qemu-ga using s3) was my main objection.
>
> I see.
>
>> But there is a 2nd topic here I'm trying to mull over: what is qemu-ga's
>> support policy for down-level hosts? backward-compatible? incompatible?
>
> That's a good question, I think we should be backward-compatible, but I think
> that's not going to be trivial.
>
>> The above approach to this problem suggests the latter (qemu-ga 1.1 has
>> RPCs that will knowingly break 1.0 qemu instances). We could solve this
>> by introducing the capabilities negotiation I mentioned early. It
>> actually wouldn't need to be anything other than qemu telling qemu-ga
>> what qemu-ga version-level it supports. By default we assume 1.0, and
>> limit qemu-ga to that until qemu-ga is told otherwise (so, no
>> sleep/hybrid suspend modes). For new RPCs we may be able to handle this
>> version automatically, since we include qemu version levels for the RPCs
>> in the schema. For functionality within an RPC (like sleep/hybrid
>> suspend modes) we could use conditional code.
>>
>> If we take that approach (maintaining backward-compatibility), we'd need
>> to introduce that code in the agent now, and require qemu/libvirt
>> execute the guest-set-support-level RPC or whatever to access these 1.1
>> features.
>
> What does guest-set-support-level do? It enables all 1.1 post features?

Well, that was my initial thought (we set host version level N, all 
RPCs/fields introduced after N are made unavailable). But if we added, 
say, a new optional parameter or RPC that wasn't dependent on a 
particular QEMU version, there's no reason to hide them from host 
programs higher up the stack (which may be aware of the new features, 
but are paired with older QEMU versions for whatever reason and so can't 
bump the support level above 1.0 without risking breakage for other stuff).

So, guest-set-support-level(N) enables all features that were marked as 
requiring QEMU version N. New features with no such dependencies 
(optional params, new RPCs) would be unguarded/enabled by default.

>
> A different approach would be to add a new field in the command dict in
> the schema file, say 'broken-in-qemu-version', and change qemu-ga to check
> that field in its main loop before executing a command. If
> 'broken-in-qemu-version'<= qemu version qemu-ga returns an not supported
> error.

Yah, still not sure what the best way to implement the check is. Though, 
I'd prefer the "positive" approach: 'requires[-at-least]-qemu-version'.

>
> For commands like the guest-suspend which is partially supported, we'd have
> to do a manual check for the qemu version as you suggested above.

Agreed, and just document qemu version dependencies in the schema. That 
may a reasonable approach for the above as well: if we introduce an RPC 
that requires a certain qemu version we just stick a version check at 
the beginning and bail if it fails. We could always get fancy with it 
later. Would make it easier to include this data in guest-info though... 
I look at it more and whip up a patch soon.

>
> That's just an idea though, I'm not sure what's the best way to do this.
>
>>
>> Technically, there's a required RPC qemu-ga clients need to execute
>> already: guest-sync. It's required because we have no way to reliably
>> detect EOF over virtio-serial, and thus an agent may send stale data to
>> a newly-connected qemu-ga client, so the client needs to do the
>> guest-sync command to find the expected response and re-sync the
>> streams. We could roll the guest-set-support-level functionality into
>> that. Basically just add another field.
>>
>>>
>>>> And if we can't detect that reliably, we're
>>>> better off leaving it out for now, because sleeping guests is not
>>>> obscure functionality, and accidentally nuking guests when a user sleeps
>>>> them (presumably because they want to retain their working state) is
>>>> much worse than telling a user to upgrade their agent, or not supported
>>>> or whatever.
>>>>
>>>>>
>>>>>> As an example, if S3 is broken in current QEMU, then we should not be
>>>>>> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
>>>>>> to return false, at which point the guest agent can send back a nice error
>>>>>> message 'Suspend is not supported on this host', instead of just having the
>>>>>> guest try to suspend&    hang or worse.
>>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Michael Roth]

On 01/05/2012 06:59 AM, Daniel P. Berrange wrote:
> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
>> On Thu, 5 Jan 2012 10:16:30 +0000
>> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>>
>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
>>>> for now.
>>>
>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
>>> these modes work, you have needlessly put users in the situation where
>>> they have to now upgrade the guest agent everywhere to take advantage
>>> of the bugfix.
>>
>> That was my thinking until v4. But after discussing with Michael the issues
>> we have with S3 I concluded that it doesn't make sense to offer an API to
>> something that doesn't work, this will just generate bug reports. Also,
>> updating to get new features is normal and expected.
>
> This is assuming that users will always upgrade their VMs&  hosts in
> lock step, which I rather doubt they will in practice. eg imagine a
> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
> and QEMU 1.2 (working S3). If they build VM disk images they will likely
> use the QEMU GA from 1.2 for all their builds, even if many of them
> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
> 'hybrid' commands available in the guest agent, even though the host
> QEMU doesn't work properly.
>
> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
> this during 1.1, you may well find yourself in an un-winnable situation
> for 1.2, where it is impossible to provide good behaviour on old hosts.
>
> So IMHO we are better off in the long run, if we include all commands
> right now, even though some don't work yet, and work to ensure we have
> good error reporting behaviour for those that don't work.
>
> As an example, if S3 is broken in current QEMU, then we should not be
> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
> to return false, at which point the guest agent can send back a nice error
> message 'Suspend is not supported on this host', instead of just having the
> guest try to suspend&  hang or worse.

This still requires we're lockstep with host QEMU (ideally that would be 
the case via push-deployment of the agent, exactly because of issues 
like this. Or at least, it'd make the upgrade process painless). And 
outside of that, I really cannot think of any nice way to check, from 
the agent, that a host has required functionality for {this,an} RPC. Not 
unless we forced a bi-directional capabilities negotiation sequence, and 
I don't like the idea of injecting this kind of data into a guest. 
libvirt could maybe filter the modes based on QEMU version, but that's 
not the only consumer of the agent.

Really I think this is a case study for why push-deployment of agents is 
the way to go. QEMU could query qemu-ga directly and generate an 'agent 
update available' event that users/frontends can use to prompt an update 
to the latest version. Then all the upgrade inertia involved with saving 
code/features for subsequent agent versions is mostly gone, and we can 
"do the right thing" with regard to broken functionality :)

Unfortunately that option isn't available yet. But it just seems wrong 
to introduce something we know is broken, to the extent that even those 
involved with it's development aren't currently capable of testing it 
fully. I mean, we know what the user expectations are, and it's not 
that, unfortunately for us :( I'd be more open to it if the bug wasn't 
so bad, but nuking your guest's working state every time you make the 
mistake of hitting the pretty "sleep" button in virt-manager or whatever 
is pretty bad.

>
> Daniel

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Daniel P. Berrange]

On Thu, Jan 05, 2012 at 09:04:57AM -0600, Michael Roth wrote:
> On 01/05/2012 06:59 AM, Daniel P. Berrange wrote:
> >On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
> >>On Thu, 5 Jan 2012 10:16:30 +0000
> >>"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> >>
> >>>On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
> >>>>This version drops modes 'sleep' and 'hybrid' because they don't work
> >>>>properly due to issues in qemu. Only the 'hibernate' mode is supported
> >>>>for now.
> >>>
> >>>IMHO this is short-sighted. When the bugs QEMU in are fixed so that
> >>>these modes work, you have needlessly put users in the situation where
> >>>they have to now upgrade the guest agent everywhere to take advantage
> >>>of the bugfix.
> >>
> >>That was my thinking until v4. But after discussing with Michael the issues
> >>we have with S3 I concluded that it doesn't make sense to offer an API to
> >>something that doesn't work, this will just generate bug reports. Also,
> >>updating to get new features is normal and expected.
> >
> >This is assuming that users will always upgrade their VMs&  hosts in
> >lock step, which I rather doubt they will in practice. eg imagine a
> >deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
> >and QEMU 1.2 (working S3). If they build VM disk images they will likely
> >use the QEMU GA from 1.2 for all their builds, even if many of them
> >will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
> >'hybrid' commands available in the guest agent, even though the host
> >QEMU doesn't work properly.
> >
> >So you *will* ultimately need to make sure that QEMU GA from 1.2, has
> >sensible behaviour when run on a QEMU 1.1 host.  If you don't address
> >this during 1.1, you may well find yourself in an un-winnable situation
> >for 1.2, where it is impossible to provide good behaviour on old hosts.
> >
> >So IMHO we are better off in the long run, if we include all commands
> >right now, even though some don't work yet, and work to ensure we have
> >good error reporting behaviour for those that don't work.
> >
> >As an example, if S3 is broken in current QEMU, then we should not be
> >advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
> >to return false, at which point the guest agent can send back a nice error
> >message 'Suspend is not supported on this host', instead of just having the
> >guest try to suspend&  hang or worse.
> 
> This still requires we're lockstep with host QEMU (ideally that
> would be the case via push-deployment of the agent, exactly because
> of issues like this. Or at least, it'd make the upgrade process
> painless). And outside of that, I really cannot think of any nice
> way to check, from the agent, that a host has required functionality
> for {this,an} RPC. Not unless we forced a bi-directional
> capabilities negotiation sequence, and I don't like the idea of
> injecting this kind of data into a guest. libvirt could maybe filter
> the modes based on QEMU version, but that's not the only consumer of
> the agent.

Err, the scenario I just described does not require lockstep
upgrade. Newer QEMU GA agent should be able to run on historical
QEMU hosts just fine. I'm also not trying to suggest we need a
general bi-directional capabilities negotiation here either.
The key is that in this particular case, QEMU should only
expose S3 to the guest if it is actually capable of working.
Then, the pm-is-supported  command will 'just work'. No
host<->guest agent negoiation is required.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v4 0/2]: qemu-ga: Add the guest-suspend command

[Michael Roth]

On 01/05/2012 09:11 AM, Daniel P. Berrange wrote:
> On Thu, Jan 05, 2012 at 09:04:57AM -0600, Michael Roth wrote:
>> On 01/05/2012 06:59 AM, Daniel P. Berrange wrote:
>>> On Thu, Jan 05, 2012 at 10:37:14AM -0200, Luiz Capitulino wrote:
>>>> On Thu, 5 Jan 2012 10:16:30 +0000
>>>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
>>>>
>>>>> On Wed, Jan 04, 2012 at 05:45:11PM -0200, Luiz Capitulino wrote:
>>>>>> This version drops modes 'sleep' and 'hybrid' because they don't work
>>>>>> properly due to issues in qemu. Only the 'hibernate' mode is supported
>>>>>> for now.
>>>>>
>>>>> IMHO this is short-sighted. When the bugs QEMU in are fixed so that
>>>>> these modes work, you have needlessly put users in the situation where
>>>>> they have to now upgrade the guest agent everywhere to take advantage
>>>>> of the bugfix.
>>>>
>>>> That was my thinking until v4. But after discussing with Michael the issues
>>>> we have with S3 I concluded that it doesn't make sense to offer an API to
>>>> something that doesn't work, this will just generate bug reports. Also,
>>>> updating to get new features is normal and expected.
>>>
>>> This is assuming that users will always upgrade their VMs&   hosts in
>>> lock step, which I rather doubt they will in practice. eg imagine a
>>> deployment might have a mixture of hosts, running QEMU 1.1 (broken S3)
>>> and QEMU 1.2 (working S3). If they build VM disk images they will likely
>>> use the QEMU GA from 1.2 for all their builds, even if many of them
>>> will only run on QEMU 1.1 hosts. So you'll end up having 'sleep' and
>>> 'hybrid' commands available in the guest agent, even though the host
>>> QEMU doesn't work properly.
>>>
>>> So you *will* ultimately need to make sure that QEMU GA from 1.2, has
>>> sensible behaviour when run on a QEMU 1.1 host.  If you don't address
>>> this during 1.1, you may well find yourself in an un-winnable situation
>>> for 1.2, where it is impossible to provide good behaviour on old hosts.
>>>
>>> So IMHO we are better off in the long run, if we include all commands
>>> right now, even though some don't work yet, and work to ensure we have
>>> good error reporting behaviour for those that don't work.
>>>
>>> As an example, if S3 is broken in current QEMU, then we should not be
>>> advertizing S3 to the guest OS. This would allow 'pm-is-supported --suspend'
>>> to return false, at which point the guest agent can send back a nice error
>>> message 'Suspend is not supported on this host', instead of just having the
>>> guest try to suspend&   hang or worse.
>>
>> This still requires we're lockstep with host QEMU (ideally that
>> would be the case via push-deployment of the agent, exactly because
>> of issues like this. Or at least, it'd make the upgrade process
>> painless). And outside of that, I really cannot think of any nice
>> way to check, from the agent, that a host has required functionality
>> for {this,an} RPC. Not unless we forced a bi-directional
>> capabilities negotiation sequence, and I don't like the idea of
>> injecting this kind of data into a guest. libvirt could maybe filter
>> the modes based on QEMU version, but that's not the only consumer of
>> the agent.
>
> Err, the scenario I just described does not require lockstep
> upgrade. Newer QEMU GA agent should be able to run on historical
> QEMU hosts just fine. I'm also not trying to suggest we need a

Bad terminology on my part, what I mean is if qemu-ga error reporting 
requires a newer qemu, we still execute the sleep on buggy hosts unless 
the host-level is adequate.

> general bi-directional capabilities negotiation here either.
> The key is that in this particular case, QEMU should only
> expose S3 to the guest if it is actually capable of working.
> Then, the pm-is-supported  command will 'just work'. No
> host<->guest agent negoiation is required.
>
> Daniel

[Qemu-devel] [PATCH v5 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

I've tried to address all review comments in this new version. The
two most important changes is that I've added the 'sleep' and 'hybrid'
modes back and now the guest is queried for suspend support (the way
I'm doing this is also worth reviewing).

This series depends on this patch from Michael:

 http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg01382.html

and from a patch I've submitted to seabios to disable S3 advertise,
as S3 is broken in qemu today.

v5

o add 'sleep' and 'hybrid' modes back
o query for suspend support using pm-is-supported & manual check
o use _exit() [Daniel]
o reopen standard file-descriptors to /dev/null [Eric]
o make the SIGCHLD handler more portable by calling waitpid() in
  a loop, as not all unix versions will raise SIGCHLD multiple
  times if several children terminate at once (I don't even know
  if Linux does that)

 qapi-schema-guest.json     |   29 ++++++
 qemu-ga.c                  |   20 ++++-
 qga/guest-agent-commands.c |  212 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+), 2 deletions(-)

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

[Luiz Capitulino]

The guest-suspend command supports three modes:

 o hibernate (suspend to disk)
 o sleep     (suspend to ram)
 o hybrid    (save RAM contents to disk, but suspend instead of
              powering off)

Before trying to suspend, the command queries the guest in order
to know whether a given mode is supported. The sleep and hybrid
modes are only supported in QEMU 1.1 and later though, because
QEMU would adverstise broken S3 support in previous versions.

The guest-suspend command will use the scripts provided by the
pm-utils package if they are available. If they aren't, a manual
process which directly writes to the "/sys/power/state" file will
be tried.

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.

The approach used to query the guest for suspend support deserves
some explanation. It's implemented by bios_supports_mode() and shown
below:

   qemu-ga
     |
 create pipe
     |
   fork()
     -----------------
     |               |
     |               |
     |             fork()
     |               --------------------------
     |               |                        |
     |               |                        |
     |               |               exec('pm-is-supported')
     |               |
     |              wait()
     |       write exit status to pipe
     |              exit
     |
  read pipe

This might look complex, but the final code is quite simple. The
purpose of that approach is to allow qemu-ga to reap its children
(semi-)automatically from its SIGCHLD handler.

Implementing this the obvious way, that's, doing the exec() call
from the first child process, would force us to introduce a more
complex way to reap qemu-ga's children. Like registering PIDs to
be reaped and having a way to wait for them when returning their
exit status to qemu-ga is necessary.

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

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 5f8a18d..6a0605b 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -219,3 +219,32 @@
 ##
 { 'command': 'guest-fsfreeze-thaw',
   'returns': 'int' }
+
+##
+# @guest-suspend
+#
+# Suspend guest execution by entering ACPI power state S3 or S4.
+#
+# 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)
+#        'sleep'     execution is suspended but the RAM retains its contents
+#                    (this corresponds to ACPI S3)
+#        'hybrid'    RAM content is saved to the disk but the guest is
+#                    suspended instead of powering off
+#
+# Notes: o This is an asynchronous request. There's no guarantee a response
+#          will be sent.
+#        o Errors will be logged to guest's syslog.
+#        o It's strongly recommended to issue the guest-sync command before
+#          sending commands when the guest resumes.
+#
+# Since: 1.1
+##
+{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
diff --git a/qemu-ga.c b/qemu-ga.c
index 647df82..f531084 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,16 @@ static void quit_handler(int sig)
     }
 }
 
+/* reap _all_ terminated children */
+static void child_handler(int sig)
+{
+    int status;
+    while (waitpid(-1, &status, WNOHANG) > 0) /* NOTHING */;
+}
+
 static void register_signal_handlers(void)
 {
-    struct sigaction sigact;
+    struct sigaction sigact, sigact_chld;
     int ret;
 
     memset(&sigact, 0, sizeof(struct sigaction));
@@ -76,6 +84,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..963270c 100644
--- a/qga/guest-agent-commands.c
+++ b/qga/guest-agent-commands.c
@@ -23,6 +23,7 @@
 
 #include <sys/types.h>
 #include <sys/ioctl.h>
+#include <sys/wait.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qerror.h"
@@ -44,6 +45,21 @@ static void slog(const char *fmt, ...)
     va_end(ap);
 }
 
+static int reopen_fd_to_null(int fd)
+{
+    int ret, nullfd;
+
+    nullfd = open("/dev/null", O_RDWR);
+    if (nullfd < 0) {
+        return -1;
+    }
+
+    ret = dup2(nullfd, fd);
+    close(nullfd);
+
+    return ret;
+}
+
 int64_t qmp_guest_sync(int64_t id, Error **errp)
 {
     return id;
@@ -574,6 +590,202 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
 }
 #endif
 
+#define LINUX_SYS_STATE_FILE "/sys/power/state"
+#define SUS_MODE_SUPPORTED 0
+#define SUS_MODE_NOT_SUPPORTED 1
+
+/**
+ * This function forks twice and the information about the mode support
+ * status is passed to the qemu-ga process via a pipe.
+ *
+ * This approach allows us to keep the way we reap terminated children
+ * in qemu-ga quite simple.
+ */
+static bool bios_supports_mode(const char *mode, Error **err)
+{
+    pid_t pid;
+    ssize_t ret;
+    int status, pipefds[2];
+
+    if (pipe(pipefds) < 0) {
+        error_set(err, QERR_UNDEFINED_ERROR);
+        return false;
+    }
+
+    pid = fork();
+    if (!pid) {
+        struct sigaction act;
+
+        memset(&act, 0, sizeof(act));
+        act.sa_handler = SIG_DFL;
+        sigaction(SIGCHLD, &act, NULL);
+
+        setsid();
+        close(pipefds[0]);
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        pid = fork();
+        if (!pid) {
+            char buf[32];
+            FILE *sysfile;
+            const char *arg;
+            const char *pmutils_bin = "pm-is-supported";
+
+            if (strcmp(mode, "hibernate") == 0) {
+                arg = "--hibernate";
+            } else if (strcmp(mode, "sleep") == 0) {
+                arg = "--suspend";
+            } else if (strcmp(mode, "hybrid") == 0) {
+                arg = "--suspend-hybrid";
+            } else {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            execlp(pmutils_bin, pmutils_bin, arg, NULL);
+
+            /*
+             * If we get here execlp() has failed. Let's try the manual
+             * approach if mode is not hybrid (as it's only suported by
+             * pm-utils)
+             */
+
+            if (strcmp(mode, "hybrid") == 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            sysfile = fopen(LINUX_SYS_STATE_FILE, "r");
+            if (!sysfile) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            if (!fgets(buf, sizeof(buf), sysfile)) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            if (strcmp(mode, "hibernate") == 0 && strstr(buf, "disk")) {
+                _exit(SUS_MODE_SUPPORTED);
+            } else if (strcmp(mode, "sleep") == 0 && strstr(buf, "mem")) {
+                _exit(SUS_MODE_SUPPORTED);
+            }
+
+            _exit(SUS_MODE_NOT_SUPPORTED);
+        }
+
+        if (pid > 0) {
+            wait(&status);
+        } else {
+            status = SUS_MODE_NOT_SUPPORTED;
+        }
+
+        ret = write(pipefds[1], &status, sizeof(status));
+        if (ret != sizeof(status)) {
+            _exit(1);
+        }
+
+        _exit(0);
+    }
+
+    ret = read(pipefds[0], &status, sizeof(status));
+    close(pipefds[0]);
+    close(pipefds[1]);
+
+    if (ret == sizeof(status) && WIFEXITED(status) &&
+        WEXITSTATUS(status) == SUS_MODE_SUPPORTED) {
+        return true;
+    }
+
+    return false;
+}
+
+static bool host_supports_mode(const char *mode)
+{
+    if (strcmp(mode, "hibernate")) {
+        /* sleep & hybrid are only supported in qemu 1.1.0 and above */
+        return ga_has_support_level(1, 1, 0);
+    }
+    return true;
+}
+
+void qmp_guest_suspend(const char *mode, Error **err)
+{
+    pid_t pid;
+    const char *pmutils_bin;
+    Error *local_err = NULL;
+
+    if (strcmp(mode, "hibernate") == 0) {
+        pmutils_bin = "pm-hibernate";
+    } else if (strcmp(mode, "sleep") == 0) {
+        pmutils_bin = "pm-suspend";
+    } else if (strcmp(mode, "hybrid") == 0) {
+        pmutils_bin = "pm-suspend-hybrid";
+    } else {
+        error_set(err, QERR_INVALID_PARAMETER, "mode");
+        return;
+    }
+
+    if (!host_supports_mode(mode)) {
+        error_set(err, QERR_UNSUPPORTED);
+        return;
+    }
+
+    if (!bios_supports_mode(mode, &local_err)) {
+        if (error_is_set(&local_err)) {
+            error_propagate(err, local_err);
+        } else {
+            error_set(err, QERR_UNSUPPORTED);
+        }
+        return;
+    }
+
+    pid = fork();
+    if (pid == 0) {
+        /* child */
+        int fd;
+        const char *cmd;
+
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        execlp(pmutils_bin, pmutils_bin, NULL);
+
+       /*
+        * If we get here execlp() has failed. Let's try the manual
+        * approach if mode is not hybrid (as it's only suported by
+        * pm-utils)
+        */
+
+        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
+        if (strcmp(mode, "hybrid") == 0) {
+            _exit(1);
+        }
+
+        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);
+        }
+
+        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
+        if (write(fd, cmd, strlen(cmd)) < 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)
 {
-- 
1.7.9.rc0.dirty

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 6717 bytes --]

On 01/13/2012 12:15 PM, Luiz Capitulino wrote:
> The guest-suspend command supports three modes:
> 
>  o hibernate (suspend to disk)
>  o sleep     (suspend to ram)
>  o hybrid    (save RAM contents to disk, but suspend instead of
>               powering off)
> 
> 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.

Maybe make it clear that this handler is only in the parent process, and
that it not only collects, but also ignores, the status of all children.

> 
> This might look complex, but the final code is quite simple. The
> purpose of that approach is to allow qemu-ga to reap its children
> (semi-)automatically from its SIGCHLD handler.

Yes, given your desire for the top-level qemu-ga signal handler to be
simple, I can see why you did a double fork, so that the intermediate
child can change the SIGCHLD behavior and actually do a blocking wait in
the case where status should not be ignored.

> @@ -44,6 +45,21 @@ static void slog(const char *fmt, ...)
>      va_end(ap);
>  }
>  
> +static int reopen_fd_to_null(int fd)
> +{
> +    int ret, nullfd;
> +
> +    nullfd = open("/dev/null", O_RDWR);
> +    if (nullfd < 0) {
> +        return -1;
> +    }
> +
> +    ret = dup2(nullfd, fd);
> +    close(nullfd);

Oops - if stdin was closed prior to entering this function, then
reopen_fd_to_null(0) will leave stdin closed on exit.  You need to check
that nullfd != fd before closing nullfd.

> +
> +    return ret;

What good is returning a failure value if the callers ignore it?  I
think you should fix the callers.

> +static bool bios_supports_mode(const char *mode, Error **err)
> +{
> +    pid_t pid;
> +    ssize_t ret;
> +    int status, pipefds[2];
> +
> +    if (pipe(pipefds) < 0) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +        return false;
> +    }
> +
> +    pid = fork();
> +    if (!pid) {
> +        struct sigaction act;
> +
> +        memset(&act, 0, sizeof(act));
> +        act.sa_handler = SIG_DFL;
> +        sigaction(SIGCHLD, &act, NULL);
> +
> +        setsid();
> +        close(pipefds[0]);
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);

Here's where I'd check for reopen failure.

> +
> +        pid = fork();
> +        if (!pid) {
> +            char buf[32];
> +            FILE *sysfile;
> +            const char *arg;
> +            const char *pmutils_bin = "pm-is-supported";
> +
> +            if (strcmp(mode, "hibernate") == 0) {

Strangely enough, POSIX doesn't include strcmp() in its list of
async-signal-safe functions (which is what you should be restricting
yourself to, if qemu-ga is multi-threaded), but in practice, I think
that is a bug of omission in POSIX, and not something you have to change
in your code.

> +                arg = "--hibernate";
> +            } else if (strcmp(mode, "sleep") == 0) {
> +                arg = "--suspend";
> +            } else if (strcmp(mode, "hybrid") == 0) {
> +                arg = "--suspend-hybrid";
> +            } else {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            execlp(pmutils_bin, pmutils_bin, arg, NULL);

Do we really want to be relying on a PATH lookup, or should we be using
an absolute path in pmutils_bin?

> +
> +            /*
> +             * If we get here execlp() has failed. Let's try the manual
> +             * approach if mode is not hybrid (as it's only suported by

s/suported/supported/

> +             * pm-utils)
> +             */
> +
> +            if (strcmp(mode, "hybrid") == 0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            sysfile = fopen(LINUX_SYS_STATE_FILE, "r");

fopen() is _not_ async-signal-safe.  You need to use open() and read(),
not fopen() and fgets().

> +            if (!sysfile) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            if (!fgets(buf, sizeof(buf), sysfile)) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            if (strcmp(mode, "hibernate") == 0 && strstr(buf, "disk")) {
> +                _exit(SUS_MODE_SUPPORTED);
> +            } else if (strcmp(mode, "sleep") == 0 && strstr(buf, "mem")) {
> +                _exit(SUS_MODE_SUPPORTED);
> +            }
> +
> +            _exit(SUS_MODE_NOT_SUPPORTED);
> +        }
> +
> +        if (pid > 0) {
> +            wait(&status);
> +        } else {
> +            status = SUS_MODE_NOT_SUPPORTED;
> +        }
> +
> +        ret = write(pipefds[1], &status, sizeof(status));
> +        if (ret != sizeof(status)) {
> +            _exit(1);

I prefer EXIT_SUCCESS and EXIT_FAILURE (from <stdlib.h>) rather than 0
and 1 when using [_]exit(), but I don't know the qemu project
conventions well enough to know if you need to change things.

> +        }
> +
> +        _exit(0);
> +    }
> +
> +    ret = read(pipefds[0], &status, sizeof(status));

You never check in the parent whether pid is -1, but blindly proceed to
do a read(); which means you will hang qemu-ga if the fork failed and
there is no process do write on the other end of the pipe.

> +    close(pipefds[0]);
> +    close(pipefds[1]);

For that matter, you should call close(pipefds[1]) _prior_ to the
read(), so that you aren't holding a writer open and can detect EOF on
the read() once the child exits.

> +void qmp_guest_suspend(const char *mode, Error **err)
> +{

> +
> +    pid = fork();
> +    if (pid == 0) {
> +        /* child */
> +        int fd;
> +        const char *cmd;
> +
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);

Check for errors?

> +
> +        execlp(pmutils_bin, pmutils_bin, NULL);

Again, is searching PATH okay?

> +
> +       /*
> +        * If we get here execlp() has failed. Let's try the manual
> +        * approach if mode is not hybrid (as it's only suported by
> +        * pm-utils)
> +        */
> +
> +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));

I didn't check whether slog() is async-signal safe (probably not, since
even snprintf() is not async-signal safe, and you are passing a printf
style format string).  But strerror() is not, so you shouldn't be using
it in the child if qemu-ga is multithreaded.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Jamie Lokier]

Eric Blake wrote:
> On 01/13/2012 12:15 PM, Luiz Capitulino wrote:
> > This might look complex, but the final code is quite simple. The
> > purpose of that approach is to allow qemu-ga to reap its children
> > (semi-)automatically from its SIGCHLD handler.
> 
> Yes, given your desire for the top-level qemu-ga signal handler to be
> simple, I can see why you did a double fork, so that the intermediate
> child can change the SIGCHLD behavior and actually do a blocking wait in
> the case where status should not be ignored.

An alternative is for SIGCHLD to write a byte to a non-blocking pipe
and do nothing else.  A main loop outside signal context reads from
the pipe, and on each read triggers a subloop of non-blocking
waitpid() getting child statuses until there are no more.  Because
it's outside signal context, it's safe to do anything with the child
statuses.

(A long time ago, on other unixes, this wasn't possible because
SIGCHLD would be retriggered until wait(), but it's not relevant on
anything modern.)

> > +            execlp(pmutils_bin, pmutils_bin, arg, NULL);
> 
> Do we really want to be relying on a PATH lookup, or should we be using
> an absolute path in pmutils_bin?

Since you mention async-signal-safety, execlp() isn't
async-signal-safe!  Last time I checked, in Glibc execlp() could call
malloc().  Also reading PATH looks at the environment, which isn't
always thread-safe either, depending on what else is going on.

I'm not sure if it's relevant to the this code, but on Glibc fork() is
not async-signal-safe and has been known to crash in signal handlers.
This is why fork() was removed from SUS async-signal-safe functions.

> I didn't check whether slog() is async-signal safe (probably not, since
> even snprintf() is not async-signal safe, and you are passing a printf
> style format string).  But strerror() is not, so you shouldn't be using
> it in the child if qemu-ga is multithreaded.

In general, why is multithreadedness relevant to async-signal-safety here?

Thanks,
-- Jamie

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

On 01/16/2012 03:51 AM, Jamie Lokier wrote:
> 
> Since you mention async-signal-safety, execlp() isn't
> async-signal-safe!  Last time I checked, in Glibc execlp() could call
> malloc().  Also reading PATH looks at the environment, which isn't
> always thread-safe either, depending on what else is going on.

That's why I questioned the use of a PATH lookup in the child.  Doing a
PATH lookup in the parent, then using an absolute name in the execle()
of the child, is indeed better.

> 
> I'm not sure if it's relevant to the this code, but on Glibc fork() is
> not async-signal-safe and has been known to crash in signal handlers.
> This is why fork() was removed from SUS async-signal-safe functions.

fork() is still in the list of async-signal-safe functions [1]; it was
only pthread_atfork() which was removed.  That is, fork() is _required_
to be async-signal-safe (and usable from signal handlers), provided that
the actions following the fork also follow safety rules.

[1]
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03

> 
>> I didn't check whether slog() is async-signal safe (probably not, since
>> even snprintf() is not async-signal safe, and you are passing a printf
>> style format string).  But strerror() is not, so you shouldn't be using
>> it in the child if qemu-ga is multithreaded.
> 
> In general, why is multithreadedness relevant to async-signal-safety here?

Because POSIX 2008 (SUS inherits from POSIX, so it has the same
restriction) states that if a multithreaded app calls fork, the child
can only portably use async-signal-safe functions up until a successful
exec or _exit.  Even though the child is _not_ operating in a signal
handler context, it _is_ operating in a context of a single thread where
other threads from the parent may have been holding locks, and thus
calling any unsafe function (that is, any function that tries to obtain
a lock) may deadlock.

I don't know if qemu-ga is intended to be a multi-threaded app, so I
don't know if being paranoid about async-signal-safety matters in this
particular case, but I _do_ know that libvirt has encountered issues
with using non-safe functions prior to exec, which is why it always
raises red flags when I see unsafe code between fork and exec.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Jamie Lokier]

Eric Blake wrote:
> On 01/16/2012 03:51 AM, Jamie Lokier wrote:
> > I'm not sure if it's relevant to the this code, but on Glibc fork() is
> > not async-signal-safe and has been known to crash in signal handlers.
> > This is why fork() was removed from SUS async-signal-safe functions.
> 
> fork() is still in the list of async-signal-safe functions [1];

You're right, but it looks like it may be removed in the next edition:

   https://www.opengroup.org/austin/docs/austin_446.txt

> it was only pthread_atfork() which was removed.

I didn't think pthread_atfork() ever was async-signal-safe.

> That is, fork() is _required_
> to be async-signal-safe (and usable from signal handlers), provided that
> the actions following the fork also follow safety rules.

Nonethless, Glibc fork() isn't async-signal-safe even if it should be:

    http://sourceware.org/bugzilla/show_bug.cgi?id=4737
> > In general, why is multithreadedness relevant to async-signal-safety here?
> 
> Because POSIX 2008 (SUS inherits from POSIX, so it has the same
> restriction) states that if a multithreaded app calls fork, the child
> can only portably use async-signal-safe functions up until a successful
> exec or _exit.  Even though the child is _not_ operating in a signal
> handler context, it _is_ operating in a context of a single thread where
> other threads from the parent may have been holding locks, and thus
> calling any unsafe function (that is, any function that tries to obtain
> a lock) may deadlock.

Somewhat confusing, when you have pthread_atfork() existing for the
entire purpose of allowing non-async-signal-safe functions, provided
the application isn't multithreaded, but libraries can be (I'm not
sure what the difference between application and library is in this
context).

http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_atfork.html

    It is suggested that programs that use fork() call an exec function
    very soon afterwards in the child process, thus resetting all
    states. In the meantime, only a short list of async-signal-safe
    library routines are promised to be available.

    Unfortunately, this solution does not address the needs of
    multi-threaded libraries. Application programs may not be aware that a
    multi-threaded library is in use, and they feel free to call any
    number of library routines between the fork() and exec calls, just as
    they always have. Indeed, they may be extant single-threaded programs
    and cannot, therefore, be expected to obey new restrictions imposed by
    the threads library.

> I don't know if qemu-ga is intended to be a multi-threaded app, so I
> don't know if being paranoid about async-signal-safety matters in this
> particular case, but I _do_ know that libvirt has encountered issues
> with using non-safe functions prior to exec, which is why it always
> raises red flags when I see unsafe code between fork and exec.

Quite right, I agree. :-)

-- Jamie

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On 01/17/2012 03:57 AM, Jamie Lokier wrote:
> You're right, but it looks like it may be removed in the next edition:
> 
>    https://www.opengroup.org/austin/docs/austin_446.txt
> 
>> it was only pthread_atfork() which was removed.
> 
> I didn't think pthread_atfork() ever was async-signal-safe.
> 
>> That is, fork() is _required_
>> to be async-signal-safe (and usable from signal handlers), provided that
>> the actions following the fork also follow safety rules.
> 
> Nonethless, Glibc fork() isn't async-signal-safe even if it should be:
> 
>     http://sourceware.org/bugzilla/show_bug.cgi?id=4737

Thanks for the (depressing) pointers.  You posted the link to the Austin
Group meeting where fork() was discusses; here's a further link to the
actual defect and resolution, which is that the next version of POSIX
_will_ be removing fork() from the list of async-signal-safe functions,
by replacing it with _Fork() which does _not_ call any of the
pthread_atfork() handlers:

http://austingroupbugs.net/view.php?id=62

You are right that the only reason that fork() is not signal-safe is
because of pthread_atfork(), so I was almost right in my above
characterization that pthread_atfork() was the culprit.  Maybe we should
start probing at configure time whether _Fork already exists, and if so,
use it instead of fork().

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Luiz Capitulino]

On Fri, 13 Jan 2012 14:48:04 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 01/13/2012 12:15 PM, Luiz Capitulino wrote:
> > The guest-suspend command supports three modes:
> > 
> >  o hibernate (suspend to disk)
> >  o sleep     (suspend to ram)
> >  o hybrid    (save RAM contents to disk, but suspend instead of
> >               powering off)
> > 
> > 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.
> 
> Maybe make it clear that this handler is only in the parent process, and
> that it not only collects, but also ignores, the status of all children.
> 
> > 
> > This might look complex, but the final code is quite simple. The
> > purpose of that approach is to allow qemu-ga to reap its children
> > (semi-)automatically from its SIGCHLD handler.
> 
> Yes, given your desire for the top-level qemu-ga signal handler to be
> simple, I can see why you did a double fork, so that the intermediate
> child can change the SIGCHLD behavior and actually do a blocking wait in
> the case where status should not be ignored.
> 
> > @@ -44,6 +45,21 @@ static void slog(const char *fmt, ...)
> >      va_end(ap);
> >  }
> >  
> > +static int reopen_fd_to_null(int fd)
> > +{
> > +    int ret, nullfd;
> > +
> > +    nullfd = open("/dev/null", O_RDWR);
> > +    if (nullfd < 0) {
> > +        return -1;
> > +    }
> > +
> > +    ret = dup2(nullfd, fd);
> > +    close(nullfd);
> 
> Oops - if stdin was closed prior to entering this function, then
> reopen_fd_to_null(0) will leave stdin closed on exit.  You need to check
> that nullfd != fd before closing nullfd.

Oh, good catch.

> 
> > +
> > +    return ret;
> 
> What good is returning a failure value if the callers ignore it?  I
> think you should fix the callers.

I did it generic enough in case it becomes useful for something else, but
I'm not checking for errors on purpose (more below).

> 
> > +static bool bios_supports_mode(const char *mode, Error **err)
> > +{
> > +    pid_t pid;
> > +    ssize_t ret;
> > +    int status, pipefds[2];
> > +
> > +    if (pipe(pipefds) < 0) {
> > +        error_set(err, QERR_UNDEFINED_ERROR);
> > +        return false;
> > +    }
> > +
> > +    pid = fork();
> > +    if (!pid) {
> > +        struct sigaction act;
> > +
> > +        memset(&act, 0, sizeof(act));
> > +        act.sa_handler = SIG_DFL;
> > +        sigaction(SIGCHLD, &act, NULL);
> > +
> > +        setsid();
> > +        close(pipefds[0]);
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> 
> Here's where I'd check for reopen failure.

The only thing I can think of doing if reopen_fd_to_null() fails is to exit.
This would indicate that the suspend mode in question is not supported.

I don't think that that failure is that catastrophic, so I think
errors should be ignored (I can change reopen_fd_to_null() to return
void...)

> 
> > +
> > +        pid = fork();
> > +        if (!pid) {
> > +            char buf[32];
> > +            FILE *sysfile;
> > +            const char *arg;
> > +            const char *pmutils_bin = "pm-is-supported";
> > +
> > +            if (strcmp(mode, "hibernate") == 0) {
> 
> Strangely enough, POSIX doesn't include strcmp() in its list of
> async-signal-safe functions (which is what you should be restricting
> yourself to, if qemu-ga is multi-threaded), but in practice, I think
> that is a bug of omission in POSIX, and not something you have to change
> in your code.
> 
> > +                arg = "--hibernate";
> > +            } else if (strcmp(mode, "sleep") == 0) {
> > +                arg = "--suspend";
> > +            } else if (strcmp(mode, "hybrid") == 0) {
> > +                arg = "--suspend-hybrid";
> > +            } else {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            execlp(pmutils_bin, pmutils_bin, arg, NULL);
> 
> Do we really want to be relying on a PATH lookup, or should we be using
> an absolute path in pmutils_bin?

Doing PATH lookup is a good idea because qemu-ga can be installed on different
Linux distros.

Now, Jamie Lokier correctly pointed out in another email that execlp() is not
async-signal-safe... We can't use it then.

Michael, I will change to execle() or execve() and use a hardcoded absolute
path. Do you have anything to add?

> 
> > +
> > +            /*
> > +             * If we get here execlp() has failed. Let's try the manual
> > +             * approach if mode is not hybrid (as it's only suported by
> 
> s/suported/supported/
> 
> > +             * pm-utils)
> > +             */
> > +
> > +            if (strcmp(mode, "hybrid") == 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            sysfile = fopen(LINUX_SYS_STATE_FILE, "r");
> 
> fopen() is _not_ async-signal-safe.  You need to use open() and read(),
> not fopen() and fgets().

Hah, I completely forgot about it when doing this :)

> 
> > +            if (!sysfile) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (!fgets(buf, sizeof(buf), sysfile)) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (strcmp(mode, "hibernate") == 0 && strstr(buf, "disk")) {
> > +                _exit(SUS_MODE_SUPPORTED);
> > +            } else if (strcmp(mode, "sleep") == 0 && strstr(buf, "mem")) {
> > +                _exit(SUS_MODE_SUPPORTED);
> > +            }
> > +
> > +            _exit(SUS_MODE_NOT_SUPPORTED);
> > +        }
> > +
> > +        if (pid > 0) {
> > +            wait(&status);
> > +        } else {
> > +            status = SUS_MODE_NOT_SUPPORTED;
> > +        }
> > +
> > +        ret = write(pipefds[1], &status, sizeof(status));
> > +        if (ret != sizeof(status)) {
> > +            _exit(1);
> 
> I prefer EXIT_SUCCESS and EXIT_FAILURE (from <stdlib.h>) rather than 0
> and 1 when using [_]exit(), but I don't know the qemu project
> conventions well enough to know if you need to change things.
> 
> > +        }
> > +
> > +        _exit(0);
> > +    }
> > +
> > +    ret = read(pipefds[0], &status, sizeof(status));
> 
> You never check in the parent whether pid is -1, but blindly proceed to
> do a read(); which means you will hang qemu-ga if the fork failed and
> there is no process do write on the other end of the pipe.
> 
> > +    close(pipefds[0]);
> > +    close(pipefds[1]);
> 
> For that matter, you should call close(pipefds[1]) _prior_ to the
> read(), so that you aren't holding a writer open and can detect EOF on
> the read() once the child exits.
> 
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> 
> > +
> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +        const char *cmd;
> > +
> > +        setsid();
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> 
> Check for errors?
> 
> > +
> > +        execlp(pmutils_bin, pmutils_bin, NULL);
> 
> Again, is searching PATH okay?
> 
> > +
> > +       /*
> > +        * If we get here execlp() has failed. Let's try the manual
> > +        * approach if mode is not hybrid (as it's only suported by
> > +        * pm-utils)
> > +        */
> > +
> > +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> 
> I didn't check whether slog() is async-signal safe (probably not, since
> even snprintf() is not async-signal safe, and you are passing a printf
> style format string).  But strerror() is not, so you shouldn't be using
> it in the child if qemu-ga is multithreaded.
> 

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

[Luiz Capitulino]

On Fri, 13 Jan 2012 14:48:04 -0700
Eric Blake <eblake@redhat.com> wrote:

> > +
> > +        pid = fork();
> > +        if (!pid) {
> > +            char buf[32];
> > +            FILE *sysfile;
> > +            const char *arg;
> > +            const char *pmutils_bin = "pm-is-supported";
> > +
> > +            if (strcmp(mode, "hibernate") == 0) {
> 
> Strangely enough, POSIX doesn't include strcmp() in its list of
> async-signal-safe functions (which is what you should be restricting
> yourself to, if qemu-ga is multi-threaded), but in practice, I think
> that is a bug of omission in POSIX, and not something you have to change
> in your code.

memset() ins't either... sigaction() either, which begins to get
annoying.

For those familiar with glib: isn't it possible to confirm it's using
threads and/or acquire a global mutex or something?

> 
> > +                arg = "--hibernate";
> > +            } else if (strcmp(mode, "sleep") == 0) {
> > +                arg = "--suspend";
> > +            } else if (strcmp(mode, "hybrid") == 0) {
> > +                arg = "--suspend-hybrid";
> > +            } else {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            execlp(pmutils_bin, pmutils_bin, arg, NULL);
> 
> Do we really want to be relying on a PATH lookup, or should we be using
> an absolute path in pmutils_bin?
> 
> > +
> > +            /*
> > +             * If we get here execlp() has failed. Let's try the manual
> > +             * approach if mode is not hybrid (as it's only suported by
> 
> s/suported/supported/
> 
> > +             * pm-utils)
> > +             */
> > +
> > +            if (strcmp(mode, "hybrid") == 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            sysfile = fopen(LINUX_SYS_STATE_FILE, "r");
> 
> fopen() is _not_ async-signal-safe.  You need to use open() and read(),
> not fopen() and fgets().
> 
> > +            if (!sysfile) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (!fgets(buf, sizeof(buf), sysfile)) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (strcmp(mode, "hibernate") == 0 && strstr(buf, "disk")) {
> > +                _exit(SUS_MODE_SUPPORTED);
> > +            } else if (strcmp(mode, "sleep") == 0 && strstr(buf, "mem")) {
> > +                _exit(SUS_MODE_SUPPORTED);
> > +            }
> > +
> > +            _exit(SUS_MODE_NOT_SUPPORTED);
> > +        }
> > +
> > +        if (pid > 0) {
> > +            wait(&status);
> > +        } else {
> > +            status = SUS_MODE_NOT_SUPPORTED;
> > +        }
> > +
> > +        ret = write(pipefds[1], &status, sizeof(status));
> > +        if (ret != sizeof(status)) {
> > +            _exit(1);
> 
> I prefer EXIT_SUCCESS and EXIT_FAILURE (from <stdlib.h>) rather than 0
> and 1 when using [_]exit(), but I don't know the qemu project
> conventions well enough to know if you need to change things.
> 
> > +        }
> > +
> > +        _exit(0);
> > +    }
> > +
> > +    ret = read(pipefds[0], &status, sizeof(status));
> 
> You never check in the parent whether pid is -1, but blindly proceed to
> do a read(); which means you will hang qemu-ga if the fork failed and
> there is no process do write on the other end of the pipe.
> 
> > +    close(pipefds[0]);
> > +    close(pipefds[1]);
> 
> For that matter, you should call close(pipefds[1]) _prior_ to the
> read(), so that you aren't holding a writer open and can detect EOF on
> the read() once the child exits.
> 
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> 
> > +
> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +        const char *cmd;
> > +
> > +        setsid();
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> 
> Check for errors?
> 
> > +
> > +        execlp(pmutils_bin, pmutils_bin, NULL);
> 
> Again, is searching PATH okay?
> 
> > +
> > +       /*
> > +        * If we get here execlp() has failed. Let's try the manual
> > +        * approach if mode is not hybrid (as it's only suported by
> > +        * pm-utils)
> > +        */
> > +
> > +        slog("could not execute %s: %s\n", pmutils_bin, strerror(errno));
> 
> I didn't check whether slog() is async-signal safe (probably not, since
> even snprintf() is not async-signal safe, and you are passing a printf
> style format string).  But strerror() is not, so you shouldn't be using
> it in the child if qemu-ga is multithreaded.
> 

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

[Daniel P. Berrange]

On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
> On Fri, 13 Jan 2012 14:48:04 -0700
> Eric Blake <eblake@redhat.com> wrote:
> 
> > > +
> > > +        pid = fork();
> > > +        if (!pid) {
> > > +            char buf[32];
> > > +            FILE *sysfile;
> > > +            const char *arg;
> > > +            const char *pmutils_bin = "pm-is-supported";
> > > +
> > > +            if (strcmp(mode, "hibernate") == 0) {
> > 
> > Strangely enough, POSIX doesn't include strcmp() in its list of
> > async-signal-safe functions (which is what you should be restricting
> > yourself to, if qemu-ga is multi-threaded), but in practice, I think
> > that is a bug of omission in POSIX, and not something you have to change
> > in your code.
> 
> memset() ins't either... sigaction() either, which begins to get
> annoying.
> 
> For those familiar with glib: isn't it possible to confirm it's using
> threads and/or acquire a global mutex or something?

The most that GLib says is

  "The GLib threading system used to be initialized with g_thread_init().
   This is no longer necessary. Since version 2.32, the GLib threading
   system is automatically initialized at the start of your program,
   and all thread-creation functions and synchronization primitives
   are available right away.

   Note that it is not safe to assume that your program has no threads
   even if you don't call g_thread_new() yourself. GLib and GIO can
   and will create threads for their own purposes in some cases, such
   as when using g_unix_signal_source_new() or when using GDBus. "

The latter paragraph is rather fuzzy, which is probably intentional.
So I think the only safe thing, in order to be future proof wrt later
GLib releases, is to just assume you have threads at all times.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Luiz Capitulino]

On Mon, 16 Jan 2012 17:13:39 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
> > On Fri, 13 Jan 2012 14:48:04 -0700
> > Eric Blake <eblake@redhat.com> wrote:
> > 
> > > > +
> > > > +        pid = fork();
> > > > +        if (!pid) {
> > > > +            char buf[32];
> > > > +            FILE *sysfile;
> > > > +            const char *arg;
> > > > +            const char *pmutils_bin = "pm-is-supported";
> > > > +
> > > > +            if (strcmp(mode, "hibernate") == 0) {
> > > 
> > > Strangely enough, POSIX doesn't include strcmp() in its list of
> > > async-signal-safe functions (which is what you should be restricting
> > > yourself to, if qemu-ga is multi-threaded), but in practice, I think
> > > that is a bug of omission in POSIX, and not something you have to change
> > > in your code.
> > 
> > memset() ins't either... sigaction() either, which begins to get
> > annoying.
> > 
> > For those familiar with glib: isn't it possible to confirm it's using
> > threads and/or acquire a global mutex or something?
> 
> The most that GLib says is
> 
>   "The GLib threading system used to be initialized with g_thread_init().
>    This is no longer necessary. Since version 2.32, the GLib threading
>    system is automatically initialized at the start of your program,
>    and all thread-creation functions and synchronization primitives
>    are available right away.
> 
>    Note that it is not safe to assume that your program has no threads
>    even if you don't call g_thread_new() yourself. GLib and GIO can
>    and will create threads for their own purposes in some cases, such
>    as when using g_unix_signal_source_new() or when using GDBus. "
> 
> The latter paragraph is rather fuzzy, which is probably intentional.
> So I think the only safe thing, in order to be future proof wrt later
> GLib releases, is to just assume you have threads at all times.

Yeah, and we do use GIO in qemu-ga...

Thanks Daniel.

> 
> 
> Daniel

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

[Luiz Capitulino]

On Mon, 16 Jan 2012 15:18:37 -0200
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Mon, 16 Jan 2012 17:13:39 +0000
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
> > > On Fri, 13 Jan 2012 14:48:04 -0700
> > > Eric Blake <eblake@redhat.com> wrote:
> > > 
> > > > > +
> > > > > +        pid = fork();
> > > > > +        if (!pid) {
> > > > > +            char buf[32];
> > > > > +            FILE *sysfile;
> > > > > +            const char *arg;
> > > > > +            const char *pmutils_bin = "pm-is-supported";
> > > > > +
> > > > > +            if (strcmp(mode, "hibernate") == 0) {
> > > > 
> > > > Strangely enough, POSIX doesn't include strcmp() in its list of
> > > > async-signal-safe functions (which is what you should be restricting
> > > > yourself to, if qemu-ga is multi-threaded), but in practice, I think
> > > > that is a bug of omission in POSIX, and not something you have to change
> > > > in your code.
> > > 
> > > memset() ins't either... sigaction() either, which begins to get
> > > annoying.
> > > 
> > > For those familiar with glib: isn't it possible to confirm it's using
> > > threads and/or acquire a global mutex or something?

Misread, sigaction() is there. The ones that aren't are strcmp(), strstr()
and memset(). Interestingly, they are all "string functions".


> > 
> > The most that GLib says is
> > 
> >   "The GLib threading system used to be initialized with g_thread_init().
> >    This is no longer necessary. Since version 2.32, the GLib threading
> >    system is automatically initialized at the start of your program,
> >    and all thread-creation functions and synchronization primitives
> >    are available right away.
> > 
> >    Note that it is not safe to assume that your program has no threads
> >    even if you don't call g_thread_new() yourself. GLib and GIO can
> >    and will create threads for their own purposes in some cases, such
> >    as when using g_unix_signal_source_new() or when using GDBus. "
> > 
> > The latter paragraph is rather fuzzy, which is probably intentional.
> > So I think the only safe thing, in order to be future proof wrt later
> > GLib releases, is to just assume you have threads at all times.
> 
> Yeah, and we do use GIO in qemu-ga...
> 
> Thanks Daniel.
> 
> > 
> > 
> > Daniel
> 

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

[Michael Roth]

On 01/16/2012 11:23 AM, Luiz Capitulino wrote:
> On Mon, 16 Jan 2012 15:18:37 -0200
> Luiz Capitulino<lcapitulino@redhat.com>  wrote:
>
>> On Mon, 16 Jan 2012 17:13:39 +0000
>> "Daniel P. Berrange"<berrange@redhat.com>  wrote:
>>
>>> On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
>>>> On Fri, 13 Jan 2012 14:48:04 -0700
>>>> Eric Blake<eblake@redhat.com>  wrote:
>>>>
>>>>>> +
>>>>>> +        pid = fork();
>>>>>> +        if (!pid) {
>>>>>> +            char buf[32];
>>>>>> +            FILE *sysfile;
>>>>>> +            const char *arg;
>>>>>> +            const char *pmutils_bin = "pm-is-supported";
>>>>>> +
>>>>>> +            if (strcmp(mode, "hibernate") == 0) {
>>>>>
>>>>> Strangely enough, POSIX doesn't include strcmp() in its list of
>>>>> async-signal-safe functions (which is what you should be restricting
>>>>> yourself to, if qemu-ga is multi-threaded), but in practice, I think
>>>>> that is a bug of omission in POSIX, and not something you have to change
>>>>> in your code.
>>>>
>>>> memset() ins't either... sigaction() either, which begins to get
>>>> annoying.
>>>>
>>>> For those familiar with glib: isn't it possible to confirm it's using
>>>> threads and/or acquire a global mutex or something?
>
> Misread, sigaction() is there. The ones that aren't are strcmp(), strstr()
> and memset(). Interestingly, they are all "string functions".
>

There seem to be things beyond that list required to be implemented as 
thread/signal safe:

http://www.unix.org/whitepapers/reentrant.html

fread()/fwrite()/f* for instance, more at `man flockfile`:

        The  stdio  functions are thread-safe.
        This is achieved by assigning to  each
        FILE  object  a  lockcount and (if the
        lockcount  is   nonzero)   an   owning
        thread.   For each library call, these
        functions wait until the  FILE  object
        is  no  longer  locked  by a different
        thread, then lock it, do the requested
        I/O, and unlock the object again.

glib seems to give itself at least some liberty in confirming whether a 
function beyond the POSIX-confirmed ones are thread safe. 
glib/gthreadpool.c:169 calls g_get_current_time(), for instance, which 
calls gettimeofday(), which isn't on the list (but does happen to be 
thread-safe). This technically renders a substantial number of functions 
glib exposes in it's APIs unsafe, since a large number of those also use 
g_get_current_time()/gettimeofday() and don't do any thread 
synchronization. The situation seems to be even more lax on win32 
(memcpy, memmove, strcmp in their GIO reader thread, for instance), but 
I'm not sure what the story is there WRT to thread safety.

qemu as well, we use memcpy/memmove/memset/fread/printf/etc even though 
it has the same glib dependencies as qemu-ga, and I don't think it's 
realistic to consider removing them.

In practice, are these functions really a problem for multi-threaded 
applications (beyond concurrent access to shared storage)? Maybe it 
would be sufficient to just check the glibc sources?

>
>>>
>>> The most that GLib says is
>>>
>>>    "The GLib threading system used to be initialized with g_thread_init().
>>>     This is no longer necessary. Since version 2.32, the GLib threading
>>>     system is automatically initialized at the start of your program,
>>>     and all thread-creation functions and synchronization primitives
>>>     are available right away.
>>>
>>>     Note that it is not safe to assume that your program has no threads
>>>     even if you don't call g_thread_new() yourself. GLib and GIO can
>>>     and will create threads for their own purposes in some cases, such
>>>     as when using g_unix_signal_source_new() or when using GDBus. "
>>>
>>> The latter paragraph is rather fuzzy, which is probably intentional.
>>> So I think the only safe thing, in order to be future proof wrt later
>>> GLib releases, is to just assume you have threads at all times.
>>
>> Yeah, and we do use GIO in qemu-ga...
>>
>> Thanks Daniel.
>>
>>>
>>>
>>> Daniel
>>
>

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

[Daniel P. Berrange]

On Mon, Jan 16, 2012 at 02:02:08PM -0600, Michael Roth wrote:
> On 01/16/2012 11:23 AM, Luiz Capitulino wrote:
> >On Mon, 16 Jan 2012 15:18:37 -0200
> >Luiz Capitulino<lcapitulino@redhat.com>  wrote:
> >
> >>On Mon, 16 Jan 2012 17:13:39 +0000
> >>"Daniel P. Berrange"<berrange@redhat.com>  wrote:
> >>
> >>>On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
> >>>>On Fri, 13 Jan 2012 14:48:04 -0700
> >>>>Eric Blake<eblake@redhat.com>  wrote:
> >>>>
> >>>>>>+
> >>>>>>+        pid = fork();
> >>>>>>+        if (!pid) {
> >>>>>>+            char buf[32];
> >>>>>>+            FILE *sysfile;
> >>>>>>+            const char *arg;
> >>>>>>+            const char *pmutils_bin = "pm-is-supported";
> >>>>>>+
> >>>>>>+            if (strcmp(mode, "hibernate") == 0) {
> >>>>>
> >>>>>Strangely enough, POSIX doesn't include strcmp() in its list of
> >>>>>async-signal-safe functions (which is what you should be restricting
> >>>>>yourself to, if qemu-ga is multi-threaded), but in practice, I think
> >>>>>that is a bug of omission in POSIX, and not something you have to change
> >>>>>in your code.
> >>>>
> >>>>memset() ins't either... sigaction() either, which begins to get
> >>>>annoying.
> >>>>
> >>>>For those familiar with glib: isn't it possible to confirm it's using
> >>>>threads and/or acquire a global mutex or something?
> >
> >Misread, sigaction() is there. The ones that aren't are strcmp(), strstr()
> >and memset(). Interestingly, they are all "string functions".
> >
> 
> There seem to be things beyond that list required to be implemented
> as thread/signal safe:
> 
> http://www.unix.org/whitepapers/reentrant.html
> 
> fread()/fwrite()/f* for instance, more at `man flockfile`:
> 
>        The  stdio  functions are thread-safe.
>        This is achieved by assigning to  each
>        FILE  object  a  lockcount and (if the
>        lockcount  is   nonzero)   an   owning
>        thread.   For each library call, these
>        functions wait until the  FILE  object
>        is  no  longer  locked  by a different
>        thread, then lock it, do the requested
>        I/O, and unlock the object again.

You need to be careful with terminology here.

   Threadsafe != async signal safe

STDIO is one of the major areas of code that is definitely not
async signal safe. Consider Thread A doing something like
fwrite(stderr, "Foo\n"), while another thread forks, and then
its child also does an fwrite(stderr, "Foo\n"). Given that
every stdio function will lock/unlock a mutex, you easily get
this sequence of events:

1.     Thread A: lock(stderr)
2.     Thread A: write(stderr, "foo\n");
3.     Thread B: fork() -> Process B1
4.     Thread A: unlock(stderr)
5.   Process B1: lock(stderr)

When the child process is started at step 3, the FILE *stderr
object will be locked by thread A.  When Thread A does the
unlock in step 4, it has no effect on Process B1. So process
B1 hangs forever in step 5.

> In practice, are these functions really a problem for multi-threaded
> applications (beyond concurrent access to shared storage)? Maybe it
> would be sufficient to just check the glibc sources?

In libvirt we have seen the hang scenarios I describe in the real world.
Causes I rememeber were use of malloc (via asprintf()), or use of stdio
FILE * functions, and use of syslog. The libvirt code still isn't 100%
in compliance with avoiding async signal safe functions, but we have
cleaned up many problems in this area.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Michael Roth]

On 01/16/2012 02:35 PM, Daniel P. Berrange wrote:
> On Mon, Jan 16, 2012 at 02:02:08PM -0600, Michael Roth wrote:
>> On 01/16/2012 11:23 AM, Luiz Capitulino wrote:
>>> On Mon, 16 Jan 2012 15:18:37 -0200
>>> Luiz Capitulino<lcapitulino@redhat.com>   wrote:
>>>
>>>> On Mon, 16 Jan 2012 17:13:39 +0000
>>>> "Daniel P. Berrange"<berrange@redhat.com>   wrote:
>>>>
>>>>> On Mon, Jan 16, 2012 at 03:08:53PM -0200, Luiz Capitulino wrote:
>>>>>> On Fri, 13 Jan 2012 14:48:04 -0700
>>>>>> Eric Blake<eblake@redhat.com>   wrote:
>>>>>>
>>>>>>>> +
>>>>>>>> +        pid = fork();
>>>>>>>> +        if (!pid) {
>>>>>>>> +            char buf[32];
>>>>>>>> +            FILE *sysfile;
>>>>>>>> +            const char *arg;
>>>>>>>> +            const char *pmutils_bin = "pm-is-supported";
>>>>>>>> +
>>>>>>>> +            if (strcmp(mode, "hibernate") == 0) {
>>>>>>>
>>>>>>> Strangely enough, POSIX doesn't include strcmp() in its list of
>>>>>>> async-signal-safe functions (which is what you should be restricting
>>>>>>> yourself to, if qemu-ga is multi-threaded), but in practice, I think
>>>>>>> that is a bug of omission in POSIX, and not something you have to change
>>>>>>> in your code.
>>>>>>
>>>>>> memset() ins't either... sigaction() either, which begins to get
>>>>>> annoying.
>>>>>>
>>>>>> For those familiar with glib: isn't it possible to confirm it's using
>>>>>> threads and/or acquire a global mutex or something?
>>>
>>> Misread, sigaction() is there. The ones that aren't are strcmp(), strstr()
>>> and memset(). Interestingly, they are all "string functions".
>>>
>>
>> There seem to be things beyond that list required to be implemented
>> as thread/signal safe:
>>
>> http://www.unix.org/whitepapers/reentrant.html
>>
>> fread()/fwrite()/f* for instance, more at `man flockfile`:
>>
>>         The  stdio  functions are thread-safe.
>>         This is achieved by assigning to  each
>>         FILE  object  a  lockcount and (if the
>>         lockcount  is   nonzero)   an   owning
>>         thread.   For each library call, these
>>         functions wait until the  FILE  object
>>         is  no  longer  locked  by a different
>>         thread, then lock it, do the requested
>>         I/O, and unlock the object again.
>
> You need to be careful with terminology here.
>
>     Threadsafe != async signal safe
>
> STDIO is one of the major areas of code that is definitely not
> async signal safe. Consider Thread A doing something like
> fwrite(stderr, "Foo\n"), while another thread forks, and then
> its child also does an fwrite(stderr, "Foo\n"). Given that
> every stdio function will lock/unlock a mutex, you easily get
> this sequence of events:
>
> 1.     Thread A: lock(stderr)
> 2.     Thread A: write(stderr, "foo\n");
> 3.     Thread B: fork() ->  Process B1
> 4.     Thread A: unlock(stderr)
> 5.   Process B1: lock(stderr)
>
> When the child process is started at step 3, the FILE *stderr
> object will be locked by thread A.  When Thread A does the
> unlock in step 4, it has no effect on Process B1. So process
> B1 hangs forever in step 5.
>

Ahh, thanks for the example. I missed that these issues were 
specifically WRT to code that was fork()'d from a multi-threaded 
application. Seemed pretty scary otherwise :)

>> In practice, are these functions really a problem for multi-threaded
>> applications (beyond concurrent access to shared storage)? Maybe it
>> would be sufficient to just check the glibc sources?
>
> In libvirt we have seen the hang scenarios I describe in the real world.
> Causes I rememeber were use of malloc (via asprintf()), or use of stdio
> FILE * functions, and use of syslog. The libvirt code still isn't 100%
> in compliance with avoiding async signal safe functions, but we have
> cleaned up many problems in this area.

Thanks, looks like I have so fixes to do in qmp_guest_shutdown. syslog 
is really unfortunate...

>
> Regards,
> Daniel

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

[Jamie Lokier]

Michael Roth wrote:
> >STDIO is one of the major areas of code that is definitely not
> >async signal safe. Consider Thread A doing something like
> >fwrite(stderr, "Foo\n"), while another thread forks, and then
> >its child also does an fwrite(stderr, "Foo\n"). Given that
> >every stdio function will lock/unlock a mutex, you easily get
> >this sequence of events:
> >
> >1.     Thread A: lock(stderr)
> >2.     Thread A: write(stderr, "foo\n");
> >3.     Thread B: fork() ->  Process B1
> >4.     Thread A: unlock(stderr)
> >5.   Process B1: lock(stderr)
> >
> >When the child process is started at step 3, the FILE *stderr
> >object will be locked by thread A.  When Thread A does the
> >unlock in step 4, it has no effect on Process B1. So process
> >B1 hangs forever in step 5.
> 
> Ahh, thanks for the example. I missed that these issues were
> specifically WRT to code that was fork()'d from a multi-threaded
> application. Seemed pretty scary otherwise :)

The pthread_atfork() mechanism, or equivalent in libc, should be
sorting out those stdio locks, but I don't know for sure what Glibc does.

I do know it traverses a stdio list on fork() though:

   http://sourceware.org/bugzilla/show_bug.cgi?id=4737#c4

Which is why Glibc's fork() is not async-signal-safe even though it's
supposed to be.

stdio in a fork() child is historical unix stuff; I expect there are
quite a lot of old applications that use stdio in a child process.
Not multi-threaded applications, but they can link to multi-threaded
libraries these without knowing.

Still there are bugs around (like Glibc's fork() not being
async-signal-safe).  It pays to be cautious.

-- Jamie

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On 01/16/2012 10:08 AM, Luiz Capitulino wrote:
>> Strangely enough, POSIX doesn't include strcmp() in its list of
>> async-signal-safe functions (which is what you should be restricting
>> yourself to, if qemu-ga is multi-threaded), but in practice, I think
>> that is a bug of omission in POSIX, and not something you have to change
>> in your code.
> 
> memset() ins't either... sigaction() either, which begins to get
> annoying.

sigaction() is required by POSIX to be async-signal-safe.  Where are you
looking when claiming it isn't?

memset(), strlen, strcpy, and friends in <string.h> are all in the class
of functions that I think are unintentional omissions from the list of
async-signal-safe functions (they don't read/modify anything but the
pointers passed in, so the _only_ reason I can think of why they _might_
have been omitted from the list is that there might be some machine
state that could be observably different if you were interrupted in the
middle of one of these operations, such as a processor flag bit when
using a rep prefix on x86 controlling which direction to move, but no
one has ever pointed me to a definitive answer to why they were omitted).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Luiz Capitulino]

On Mon, 16 Jan 2012 13:08:23 -0700
Eric Blake <eblake@redhat.com> wrote:

> On 01/16/2012 10:08 AM, Luiz Capitulino wrote:
> >> Strangely enough, POSIX doesn't include strcmp() in its list of
> >> async-signal-safe functions (which is what you should be restricting
> >> yourself to, if qemu-ga is multi-threaded), but in practice, I think
> >> that is a bug of omission in POSIX, and not something you have to change
> >> in your code.
> > 
> > memset() ins't either... sigaction() either, which begins to get
> > annoying.
> 
> sigaction() is required by POSIX to be async-signal-safe.  Where are you
> looking when claiming it isn't?

I did a bad search on:

 http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html

when I wrote that email. A few seconds later I saw that sigaction() is there.

> memset(), strlen, strcpy, and friends in <string.h> are all in the class
> of functions that I think are unintentional omissions from the list of
> async-signal-safe functions (they don't read/modify anything but the
> pointers passed in, so the _only_ reason I can think of why they _might_
> have been omitted from the list is that there might be some machine
> state that could be observably different if you were interrupted in the
> middle of one of these operations, such as a processor flag bit when
> using a rep prefix on x86 controlling which direction to move, but no
> one has ever pointed me to a definitive answer to why they were omitted).

If this is right we shouldn't be using them then...

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On 01/16/2012 01:19 PM, Luiz Capitulino wrote:
>> memset(), strlen, strcpy, and friends in <string.h> are all in the class
>> of functions that I think are unintentional omissions from the list of
>> async-signal-safe functions (they don't read/modify anything but the
>> pointers passed in, so the _only_ reason I can think of why they _might_
>> have been omitted from the list is that there might be some machine
>> state that could be observably different if you were interrupted in the
>> middle of one of these operations, such as a processor flag bit when
>> using a rep prefix on x86 controlling which direction to move, but no
>> one has ever pointed me to a definitive answer to why they were omitted).
> 
> If this is right we shouldn't be using them then...

The _nice_ thing is that the functions in <string.h> are trivially
replaceable by naive variants that _are_ async-signal-safe, since the
algorithms behind them are so trivial.  It's just that it's annoying to
have to tell users that they have to write non-optimized code when doing
string ops in a signal handler or after a fork (C code tends to not be
as nice as the hand-tuned assembly in glibc for all these low-level
functions), for what so far appears to be a theoretical rather than a
confirmed restriction on why the standard does not require async-safety.

I guess it's time for me to follow through with my threat to file a bug
against the POSIX folks to get the string functions added to the list of
async-signal-safe, and/or give me stronger justification why they are
not already there.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH v6 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

Several fixes, but no major changes.

This series depends on this patch from Michael:

 http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg01382.html

v6

o improve schema documentation
o change reopen_fd_to_null() to return void
o fix reopen_fd_to_null() no to close stdin [Eric]
o use execle() and do PATH lookup in the parent [Eric/Jamie]
o do not use fopen()/fgets() [Eric]
o check fork() return value [Eric]
o use EXIT_SUCCESS/EXIT_FAILURE [Eric]

 qapi-schema-guest.json     |   32 ++++++
 qemu-ga.c                  |   20 +++-
 qga/guest-agent-commands.c |  263 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 313 insertions(+), 2 deletions(-)

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

[Luiz Capitulino]

The guest-suspend command supports three modes:

 o hibernate (suspend to disk)
 o sleep     (suspend to ram)
 o hybrid    (save RAM contents to disk, but suspend instead of
              powering off)

Before trying to suspend, the command queries the guest in order
to know whether the given mode is supported. The sleep and hybrid
modes are only supported in QEMU 1.1 and later though, because
QEMU's S3 support is broken in previous versions.

The guest-suspend command will use the scripts provided by the
pm-utils package if they are available. If they aren't, a manual
process which directly writes to the "/sys/power/state" file is
used.

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

The approach used to query the guest for suspend support deserves
some explanation. It's implemented by bios_supports_mode() and shown
below:

   qemu-ga
     |
 create pipe
     |
   fork()
     -----------------
     |               |
     |               |
     |             fork()
     |               --------------------------
     |               |                        |
     |               |                        |
     |               |               exec('pm-is-supported')
     |               |
     |              wait()
     |       write exit status to pipe
     |              exit
     |
  read pipe

This might look complex, but the resulting code is quite simple.
The purpose of that approach is to allow qemu-ga to reap its
children (semi-)automatically from its SIGCHLD handler.

Implementing this the obvious way, that's, doing the exec() call
from the first child process, would force us to introduce a more
complex way to reap qemu-ga's children. Like registering PIDs to
be reaped and having a way to wait for them when returning their
exit status to qemu-ga is necessary. The approach explained
above avoids that complexity.

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

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 5f8a18d..7dd9267 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -219,3 +219,35 @@
 ##
 { 'command': 'guest-fsfreeze-thaw',
   'returns': 'int' }
+
+##
+# @guest-suspend
+#
+# Suspend guest execution by entering ACPI power state S3 or S4.
+#
+# 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)
+#        'sleep'     execution is suspended but the RAM retains its contents
+#                    (this corresponds to ACPI S3)
+#        'hybrid'    RAM content is saved to the disk but the guest is
+#                    suspended instead of powering off
+#
+# Returns: nothing on success
+#          If @mode is not supported by the guest, Unsupported
+#
+# Notes: o This is an asynchronous request. There's no guarantee a response
+#          will be sent.
+#        o Errors will be logged to guest's syslog.
+#        o It's strongly recommended to issue the guest-sync command before
+#          sending commands when the guest resumes.
+#
+# Since: 1.1
+##
+{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
diff --git a/qemu-ga.c b/qemu-ga.c
index 647df82..f531084 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,16 @@ static void quit_handler(int sig)
     }
 }
 
+/* reap _all_ terminated children */
+static void child_handler(int sig)
+{
+    int status;
+    while (waitpid(-1, &status, WNOHANG) > 0) /* NOTHING */;
+}
+
 static void register_signal_handlers(void)
 {
-    struct sigaction sigact;
+    struct sigaction sigact, sigact_chld;
     int ret;
 
     memset(&sigact, 0, sizeof(struct sigaction));
@@ -76,6 +84,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..e64b0e0 100644
--- a/qga/guest-agent-commands.c
+++ b/qga/guest-agent-commands.c
@@ -23,6 +23,7 @@
 
 #include <sys/types.h>
 #include <sys/ioctl.h>
+#include <sys/wait.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qerror.h"
@@ -44,6 +45,54 @@ static void slog(const char *fmt, ...)
     va_end(ap);
 }
 
+static void reopen_fd_to_null(int fd)
+{
+    int nullfd;
+
+    nullfd = open("/dev/null", O_RDWR);
+    if (nullfd < 0) {
+        return;
+    }
+
+    dup2(nullfd, fd);
+
+    if (nullfd != fd) {
+        close(nullfd);
+    }
+}
+
+/* Try to find executable file 'file'. If it's found, its absolute path is
+   returned in 'abs_path' and the function returns true. If it's not found,
+   the function will return false and 'abs_path' will contain zeros */
+static bool find_executable_file(const char *file, char *abs_path, size_t len)
+{
+    char *path, *saveptr;
+    const char *token, *delim = ":";
+ 
+    if (!getenv("PATH")) {
+        return false;
+    }
+
+    path = g_strdup(getenv("PATH"));
+    if (!path) {
+        return false;
+    }
+
+    for (token = strtok_r(path, delim, &saveptr); token;
+         token = strtok_r(NULL, delim, &saveptr)) {
+        snprintf(abs_path, len, "%s/%s", token, file);
+        if (access(abs_path, X_OK) == 0) {
+            g_free(path);
+            return true;
+        }
+    }
+
+    g_free(path);
+    memset(abs_path, 0, len);
+
+    return false;
+}
+
 int64_t qmp_guest_sync(int64_t id, Error **errp)
 {
     return id;
@@ -574,6 +623,220 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
 }
 #endif
 
+#define LINUX_SYS_STATE_FILE "/sys/power/state"
+#define SUS_MODE_SUPPORTED 0
+#define SUS_MODE_NOT_SUPPORTED 1
+
+/**
+ * This function forks twice and the information about the mode support
+ * status is passed to the qemu-ga process via a pipe.
+ *
+ * This approach allows us to keep the way we reap terminated children
+ * in qemu-ga quite simple.
+ */
+static bool bios_supports_mode(const char *mode, Error **err)
+{
+    pid_t pid;
+    ssize_t ret;
+    bool has_pmutils;
+    int status, pipefds[2];
+    char pmutils_path[PATH_MAX];
+    const char *pmutils_bin = "pm-is-supported";
+
+    if (pipe(pipefds) < 0) {
+        error_set(err, QERR_UNDEFINED_ERROR);
+        return false;
+    }
+
+    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
+                                       sizeof(pmutils_path));
+
+    pid = fork();
+    if (!pid) {
+        struct sigaction act;
+
+        memset(&act, 0, sizeof(act));
+        act.sa_handler = SIG_DFL;
+        sigaction(SIGCHLD, &act, NULL);
+
+        setsid();
+        close(pipefds[0]);
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        pid = fork();
+        if (!pid) {
+            int fd;
+            char buf[32]; /* hopefully big enough */
+            const char *arg;
+
+            if (strcmp(mode, "hibernate") == 0) {
+                arg = "--hibernate";
+            } else if (strcmp(mode, "sleep") == 0) {
+                arg = "--suspend";
+            } else if (strcmp(mode, "hybrid") == 0) {
+                arg = "--suspend-hybrid";
+            } else {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            if (has_pmutils) {
+                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
+            }
+
+            /*
+             * If we get here either pm-utils is not installed or  execle() has
+             * failed. Let's try the manual approach if mode is not hybrid (as
+             * it's only supported by pm-utils)
+             */
+
+            if (strcmp(mode, "hybrid") == 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
+            if (fd < 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            ret = read(fd, buf, sizeof(buf));
+            if (ret <= 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            buf[ret] = '\0';
+            if (strcmp(mode, "hibernate") == 0 && strstr(buf, "disk")) {
+                _exit(SUS_MODE_SUPPORTED);
+            } else if (strcmp(mode, "sleep") == 0 && strstr(buf, "mem")) {
+                _exit(SUS_MODE_SUPPORTED);
+            }
+
+            _exit(SUS_MODE_NOT_SUPPORTED);
+        }
+
+        if (pid > 0) {
+            wait(&status);
+        } else {
+            status = SUS_MODE_NOT_SUPPORTED;
+        }
+
+        ret = write(pipefds[1], &status, sizeof(status));
+        if (ret != sizeof(status)) {
+            _exit(EXIT_FAILURE);
+        }
+
+        _exit(EXIT_SUCCESS);
+    }
+
+    close(pipefds[1]);
+
+    if (pid > 0) {
+        ret = read(pipefds[0], &status, sizeof(status));
+        if (ret == sizeof(status) && WIFEXITED(status) &&
+            WEXITSTATUS(status) == SUS_MODE_SUPPORTED) {
+            close(pipefds[0]);
+            return true;
+        }
+    }
+
+    close(pipefds[0]);
+    return false;
+}
+
+static bool host_supports_mode(const char *mode)
+{
+    if (strcmp(mode, "hibernate")) {
+        /* sleep & hybrid are only supported in qemu 1.1.0 and above */
+        return ga_has_support_level(1, 1, 0);
+    }
+    return true;
+}
+
+void qmp_guest_suspend(const char *mode, Error **err)
+{
+    pid_t pid;
+    bool has_pmutils;
+    Error *local_err = NULL;
+    const char *pmutils_bin;
+    char pmutils_path[PATH_MAX];
+
+    if (strcmp(mode, "hibernate") == 0) {
+        pmutils_bin = "pm-hibernate";
+    } else if (strcmp(mode, "sleep") == 0) {
+        pmutils_bin = "pm-suspend";
+    } else if (strcmp(mode, "hybrid") == 0) {
+        pmutils_bin = "pm-suspend-hybrid";
+    } else {
+        error_set(err, QERR_INVALID_PARAMETER, "mode");
+        return;
+    }
+
+    if (!host_supports_mode(mode)) {
+        error_set(err, QERR_UNSUPPORTED);
+        return;
+    }
+
+    if (!bios_supports_mode(mode, &local_err)) {
+        if (error_is_set(&local_err)) {
+            error_propagate(err, local_err);
+        } else {
+            error_set(err, QERR_UNSUPPORTED);
+        }
+        return;
+    }
+
+    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
+                                       sizeof(pmutils_path));
+
+    pid = fork();
+    if (pid == 0) {
+        /* child */
+        int fd;
+        const char *cmd;
+
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        if (has_pmutils) {
+            execle(pmutils_path, pmutils_bin, NULL, environ);
+        }
+
+        /*
+         * If we get here either pm-utils is not installed or  execle() has
+         * failed. Let's try the manual approach if mode is not hybrid (as
+         * it's only supported by pm-utils)
+         */
+
+        slog("could not execute %s\n", pmutils_bin);
+        if (strcmp(mode, "hybrid") == 0) {
+            _exit(EXIT_FAILURE);
+        }
+
+        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(EXIT_FAILURE);
+        }
+
+        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
+        if (write(fd, cmd, strlen(cmd)) < 0) {
+            slog("failued to write to %s\n", LINUX_SYS_STATE_FILE);
+            _exit(EXIT_FAILURE);
+        }
+
+        _exit(EXIT_SUCCESS);
+    } 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)
 {
-- 
1.7.9.rc0.dirty

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

[Daniel P. Berrange]

On Mon, Jan 16, 2012 at 06:09:52PM -0200, Luiz Capitulino wrote:

> +/* Try to find executable file 'file'. If it's found, its absolute path is
> +   returned in 'abs_path' and the function returns true. If it's not found,
> +   the function will return false and 'abs_path' will contain zeros */
> +static bool find_executable_file(const char *file, char *abs_path, size_t len)
> +{
> +    char *path, *saveptr;
> +    const char *token, *delim = ":";
> + 
> +    if (!getenv("PATH")) {
> +        return false;
> +    }
> +
> +    path = g_strdup(getenv("PATH"));
> +    if (!path) {
> +        return false;
> +    }
> +
> +    for (token = strtok_r(path, delim, &saveptr); token;
> +         token = strtok_r(NULL, delim, &saveptr)) {
> +        snprintf(abs_path, len, "%s/%s", token, file);
> +        if (access(abs_path, X_OK) == 0) {
> +            g_free(path);
> +            return true;
> +        }
> +    }
> +
> +    g_free(path);
> +    memset(abs_path, 0, len);
> +
> +    return false;
> +}

I hope you don't hate me for now pointing out...

   g_find_program_in_path()

http://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-find-program-in-path


> +
>  int64_t qmp_guest_sync(int64_t id, Error **errp)
>  {
>      return id;
> @@ -574,6 +623,220 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>  }
>  #endif
>  
> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> +#define SUS_MODE_SUPPORTED 0
> +#define SUS_MODE_NOT_SUPPORTED 1
> +
> +/**
> + * This function forks twice and the information about the mode support
> + * status is passed to the qemu-ga process via a pipe.
> + *
> + * This approach allows us to keep the way we reap terminated children
> + * in qemu-ga quite simple.
> + */
> +static bool bios_supports_mode(const char *mode, Error **err)
> +{
> +    pid_t pid;
> +    ssize_t ret;
> +    bool has_pmutils;
> +    int status, pipefds[2];
> +    char pmutils_path[PATH_MAX];
> +    const char *pmutils_bin = "pm-is-supported";
> +
> +    if (pipe(pipefds) < 0) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +        return false;
> +    }
> +
> +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> +                                       sizeof(pmutils_path));
> +
> +    pid = fork();
> +    if (!pid) {
> +        struct sigaction act;
> +
> +        memset(&act, 0, sizeof(act));
> +        act.sa_handler = SIG_DFL;
> +        sigaction(SIGCHLD, &act, NULL);
> +
> +        setsid();
> +        close(pipefds[0]);
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        pid = fork();
> +        if (!pid) {
> +            int fd;
> +            char buf[32]; /* hopefully big enough */
> +            const char *arg;
> +
> +            if (strcmp(mode, "hibernate") == 0) {
> +                arg = "--hibernate";
> +            } else if (strcmp(mode, "sleep") == 0) {
> +                arg = "--suspend";
> +            } else if (strcmp(mode, "hybrid") == 0) {
> +                arg = "--suspend-hybrid";
> +            } else {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            if (has_pmutils) {
> +                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
> +            }
> +
> +            /*
> +             * If we get here either pm-utils is not installed or  execle() has
> +             * failed. Let's try the manual approach if mode is not hybrid (as
> +             * it's only supported by pm-utils)
> +             */
> +
> +            if (strcmp(mode, "hybrid") == 0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
> +            if (fd < 0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            ret = read(fd, buf, sizeof(buf));

You want "sizeof(buf)-1" here, otherwise if read return 'ret'
set to exactly sizeof(buf)...

> +            if (ret <= 0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            buf[ret] = '\0';

...this becomes a stack overflow.

> +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> +                                       sizeof(pmutils_path));
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        /* child */
> +        int fd;
> +        const char *cmd;
> +
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        if (has_pmutils) {
> +            execle(pmutils_path, pmutils_bin, NULL, environ);

You could just use  execl()  and drop the trailing 'environ' here,
since that is the default anyway.

> +        }
> +
> +        /*
> +         * If we get here either pm-utils is not installed or  execle() has
> +         * failed. Let's try the manual approach if mode is not hybrid (as
> +         * it's only supported by pm-utils)
> +         */
> +
> +        slog("could not execute %s\n", pmutils_bin);
> +        if (strcmp(mode, "hybrid") == 0) {
> +            _exit(EXIT_FAILURE);
> +        }
> +
> +        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(EXIT_FAILURE);
> +        }
> +
> +        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> +        if (write(fd, cmd, strlen(cmd)) < 0) {
> +            slog("failued to write to %s\n", LINUX_SYS_STATE_FILE);
> +            _exit(EXIT_FAILURE);
> +        }
> +
> +        _exit(EXIT_SUCCESS);
> +    } 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)
>  {


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Luiz Capitulino]

On Mon, 16 Jan 2012 21:06:27 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Mon, Jan 16, 2012 at 06:09:52PM -0200, Luiz Capitulino wrote:
> 
> > +/* Try to find executable file 'file'. If it's found, its absolute path is
> > +   returned in 'abs_path' and the function returns true. If it's not found,
> > +   the function will return false and 'abs_path' will contain zeros */
> > +static bool find_executable_file(const char *file, char *abs_path, size_t len)
> > +{
> > +    char *path, *saveptr;
> > +    const char *token, *delim = ":";
> > + 
> > +    if (!getenv("PATH")) {
> > +        return false;
> > +    }
> > +
> > +    path = g_strdup(getenv("PATH"));
> > +    if (!path) {
> > +        return false;
> > +    }
> > +
> > +    for (token = strtok_r(path, delim, &saveptr); token;
> > +         token = strtok_r(NULL, delim, &saveptr)) {
> > +        snprintf(abs_path, len, "%s/%s", token, file);
> > +        if (access(abs_path, X_OK) == 0) {
> > +            g_free(path);
> > +            return true;
> > +        }
> > +    }
> > +
> > +    g_free(path);
> > +    memset(abs_path, 0, len);
> > +
> > +    return false;
> > +}
> 
> I hope you don't hate me for now pointing out...
> 
>    g_find_program_in_path()
> 
> http://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-find-program-in-path

Oh, Thanks.

Note that it returns an allocated string. I won't free it in the child,
as it will be automatically freed by exec() or by exit().

> > +
> >  int64_t qmp_guest_sync(int64_t id, Error **errp)
> >  {
> >      return id;
> > @@ -574,6 +623,220 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >  }
> >  #endif
> >  
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +#define SUS_MODE_SUPPORTED 0
> > +#define SUS_MODE_NOT_SUPPORTED 1
> > +
> > +/**
> > + * This function forks twice and the information about the mode support
> > + * status is passed to the qemu-ga process via a pipe.
> > + *
> > + * This approach allows us to keep the way we reap terminated children
> > + * in qemu-ga quite simple.
> > + */
> > +static bool bios_supports_mode(const char *mode, Error **err)
> > +{
> > +    pid_t pid;
> > +    ssize_t ret;
> > +    bool has_pmutils;
> > +    int status, pipefds[2];
> > +    char pmutils_path[PATH_MAX];
> > +    const char *pmutils_bin = "pm-is-supported";
> > +
> > +    if (pipe(pipefds) < 0) {
> > +        error_set(err, QERR_UNDEFINED_ERROR);
> > +        return false;
> > +    }
> > +
> > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > +                                       sizeof(pmutils_path));
> > +
> > +    pid = fork();
> > +    if (!pid) {
> > +        struct sigaction act;
> > +
> > +        memset(&act, 0, sizeof(act));
> > +        act.sa_handler = SIG_DFL;
> > +        sigaction(SIGCHLD, &act, NULL);
> > +
> > +        setsid();
> > +        close(pipefds[0]);
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> > +
> > +        pid = fork();
> > +        if (!pid) {
> > +            int fd;
> > +            char buf[32]; /* hopefully big enough */
> > +            const char *arg;
> > +
> > +            if (strcmp(mode, "hibernate") == 0) {
> > +                arg = "--hibernate";
> > +            } else if (strcmp(mode, "sleep") == 0) {
> > +                arg = "--suspend";
> > +            } else if (strcmp(mode, "hybrid") == 0) {
> > +                arg = "--suspend-hybrid";
> > +            } else {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (has_pmutils) {
> > +                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
> > +            }
> > +
> > +            /*
> > +             * If we get here either pm-utils is not installed or  execle() has
> > +             * failed. Let's try the manual approach if mode is not hybrid (as
> > +             * it's only supported by pm-utils)
> > +             */
> > +
> > +            if (strcmp(mode, "hybrid") == 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
> > +            if (fd < 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            ret = read(fd, buf, sizeof(buf));
> 
> You want "sizeof(buf)-1" here, otherwise if read return 'ret'
> set to exactly sizeof(buf)...

Good catch.

> 
> > +            if (ret <= 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            buf[ret] = '\0';
> 
> ...this becomes a stack overflow.
> 
> > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > +                                       sizeof(pmutils_path));
> > +
> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +        const char *cmd;
> > +
> > +        setsid();
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> > +
> > +        if (has_pmutils) {
> > +            execle(pmutils_path, pmutils_bin, NULL, environ);
> 
> You could just use  execl()  and drop the trailing 'environ' here,
> since that is the default anyway.

execl() is not in the async-signal-safe list.

> 
> > +        }
> > +
> > +        /*
> > +         * If we get here either pm-utils is not installed or  execle() has
> > +         * failed. Let's try the manual approach if mode is not hybrid (as
> > +         * it's only supported by pm-utils)
> > +         */
> > +
> > +        slog("could not execute %s\n", pmutils_bin);
> > +        if (strcmp(mode, "hybrid") == 0) {
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        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(EXIT_FAILURE);
> > +        }
> > +
> > +        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> > +        if (write(fd, cmd, strlen(cmd)) < 0) {
> > +            slog("failued to write to %s\n", LINUX_SYS_STATE_FILE);
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        _exit(EXIT_SUCCESS);
> > +    } 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)
> >  {
> 
> 
> Regards,
> Daniel

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

[Daniel P. Berrange]

On Tue, Jan 17, 2012 at 10:18:34AM -0200, Luiz Capitulino wrote:
> On Mon, 16 Jan 2012 21:06:27 +0000
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > > +                                       sizeof(pmutils_path));
> > > +
> > > +    pid = fork();
> > > +    if (pid == 0) {
> > > +        /* child */
> > > +        int fd;
> > > +        const char *cmd;
> > > +
> > > +        setsid();
> > > +        reopen_fd_to_null(0);
> > > +        reopen_fd_to_null(1);
> > > +        reopen_fd_to_null(2);
> > > +
> > > +        if (has_pmutils) {
> > > +            execle(pmutils_path, pmutils_bin, NULL, environ);
> > 
> > You could just use  execl()  and drop the trailing 'environ' here,
> > since that is the default anyway.
> 
> execl() is not in the async-signal-safe list.

It was not in POSIX.1-2004, but POSIX.1-2008 added it. I don't thing
this is worth arguing over though, so just leave it as you have :-)

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Michael Roth]

On 01/16/2012 02:09 PM, Luiz Capitulino wrote:
> The guest-suspend command supports three modes:
>
>   o hibernate (suspend to disk)
>   o sleep     (suspend to ram)
>   o hybrid    (save RAM contents to disk, but suspend instead of
>                powering off)
>
> Before trying to suspend, the command queries the guest in order
> to know whether the given mode is supported. The sleep and hybrid
> modes are only supported in QEMU 1.1 and later though, because
> QEMU's S3 support is broken in previous versions.
>
> The guest-suspend command will use the scripts provided by the
> pm-utils package if they are available. If they aren't, a manual
> process which directly writes to the "/sys/power/state" file is
> used.
>
> To reap terminated children, a new signal handler is installed in
> the parent to catch SIGCHLD signals and a non-blocking call to
> waitpid() is done to collect their exit statuses. The statuses,
> however, are discarded.
>
> The approach used to query the guest for suspend support deserves
> some explanation. It's implemented by bios_supports_mode() and shown
> below:
>
>     qemu-ga
>       |
>   create pipe
>       |
>     fork()
>       -----------------
>       |               |
>       |               |
>       |             fork()
>       |               --------------------------
>       |               |                        |
>       |               |                        |
>       |               |               exec('pm-is-supported')
>       |               |
>       |              wait()
>       |       write exit status to pipe
>       |              exit
>       |
>    read pipe
>
> This might look complex, but the resulting code is quite simple.
> The purpose of that approach is to allow qemu-ga to reap its
> children (semi-)automatically from its SIGCHLD handler.
>
> Implementing this the obvious way, that's, doing the exec() call
> from the first child process, would force us to introduce a more
> complex way to reap qemu-ga's children. Like registering PIDs to
> be reaped and having a way to wait for them when returning their
> exit status to qemu-ga is necessary. The approach explained
> above avoids that complexity.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   qapi-schema-guest.json     |   32 ++++++
>   qemu-ga.c                  |   18 +++-
>   qga/guest-agent-commands.c |  263 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 312 insertions(+), 1 deletions(-)
>
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 5f8a18d..7dd9267 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -219,3 +219,35 @@
>   ##
>   { 'command': 'guest-fsfreeze-thaw',
>     'returns': 'int' }
> +
> +##
> +# @guest-suspend
> +#
> +# Suspend guest execution by entering ACPI power state S3 or S4.
> +#
> +# 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)
> +#        'sleep'     execution is suspended but the RAM retains its contents
> +#                    (this corresponds to ACPI S3)
> +#        'hybrid'    RAM content is saved to the disk but the guest is
> +#                    suspended instead of powering off
> +#
> +# Returns: nothing on success
> +#          If @mode is not supported by the guest, Unsupported
> +#
> +# Notes: o This is an asynchronous request. There's no guarantee a response
> +#          will be sent.
> +#        o Errors will be logged to guest's syslog.
> +#        o It's strongly recommended to issue the guest-sync command before
> +#          sending commands when the guest resumes.
> +#
> +# Since: 1.1
> +##
> +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 647df82..f531084 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,16 @@ static void quit_handler(int sig)
>       }
>   }
>
> +/* reap _all_ terminated children */
> +static void child_handler(int sig)
> +{
> +    int status;
> +    while (waitpid(-1,&status, WNOHANG)>  0) /* NOTHING */;
> +}
> +
>   static void register_signal_handlers(void)
>   {
> -    struct sigaction sigact;
> +    struct sigaction sigact, sigact_chld;
>       int ret;
>
>       memset(&sigact, 0, sizeof(struct sigaction));
> @@ -76,6 +84,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..e64b0e0 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -23,6 +23,7 @@
>
>   #include<sys/types.h>
>   #include<sys/ioctl.h>
> +#include<sys/wait.h>
>   #include "qga/guest-agent-core.h"
>   #include "qga-qmp-commands.h"
>   #include "qerror.h"
> @@ -44,6 +45,54 @@ static void slog(const char *fmt, ...)
>       va_end(ap);
>   }
>
> +static void reopen_fd_to_null(int fd)
> +{
> +    int nullfd;
> +
> +    nullfd = open("/dev/null", O_RDWR);
> +    if (nullfd<  0) {
> +        return;
> +    }
> +
> +    dup2(nullfd, fd);
> +
> +    if (nullfd != fd) {
> +        close(nullfd);
> +    }
> +}
> +
> +/* Try to find executable file 'file'. If it's found, its absolute path is
> +   returned in 'abs_path' and the function returns true. If it's not found,
> +   the function will return false and 'abs_path' will contain zeros */
> +static bool find_executable_file(const char *file, char *abs_path, size_t len)
> +{
> +    char *path, *saveptr;
> +    const char *token, *delim = ":";
> +
> +    if (!getenv("PATH")) {
> +        return false;
> +    }
> +
> +    path = g_strdup(getenv("PATH"));
> +    if (!path) {
> +        return false;
> +    }
> +
> +    for (token = strtok_r(path, delim,&saveptr); token;
> +         token = strtok_r(NULL, delim,&saveptr)) {
> +        snprintf(abs_path, len, "%s/%s", token, file);
> +        if (access(abs_path, X_OK) == 0) {
> +            g_free(path);
> +            return true;
> +        }
> +    }
> +
> +    g_free(path);
> +    memset(abs_path, 0, len);
> +
> +    return false;
> +}
> +
>   int64_t qmp_guest_sync(int64_t id, Error **errp)
>   {
>       return id;
> @@ -574,6 +623,220 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
>   }
>   #endif
>
> +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> +#define SUS_MODE_SUPPORTED 0
> +#define SUS_MODE_NOT_SUPPORTED 1
> +
> +/**
> + * This function forks twice and the information about the mode support
> + * status is passed to the qemu-ga process via a pipe.
> + *
> + * This approach allows us to keep the way we reap terminated children
> + * in qemu-ga quite simple.
> + */
> +static bool bios_supports_mode(const char *mode, Error **err)
> +{
> +    pid_t pid;
> +    ssize_t ret;
> +    bool has_pmutils;
> +    int status, pipefds[2];
> +    char pmutils_path[PATH_MAX];
> +    const char *pmutils_bin = "pm-is-supported";
> +
> +    if (pipe(pipefds)<  0) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +        return false;
> +    }
> +
> +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> +                                       sizeof(pmutils_path));
> +
> +    pid = fork();
> +    if (!pid) {
> +        struct sigaction act;
> +
> +        memset(&act, 0, sizeof(act));
> +        act.sa_handler = SIG_DFL;
> +        sigaction(SIGCHLD,&act, NULL);
> +
> +        setsid();
> +        close(pipefds[0]);
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        pid = fork();
> +        if (!pid) {
> +            int fd;
> +            char buf[32]; /* hopefully big enough */
> +            const char *arg;
> +
> +            if (strcmp(mode, "hibernate") == 0) {
> +                arg = "--hibernate";
> +            } else if (strcmp(mode, "sleep") == 0) {
> +                arg = "--suspend";
> +            } else if (strcmp(mode, "hybrid") == 0) {
> +                arg = "--suspend-hybrid";
> +            } else {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            if (has_pmutils) {
> +                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
> +            }
> +
> +            /*
> +             * If we get here either pm-utils is not installed or  execle() has
> +             * failed. Let's try the manual approach if mode is not hybrid (as
> +             * it's only supported by pm-utils)
> +             */
> +
> +            if (strcmp(mode, "hybrid") == 0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
> +            if (fd<  0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            ret = read(fd, buf, sizeof(buf));
> +            if (ret<= 0) {
> +                _exit(SUS_MODE_NOT_SUPPORTED);
> +            }
> +
> +            buf[ret] = '\0';
> +            if (strcmp(mode, "hibernate") == 0&&  strstr(buf, "disk")) {
> +                _exit(SUS_MODE_SUPPORTED);
> +            } else if (strcmp(mode, "sleep") == 0&&  strstr(buf, "mem")) {
> +                _exit(SUS_MODE_SUPPORTED);
> +            }
> +
> +            _exit(SUS_MODE_NOT_SUPPORTED);
> +        }
> +
> +        if (pid>  0) {
> +            wait(&status);
> +        } else {
> +            status = SUS_MODE_NOT_SUPPORTED;
> +        }
> +
> +        ret = write(pipefds[1],&status, sizeof(status));
> +        if (ret != sizeof(status)) {
> +            _exit(EXIT_FAILURE);
> +        }
> +
> +        _exit(EXIT_SUCCESS);
> +    }
> +
> +    close(pipefds[1]);
> +
> +    if (pid>  0) {
> +        ret = read(pipefds[0],&status, sizeof(status));
> +        if (ret == sizeof(status)&&  WIFEXITED(status)&&
> +            WEXITSTATUS(status) == SUS_MODE_SUPPORTED) {
> +            close(pipefds[0]);
> +            return true;
> +        }
> +    }
> +
> +    close(pipefds[0]);
> +    return false;
> +}
> +
> +static bool host_supports_mode(const char *mode)
> +{
> +    if (strcmp(mode, "hibernate")) {
> +        /* sleep&  hybrid are only supported in qemu 1.1.0 and above */
> +        return ga_has_support_level(1, 1, 0);
> +    }
> +    return true;
> +}
> +
> +void qmp_guest_suspend(const char *mode, Error **err)
> +{
> +    pid_t pid;
> +    bool has_pmutils;
> +    Error *local_err = NULL;
> +    const char *pmutils_bin;
> +    char pmutils_path[PATH_MAX];
> +
> +    if (strcmp(mode, "hibernate") == 0) {
> +        pmutils_bin = "pm-hibernate";
> +    } else if (strcmp(mode, "sleep") == 0) {
> +        pmutils_bin = "pm-suspend";
> +    } else if (strcmp(mode, "hybrid") == 0) {
> +        pmutils_bin = "pm-suspend-hybrid";
> +    } else {
> +        error_set(err, QERR_INVALID_PARAMETER, "mode");
> +        return;
> +    }
> +
> +    if (!host_supports_mode(mode)) {
> +        error_set(err, QERR_UNSUPPORTED);
> +        return;
> +    }
> +
> +    if (!bios_supports_mode(mode,&local_err)) {
> +        if (error_is_set(&local_err)) {
> +            error_propagate(err, local_err);
> +        } else {
> +            error_set(err, QERR_UNSUPPORTED);
> +        }
> +        return;
> +    }
> +
> +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> +                                       sizeof(pmutils_path));
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        /* child */
> +        int fd;
> +        const char *cmd;
> +
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        if (has_pmutils) {
> +            execle(pmutils_path, pmutils_bin, NULL, environ);
> +        }
> +
> +        /*
> +         * If we get here either pm-utils is not installed or  execle() has
> +         * failed. Let's try the manual approach if mode is not hybrid (as
> +         * it's only supported by pm-utils)
> +         */
> +
> +        slog("could not execute %s\n", pmutils_bin);

slog() is actually a wrapper around syslog(), so I guess we need to 
scrap these in light of the previous discussion. shutdown() has the same 
problem though...

So maybe, just leave these in, and I'll follow up with a patch that logs 
to a separate file or something if pid != PARENT_PID.

> +        if (strcmp(mode, "hybrid") == 0) {
> +            _exit(EXIT_FAILURE);
> +        }
> +
> +        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(EXIT_FAILURE);
> +        }
> +
> +        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> +        if (write(fd, cmd, strlen(cmd))<  0) {
> +            slog("failued to write to %s\n", LINUX_SYS_STATE_FILE);
> +            _exit(EXIT_FAILURE);
> +        }
> +
> +        _exit(EXIT_SUCCESS);
> +    } 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)
>   {

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

[Luiz Capitulino]

On Mon, 16 Jan 2012 16:17:34 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 01/16/2012 02:09 PM, Luiz Capitulino wrote:
> > The guest-suspend command supports three modes:
> >
> >   o hibernate (suspend to disk)
> >   o sleep     (suspend to ram)
> >   o hybrid    (save RAM contents to disk, but suspend instead of
> >                powering off)
> >
> > Before trying to suspend, the command queries the guest in order
> > to know whether the given mode is supported. The sleep and hybrid
> > modes are only supported in QEMU 1.1 and later though, because
> > QEMU's S3 support is broken in previous versions.
> >
> > The guest-suspend command will use the scripts provided by the
> > pm-utils package if they are available. If they aren't, a manual
> > process which directly writes to the "/sys/power/state" file is
> > used.
> >
> > To reap terminated children, a new signal handler is installed in
> > the parent to catch SIGCHLD signals and a non-blocking call to
> > waitpid() is done to collect their exit statuses. The statuses,
> > however, are discarded.
> >
> > The approach used to query the guest for suspend support deserves
> > some explanation. It's implemented by bios_supports_mode() and shown
> > below:
> >
> >     qemu-ga
> >       |
> >   create pipe
> >       |
> >     fork()
> >       -----------------
> >       |               |
> >       |               |
> >       |             fork()
> >       |               --------------------------
> >       |               |                        |
> >       |               |                        |
> >       |               |               exec('pm-is-supported')
> >       |               |
> >       |              wait()
> >       |       write exit status to pipe
> >       |              exit
> >       |
> >    read pipe
> >
> > This might look complex, but the resulting code is quite simple.
> > The purpose of that approach is to allow qemu-ga to reap its
> > children (semi-)automatically from its SIGCHLD handler.
> >
> > Implementing this the obvious way, that's, doing the exec() call
> > from the first child process, would force us to introduce a more
> > complex way to reap qemu-ga's children. Like registering PIDs to
> > be reaped and having a way to wait for them when returning their
> > exit status to qemu-ga is necessary. The approach explained
> > above avoids that complexity.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> >   qapi-schema-guest.json     |   32 ++++++
> >   qemu-ga.c                  |   18 +++-
> >   qga/guest-agent-commands.c |  263 ++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 312 insertions(+), 1 deletions(-)
> >
> > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> > index 5f8a18d..7dd9267 100644
> > --- a/qapi-schema-guest.json
> > +++ b/qapi-schema-guest.json
> > @@ -219,3 +219,35 @@
> >   ##
> >   { 'command': 'guest-fsfreeze-thaw',
> >     'returns': 'int' }
> > +
> > +##
> > +# @guest-suspend
> > +#
> > +# Suspend guest execution by entering ACPI power state S3 or S4.
> > +#
> > +# 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)
> > +#        'sleep'     execution is suspended but the RAM retains its contents
> > +#                    (this corresponds to ACPI S3)
> > +#        'hybrid'    RAM content is saved to the disk but the guest is
> > +#                    suspended instead of powering off
> > +#
> > +# Returns: nothing on success
> > +#          If @mode is not supported by the guest, Unsupported
> > +#
> > +# Notes: o This is an asynchronous request. There's no guarantee a response
> > +#          will be sent.
> > +#        o Errors will be logged to guest's syslog.
> > +#        o It's strongly recommended to issue the guest-sync command before
> > +#          sending commands when the guest resumes.
> > +#
> > +# Since: 1.1
> > +##
> > +{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
> > diff --git a/qemu-ga.c b/qemu-ga.c
> > index 647df82..f531084 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,16 @@ static void quit_handler(int sig)
> >       }
> >   }
> >
> > +/* reap _all_ terminated children */
> > +static void child_handler(int sig)
> > +{
> > +    int status;
> > +    while (waitpid(-1,&status, WNOHANG)>  0) /* NOTHING */;
> > +}
> > +
> >   static void register_signal_handlers(void)
> >   {
> > -    struct sigaction sigact;
> > +    struct sigaction sigact, sigact_chld;
> >       int ret;
> >
> >       memset(&sigact, 0, sizeof(struct sigaction));
> > @@ -76,6 +84,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..e64b0e0 100644
> > --- a/qga/guest-agent-commands.c
> > +++ b/qga/guest-agent-commands.c
> > @@ -23,6 +23,7 @@
> >
> >   #include<sys/types.h>
> >   #include<sys/ioctl.h>
> > +#include<sys/wait.h>
> >   #include "qga/guest-agent-core.h"
> >   #include "qga-qmp-commands.h"
> >   #include "qerror.h"
> > @@ -44,6 +45,54 @@ static void slog(const char *fmt, ...)
> >       va_end(ap);
> >   }
> >
> > +static void reopen_fd_to_null(int fd)
> > +{
> > +    int nullfd;
> > +
> > +    nullfd = open("/dev/null", O_RDWR);
> > +    if (nullfd<  0) {
> > +        return;
> > +    }
> > +
> > +    dup2(nullfd, fd);
> > +
> > +    if (nullfd != fd) {
> > +        close(nullfd);
> > +    }
> > +}
> > +
> > +/* Try to find executable file 'file'. If it's found, its absolute path is
> > +   returned in 'abs_path' and the function returns true. If it's not found,
> > +   the function will return false and 'abs_path' will contain zeros */
> > +static bool find_executable_file(const char *file, char *abs_path, size_t len)
> > +{
> > +    char *path, *saveptr;
> > +    const char *token, *delim = ":";
> > +
> > +    if (!getenv("PATH")) {
> > +        return false;
> > +    }
> > +
> > +    path = g_strdup(getenv("PATH"));
> > +    if (!path) {
> > +        return false;
> > +    }
> > +
> > +    for (token = strtok_r(path, delim,&saveptr); token;
> > +         token = strtok_r(NULL, delim,&saveptr)) {
> > +        snprintf(abs_path, len, "%s/%s", token, file);
> > +        if (access(abs_path, X_OK) == 0) {
> > +            g_free(path);
> > +            return true;
> > +        }
> > +    }
> > +
> > +    g_free(path);
> > +    memset(abs_path, 0, len);
> > +
> > +    return false;
> > +}
> > +
> >   int64_t qmp_guest_sync(int64_t id, Error **errp)
> >   {
> >       return id;
> > @@ -574,6 +623,220 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >   }
> >   #endif
> >
> > +#define LINUX_SYS_STATE_FILE "/sys/power/state"
> > +#define SUS_MODE_SUPPORTED 0
> > +#define SUS_MODE_NOT_SUPPORTED 1
> > +
> > +/**
> > + * This function forks twice and the information about the mode support
> > + * status is passed to the qemu-ga process via a pipe.
> > + *
> > + * This approach allows us to keep the way we reap terminated children
> > + * in qemu-ga quite simple.
> > + */
> > +static bool bios_supports_mode(const char *mode, Error **err)
> > +{
> > +    pid_t pid;
> > +    ssize_t ret;
> > +    bool has_pmutils;
> > +    int status, pipefds[2];
> > +    char pmutils_path[PATH_MAX];
> > +    const char *pmutils_bin = "pm-is-supported";
> > +
> > +    if (pipe(pipefds)<  0) {
> > +        error_set(err, QERR_UNDEFINED_ERROR);
> > +        return false;
> > +    }
> > +
> > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > +                                       sizeof(pmutils_path));
> > +
> > +    pid = fork();
> > +    if (!pid) {
> > +        struct sigaction act;
> > +
> > +        memset(&act, 0, sizeof(act));
> > +        act.sa_handler = SIG_DFL;
> > +        sigaction(SIGCHLD,&act, NULL);
> > +
> > +        setsid();
> > +        close(pipefds[0]);
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> > +
> > +        pid = fork();
> > +        if (!pid) {
> > +            int fd;
> > +            char buf[32]; /* hopefully big enough */
> > +            const char *arg;
> > +
> > +            if (strcmp(mode, "hibernate") == 0) {
> > +                arg = "--hibernate";
> > +            } else if (strcmp(mode, "sleep") == 0) {
> > +                arg = "--suspend";
> > +            } else if (strcmp(mode, "hybrid") == 0) {
> > +                arg = "--suspend-hybrid";
> > +            } else {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            if (has_pmutils) {
> > +                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
> > +            }
> > +
> > +            /*
> > +             * If we get here either pm-utils is not installed or  execle() has
> > +             * failed. Let's try the manual approach if mode is not hybrid (as
> > +             * it's only supported by pm-utils)
> > +             */
> > +
> > +            if (strcmp(mode, "hybrid") == 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
> > +            if (fd<  0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            ret = read(fd, buf, sizeof(buf));
> > +            if (ret<= 0) {
> > +                _exit(SUS_MODE_NOT_SUPPORTED);
> > +            }
> > +
> > +            buf[ret] = '\0';
> > +            if (strcmp(mode, "hibernate") == 0&&  strstr(buf, "disk")) {
> > +                _exit(SUS_MODE_SUPPORTED);
> > +            } else if (strcmp(mode, "sleep") == 0&&  strstr(buf, "mem")) {
> > +                _exit(SUS_MODE_SUPPORTED);
> > +            }
> > +
> > +            _exit(SUS_MODE_NOT_SUPPORTED);
> > +        }
> > +
> > +        if (pid>  0) {
> > +            wait(&status);
> > +        } else {
> > +            status = SUS_MODE_NOT_SUPPORTED;
> > +        }
> > +
> > +        ret = write(pipefds[1],&status, sizeof(status));
> > +        if (ret != sizeof(status)) {
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        _exit(EXIT_SUCCESS);
> > +    }
> > +
> > +    close(pipefds[1]);
> > +
> > +    if (pid>  0) {
> > +        ret = read(pipefds[0],&status, sizeof(status));
> > +        if (ret == sizeof(status)&&  WIFEXITED(status)&&
> > +            WEXITSTATUS(status) == SUS_MODE_SUPPORTED) {
> > +            close(pipefds[0]);
> > +            return true;
> > +        }
> > +    }
> > +
> > +    close(pipefds[0]);
> > +    return false;
> > +}
> > +
> > +static bool host_supports_mode(const char *mode)
> > +{
> > +    if (strcmp(mode, "hibernate")) {
> > +        /* sleep&  hybrid are only supported in qemu 1.1.0 and above */
> > +        return ga_has_support_level(1, 1, 0);
> > +    }
> > +    return true;
> > +}
> > +
> > +void qmp_guest_suspend(const char *mode, Error **err)
> > +{
> > +    pid_t pid;
> > +    bool has_pmutils;
> > +    Error *local_err = NULL;
> > +    const char *pmutils_bin;
> > +    char pmutils_path[PATH_MAX];
> > +
> > +    if (strcmp(mode, "hibernate") == 0) {
> > +        pmutils_bin = "pm-hibernate";
> > +    } else if (strcmp(mode, "sleep") == 0) {
> > +        pmutils_bin = "pm-suspend";
> > +    } else if (strcmp(mode, "hybrid") == 0) {
> > +        pmutils_bin = "pm-suspend-hybrid";
> > +    } else {
> > +        error_set(err, QERR_INVALID_PARAMETER, "mode");
> > +        return;
> > +    }
> > +
> > +    if (!host_supports_mode(mode)) {
> > +        error_set(err, QERR_UNSUPPORTED);
> > +        return;
> > +    }
> > +
> > +    if (!bios_supports_mode(mode,&local_err)) {
> > +        if (error_is_set(&local_err)) {
> > +            error_propagate(err, local_err);
> > +        } else {
> > +            error_set(err, QERR_UNSUPPORTED);
> > +        }
> > +        return;
> > +    }
> > +
> > +    has_pmutils = find_executable_file(pmutils_bin, pmutils_path,
> > +                                       sizeof(pmutils_path));
> > +
> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* child */
> > +        int fd;
> > +        const char *cmd;
> > +
> > +        setsid();
> > +        reopen_fd_to_null(0);
> > +        reopen_fd_to_null(1);
> > +        reopen_fd_to_null(2);
> > +
> > +        if (has_pmutils) {
> > +            execle(pmutils_path, pmutils_bin, NULL, environ);
> > +        }
> > +
> > +        /*
> > +         * If we get here either pm-utils is not installed or  execle() has
> > +         * failed. Let's try the manual approach if mode is not hybrid (as
> > +         * it's only supported by pm-utils)
> > +         */
> > +
> > +        slog("could not execute %s\n", pmutils_bin);
> 
> slog() is actually a wrapper around syslog(), so I guess we need to 
> scrap these in light of the previous discussion. shutdown() has the same 
> problem though...
> 
> So maybe, just leave these in, and I'll follow up with a patch that logs 
> to a separate file or something if pid != PARENT_PID.

Yes, what I had in mind was to leave them there and fix slog() in parallel.

But as we're putting a big effort on making qmp_guest_suspend() as safe as
possible, I think I'll drop them for now and only add them back when they
get fixed.

> 
> > +        if (strcmp(mode, "hybrid") == 0) {
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        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(EXIT_FAILURE);
> > +        }
> > +
> > +        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
> > +        if (write(fd, cmd, strlen(cmd))<  0) {
> > +            slog("failued to write to %s\n", LINUX_SYS_STATE_FILE);
> > +            _exit(EXIT_FAILURE);
> > +        }
> > +
> > +        _exit(EXIT_SUCCESS);
> > +    } 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)
> >   {
> 

[Qemu-devel] [PATCH v7 0/2]: qemu-ga: Add the guest-suspend command

[Luiz Capitulino]

Small fixes, no major changes.

This series depends on the following series from Michael:

http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg02110.html

v7

o drop find_executable_file() and use g_find_program_in_path()
  instead [Daniel]
o fix off by one bug [Daniel]
o drop slog() usage [Michael]

 qapi-schema-guest.json     |   32 ++++++
 qemu-ga.c                  |   20 ++++-
 qga/guest-agent-commands.c |  226 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+), 2 deletions(-)

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

[Luiz Capitulino]

The guest-suspend command supports three modes:

 o hibernate (suspend to disk)
 o sleep     (suspend to ram)
 o hybrid    (save RAM contents to disk, but suspend instead of
              powering off)

Before trying to suspend, the command queries the guest in order
to know whether the given mode is supported. The sleep and hybrid
modes are only supported in QEMU 1.1 and later though, because
QEMU's S3 support is broken in previous versions.

The guest-suspend command will use the scripts provided by the
pm-utils package if they are available. If they aren't, a manual
process which directly writes to the "/sys/power/state" file is
used.

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

The approach used to query the guest for suspend support deserves
some explanation. It's implemented by bios_supports_mode() and shown
below:

   qemu-ga
     |
 create pipe
     |
   fork()
     -----------------
     |               |
     |               |
     |             fork()
     |               --------------------------
     |               |                        |
     |               |                        |
     |               |               exec('pm-is-supported')
     |               |
     |              wait()
     |       write exit status to pipe
     |              exit
     |
  read pipe

This might look complex, but the resulting code is quite simple.
The purpose of that approach is to allow qemu-ga to reap its
children (semi-)automatically from its SIGCHLD handler.

Implementing this the obvious way, that's, doing the exec() call
from the first child process, would force us to introduce a more
complex way to reap qemu-ga's children. Like registering PIDs to
be reaped and having a way to wait for them when returning their
exit status to qemu-ga is necessary. The approach explained
above avoids that complexity.

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

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 5f8a18d..7dd9267 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -219,3 +219,35 @@
 ##
 { 'command': 'guest-fsfreeze-thaw',
   'returns': 'int' }
+
+##
+# @guest-suspend
+#
+# Suspend guest execution by entering ACPI power state S3 or S4.
+#
+# 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)
+#        'sleep'     execution is suspended but the RAM retains its contents
+#                    (this corresponds to ACPI S3)
+#        'hybrid'    RAM content is saved to the disk but the guest is
+#                    suspended instead of powering off
+#
+# Returns: nothing on success
+#          If @mode is not supported by the guest, Unsupported
+#
+# Notes: o This is an asynchronous request. There's no guarantee a response
+#          will be sent.
+#        o Errors will be logged to guest's syslog.
+#        o It's strongly recommended to issue the guest-sync command before
+#          sending commands when the guest resumes.
+#
+# Since: 1.1
+##
+{ 'command': 'guest-suspend', 'data': { 'mode': 'str' } }
diff --git a/qemu-ga.c b/qemu-ga.c
index 647df82..f531084 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,16 @@ static void quit_handler(int sig)
     }
 }
 
+/* reap _all_ terminated children */
+static void child_handler(int sig)
+{
+    int status;
+    while (waitpid(-1, &status, WNOHANG) > 0) /* NOTHING */;
+}
+
 static void register_signal_handlers(void)
 {
-    struct sigaction sigact;
+    struct sigaction sigact, sigact_chld;
     int ret;
 
     memset(&sigact, 0, sizeof(struct sigaction));
@@ -76,6 +84,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..5e81225 100644
--- a/qga/guest-agent-commands.c
+++ b/qga/guest-agent-commands.c
@@ -23,6 +23,7 @@
 
 #include <sys/types.h>
 #include <sys/ioctl.h>
+#include <sys/wait.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qerror.h"
@@ -44,6 +45,22 @@ static void slog(const char *fmt, ...)
     va_end(ap);
 }
 
+static void reopen_fd_to_null(int fd)
+{
+    int nullfd;
+
+    nullfd = open("/dev/null", O_RDWR);
+    if (nullfd < 0) {
+        return;
+    }
+
+    dup2(nullfd, fd);
+
+    if (nullfd != fd) {
+        close(nullfd);
+    }
+}
+
 int64_t qmp_guest_sync(int64_t id, Error **errp)
 {
     return id;
@@ -574,6 +591,215 @@ int64_t qmp_guest_fsfreeze_thaw(Error **err)
 }
 #endif
 
+#define LINUX_SYS_STATE_FILE "/sys/power/state"
+#define SUS_MODE_SUPPORTED 0
+#define SUS_MODE_NOT_SUPPORTED 1
+
+/**
+ * This function forks twice and the information about the mode support
+ * status is passed to the qemu-ga process via a pipe.
+ *
+ * This approach allows us to keep the way we reap terminated children
+ * in qemu-ga quite simple.
+ */
+static bool bios_supports_mode(const char *mode, Error **err)
+{
+    pid_t pid;
+    ssize_t ret;
+    int status, pipefds[2];
+    char *pmutils_path;
+    const char *pmutils_bin = "pm-is-supported";
+
+    if (pipe(pipefds) < 0) {
+        error_set(err, QERR_UNDEFINED_ERROR);
+        return false;
+    }
+
+    pmutils_path = g_find_program_in_path(pmutils_bin);
+
+    pid = fork();
+    if (!pid) {
+        struct sigaction act;
+
+        memset(&act, 0, sizeof(act));
+        act.sa_handler = SIG_DFL;
+        sigaction(SIGCHLD, &act, NULL);
+
+        setsid();
+        close(pipefds[0]);
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        pid = fork();
+        if (!pid) {
+            int fd;
+            char buf[32]; /* hopefully big enough */
+            const char *arg;
+
+            if (strcmp(mode, "hibernate") == 0) {
+                arg = "--hibernate";
+            } else if (strcmp(mode, "sleep") == 0) {
+                arg = "--suspend";
+            } else if (strcmp(mode, "hybrid") == 0) {
+                arg = "--suspend-hybrid";
+            } else {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            if (pmutils_path) {
+                execle(pmutils_path, pmutils_bin, arg, NULL, environ);
+            }
+
+            /*
+             * If we get here either pm-utils is not installed or  execle() has
+             * failed. Let's try the manual approach if mode is not hybrid (as
+             * it's only supported by pm-utils)
+             */
+
+            if (strcmp(mode, "hybrid") == 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
+            if (fd < 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+
+            ret = read(fd, buf, sizeof(buf)-1);
+            if (ret <= 0) {
+                _exit(SUS_MODE_NOT_SUPPORTED);
+            }
+            buf[ret] = '\0';
+
+            if (strcmp(mode, "hibernate") == 0 && strstr(buf, "disk")) {
+                _exit(SUS_MODE_SUPPORTED);
+            } else if (strcmp(mode, "sleep") == 0 && strstr(buf, "mem")) {
+                _exit(SUS_MODE_SUPPORTED);
+            }
+
+            _exit(SUS_MODE_NOT_SUPPORTED);
+        }
+
+        if (pid > 0) {
+            wait(&status);
+        } else {
+            status = SUS_MODE_NOT_SUPPORTED;
+        }
+
+        ret = write(pipefds[1], &status, sizeof(status));
+        if (ret != sizeof(status)) {
+            _exit(EXIT_FAILURE);
+        }
+
+        _exit(EXIT_SUCCESS);
+    }
+
+    close(pipefds[1]);
+    g_free(pmutils_path);
+
+    if (pid > 0) {
+        ret = read(pipefds[0], &status, sizeof(status));
+        if (ret == sizeof(status) && WIFEXITED(status) &&
+            WEXITSTATUS(status) == SUS_MODE_SUPPORTED) {
+            close(pipefds[0]);
+            return true;
+        }
+    }
+
+    close(pipefds[0]);
+    return false;
+}
+
+static bool host_supports_mode(const char *mode)
+{
+    if (strcmp(mode, "hibernate")) {
+        /* sleep & hybrid are only supported in qemu 1.1.0 and above */
+        return ga_has_support_level(1, 1, 0);
+    }
+    return true;
+}
+
+void qmp_guest_suspend(const char *mode, Error **err)
+{
+    pid_t pid;
+    char *pmutils_path;
+    const char *pmutils_bin;
+    Error *local_err = NULL;
+
+    if (strcmp(mode, "hibernate") == 0) {
+        pmutils_bin = "pm-hibernate";
+    } else if (strcmp(mode, "sleep") == 0) {
+        pmutils_bin = "pm-suspend";
+    } else if (strcmp(mode, "hybrid") == 0) {
+        pmutils_bin = "pm-suspend-hybrid";
+    } else {
+        error_set(err, QERR_INVALID_PARAMETER, "mode");
+        return;
+    }
+
+    if (!host_supports_mode(mode)) {
+        error_set(err, QERR_UNSUPPORTED);
+        return;
+    }
+
+    if (!bios_supports_mode(mode, &local_err)) {
+        if (error_is_set(&local_err)) {
+            error_propagate(err, local_err);
+        } else {
+            error_set(err, QERR_UNSUPPORTED);
+        }
+        return;
+    }
+
+    pmutils_path = g_find_program_in_path(pmutils_bin);
+
+    pid = fork();
+    if (pid == 0) {
+        /* child */
+        int fd;
+        const char *cmd;
+
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        if (pmutils_path) {
+            execle(pmutils_path, pmutils_bin, NULL, environ);
+        }
+
+        /*
+         * If we get here either pm-utils is not installed or  execle() has
+         * failed. Let's try the manual approach if mode is not hybrid (as
+         * it's only supported by pm-utils)
+         */
+
+        if (strcmp(mode, "hybrid") == 0) {
+            _exit(EXIT_FAILURE);
+        }
+
+        fd = open(LINUX_SYS_STATE_FILE, O_WRONLY);
+        if (fd < 0) {
+            _exit(EXIT_FAILURE);
+        }
+
+        cmd = strcmp(mode, "sleep") == 0 ? "mem" : "disk";
+        if (write(fd, cmd, strlen(cmd)) < 0) {
+            _exit(EXIT_FAILURE);
+        }
+
+        _exit(EXIT_SUCCESS);
+    }
+
+    g_free(pmutils_path);
+
+    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)
 {
-- 
1.7.9.rc0.dirty