Xen devel - Dec 2011 - qemu_timer_pending/qemu_get_timer: cope with NULL timers

qemu_timer_pending and qemu_get_timer: don''t crash if the timer passed
as an argument is NULL.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/vl.c b/vl.c
index f07a659..f3b3d02 100644
--- a/vl.c
+++ b/vl.c
@@ -1201,6 +1201,10 @@ void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
 int qemu_timer_pending(QEMUTimer *ts)
 {
     QEMUTimer *t;
+
+    if (ts == NULL)
+        return 0;
+
     for(t = active_timers[ts->clock->type]; t != NULL; t = t->next) {
         if (t == ts)
             return 1;
@@ -1272,6 +1276,9 @@ void qemu_get_timer(QEMUFile *f, QEMUTimer *ts)
 {
     uint64_t expire_time;
 
+    if (ts == NULL)
+        return;
+
     expire_time = qemu_get_be64(f);
     if (expire_time != -1) {
         qemu_mod_timer(ts, expire_time);

Stefano Stabellini writes ("qemu_timer_pending/qemu_get_timer: cope with
NULL timers"):
> qemu_timer_pending and qemu_get_timer: don''t crash if the timer
passed
> as an argument is NULL.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

On Mon, 2011-12-05 at 10:54 +0000, Stefano Stabellini
wrote:
> qemu_timer_pending and qemu_get_timer: don''t crash if the timer
passed
> as an argument is NULL.
It would have been useful to mention why a NULL timer is valid here.
Apart from just being good changelog practice to explain the actual
reason for a change this would also help tell us why this issue isn''t
being solved further up the stack. One the face of it this seem like
this ought to be a bug in the caller of qemu_mod_timer.

Ian.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/vl.c b/vl.c
> index f07a659..f3b3d02 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1201,6 +1201,10 @@ void qemu_mod_timer(QEMUTimer *ts, int64_t
expire_time)
>  int qemu_timer_pending(QEMUTimer *ts)
>  {
>      QEMUTimer *t;
> +
> +    if (ts == NULL)
> +        return 0;
> +
>      for(t = active_timers[ts->clock->type]; t != NULL; t =
t->next) {
>          if (t == ts)
>              return 1;
> @@ -1272,6 +1276,9 @@ void qemu_get_timer(QEMUFile *f, QEMUTimer *ts)
>  {
>      uint64_t expire_time;
>  
> +    if (ts == NULL)
> +        return;
> +
>      expire_time = qemu_get_be64(f);
>      if (expire_time != -1) {
>          qemu_mod_timer(ts, expire_time);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

On Mon, 5 Dec 2011, Ian Campbell wrote:
> On Mon, 2011-12-05 at 10:54 +0000, Stefano Stabellini wrote:
> > qemu_timer_pending and qemu_get_timer: don''t crash if the
timer passed
> > as an argument is NULL.
> 
> It would have been useful to mention why a NULL timer is valid here.
> Apart from just being good changelog practice to explain the actual
> reason for a change this would also help tell us why this issue
isn''t
> being solved further up the stack. One the face of it this seem like
> this ought to be a bug in the caller of qemu_mod_timer.
Yes, I have been a little bit too concise.

qemu_mod_timer is not the only caller of
qemu_timer_pending/qemu_get_timer: they are also called by some qemu
save/restore related functions, where the timer being saved and restored
can actually be NULL.

On Mon, 2011-12-05 at 14:53 +0000, Stefano Stabellini
wrote:
> On Mon, 5 Dec 2011, Ian Campbell wrote:
> > On Mon, 2011-12-05 at 10:54 +0000, Stefano Stabellini wrote:
> > > qemu_timer_pending and qemu_get_timer: don''t crash if
the timer passed
> > > as an argument is NULL.
> > 
> > It would have been useful to mention why a NULL timer is valid here.
> > Apart from just being good changelog practice to explain the actual
> > reason for a change this would also help tell us why this issue
isn''t
> > being solved further up the stack. One the face of it this seem like
> > this ought to be a bug in the caller of qemu_mod_timer.
> 
> Yes, I have been a little bit too concise.
> 
> qemu_mod_timer is not the only caller of
> qemu_timer_pending/qemu_get_timer: they are also called by some qemu
> save/restore related functions, where the timer being saved and restored
> can actually be NULL.
Thanks.

It sounds to me like the NULL check should have been in the save/restore
code but the patch is in so lets not worry about it.

Ian.

Ian Campbell writes ("Re: [Xen-devel] qemu_timer_pending/qemu_get_timer:
cope with NULL timers"):
> It sounds to me like the NULL check should have been in the save/restore
> code but the patch is in so lets not worry about it.
We now know that while this patch was attempting to fix the hvm
save/restore regression, it introduced a new bug of its own, now
thankfully fixed (we think!)

I think if the patch author had taken the trouble to write a comment
explaining the new behaviour, there would have been a better chance of
someone realising that turning an existing restore-read function into
a no-op wasn''t a sound thing to do.  So your criticism was right.

Of course this patch was itself intended as a fix for a regression -
so I applied it with perhaps less time for review than usual.  Perhaps
the lesson for me is that I should have been more ready to revert the
original patch and wait for a corrected version.

Given the new state of the code I don''t think it''s plausible
to move
the null check into the caller, but the resulting arrangements are
definitely tangled.  If this weren''t a maintenance-only branch, I
would want to see some cleanup but I think this rather messy approach
is OK in this case.

But I have added a comment on the new semantics of the qemu_get_timer.

Thanks,
Ian.

commit 54e24021005458ad0a361c1d83011b751726a94b
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Thu Dec 8 16:38:06 2011 +0000

    qemu_get_timer: Provide a comment about the behaviour on ts==NULL
    
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/vl.c b/vl.c
index 9e0a556..be8587a 100644
--- a/vl.c
+++ b/vl.c
@@ -1274,6 +1274,8 @@ void qemu_put_timer(QEMUFile *f, QEMUTimer *ts)
 
 void qemu_get_timer(QEMUFile *f, QEMUTimer *ts)
 {
+    /* If ts==NULL, reads the relevant amount of data from the
+       savefile but discards it */
     uint64_t expire_time;
 
     expire_time = qemu_get_be64(f);