diff mbox

Moving alarm_timer assignment before atexit()

Message ID 20130807081729.GA1624@amosk.info
State New
Headers show

Commit Message

Amos Kong Aug. 7, 2013, 8:17 a.m. UTC 
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 <stefanha@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) ?


 #ifdef CONFIG_POSIX

Comments

Stefan Hajnoczi Aug. 8, 2013, 8:19 a.m. UTC | #1 
On Wed, Aug 07, 2013 at 04:17:29PM +0800, Amos Kong wrote:
> 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);
> +    }
>  }

This is unnecessary once your other patch has been applied since t will
be initialized before quit_timers() is installed.

If we do ever hit the problem it should be debugged because assumptions
about the timer lifecycle must be broken.  So avoiding the segfault
isn't really useful here, I think.

Stefan
diff mbox

Patch

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);
+    }
 }