Patchwork qtest: setitimer() failures on Darwin and illumos

login
register
mail settings
Submitter Stefano Stabellini
Date May 29, 2012, 12:01 p.m.
Message ID <alpine.DEB.2.00.1205291146330.26786@kaball-desktop>
Download mbox | patch
Permalink /patch/161738/
State New
Headers show

Comments

Stefano Stabellini - May 29, 2012, 12:01 p.m.
On Mon, 28 May 2012, Paolo Bonzini wrote:
> Il 28/05/2012 21:40, Andreas Färber ha scritto:
> > I'm seeing qemu-timer.c:unix_rearm_timer()'s setitimer() abort with
> > EINVAL during `make check` on both platforms. The value of
> > nearest_delta_ns appears to be INT64_MAX. Is this expected? Is it
> > possible that this value is too large for it_value on some platforms?
> > Apple's man page mentions that as possible reason for EINVAL but doesn't
> > describe how to obtain such an upper value, nor of course where in the
> > QEMU code base we would need to make adaptions. ;)
> > 
> > Any suggestions?
> 
> You shouldn't call the rearm function at all if you get INT64_MAX.  This
> applies to all timers.

Yep. In fact qemu_rearm_alarm_timer returns immediately if none of the
clocks have active timers.
However if at least one of them does, we call qemu_next_alarm_deadline
(that potentially can return INT64_MAX) and then rearm.

So for example if the clock that has active timers is disabled (I don't
if it is possible to get in this state), qemu_next_alarm_deadline would
return INT64_MAX.
I think we should make the appended change in order to make the code
more reliable.

Regarding setitimer returning -EINVAL, we could check for out of bound
parameters in unix_rearm_timer, but we need to know exactly what the
upper limit is. I tried to look for the Darwin manpage of setitimer, but
that information is missing.
Could you please run a couple of tests to find out what the actual limit
is?

---
Paolo Bonzini - May 29, 2012, 12:21 p.m.
Il 29/05/2012 14:01, Stefano Stabellini ha scritto:
> On Mon, 28 May 2012, Paolo Bonzini wrote:
>> Il 28/05/2012 21:40, Andreas Färber ha scritto:
>>> I'm seeing qemu-timer.c:unix_rearm_timer()'s setitimer() abort with
>>> EINVAL during `make check` on both platforms. The value of
>>> nearest_delta_ns appears to be INT64_MAX. Is this expected? Is it
>>> possible that this value is too large for it_value on some platforms?
>>> Apple's man page mentions that as possible reason for EINVAL but doesn't
>>> describe how to obtain such an upper value, nor of course where in the
>>> QEMU code base we would need to make adaptions. ;)
>>>
>>> Any suggestions?
>>
>> You shouldn't call the rearm function at all if you get INT64_MAX.  This
>> applies to all timers.
> 
> Yep. In fact qemu_rearm_alarm_timer returns immediately if none of the
> clocks have active timers.
> However if at least one of them does, we call qemu_next_alarm_deadline
> (that potentially can return INT64_MAX) and then rearm.
> 
> So for example if the clock that has active timers is disabled (I don't
> if it is possible to get in this state), qemu_next_alarm_deadline would
> return INT64_MAX.
> I think we should make the appended change in order to make the code
> more reliable.

Yes, that makes sense, can you submit it with SoB?

Paolo

Patch

diff --git a/qemu-timer.c b/qemu-timer.c
index de98977..81ff824 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -112,14 +112,10 @@  static int64_t qemu_next_alarm_deadline(void)
 
 static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
 {
-    int64_t nearest_delta_ns;
-    if (!rt_clock->active_timers &&
-        !vm_clock->active_timers &&
-        !host_clock->active_timers) {
-        return;
+    int64_t nearest_delta_ns = qemu_next_alarm_deadline();
+    if (nearest_delta_ns < INT64_MAX) {
+        t->rearm(t, nearest_delta_ns);
     }
-    nearest_delta_ns = qemu_next_alarm_deadline();
-    t->rearm(t, nearest_delta_ns);
 }
 
 /* TODO: MIN_TIMER_REARM_NS should be optimized */