From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48549) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STzaN-0004sw-1o for qemu-devel@nongnu.org; Mon, 14 May 2012 14:01:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1STzaJ-0000Fi-ID for qemu-devel@nongnu.org; Mon, 14 May 2012 14:01:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STzaJ-0000Ez-9G for qemu-devel@nongnu.org; Mon, 14 May 2012 14:01:27 -0400 Date: Mon, 14 May 2012 15:01:17 -0300 From: Luiz Capitulino Message-ID: <20120514150117.1cc082a5@doriath.home> In-Reply-To: <4FB14611.7090808@redhat.com> References: <20120514144058.2ffac223@doriath.home> <4FB14611.7090808@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-ga: guest-shutdown: use only async-signal-safe functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel , mdroth@linux.vnet.ibm.com On Mon, 14 May 2012 11:51:13 -0600 Eric Blake wrote: > On 05/14/2012 11:40 AM, Luiz Capitulino wrote: > > POSIX mandates[1] that a child process of a multi-thread program uses > > only async-signal-safe functions before exec(). We consider qemu-ga > > to be multi-thread, because it uses glib. > > > > However, qmp_guest_shutdown() uses functions that are not > > async-signal-safe. Fix it the following way: > > > > - fclose() -> reopen_fd_to_null() > > - execl() -> execle() > > - exit() -> _exit() > > - drop slog() usage (which is not safe) > > > > [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html > > > > > # @guest-shutdown: > > # > > # Initiate guest-activated shutdown. Note: this is an asynchronous > > -# shutdown request, with no guaruntee of successful shutdown. Errors > > -# will be logged to guest's syslog. > > +# shutdown request, with no guaruntee of successful shutdown. > > As long as you are changing docs, fix the typo: > > s/guaruntee/guarantee/ > > > +++ b/qga/commands-posix.c > > @@ -57,16 +57,13 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err) > > if (pid == 0) { > > /* child, start the shutdown */ > > setsid(); > > - fclose(stdin); > > - fclose(stdout); > > - fclose(stderr); > > - > > - ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0", > > - "hypervisor initiated shutdown", (char*)NULL); > > - if (ret) { > > - slog("guest-shutdown failed: %s", strerror(errno)); > > - } > > - exit(!!ret); > > + reopen_fd_to_null(0); > > + reopen_fd_to_null(1); > > + reopen_fd_to_null(2); > > I prefer the POSIX-mandated macros STDIN_FILENO, STDOUT_FILENO, and > STDERR_FILENO, but don't know if qemu intends to rely on them (according > to gnulib, at least older mingw lacked those macro names from > ). So I won't make you change this. > > > + > > + ret = execle("/sbin/shutdown", "shutdown", shutdown_flag, "+0", > > + "hypervisor initiated shutdown", (char*)NULL, environ); > > Where was 'environ' declared? POSIX says that environ must exist, but > that it is the one variable where you must declare it yourself rather > than getting it from a public header. (For convenience, glibc declares > environ in when using _GNU_SOURCE, but when you are asking > for strict standards namespace compliance, it disappears.) I'll declare it then. > > > + _exit(!!ret); > > Why are we even bothering with ret? If execle() returns, we _know_ we > had a failure, and !!ret will always be 1. True, will drop it.