On Mon, 14 May 2012 11:51:13 -0600 Eric Blake <ebl...@redhat.com> 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 > <unistd.h>). 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 <unistd.h> 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.
