Ian Campbell
2010-Oct-12 16:05 UTC
[Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1286899422 -3600
# Node ID 92be1317280b14e63b381285cdf342dfae014e66
# Parent 372959917c012db7d90aad0626a4af6b8f9b186f
tools: add closure to xc_domain_save switch_qemu_logdirty callback
Also move the function into struct save_callbacks with the others.
Use this in libxl to pass the save context to
libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse
the parent''s xenstore handle, gc context etc.
Also add an apparently missing libxl__free_all to
libxl__domain_suspend_common.
(Now that switch_qemu_logdirty takes a closure it''s not clear if
checkpoint can be changed to use the callback rather than doing the
switch itself and implementing a nop callback, Branden?)
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Change from v1:
Compile test from missed call to xc_domain_save in
tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
diff -r 372959917c01 -r 92be1317280b tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Tue Oct 12 17:03:42 2010 +0100
+++ b/tools/libxc/xc_domain_save.c Tue Oct 12 17:03:42 2010 +0100
@@ -879,7 +879,7 @@ int xc_domain_save(xc_interface *xch, in
int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t
max_iters,
uint32_t max_factor, uint32_t flags,
struct save_callbacks* callbacks,
- int hvm, void (*switch_qemu_logdirty)(int, unsigned))
+ int hvm)
{
xc_dominfo_t info;
DECLARE_DOMCTL;
@@ -1015,7 +1015,7 @@ int xc_domain_save(xc_interface *xch, in
/* Enable qemu-dm logging dirty pages to xen */
if ( hvm )
- switch_qemu_logdirty(dom, 1);
+ callbacks->switch_qemu_logdirty(dom, 1, callbacks->data);
}
else
{
@@ -1876,7 +1876,7 @@ int xc_domain_save(xc_interface *xch, in
NULL, 0, NULL, 0, NULL) < 0 )
DPRINTF("Warning - couldn''t disable shadow
mode");
if ( hvm )
- switch_qemu_logdirty(dom, 0);
+ callbacks->switch_qemu_logdirty(dom, 0, callbacks->data);
}
if ( live_shinfo )
diff -r 372959917c01 -r 92be1317280b tools/libxc/xenguest.h
--- a/tools/libxc/xenguest.h Tue Oct 12 17:03:42 2010 +0100
+++ b/tools/libxc/xenguest.h Tue Oct 12 17:03:42 2010 +0100
@@ -40,6 +40,8 @@ struct save_callbacks {
* 1: take another checkpoint */
int (*checkpoint)(void* data);
+ void (*switch_qemu_logdirty)(int domid, unsigned enable, void *data); /*
HVM only */
+
/* to be provided as the first argument to each callback function */
void* data;
};
@@ -55,7 +57,7 @@ int xc_domain_save(xc_interface *xch, in
int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t
max_iters,
uint32_t max_factor, uint32_t flags /* XCFLAGS_xxx */,
struct save_callbacks* callbacks,
- int hvm, void (*switch_qemu_logdirty)(int, unsigned)); /*
HVM only */
+ int hvm);
/**
diff -r 372959917c01 -r 92be1317280b tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Tue Oct 12 17:03:42 2010 +0100
+++ b/tools/libxl/libxl_dom.c Tue Oct 12 17:03:42 2010 +0100
@@ -325,21 +325,18 @@ struct suspendinfo {
unsigned int flags;
};
-static void libxl__domain_suspend_common_switch_qemu_logdirty(int domid,
unsigned int enable)
+static void libxl__domain_suspend_common_switch_qemu_logdirty(int domid,
unsigned int enable, void *data)
{
- struct xs_handle *xsh;
- char path[64];
+ struct suspendinfo *si = data;
+ libxl_ctx *ctx = libxl__gc_owner(si->gc);
+ char *path;
- snprintf(path, sizeof(path),
"/local/domain/0/device-model/%u/logdirty/cmd", domid);
-
- xsh = xs_daemon_open();
+ path = libxl__sprintf(si->gc,
"/local/domain/0/device-model/%u/logdirty/cmd", domid);
if (enable)
- xs_write(xsh, XBT_NULL, path, "enable",
strlen("enable"));
+ xs_write(ctx->xsh, XBT_NULL, path, "enable",
strlen("enable"));
else
- xs_write(xsh, XBT_NULL, path, "disable",
strlen("disable"));
-
- xs_daemon_close(xsh);
+ xs_write(ctx->xsh, XBT_NULL, path, "disable",
strlen("disable"));
}
static int libxl__domain_suspend_common_callback(void *data)
@@ -437,11 +434,11 @@ int libxl__domain_suspend_common(libxl_c
memset(&callbacks, 0, sizeof(callbacks));
callbacks.suspend = libxl__domain_suspend_common_callback;
+ callbacks.switch_qemu_logdirty =
libxl__domain_suspend_common_switch_qemu_logdirty;
callbacks.data = &si;
xc_domain_save(ctx->xch, fd, domid, 0, 0, flags,
- &callbacks, hvm,
- &libxl__domain_suspend_common_switch_qemu_logdirty);
+ &callbacks, hvm);
if (si.suspend_eventchn > 0)
xc_suspend_evtchn_release(ctx->xch, si.xce, domid,
si.suspend_eventchn);
@@ -450,6 +447,7 @@ int libxl__domain_suspend_common(libxl_c
rc = 0;
out:
+ libxl__free_all(&gc);
return rc;
}
diff -r 372959917c01 -r 92be1317280b
tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 12 17:03:42
2010 +0100
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 12 17:03:42
2010 +0100
@@ -164,7 +164,7 @@ void checkpoint_close(checkpoint_state*
/* 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)
+static void noop_switch_logdirty(int domid, unsigned enable, void *data)
{
return;
}
@@ -189,8 +189,9 @@ int checkpoint_start(checkpoint_state* s
return rc;
}
- rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks,
hvm,
- noop_switch_logdirty);
+ callbacks->switch_qemu_logdirty = noop_switch_logdirty;
+
+ rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks,
hvm);
if (hvm)
switch_qemu_logdirty(s, 0);
diff -r 372959917c01 -r 92be1317280b tools/xcutils/xc_save.c
--- a/tools/xcutils/xc_save.c Tue Oct 12 17:03:42 2010 +0100
+++ b/tools/xcutils/xc_save.c Tue Oct 12 17:03:42 2010 +0100
@@ -102,7 +102,7 @@ static int suspend(void* data)
* active buffer. */
-static void switch_qemu_logdirty(int domid, unsigned int enable)
+static void switch_qemu_logdirty(int domid, unsigned int enable, void *data)
{
struct xs_handle *xs;
char *path, *p, *ret_str, *cmd_str, **watch;
@@ -205,9 +205,9 @@ main(int argc, char **argv)
}
memset(&callbacks, 0, sizeof(callbacks));
callbacks.suspend = suspend;
+ callbacks.switch_qemu_logdirty = switch_qemu_logdirty;
ret = xc_domain_save(si.xch, io_fd, si.domid, maxit, max_f, si.flags,
- &callbacks, !!(si.flags & XCFLAGS_HVM),
- &switch_qemu_logdirty);
+ &callbacks, !!(si.flags & XCFLAGS_HVM));
if (si.suspend_evtchn > 0)
xc_suspend_evtchn_release(si.xch, si.xce, si.domid, si.suspend_evtchn);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Oct-13 11:14 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
On Tue, 12 Oct 2010, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1286899422 -3600 > # Node ID 92be1317280b14e63b381285cdf342dfae014e66 > # Parent 372959917c012db7d90aad0626a4af6b8f9b186f > tools: add closure to xc_domain_save switch_qemu_logdirty callback > > Also move the function into struct save_callbacks with the others. > > Use this in libxl to pass the save context to > libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse > the parent''s xenstore handle, gc context etc. > > Also add an apparently missing libxl__free_all to > libxl__domain_suspend_common. > > (Now that switch_qemu_logdirty takes a closure it''s not clear if > checkpoint can be changed to use the callback rather than doing the > switch itself and implementing a nop callback, Branden?) > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > Change from v1: > Compile test from missed call to xc_domain_save in > tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > > diff -r 372959917c01 -r 92be1317280b tools/libxc/xc_domain_save.c > --- a/tools/libxc/xc_domain_save.c Tue Oct 12 17:03:42 2010 +0100 > +++ b/tools/libxc/xc_domain_save.c Tue Oct 12 17:03:42 2010 +0100 > @@ -879,7 +879,7 @@ int xc_domain_save(xc_interface *xch, in > int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iters, > uint32_t max_factor, uint32_t flags, > struct save_callbacks* callbacks, > - int hvm, void (*switch_qemu_logdirty)(int, unsigned)) > + int hvm) > { > xc_dominfo_t info; > DECLARE_DOMCTL; > @@ -1015,7 +1015,7 @@ int xc_domain_save(xc_interface *xch, in > > /* Enable qemu-dm logging dirty pages to xen */ > if ( hvm ) > - switch_qemu_logdirty(dom, 1); > + callbacks->switch_qemu_logdirty(dom, 1, callbacks->data); > } > else > { > @@ -1876,7 +1876,7 @@ int xc_domain_save(xc_interface *xch, in > NULL, 0, NULL, 0, NULL) < 0 ) > DPRINTF("Warning - couldn''t disable shadow mode"); > if ( hvm ) > - switch_qemu_logdirty(dom, 0); > + callbacks->switch_qemu_logdirty(dom, 0, callbacks->data); > } > > if ( live_shinfo )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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-13 12:46 UTC
[Xen-devel] Re: [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
Brendan, forgot to CC you on this. Sorry for mangling your name below too. On Tue, 2010-10-12 at 17:05 +0100, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1286899422 -3600 > # Node ID 92be1317280b14e63b381285cdf342dfae014e66 > # Parent 372959917c012db7d90aad0626a4af6b8f9b186f > tools: add closure to xc_domain_save switch_qemu_logdirty callback > > Also move the function into struct save_callbacks with the others. > > Use this in libxl to pass the save context to > libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse > the parent''s xenstore handle, gc context etc. > > Also add an apparently missing libxl__free_all to > libxl__domain_suspend_common. > > (Now that switch_qemu_logdirty takes a closure it''s not clear if > checkpoint can be changed to use the callback rather than doing the > switch itself and implementing a nop callback, Branden?) > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > Change from v1: > Compile test from missed call to xc_domain_save in > tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > > diff -r 372959917c01 -r 92be1317280b tools/libxc/xc_domain_save.c > --- a/tools/libxc/xc_domain_save.c Tue Oct 12 17:03:42 2010 +0100 > +++ b/tools/libxc/xc_domain_save.c Tue Oct 12 17:03:42 2010 +0100 > @@ -879,7 +879,7 @@ int xc_domain_save(xc_interface *xch, in > int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iters, > uint32_t max_factor, uint32_t flags, > struct save_callbacks* callbacks, > - int hvm, void (*switch_qemu_logdirty)(int, unsigned)) > + int hvm) > { > xc_dominfo_t info; > DECLARE_DOMCTL; > @@ -1015,7 +1015,7 @@ int xc_domain_save(xc_interface *xch, in > > /* Enable qemu-dm logging dirty pages to xen */ > if ( hvm ) > - switch_qemu_logdirty(dom, 1); > + callbacks->switch_qemu_logdirty(dom, 1, callbacks->data); > } > else > { > @@ -1876,7 +1876,7 @@ int xc_domain_save(xc_interface *xch, in > NULL, 0, NULL, 0, NULL) < 0 ) > DPRINTF("Warning - couldn''t disable shadow mode"); > if ( hvm ) > - switch_qemu_logdirty(dom, 0); > + callbacks->switch_qemu_logdirty(dom, 0, callbacks->data); > } > > if ( live_shinfo ) > diff -r 372959917c01 -r 92be1317280b tools/libxc/xenguest.h > --- a/tools/libxc/xenguest.h Tue Oct 12 17:03:42 2010 +0100 > +++ b/tools/libxc/xenguest.h Tue Oct 12 17:03:42 2010 +0100 > @@ -40,6 +40,8 @@ struct save_callbacks { > * 1: take another checkpoint */ > int (*checkpoint)(void* data); > > + void (*switch_qemu_logdirty)(int domid, unsigned enable, void *data); /* HVM only */ > + > /* to be provided as the first argument to each callback function */ > void* data; > }; > @@ -55,7 +57,7 @@ int xc_domain_save(xc_interface *xch, in > int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iters, > uint32_t max_factor, uint32_t flags /* XCFLAGS_xxx */, > struct save_callbacks* callbacks, > - int hvm, void (*switch_qemu_logdirty)(int, unsigned)); /* HVM only */ > + int hvm); > > > /** > diff -r 372959917c01 -r 92be1317280b tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Tue Oct 12 17:03:42 2010 +0100 > +++ b/tools/libxl/libxl_dom.c Tue Oct 12 17:03:42 2010 +0100 > @@ -325,21 +325,18 @@ struct suspendinfo { > unsigned int flags; > }; > > -static void libxl__domain_suspend_common_switch_qemu_logdirty(int domid, unsigned int enable) > +static void libxl__domain_suspend_common_switch_qemu_logdirty(int domid, unsigned int enable, void *data) > { > - struct xs_handle *xsh; > - char path[64]; > + struct suspendinfo *si = data; > + libxl_ctx *ctx = libxl__gc_owner(si->gc); > + char *path; > > - snprintf(path, sizeof(path), "/local/domain/0/device-model/%u/logdirty/cmd", domid); > - > - xsh = xs_daemon_open(); > + path = libxl__sprintf(si->gc, "/local/domain/0/device-model/%u/logdirty/cmd", domid); > > if (enable) > - xs_write(xsh, XBT_NULL, path, "enable", strlen("enable")); > + xs_write(ctx->xsh, XBT_NULL, path, "enable", strlen("enable")); > else > - xs_write(xsh, XBT_NULL, path, "disable", strlen("disable")); > - > - xs_daemon_close(xsh); > + xs_write(ctx->xsh, XBT_NULL, path, "disable", strlen("disable")); > } > > static int libxl__domain_suspend_common_callback(void *data) > @@ -437,11 +434,11 @@ int libxl__domain_suspend_common(libxl_c > > memset(&callbacks, 0, sizeof(callbacks)); > callbacks.suspend = libxl__domain_suspend_common_callback; > + callbacks.switch_qemu_logdirty = libxl__domain_suspend_common_switch_qemu_logdirty; > callbacks.data = &si; > > xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, > - &callbacks, hvm, > - &libxl__domain_suspend_common_switch_qemu_logdirty); > + &callbacks, hvm); > > if (si.suspend_eventchn > 0) > xc_suspend_evtchn_release(ctx->xch, si.xce, domid, si.suspend_eventchn); > @@ -450,6 +447,7 @@ int libxl__domain_suspend_common(libxl_c > > rc = 0; > out: > + libxl__free_all(&gc); > return rc; > } > > diff -r 372959917c01 -r 92be1317280b tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 12 17:03:42 2010 +0100 > +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 12 17:03:42 2010 +0100 > @@ -164,7 +164,7 @@ void checkpoint_close(checkpoint_state* > > /* 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) > +static void noop_switch_logdirty(int domid, unsigned enable, void *data) > { > return; > } > @@ -189,8 +189,9 @@ int checkpoint_start(checkpoint_state* s > return rc; > } > > - rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm, > - noop_switch_logdirty); > + callbacks->switch_qemu_logdirty = noop_switch_logdirty; > + > + rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm); > > if (hvm) > switch_qemu_logdirty(s, 0); > diff -r 372959917c01 -r 92be1317280b tools/xcutils/xc_save.c > --- a/tools/xcutils/xc_save.c Tue Oct 12 17:03:42 2010 +0100 > +++ b/tools/xcutils/xc_save.c Tue Oct 12 17:03:42 2010 +0100 > @@ -102,7 +102,7 @@ static int suspend(void* data) > * active buffer. */ > > > -static void switch_qemu_logdirty(int domid, unsigned int enable) > +static void switch_qemu_logdirty(int domid, unsigned int enable, void *data) > { > struct xs_handle *xs; > char *path, *p, *ret_str, *cmd_str, **watch; > @@ -205,9 +205,9 @@ main(int argc, char **argv) > } > memset(&callbacks, 0, sizeof(callbacks)); > callbacks.suspend = suspend; > + callbacks.switch_qemu_logdirty = switch_qemu_logdirty; > ret = xc_domain_save(si.xch, io_fd, si.domid, maxit, max_f, si.flags, > - &callbacks, !!(si.flags & XCFLAGS_HVM), > - &switch_qemu_logdirty); > + &callbacks, !!(si.flags & XCFLAGS_HVM)); > > if (si.suspend_evtchn > 0) > xc_suspend_evtchn_release(si.xch, si.xce, si.domid, si.suspend_evtchn);_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-13 12:46 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
On Wed, 2010-10-13 at 12:14 +0100, Stefano Stabellini wrote:> On Tue, 12 Oct 2010, Ian Campbell wrote: > > @@ -1876,7 +1876,7 @@ int xc_domain_save(xc_interface *xch, in > > NULL, 0, NULL, 0, NULL) < 0 ) > > DPRINTF("Warning - couldn''t disable shadow mode"); > > if ( hvm ) > > - switch_qemu_logdirty(dom, 0); > > + callbacks->switch_qemu_logdirty(dom, 0, callbacks->data); > > } > > > > if ( live_shinfo ) > > 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 Signed-off-by: Ian Campbell <ian.campbell@citrix.com> 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@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Oct-13 13:10 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
On Wed, 13 Oct 2010, Ian Campbell wrote:> On Wed, 2010-10-13 at 12:14 +0100, Stefano Stabellini wrote: > > On Tue, 12 Oct 2010, Ian Campbell wrote: > > > @@ -1876,7 +1876,7 @@ int xc_domain_save(xc_interface *xch, in > > > NULL, 0, NULL, 0, NULL) < 0 ) > > > DPRINTF("Warning - couldn''t disable shadow mode"); > > > if ( hvm ) > > > - switch_qemu_logdirty(dom, 0); > > > + callbacks->switch_qemu_logdirty(dom, 0, callbacks->data); > > > } > > > > > > if ( live_shinfo ) > > > > 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 > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>yeah, this patch is OK too _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Brendan Cully
2010-Oct-13 16:37 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
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 saveEither 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@citrix.com> > > 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@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-13 16:51 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
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@lists.xensource.com http://lists.xensource.com/xen-devel
Brendan Cully
2010-Oct-13 17:12 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
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@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-14 08:17 UTC
Re: [Xen-devel] [PATCH] tools: add closure to xc_domain_save switch_qemu_logdirty callback
On Wed, 2010-10-13 at 18:12 +0100, Brendan Cully wrote:> 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: > > > 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.thanks.> > 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.I''ll update the patch to do so and repost.> 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 :)libxenctrl doesn''t link against libxenstore so it can''t do it itself. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-19 08:21 UTC
[Xen-devel] [PATCH] tools: cleanup domain save switch_qemu_logdirty callback
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1287476238 -3600
# Node ID 0b5d85ea10f8fff3f654c564c0e66900e83e8012
# Parent ca91b58834a35c70f33f6ceb28c02c60f190b6ed
tools: cleanup domain save switch_qemu_logdirty callback
Move the function into struct save_callbacks with the others and add
the void *closure to the callback arguments.
Add and propagate an error return code from the callback.
Use this in libxl to pass the save context to
libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse
the parent''s xenstore handle, gc context etc.
Also add an apparently missing libxl__free_all to
libxl__domain_suspend_common.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes from v1:
Compile test from missed call to xc_domain_save in
tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
Changes from v2:
Check for missing callback and return error.
Similarly update tools/libxc/ia64/xc_ia64_linux_save.c
Add return value to callback and propagate failure.
Update comment to indicate that closure is last argument to callback
(all existing callbacks only had the one argument so the first/last
distinction is moot)
diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/libxc/ia64/xc_ia64_linux_save.c
--- a/tools/libxc/ia64/xc_ia64_linux_save.c Tue Oct 19 09:07:47 2010 +0100
+++ b/tools/libxc/ia64/xc_ia64_linux_save.c Tue Oct 19 09:17:18 2010 +0100
@@ -397,8 +397,7 @@ out:
int
xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iters,
uint32_t max_factor, uint32_t flags,
- struct save_callbacks* callbacks,
- int hvm, void (*switch_qemu_logdirty)(int, unsigned))
+ struct save_callbacks* callbacks, int hvm)
{
DECLARE_DOMCTL;
xc_dominfo_t info;
@@ -450,6 +449,14 @@ xc_domain_save(xc_interface *xch, int io
void *p;
efi_memory_desc_t *md;
struct xen_ia64_p2m_table p2m_table;
+
+ if ( hvm && !callbacks->switch_qemu_logdirty )
+ {
+ ERROR("No switch_qemu_logdirty callback given.");
+ errno = EINVAL;
+ return 1;
+ }
+
xc_ia64_p2m_init(&p2m_table);
if (debug)
@@ -556,8 +563,10 @@ xc_domain_save(xc_interface *xch, int io
}
/* Enable qemu-dm logging dirty pages to xen */
- if (hvm)
- switch_qemu_logdirty(dom, 1);
+ if (hvm && !callbacks->switch_qemu_logdirty(dom, 1,
callbacks->data)) {
+ ERROR("Unable to enable qemu log-dirty mode");
+ goto out;
+ }
} else {
/* This is a non-live suspend. Issue the call back to get the
@@ -785,8 +794,9 @@ xc_domain_save(xc_interface *xch, int io
NULL, 0, NULL, 0, NULL ) < 0) {
DPRINTF("Warning - couldn''t disable shadow
mode");
}
- if ( hvm )
- switch_qemu_logdirty(dom, 0);
+ if ( hvm && !callbacks->switch_qemu_logdirty(dom, 0) ) {
+ DPRINTF("Warning - couldn''t disable qemu log-dirty
mode");
+ }
}
unlock_pages(to_send, bitmap_size);
diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c Tue Oct 19 09:07:47 2010 +0100
+++ b/tools/libxc/xc_domain_save.c Tue Oct 19 09:17:18 2010 +0100
@@ -873,8 +873,7 @@ static int save_tsc_info(xc_interface *x
int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t
max_iters,
uint32_t max_factor, uint32_t flags,
- struct save_callbacks* callbacks,
- int hvm, void (*switch_qemu_logdirty)(int, unsigned))
+ struct save_callbacks* callbacks, int hvm)
{
xc_dominfo_t info;
DECLARE_DOMCTL;
@@ -938,6 +937,13 @@ int xc_domain_save(xc_interface *xch, in
int completed = 0;
+ if ( hvm && !callbacks->switch_qemu_logdirty )
+ {
+ ERROR("No switch_qemu_logdirty callback provided.");
+ errno = EINVAL;
+ return 1;
+ }
+
outbuf_init(xch, &ob, OUTBUF_SIZE);
/* If no explicit control parameters given, use defaults */
@@ -1009,8 +1015,11 @@ int xc_domain_save(xc_interface *xch, in
}
/* Enable qemu-dm logging dirty pages to xen */
- if ( hvm )
- switch_qemu_logdirty(dom, 1);
+ if ( hvm && !callbacks->switch_qemu_logdirty(dom, 1,
callbacks->data) )
+ {
+ PERROR("Couldn''t enable qemu log-dirty mode (errno
%d)", errno);
+ goto out;
+ }
}
else
{
@@ -1870,8 +1879,8 @@ 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 )
- switch_qemu_logdirty(dom, 0);
+ if ( hvm && !callbacks->switch_qemu_logdirty(dom, 0,
callbacks->data) )
+ DPRINTF("Warning - couldn''t disable qemu log-dirty
mode");
}
if ( live_shinfo )
diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/libxc/xenguest.h
--- a/tools/libxc/xenguest.h Tue Oct 19 09:07:47 2010 +0100
+++ b/tools/libxc/xenguest.h Tue Oct 19 09:17:18 2010 +0100
@@ -40,7 +40,10 @@ struct save_callbacks {
* 1: take another checkpoint */
int (*checkpoint)(void* data);
- /* to be provided as the first argument to each callback function */
+ /* Enable qemu-dm logging dirty pages to xen */
+ int (*switch_qemu_logdirty)(int domid, unsigned enable, void *data); /* HVM
only */
+
+ /* to be provided as the last argument to each callback function */
void* data;
};
@@ -54,8 +57,7 @@ struct save_callbacks {
*/
int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t
max_iters,
uint32_t max_factor, uint32_t flags /* XCFLAGS_xxx */,
- struct save_callbacks* callbacks,
- int hvm, void (*switch_qemu_logdirty)(int, unsigned)); /*
HVM only */
+ struct save_callbacks* callbacks, int hvm);
/**
diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c Tue Oct 19 09:07:47 2010 +0100
+++ b/tools/libxl/libxl_dom.c Tue Oct 19 09:17:18 2010 +0100
@@ -325,21 +325,23 @@ struct suspendinfo {
unsigned int flags;
};
-static void libxl__domain_suspend_common_switch_qemu_logdirty(int domid,
unsigned int enable)
+static int libxl__domain_suspend_common_switch_qemu_logdirty(int domid,
unsigned int enable, void *data)
{
- struct xs_handle *xsh;
- char path[64];
+ struct suspendinfo *si = data;
+ libxl_ctx *ctx = libxl__gc_owner(si->gc);
+ char *path;
+ bool rc;
- snprintf(path, sizeof(path),
"/local/domain/0/device-model/%u/logdirty/cmd", domid);
-
- xsh = xs_daemon_open();
+ path = libxl__sprintf(si->gc,
"/local/domain/0/device-model/%u/logdirty/cmd", domid);
+ if (!path)
+ return 1;
if (enable)
- xs_write(xsh, XBT_NULL, path, "enable",
strlen("enable"));
+ rc = xs_write(ctx->xsh, XBT_NULL, path, "enable",
strlen("enable"));
else
- xs_write(xsh, XBT_NULL, path, "disable",
strlen("disable"));
+ rc = xs_write(ctx->xsh, XBT_NULL, path, "disable",
strlen("disable"));
- xs_daemon_close(xsh);
+ return rc ? 0 : 1;
}
static int libxl__domain_suspend_common_callback(void *data)
@@ -437,11 +439,10 @@ int libxl__domain_suspend_common(libxl_c
memset(&callbacks, 0, sizeof(callbacks));
callbacks.suspend = libxl__domain_suspend_common_callback;
+ callbacks.switch_qemu_logdirty =
libxl__domain_suspend_common_switch_qemu_logdirty;
callbacks.data = &si;
- xc_domain_save(ctx->xch, fd, domid, 0, 0, flags,
- &callbacks, hvm,
- &libxl__domain_suspend_common_switch_qemu_logdirty);
+ xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, &callbacks, hvm);
if (si.suspend_eventchn > 0)
xc_suspend_evtchn_release(ctx->xch, si.xce, domid,
si.suspend_eventchn);
@@ -450,6 +451,7 @@ int libxl__domain_suspend_common(libxl_c
rc = 0;
out:
+ libxl__free_all(&gc);
return rc;
}
diff -r ca91b58834a3 -r 0b5d85ea10f8
tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
--- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 19 09:07:47
2010 +0100
+++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 19 09:17:18
2010 +0100
@@ -164,9 +164,9 @@ void checkpoint_close(checkpoint_state*
/* 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)
+static int noop_switch_logdirty(int domid, unsigned enable, void *data)
{
- return;
+ return 0;
}
int checkpoint_start(checkpoint_state* s, int fd,
@@ -185,12 +185,13 @@ int checkpoint_start(checkpoint_state* s
hvm = s->domtype > dt_pv;
if (hvm) {
flags |= XCFLAGS_HVM;
- if ((rc = switch_qemu_logdirty(s, 1)))
- return rc;
+ if (!switch_qemu_logdirty(s, 1))
+ return -1;
}
- rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks,
hvm,
- noop_switch_logdirty);
+ callbacks->switch_qemu_logdirty = noop_switch_logdirty;
+
+ rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks,
hvm);
if (hvm)
switch_qemu_logdirty(s, 0);
@@ -540,7 +541,7 @@ static int switch_qemu_logdirty(checkpoi
strcpy(tail, "ret");
if (!xs_watch(s->xsh, path, "qemu-logdirty-ret")) {
s->errstr = "error watching qemu logdirty return";
- return -1;
+ return 1;
}
/* null fire. XXX unify with shutdown watch! */
vec = xs_read_watch(s->xsh, &len);
@@ -550,7 +551,7 @@ static int switch_qemu_logdirty(checkpoi
cmd = enable ? "enable" : "disable";
if (!xs_write(s->xsh, XBT_NULL, path, cmd, strlen(cmd))) {
s->errstr = "error signalling qemu logdirty";
- return -1;
+ return 1;
}
vec = xs_read_watch(s->xsh, &len);
@@ -564,7 +565,7 @@ static int switch_qemu_logdirty(checkpoi
if (len)
free(response);
s->errstr = "qemu logdirty command failed";
- return -1;
+ return 1;
}
free(response);
fprintf(stderr, "qemu logdirty mode: %s\n", cmd);
diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/xcutils/xc_save.c
--- a/tools/xcutils/xc_save.c Tue Oct 19 09:07:47 2010 +0100
+++ b/tools/xcutils/xc_save.c Tue Oct 19 09:17:18 2010 +0100
@@ -102,7 +102,7 @@ static int suspend(void* data)
* active buffer. */
-static void switch_qemu_logdirty(int domid, unsigned int enable)
+static int switch_qemu_logdirty(int domid, unsigned int enable, void *data)
{
struct xs_handle *xs;
char *path, *p, *ret_str, *cmd_str, **watch;
@@ -159,6 +159,8 @@ static void switch_qemu_logdirty(int dom
free(path);
free(cmd_str);
free(ret_str);
+
+ return 0;
}
int
@@ -205,9 +207,9 @@ main(int argc, char **argv)
}
memset(&callbacks, 0, sizeof(callbacks));
callbacks.suspend = suspend;
+ callbacks.switch_qemu_logdirty = switch_qemu_logdirty;
ret = xc_domain_save(si.xch, io_fd, si.domid, maxit, max_f, si.flags,
- &callbacks, !!(si.flags & XCFLAGS_HVM),
- &switch_qemu_logdirty);
+ &callbacks, !!(si.flags & XCFLAGS_HVM));
if (si.suspend_evtchn > 0)
xc_suspend_evtchn_release(si.xch, si.xce, si.domid, si.suspend_evtchn);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Brendan Cully
2010-Oct-19 21:12 UTC
[Xen-devel] Re: [PATCH] tools: cleanup domain save switch_qemu_logdirty callback
On Tuesday, 19 October 2010 at 09:21, Ian Campbell wrote:> # HG changeset patch > # User Ian Campbell <ian.campbell@citrix.com> > # Date 1287476238 -3600 > # Node ID 0b5d85ea10f8fff3f654c564c0e66900e83e8012 > # Parent ca91b58834a35c70f33f6ceb28c02c60f190b6ed > tools: cleanup domain save switch_qemu_logdirty callback > > Move the function into struct save_callbacks with the others and add > the void *closure to the callback arguments. > > Add and propagate an error return code from the callback. > > Use this in libxl to pass the save context to > libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse > the parent''s xenstore handle, gc context etc. > > Also add an apparently missing libxl__free_all to > libxl__domain_suspend_common. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > Changes from v1: > Compile test from missed call to xc_domain_save in > tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > Changes from v2: > Check for missing callback and return error. > Similarly update tools/libxc/ia64/xc_ia64_linux_save.c > Add return value to callback and propagate failure. > Update comment to indicate that closure is last argument to callback > (all existing callbacks only had the one argument so the first/last > distinction is moot)This reads well, but I don''t understand the purpose of all the return code juggling you''ve done in libcheckpoint.c here:> diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 19 09:07:47 2010 +0100 > +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 19 09:17:18 2010 +0100 > @@ -164,9 +164,9 @@ void checkpoint_close(checkpoint_state* > > /* 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) > +static int noop_switch_logdirty(int domid, unsigned enable, void *data) > { > - return; > + return 0; > } > > int checkpoint_start(checkpoint_state* s, int fd, > @@ -185,12 +185,13 @@ int checkpoint_start(checkpoint_state* s > hvm = s->domtype > dt_pv; > if (hvm) { > flags |= XCFLAGS_HVM; > - if ((rc = switch_qemu_logdirty(s, 1))) > - return rc; > + if (!switch_qemu_logdirty(s, 1)) > + return -1; > } > > - rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm, > - noop_switch_logdirty); > + callbacks->switch_qemu_logdirty = noop_switch_logdirty; > + > + rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm); > > if (hvm) > switch_qemu_logdirty(s, 0); > @@ -540,7 +541,7 @@ static int switch_qemu_logdirty(checkpoi > strcpy(tail, "ret"); > if (!xs_watch(s->xsh, path, "qemu-logdirty-ret")) { > s->errstr = "error watching qemu logdirty return"; > - return -1; > + return 1; > } > /* null fire. XXX unify with shutdown watch! */ > vec = xs_read_watch(s->xsh, &len); > @@ -550,7 +551,7 @@ static int switch_qemu_logdirty(checkpoi > cmd = enable ? "enable" : "disable"; > if (!xs_write(s->xsh, XBT_NULL, path, cmd, strlen(cmd))) { > s->errstr = "error signalling qemu logdirty"; > - return -1; > + return 1; > } > > vec = xs_read_watch(s->xsh, &len); > @@ -564,7 +565,7 @@ static int switch_qemu_logdirty(checkpoi > if (len) > free(response); > s->errstr = "qemu logdirty command failed"; > - return -1; > + return 1; > } > free(response); > fprintf(stderr, "qemu logdirty mode: %s\n", cmd);I haven''t compiled or run this, I''ll wait for it to hit the tree. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-19 21:30 UTC
[Xen-devel] Re: [PATCH] tools: cleanup domain save switch_qemu_logdirty callback
On Tue, 2010-10-19 at 22:12 +0100, Brendan Cully wrote:> On Tuesday, 19 October 2010 at 09:21, Ian Campbell wrote: > > # HG changeset patch > > # User Ian Campbell <ian.campbell@citrix.com> > > # Date 1287476238 -3600 > > # Node ID 0b5d85ea10f8fff3f654c564c0e66900e83e8012 > > # Parent ca91b58834a35c70f33f6ceb28c02c60f190b6ed > > tools: cleanup domain save switch_qemu_logdirty callback > > > > Move the function into struct save_callbacks with the others and add > > the void *closure to the callback arguments. > > > > Add and propagate an error return code from the callback. > > > > Use this in libxl to pass the save context to > > libxl__domain_suspend_common_switch_qemu_logdirty allowing us to reuse > > the parent''s xenstore handle, gc context etc. > > > > Also add an apparently missing libxl__free_all to > > libxl__domain_suspend_common. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > --- > > Changes from v1: > > Compile test from missed call to xc_domain_save in > > tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > > Changes from v2: > > Check for missing callback and return error. > > Similarly update tools/libxc/ia64/xc_ia64_linux_save.c > > Add return value to callback and propagate failure. > > Update comment to indicate that closure is last argument to callback > > (all existing callbacks only had the one argument so the first/last > > distinction is moot) > > This reads well, but I don''t understand the purpose of all the return > code juggling you''ve done in libcheckpoint.c here:It makes the callback return code match the convention used by xc_domain_save itself. It isn''t necessarily the best convention but I thought the overall consistency was more important. Ian.> > > diff -r ca91b58834a3 -r 0b5d85ea10f8 tools/python/xen/lowlevel/checkpoint/libcheckpoint.c > > --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 19 09:07:47 2010 +0100 > > +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c Tue Oct 19 09:17:18 2010 +0100 > > @@ -164,9 +164,9 @@ void checkpoint_close(checkpoint_state* > > > > /* 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) > > +static int noop_switch_logdirty(int domid, unsigned enable, void *data) > > { > > - return; > > + return 0; > > } > > > > int checkpoint_start(checkpoint_state* s, int fd, > > @@ -185,12 +185,13 @@ int checkpoint_start(checkpoint_state* s > > hvm = s->domtype > dt_pv; > > if (hvm) { > > flags |= XCFLAGS_HVM; > > - if ((rc = switch_qemu_logdirty(s, 1))) > > - return rc; > > + if (!switch_qemu_logdirty(s, 1)) > > + return -1; > > } > > > > - rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm, > > - noop_switch_logdirty); > > + callbacks->switch_qemu_logdirty = noop_switch_logdirty; > > + > > + rc = xc_domain_save(s->xch, fd, s->domid, 0, 0, flags, callbacks, hvm); > > > > if (hvm) > > switch_qemu_logdirty(s, 0); > > @@ -540,7 +541,7 @@ static int switch_qemu_logdirty(checkpoi > > strcpy(tail, "ret"); > > if (!xs_watch(s->xsh, path, "qemu-logdirty-ret")) { > > s->errstr = "error watching qemu logdirty return"; > > - return -1; > > + return 1; > > } > > /* null fire. XXX unify with shutdown watch! */ > > vec = xs_read_watch(s->xsh, &len); > > @@ -550,7 +551,7 @@ static int switch_qemu_logdirty(checkpoi > > cmd = enable ? "enable" : "disable"; > > if (!xs_write(s->xsh, XBT_NULL, path, cmd, strlen(cmd))) { > > s->errstr = "error signalling qemu logdirty"; > > - return -1; > > + return 1; > > } > > > > vec = xs_read_watch(s->xsh, &len); > > @@ -564,7 +565,7 @@ static int switch_qemu_logdirty(checkpoi > > if (len) > > free(response); > > s->errstr = "qemu logdirty command failed"; > > - return -1; > > + return 1; > > } > > free(response); > > fprintf(stderr, "qemu logdirty mode: %s\n", cmd); > > I haven''t compiled or run this, I''ll wait for it to hit the tree._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel