Re: [Xen-devel] qemu_timer_pending/qemu_get_timer: cope with NULL timers

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel