Date: Wed, 7 Aug 2013 16:17:29 +0800
From: Amos Kong <akong@redhat.com>
Message-ID: <20130807081729.GA1624@amosk.info>
References: <1375831766-3444-1-git-send-email-akong@redhat.com>
	<5201EB97.9090207@redhat.com>
	<20130807075719.GH19825@stefanha-thinkpad.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20130807075719.GH19825@stefanha-thinkpad.redhat.com>
Subject: Re: [Qemu-devel] [PATCH] Moving alarm_timer assignment before
 atexit()
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: vyasevic@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org

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) ?


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.