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

[Brendan Cully <brendan@xxxxxxxxx>]

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.

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

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.

> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> 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:45:25 2010 +0100
> @@ -1014,7 +1014,7 @@ int xc_domain_save(xc_interface *xch, in
>          }
>  
>          /* Enable qemu-dm logging dirty pages to xen */
> -        if ( hvm )
> +        if ( hvm && callbacks->switch_qemu_logdirty )
>              callbacks->switch_qemu_logdirty(dom, 1, callbacks->data);
>      }
>      else
> @@ -1875,7 +1875,7 @@ int xc_domain_save(xc_interface *xch, in
>                                 XEN_DOMCTL_SHADOW_OP_OFF,
>                                 NULL, 0, NULL, 0, NULL) < 0 )
>              DPRINTF("Warning - couldn't disable shadow mode");
> -        if ( hvm )
> +        if ( hvm && callbacks->switch_qemu_logdirty )
>              callbacks->switch_qemu_logdirty(dom, 0, callbacks->data);
>      }
>  
> diff -r 067d47ab6dc9 tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c    Wed Oct 13 
> 11:08:22 2010 +0100
> +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c    Wed Oct 13 
> 13:45:25 2010 +0100
> @@ -162,13 +162,6 @@ void checkpoint_close(checkpoint_state* 
>    s->fd = -1;
>  }
>  
> -/* we toggle logdirty ourselves around the xc_domain_save call --
> - * it avoids having to pass around checkpoint_state */
> -static void noop_switch_logdirty(int domid, unsigned enable, void *data)
> -{
> -    return;
> -}
> -
>  int checkpoint_start(checkpoint_state* s, int fd,
>                      struct save_callbacks* callbacks)
>  {
> @@ -189,7 +182,7 @@ int checkpoint_start(checkpoint_state* s
>             return rc;
>      }
>  
> -    callbacks->switch_qemu_logdirty = noop_switch_logdirty;
> +    callbacks->switch_qemu_logdirty = NULL;
>  
>      rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm);
>  
> 
> 

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