On Wednesday, 13 October 2010 at 17:51, Ian Campbell wrote:
> 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);
This looks good to me.
> > 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.
ok.
> > 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?
sure, when it hits the tree. It looks like your changes will work with
the existing code until then.
> 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.
Since switch_qemu_logdirty runs through xenstore and depends on a
remote response, I don't think it can be guaranteed not to
fail, or to fail safely (if the remote hasn't turned on logdirty,
checkpointing is broken). So ideally we'd propagate the error.
Honestly, I'm not even sure why this is a callback, except that it's
an easy way to reuse an existing xenstore handle. Shouldn't
xc_domain_save just handle this internally, like it does for non-QEMU
logdirty? Maybe that's a job for another day :)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home