| Message ID | 20120514144058.2ffac223@doriath.home |
|---|---|
| State | New |
| Headers | show |
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.) > + _exit(!!ret); Why are we even bothering with ret? If execle() returns, we _know_ we had a failure, and !!ret will always be 1.
On Mon, 14 May 2012 11:51:13 -0600 Eric Blake <eblake@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.
On Mon, 14 May 2012 15:01:17 -0300 Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > + > > > + 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. -Wredundant-decls doesn't like it: /home/lcapitulino/work/src/qmp-unstable/qga/commands-posix.c:38:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls] /usr/include/unistd.h:546:15: note: previous declaration of ‘environ’ was here LINK qemu-ga
On 05/14/2012 12:03 PM, Luiz Capitulino wrote: > On Mon, 14 May 2012 15:01:17 -0300 > Luiz Capitulino <lcapitulino@redhat.com> wrote: > >>>> + >>>> + 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. > > -Wredundant-decls doesn't like it: > > /home/lcapitulino/work/src/qmp-unstable/qga/commands-posix.c:38:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls] > /usr/include/unistd.h:546:15: note: previous declaration of ‘environ’ was here > LINK qemu-ga Hmm, gnulib works around that by probing for whether environ was declared at configure time, in order to conditionalize whether to output a declaration of its own; but obviously we aren't using gnulib. Maybe, since we know that the glibc declaration is guarded by _GNU_SOURCE, we could likewise guard our declaration to only occur in the situations where glibc is not declaring it? Or maybe we can factor things into a common header used by other qemu files, where the header file itself is tagged in such a way to silence gcc warnings about any possible duplicate declaration, while still leaving the .c files that use this common header clean for use of -Wredundant-decls?
On Mon, 14 May 2012 12:41:16 -0600 Eric Blake <eblake@redhat.com> wrote: > On 05/14/2012 12:03 PM, Luiz Capitulino wrote: > > On Mon, 14 May 2012 15:01:17 -0300 > > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > >>>> + > >>>> + 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. > > > > -Wredundant-decls doesn't like it: > > > > /home/lcapitulino/work/src/qmp-unstable/qga/commands-posix.c:38:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls] > > /usr/include/unistd.h:546:15: note: previous declaration of ‘environ’ was here > > LINK qemu-ga > > Hmm, gnulib works around that by probing for whether environ was > declared at configure time, in order to conditionalize whether to output > a declaration of its own; but obviously we aren't using gnulib. Maybe, > since we know that the glibc declaration is guarded by _GNU_SOURCE, we > could likewise guard our declaration to only occur in the situations > where glibc is not declaring it? Or maybe we can factor things into a > common header used by other qemu files, where the header file itself is > tagged in such a way to silence gcc warnings about any possible > duplicate declaration, while still leaving the .c files that use this > common header clean for use of -Wredundant-decls? What's the worst case if we leave it as is? The build will brake if environ ends up not being declared by some arch, right? In that case we could leave it as is and fix it when it brakes :)
On 05/14/2012 01:59 PM, Luiz Capitulino wrote: >>>> I'll declare it then. >>> >>> -Wredundant-decls doesn't like it: >>> >>> /home/lcapitulino/work/src/qmp-unstable/qga/commands-posix.c:38:15: warning: redundant redeclaration of ‘environ’ [-Wredundant-decls] >>> /usr/include/unistd.h:546:15: note: previous declaration of ‘environ’ was here >>> LINK qemu-ga > > What's the worst case if we leave it as is? The build will brake if environ > ends up not being declared by some arch, right? In that case we could leave it > as is and fix it when it brakes :) Yep, which is precisely why I added my reviewed-by to v2.
diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index 1c949ff..8fd25be 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -126,8 +126,7 @@ # @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. # # @mode: #optional "halt", "powerdown" (default), or "reboot" # diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 9a59276..f2c5e94 100644 --- a/qga/commands-posix.c +++ 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); + + ret = execle("/sbin/shutdown", "shutdown", shutdown_flag, "+0", + "hypervisor initiated shutdown", (char*)NULL, environ); + _exit(!!ret); } else if (pid < 0) { goto exit_err; }
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 Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qapi-schema-guest.json | 3 +-- qga/commands-posix.c | 17 +++++++---------- 2 files changed, 8 insertions(+), 12 deletions(-)