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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]