| Message ID | 1326482122-12619-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 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
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. >
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.
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. >
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
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
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 >
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 >> >
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).
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...
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
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.
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
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
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
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().
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) {
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(-)