Mailing List Archive

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel