[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qemu-ga: guest-shutdown: use only async-signal-
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH] qemu-ga: guest-shutdown: use only async-signal-safe functions |
Date: |
Mon, 14 May 2012 15:01:17 -0300 |
On Mon, 14 May 2012 11:51:13 -0600
Eric Blake <address@hidden> 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.