* [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context
@ 2011-09-15 17:22 Laszlo Ersek
2011-09-15 19:16 ` Anthony Liguori
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Laszlo Ersek @ 2011-09-15 17:22 UTC (permalink / raw)
To: qemu-devel, lersek
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);
+ if ((size_t)len < sizeof buf) {
+ ret = write(STDERR_FILENO, buf, len); /* shut up gcc */
+ }
+ _exit(1);
}
}
diff --git a/sysemu.h b/sysemu.h
index 9090457..52a71ef 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -119,7 +119,7 @@ extern int max_cpus;
extern int cursor_hide;
extern int graphic_rotate;
extern int no_quit;
-extern int no_shutdown;
+extern volatile int no_shutdown;
extern int semihosting_enabled;
extern int old_param;
extern int boot_menu;
diff --git a/vl.c b/vl.c
index b773d2f..21bc6b4 100644
--- a/vl.c
+++ b/vl.c
@@ -215,7 +215,7 @@ int acpi_enabled = 1;
int no_hpet = 0;
int fd_bootchk = 1;
int no_reboot = 0;
-int no_shutdown = 0;
+volatile int no_shutdown = 0;
int cursor_hide = 1;
int graphic_rotate = 0;
uint8_t irq0override = 1;
@@ -1178,8 +1178,8 @@ typedef struct QEMUResetEntry {
static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
QTAILQ_HEAD_INITIALIZER(reset_handlers);
static int reset_requested;
-static int shutdown_requested, shutdown_signal = -1;
-static pid_t shutdown_pid;
+static volatile int shutdown_requested, shutdown_signal = -1;
+static volatile pid_t shutdown_pid;
static int powerdown_requested;
static int debug_requested;
static int vmstop_requested;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context
2011-09-15 17:22 [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context Laszlo Ersek
@ 2011-09-15 19:16 ` Anthony Liguori
2011-09-16 8:08 ` Laszlo Ersek
2011-09-15 19:44 ` Peter Maydell
2011-09-16 8:52 ` [Qemu-devel] [PATCH v2] " Laszlo Ersek
2 siblings, 1 reply; 7+ messages in thread
From: Anthony Liguori @ 2011-09-15 19:16 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context
2011-09-15 17:22 [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context Laszlo Ersek
2011-09-15 19:16 ` Anthony Liguori
@ 2011-09-15 19:44 ` Peter Maydell
2011-09-16 7:58 ` Laszlo Ersek
2011-09-16 8:52 ` [Qemu-devel] [PATCH v2] " Laszlo Ersek
2 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2011-09-15 19:44 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context
2011-09-15 19:44 ` Peter Maydell
@ 2011-09-16 7:58 ` Laszlo Ersek
2011-09-16 9:09 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2011-09-16 7:58 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context
2011-09-15 19:16 ` Anthony Liguori
@ 2011-09-16 8:08 ` Laszlo Ersek
0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2011-09-16 8:08 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2] main loop: fix some accesses made in sighandler context
2011-09-15 17:22 [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context Laszlo Ersek
2011-09-15 19:16 ` Anthony Liguori
2011-09-15 19:44 ` Peter Maydell
@ 2011-09-16 8:52 ` Laszlo Ersek
2 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2011-09-16 8:52 UTC (permalink / raw)
To: qemu-devel, lersek
Make variables volatile. "sig_atomic_t" should cover "int" and "pid_t", but
where it doesn't, the patch should still do no harm.
Also replace calls to functions that are not required to be async-signal-safe
[1].
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
Build tested only.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
cpus.c | 7 ++++---
sysemu.h | 2 +-
vl.c | 6 +++---
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/cpus.c b/cpus.c
index 54c188c..b38b334 100644
--- a/cpus.c
+++ b/cpus.c
@@ -289,9 +289,10 @@ 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);
+ static const char err[] = "qemu_event_increment: write() failed\n";
+
+ ret = write(STDERR_FILENO, err, sizeof err - 1u);
+ _exit(1);
}
}
diff --git a/sysemu.h b/sysemu.h
index 9090457..52a71ef 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -119,7 +119,7 @@ extern int max_cpus;
extern int cursor_hide;
extern int graphic_rotate;
extern int no_quit;
-extern int no_shutdown;
+extern volatile int no_shutdown;
extern int semihosting_enabled;
extern int old_param;
extern int boot_menu;
diff --git a/vl.c b/vl.c
index b773d2f..21bc6b4 100644
--- a/vl.c
+++ b/vl.c
@@ -215,7 +215,7 @@ int acpi_enabled = 1;
int no_hpet = 0;
int fd_bootchk = 1;
int no_reboot = 0;
-int no_shutdown = 0;
+volatile int no_shutdown = 0;
int cursor_hide = 1;
int graphic_rotate = 0;
uint8_t irq0override = 1;
@@ -1178,8 +1178,8 @@ typedef struct QEMUResetEntry {
static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
QTAILQ_HEAD_INITIALIZER(reset_handlers);
static int reset_requested;
-static int shutdown_requested, shutdown_signal = -1;
-static pid_t shutdown_pid;
+static volatile int shutdown_requested, shutdown_signal = -1;
+static volatile pid_t shutdown_pid;
static int powerdown_requested;
static int debug_requested;
static int vmstop_requested;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context
2011-09-16 7:58 ` Laszlo Ersek
@ 2011-09-16 9:09 ` Markus Armbruster
0 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2011-09-16 9:09 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Peter Maydell, qemu-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-09-16 9:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-15 17:22 [Qemu-devel] [PATCH RFC] main loop: fix some accesses made in sighandler context Laszlo Ersek
2011-09-15 19:16 ` Anthony Liguori
2011-09-16 8:08 ` Laszlo Ersek
2011-09-15 19:44 ` Peter Maydell
2011-09-16 7:58 ` Laszlo Ersek
2011-09-16 9:09 ` Markus Armbruster
2011-09-16 8:52 ` [Qemu-devel] [PATCH v2] " Laszlo Ersek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.