Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Wed, 2010-10-13 at 17:37 +0100, Brendan Cully wrote:
> On Wednesday, 13 October 2010 at 13:46, Ian Campbell wrote:
> > On Wed, 2010-10-13 at 12:14 +0100, Stefano Stabellini wrote:
> > > I think that at the beginning of xc_domain_save we should check if
> > > callbacks->switch_qemu_logdirty is NULL and print an error an return in
> > > that case.
> > 
> > We didn't do so for the original function pointer parameter.
> > 
> > Not passing in this callback is a pretty fundamental error in the
> > caller, there's not really anything they can do with the error code.
> > This change will break compilation for any caller which has not been
> > updated so I don't think there is too much danger of toolstacks missing
> > the need for the change.
> > 
> > Propagating an EINVAL doesn't really help unless all callers reliably
> > test the return code and do something sensible with it.
> > 
> > On the other hand given that at least one caller has a valid reason not
> > to use the callback (unless this changes means it now could, as I
> > wondered in the original changelog but forgot to CC Brendan about) then
> > I think this would be more reasonable than EINVAL
> > 
> > Subject: libxc allow omission of hvm switch_qemu_logdirty on save
> 
> Either of these is ok with me, but I'd probably be inclined to make it
> an error to not have a switch_qemu_logdirty callback in the HVM
> case. It's either missing by accident, in which case allowing NULL
> means silent failure, or by design (as with the current python
> checkpoint bindings). If it's by design, that can be signalled by
> providing a dummy callback.

OK then I think we should add this:

diff -r 067d47ab6dc9 tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c      Wed Oct 13 11:08:22 2010 +0100
+++ b/tools/libxc/xc_domain_save.c      Wed Oct 13 13:40:16 2010 +0100
@@ -942,6 +942,13 @@ int xc_domain_save(xc_interface *xch, in
     struct domain_info_context *dinfo = &ctx->dinfo;
 
     int completed = 0;
+
+    if ( hvm && !callbacks->switch_qemu_logdirty )
+    {
+        ERROR("No switch_qemu_logdirty callback given.");
+        errno = EINVAL;
+        return 1;
+    }
 
     outbuf_init(xch, &ob, OUTBUF_SIZE);
 

> And now that the function has the closure data, I agree that it's
> cleaner for the python bindings to use the callback. (By the way, do
> we still need the domid argument to switch_qemu_logdirty? It seems
> redundant).

Yeah, I guess it could be in the closure. On the other hand its the one
thing all callers are going to want to know and we have it to hand when
we call the function.

> I could make the python bindings patch if you'd like (or you're
> welcome to :). Just let me know when the changes have hit the tree.

I took a look but I think I'd only end up breaking something, would you
mind?

One issue which was immediately apparent on my quick glance is that the
callback returns void but the switch_qemu_logdirty in libcheckpoint.c
can fail. Do you think we need to propagate an error code or can that
switch_qemu_logdirty be made to not fail (or safely ignore failure)? I
suspect libxl's error handling in this area could be improved if there
was error propagation here.

Ian.


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