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

[Luiz Capitulino <lcapitulino@redhat.com>]

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