From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38385) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rloyp-0006py-RC for qemu-devel@nongnu.org; Fri, 13 Jan 2012 16:48:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rloyo-0005OU-4J for qemu-devel@nongnu.org; Fri, 13 Jan 2012 16:48:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:29322) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rloyn-0005OQ-TY for qemu-devel@nongnu.org; Fri, 13 Jan 2012 16:48:10 -0500 Message-ID: <4F10A694.8030900@redhat.com> Date: Fri, 13 Jan 2012 14:48:04 -0700 From: Eric Blake MIME-Version: 1.0 References: <1326482122-12619-1-git-send-email-lcapitulino@redhat.com> <1326482122-12619-3-git-send-email-lcapitulino@redhat.com> In-Reply-To: <1326482122-12619-3-git-send-email-lcapitulino@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enigEF5E2FFF594CDA589E890DBB" Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-ga: Add the guest-suspend command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: jcody@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigEF5E2FFF594CDA589E890DBB Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 01/13/2012 12:15 PM, Luiz Capitulino wrote: > The guest-suspend command supports three modes: >=20 > o hibernate (suspend to disk) > o sleep (suspend to ram) > o hybrid (save RAM contents to disk, but suspend instead of > powering off) >=20 > 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. >=20 > 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); > } > =20 > +static int reopen_fd_to_null(int fd) > +{ > + int ret, nullfd; > + > + nullfd =3D open("/dev/null", O_RDWR); > + if (nullfd < 0) { > + return -1; > + } > + > + ret =3D 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 !=3D 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 =3D fork(); > + if (!pid) { > + struct sigaction act; > + > + memset(&act, 0, sizeof(act)); > + act.sa_handler =3D 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 =3D fork(); > + if (!pid) { > + char buf[32]; > + FILE *sysfile; > + const char *arg; > + const char *pmutils_bin =3D "pm-is-supported"; > + > + if (strcmp(mode, "hibernate") =3D=3D 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 =3D "--hibernate"; > + } else if (strcmp(mode, "sleep") =3D=3D 0) { > + arg =3D "--suspend"; > + } else if (strcmp(mode, "hybrid") =3D=3D 0) { > + arg =3D "--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 manua= l > + * approach if mode is not hybrid (as it's only suported b= y s/suported/supported/ > + * pm-utils) > + */ > + > + if (strcmp(mode, "hybrid") =3D=3D 0) { > + _exit(SUS_MODE_NOT_SUPPORTED); > + } > + > + sysfile =3D 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") =3D=3D 0 && strstr(buf, "dis= k")) { > + _exit(SUS_MODE_SUPPORTED); > + } else if (strcmp(mode, "sleep") =3D=3D 0 && strstr(buf, "= mem")) { > + _exit(SUS_MODE_SUPPORTED); > + } > + > + _exit(SUS_MODE_NOT_SUPPORTED); > + } > + > + if (pid > 0) { > + wait(&status); > + } else { > + status =3D SUS_MODE_NOT_SUPPORTED; > + } > + > + ret =3D write(pipefds[1], &status, sizeof(status)); > + if (ret !=3D sizeof(status)) { > + _exit(1); I prefer EXIT_SUCCESS and EXIT_FAILURE (from ) 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 =3D 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 =3D fork(); > + if (pid =3D=3D 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. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enigEF5E2FFF594CDA589E890DBB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQEcBAEBCAAGBQJPEKaVAAoJEKeha0olJ0Nq1rkIAK4p6V+tl10AsnQR5k1OSALV VTeselwd7SnCVBHFKro9Z6hPO/AsQx8kYmKs0k0AVV0CGbQdlS4yZhPlmS/nWaFW 9D+mtwE+nrZNMDKb8WmkW9WyuzNkg+gIb3WrsXo7AX3Y5nmq87/wGsUsT2WtPAFx hPjipYMZT0yck99SVGiDEJQRpjwcVQUHr/fYYTXQ1qJvT3eiPdLvw9vx9U7IUHQa 5c8VLro/b1yJ7Su7rlHujsPyBmk9QSg/mPX0C43Qff7zxe5UF77Z4ZaA5BemNVK/ ndazoiMIqJ7O6fZaHQbR/NeydTNkp53ULQirPPPra3mJtM8dMdH6vWc4uYbApnU= =5yCk -----END PGP SIGNATURE----- --------------enigEF5E2FFF594CDA589E890DBB--