[Qemu-devel] [PATCH v2] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Qemu-devel] [PATCH v2] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Luiz Capitulino]

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>
---

o v2

- fix doc typo
- drop 'ret' and use EXIT_FAILURE instead

 qapi-schema-guest.json |    3 +--
 qga/commands-posix.c   |   19 ++++++++-----------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 1c949ff..bd2256d 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 guarantee of successful shutdown.
 #
 # @mode: #optional "halt", "powerdown" (default), or "reboot"
 #
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 9a59276..15ce928 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -37,8 +37,8 @@
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
     const char *shutdown_flag;
-    int ret, status;
     pid_t rpid, pid;
+    int status;
 
     slog("guest-shutdown called, mode: %s", mode);
     if (!has_mode || strcmp(mode, "powerdown") == 0) {
@@ -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);
+
+        execle("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
+               "hypervisor initiated shutdown", (char*)NULL, environ);
+        _exit(EXIT_FAILURE);
     } else if (pid < 0) {
         goto exit_err;
     }
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH v2] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On 05/14/2012 12:25 PM, 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
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> 
> o v2
> 
> - fix doc typo
> - drop 'ret' and use EXIT_FAILURE instead

Not addressed was the 'environ' declaration issue, but I'm okay if you
save it for another day when we actually get a compile failure for using
an undeclared variable (since what you have _does_ work on glibc).

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Michael Roth]

On Mon, May 14, 2012 at 03:25:20PM -0300, 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
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Thanks, applied to qga branch.

> ---
> 
> o v2
> 
> - fix doc typo
> - drop 'ret' and use EXIT_FAILURE instead
> 
>  qapi-schema-guest.json |    3 +--
>  qga/commands-posix.c   |   19 ++++++++-----------
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 1c949ff..bd2256d 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 guarantee of successful shutdown.
>  #
>  # @mode: #optional "halt", "powerdown" (default), or "reboot"
>  #
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 9a59276..15ce928 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -37,8 +37,8 @@
>  void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>  {
>      const char *shutdown_flag;
> -    int ret, status;
>      pid_t rpid, pid;
> +    int status;
> 
>      slog("guest-shutdown called, mode: %s", mode);
>      if (!has_mode || strcmp(mode, "powerdown") == 0) {
> @@ -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);
> +
> +        execle("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> +               "hypervisor initiated shutdown", (char*)NULL, environ);
> +        _exit(EXIT_FAILURE);
>      } else if (pid < 0) {
>          goto exit_err;
>      }
> -- 
> 1.7.9.2.384.g4a92a
>