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

[Qemu-devel] [PATCH] 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>
---
 qapi-schema-guest.json |    3 +--
 qga/commands-posix.c   |   17 +++++++----------
 2 files changed, 8 insertions(+), 12 deletions(-)

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;
     }
-- 
1.7.9.2.384.g4a92a

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

[Eric Blake]

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

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.

-- 
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] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Luiz Capitulino]

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.

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

[Luiz Capitulino]

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

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

[Eric Blake]

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

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?

-- 
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] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Luiz Capitulino]

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

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

[Eric Blake]

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

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.

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