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