[RFC] main loop: fix some accesses made in sighandler context

Make variables volatile ("sig_atomic_t" should cover "int" and "pid_t").

Also replace calls to functions that are not required to be async-signal-safe
[1]. (I haven't checked if any signal masks and/or previous suspension of the
interrupted thread keep the current calls safe.)

termsig_handler()
  -> qemu_system_killed(): shutdown_signal, shutdown_pid, no_shutdown [2]
    -> qemu_system_shutdown_request(): shutdown_requested
      -> qemu_notify_event()
        -> qemu_event_increment(): fprintf(), strerror(), exit()

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03
[2] http://lists.nongnu.org/archive/html/qemu-devel/2011-09/msg01757.html

"checkpatch.pl" warned four times about "volatile", and considered the
zero-initialization of "no_shutdown" (which has static storage duration) an
error.

Build tested only. Please CC me on any followup, I'm not subscribed. Thank you.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 cpus.c   |   13 ++++++++++---
 sysemu.h |    2 +-
 vl.c     |    6 +++---
 3 files changed, 14 insertions(+), 7 deletions(-)

On 09/15/2011 12:22 PM, Laszlo Ersek wrote:
> Make variables volatile ("sig_atomic_t" should cover "int" and "pid_t").
>
> Also replace calls to functions that are not required to be async-signal-safe
> [1]. (I haven't checked if any signal masks and/or previous suspension of the
> interrupted thread keep the current calls safe.)
>
> termsig_handler()
>    ->  qemu_system_killed(): shutdown_signal, shutdown_pid, no_shutdown [2]
>      ->  qemu_system_shutdown_request(): shutdown_requested
>        ->  qemu_notify_event()
>          ->  qemu_event_increment(): fprintf(), strerror(), exit()
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03
> [2] http://lists.nongnu.org/archive/html/qemu-devel/2011-09/msg01757.html
>
> "checkpatch.pl" warned four times about "volatile", and considered the
> zero-initialization of "no_shutdown" (which has static storage duration) an
> error.
>
> Build tested only. Please CC me on any followup, I'm not subscribed. Thank you.
>
> Signed-off-by: Laszlo Ersek<lersek@redhat.com>
> ---
>   cpus.c   |   13 ++++++++++---
>   sysemu.h |    2 +-
>   vl.c     |    6 +++---
>   3 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 54c188c..ed51247 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -289,9 +289,16 @@ static void qemu_event_increment(void)
>
>       /* EAGAIN is fine, a read must be pending.  */
>       if (ret<  0&&  errno != EAGAIN) {
> -        fprintf(stderr, "qemu_event_increment: write() failed: %s\n",
> -                strerror(errno));
> -        exit (1);
> +        int len;
> +        char buf[128];
> +
> +        /* Don't bother with strerror_[rl]. Make a single attempt to write. */
> +        len = snprintf(buf, sizeof buf,
> +                       "qemu_event_increment: write() failed: %d\n", errno);

I don't think you can rely on snprintf being signal safe.  I think you should 
just exit on failure.

OpenBSD lists snprintf as signal safe, but "probably not on other systems."

Regards,

Anthony Liguori

On 15 September 2011 18:22, Laszlo Ersek <lersek@redhat.com> wrote:
> -int no_shutdown = 0;
> +volatile int no_shutdown = 0;

So why 'volatile' and not 'sig_atomic_t', then?

thanks
-- PMM

On 09/15/11 21:44, Peter Maydell wrote:
> On 15 September 2011 18:22, Laszlo Ersek<lersek@redhat.com>  wrote:
>> -int no_shutdown = 0;
>> +volatile int no_shutdown = 0;
>
> So why 'volatile' and not 'sig_atomic_t', then?

The sigaction() spec  says"volatile sig_atomic_t", so that would be 
ideal. My assumption was that "sig_atomic_t" (which is allowed by POSIX 
not to be wider than "char") would be in practice at least as wide as 
"int" and "pid_t". Should my assumption be wrong on some platforms, 
qualifying the variables "volatile" while keeping their current types 
(int / pid_t) does less damage (no damage) than narrowing their types.

lacos

On 09/15/11 21:16, Anthony Liguori wrote:
> On 09/15/2011 12:22 PM, Laszlo Ersek wrote:

>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03
>
> I don't think you can rely on snprintf being signal safe. I think you
> should just exit on failure.
>
> OpenBSD lists snprintf as signal safe, but "probably not on other systems."

I wasn't diligent enough to look up snprintf() in the table I linked 
myself. In other news, I hold a Programmers' Darwin Award. Will send v2.

Thanks,
lacos

Laszlo Ersek <lersek@redhat.com> writes:

> On 09/15/11 21:44, Peter Maydell wrote:
>> On 15 September 2011 18:22, Laszlo Ersek<lersek@redhat.com>  wrote:
>>> -int no_shutdown = 0;
>>> +volatile int no_shutdown = 0;
>>
>> So why 'volatile' and not 'sig_atomic_t', then?
>
> The sigaction() spec  says"volatile sig_atomic_t", so that would be
> ideal. My assumption was that "sig_atomic_t" (which is allowed by
> POSIX not to be wider than "char") would be in practice at least as

Inherited from the C standard.

> wide as "int" and "pid_t". Should my assumption be wrong on some
> platforms, qualifying the variables "volatile" while keeping their
> current types (int / pid_t) does less damage (no damage) than
> narrowing their types.

info libc says:

    In practice, you can assume that `int' is atomic.  You can also
    assume that pointer types are atomic; that is very convenient.  Both
    of these assumptions are true on all of the machines that the GNU C
    library supports and on all POSIX systems we know of.

If you're programming for a machine where int isn't atomic, you very
likely got more serious issues to worry about :)

Non-atomic pid_t would be weird, but not quite as weird as non-atomic
int.

Regardless, no_shutdown is used like bool, so you could easily make it
sig_atomic_t.