Ian Jackson
2012-Jul-24 16:01 UTC
[PATCH] libxl: fix transaction leak in logdirty error path
libxl__domain_suspend_common_switch_qemu_logdirty would leak t if
there was an error. Fix this.
Also, document the intended usage for libxl__xs_transaction_* in the
doc comment in libxl_internal.h.
Reported-by: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_dom.c | 1 +
tools/libxl/libxl_internal.h | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index b6111ad..b8a4aae 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -627,6 +627,7 @@ void libxl__domain_suspend_common_switch_qemu_logdirty
out:
LOG(ERROR,"logdirty switch failed (rc=%d), aborting suspend",rc);
+ libxl__xs_transaction_abort(gc, &t);
switch_logdirty_done(egc,dss,-1);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e938660..43bb69b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -543,6 +543,42 @@ int libxl__xs_rm_checked(libxl__gc *gc, xs_transaction_t t,
const char *path);
* +1 commit conflict; transaction has been destroyed and caller
* must go round again (call _start again and retry)
* 0 committed successfully
+ *
+ * The intended usage pattern looks like this:
+ * int some_function()
+ * {
+ * int rc;
+ * xs_transaction_t t = 0;
+ * // other initialisations
+ *
+ * // do whatever you need to do before the xenstore stuff
+ * // errors? set rc and goto out.
+ *
+ * for (;;) {
+ * rc = libxl__xs_transaction_start(gc, &t);
+ * if (rc) goto out;
+ *
+ * // do your work here, including all xenstore reads and writes
+ * // libxl__xs_*_checked are useful; pass them t.
+ * // errors? set rc and goto out.
+ *
+ * rc = libxl__xs_transaction_commit(gc, &t);
+ * if (!rc) break;
+ * if (rc<0) goto out;
+ * }
+ *
+ * // now the xenstore transaction succeeded
+ * // do whatever else you need to do
+ * // errors? set rc and goto out.
+ *
+ * return something;
+ *
+ * out:
+ * // other cleanups
+ * libxl__xs_transaction_abort(gc, &t);
+ * // other cleanups
+ * return rc;
+ * }
*/
int libxl__xs_transaction_start(libxl__gc *gc, xs_transaction_t *t);
int libxl__xs_transaction_commit(libxl__gc *gc, xs_transaction_t *t);
--
tg: (c3a0480..) t/xen/xl.fix.logdirty-trans-leak (depends on:
t/xen/xl.bootloader.fix.no-blunder-on)
Ian Campbell
2012-Jul-24 16:04 UTC
Re: [PATCH] libxl: fix transaction leak in logdirty error path
On Tue, 2012-07-24 at 17:01 +0100, Ian Jackson wrote:> libxl__domain_suspend_common_switch_qemu_logdirty would leak t if > there was an error. Fix this. > > Also, document the intended usage for libxl__xs_transaction_* in the > doc comment in libxl_internal.h. > > Reported-by: Ian Campbell <Ian.Campbell@citrix.com> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> > --- > tools/libxl/libxl_dom.c | 1 + > tools/libxl/libxl_internal.h | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+), 0 deletions(-) > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index b6111ad..b8a4aae 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -627,6 +627,7 @@ void libxl__domain_suspend_common_switch_qemu_logdirty > > out: > LOG(ERROR,"logdirty switch failed (rc=%d), aborting suspend",rc); > + libxl__xs_transaction_abort(gc, &t); > switch_logdirty_done(egc,dss,-1); > } > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index e938660..43bb69b 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -543,6 +543,42 @@ int libxl__xs_rm_checked(libxl__gc *gc, xs_transaction_t t, const char *path); > * +1 commit conflict; transaction has been destroyed and caller > * must go round again (call _start again and retry) > * 0 committed successfully > + * > + * The intended usage pattern looks like this: > + * int some_function() > + * { > + * int rc; > + * xs_transaction_t t = 0; > + * // other initialisations > + * > + * // do whatever you need to do before the xenstore stuff > + * // errors? set rc and goto out. > + * > + * for (;;) { > + * rc = libxl__xs_transaction_start(gc, &t); > + * if (rc) goto out; > + * > + * // do your work here, including all xenstore reads and writes > + * // libxl__xs_*_checked are useful; pass them t. > + * // errors? set rc and goto out. > + * > + * rc = libxl__xs_transaction_commit(gc, &t); > + * if (!rc) break; > + * if (rc<0) goto out; > + * } > + * > + * // now the xenstore transaction succeeded > + * // do whatever else you need to do > + * // errors? set rc and goto out. > + * > + * return something; > + * > + * out: > + * // other cleanups > + * libxl__xs_transaction_abort(gc, &t); > + * // other cleanups > + * return rc; > + * } > */ > int libxl__xs_transaction_start(libxl__gc *gc, xs_transaction_t *t); > int libxl__xs_transaction_commit(libxl__gc *gc, xs_transaction_t *t);
Ian Campbell
2012-Jul-25 16:44 UTC
Re: [PATCH] libxl: fix transaction leak in logdirty error path
On Tue, 2012-07-24 at 17:04 +0100, Ian Campbell wrote:> On Tue, 2012-07-24 at 17:01 +0100, Ian Jackson wrote: > > libxl__domain_suspend_common_switch_qemu_logdirty would leak t if > > there was an error. Fix this. > > > > Also, document the intended usage for libxl__xs_transaction_* in the > > doc comment in libxl_internal.h. > > > > Reported-by: Ian Campbell <Ian.Campbell@citrix.com> > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>Applied, squashing some trailing whitespace as I went.> > > > > --- > > tools/libxl/libxl_dom.c | 1 + > > tools/libxl/libxl_internal.h | 36 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 37 insertions(+), 0 deletions(-) > > > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > > index b6111ad..b8a4aae 100644 > > --- a/tools/libxl/libxl_dom.c > > +++ b/tools/libxl/libxl_dom.c > > @@ -627,6 +627,7 @@ void libxl__domain_suspend_common_switch_qemu_logdirty > > > > out: > > LOG(ERROR,"logdirty switch failed (rc=%d), aborting suspend",rc); > > + libxl__xs_transaction_abort(gc, &t); > > switch_logdirty_done(egc,dss,-1); > > } > > > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > > index e938660..43bb69b 100644 > > --- a/tools/libxl/libxl_internal.h > > +++ b/tools/libxl/libxl_internal.h > > @@ -543,6 +543,42 @@ int libxl__xs_rm_checked(libxl__gc *gc, xs_transaction_t t, const char *path); > > * +1 commit conflict; transaction has been destroyed and caller > > * must go round again (call _start again and retry) > > * 0 committed successfully > > + * > > + * The intended usage pattern looks like this: > > + * int some_function() > > + * { > > + * int rc; > > + * xs_transaction_t t = 0; > > + * // other initialisations > > + * > > + * // do whatever you need to do before the xenstore stuff > > + * // errors? set rc and goto out. > > + * > > + * for (;;) { > > + * rc = libxl__xs_transaction_start(gc, &t); > > + * if (rc) goto out; > > + * > > + * // do your work here, including all xenstore reads and writes > > + * // libxl__xs_*_checked are useful; pass them t. > > + * // errors? set rc and goto out. > > + * > > + * rc = libxl__xs_transaction_commit(gc, &t); > > + * if (!rc) break; > > + * if (rc<0) goto out; > > + * } > > + * > > + * // now the xenstore transaction succeeded > > + * // do whatever else you need to do > > + * // errors? set rc and goto out. > > + * > > + * return something; > > + * > > + * out: > > + * // other cleanups > > + * libxl__xs_transaction_abort(gc, &t); > > + * // other cleanups > > + * return rc; > > + * } > > */ > > int libxl__xs_transaction_start(libxl__gc *gc, xs_transaction_t *t); > > int libxl__xs_transaction_commit(libxl__gc *gc, xs_transaction_t *t); > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel