On Fri, 13 Jan 2012 14:48:04 -0700 Eric Blake <ebl...@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. >