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