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

[Luiz Capitulino]

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