On Wed, Aug 07, 2013 at 09:57:19AM +0200, Stefan Hajnoczi wrote:
> On Wed, Aug 07, 2013 at 08:39:19AM +0200, Laszlo Ersek wrote:
> > On 08/07/13 01:29, Amos Kong wrote:
> > > We register exit clean function by atexit(),
> > > but alarm_timer is NULL here. If exit is caused
> > > between atexit() and alarm_timer assignment,
> > > real timer can't be cleaned.
> >
> > That's correct in general, but I don't see how it could happen in the
> > code being patched. pthread_atfork() won't call exit().
I try to sleep 10 seconds after atexit(), but no crash occurred when I
killed qemu process.
atexit(quit_timer);
sleep(10); // kill qemu by 'pkill qemu', no crash
pthread_atfork();
alarm_timer = t;
> Agreed. I can remember thinking about this when reading the code and
> deciding not to bother changing it.
>
> Since the patch is on the list though, we might as well apply it.
>
> The only thing I suggest changing is to note that this is currently not
> a bug, just a clean-up.
It seems just a cleanup.
> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
BTW, can we add a check in quit_timers() to avoid one kind of crash
(try to quit the uninited timers, or quit timer repeatedly) ?
diff --git a/qemu-timer.c b/qemu-timer.c
index b2d95e2..023e4ae 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -728,8 +728,10 @@ static void win32_rearm_timer(struct
qemu_alarm_timer *t,
static void quit_timers(void)
{
struct qemu_alarm_timer *t = alarm_timer;
- alarm_timer = NULL;
- t->stop(t);
+ if (t) {
+ alarm_timer = NULL;
+ t->stop(t);
+ }
}
#ifdef CONFIG_POSIX
--
Amos.
