* [PATCH 01/11] libxl: unify libxl__device_destroy and device_hotplug_done
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:11 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP Ian Jackson
` (9 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
device_hotplug_done contains an open-coded but improved version of
libxl__device_destroy. So move the contents of device_hotplug_done
into libxl__device_destroy, deleting the old code, and replace it at
its old location with a function call.
Also fix the error handling: the rc from the destroy should be
propagated into the aodev.
Reported-by: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_device.c | 35 ++++++++++++-----------------------
1 files changed, 12 insertions(+), 23 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index da0c3ea..3658bd1 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -513,17 +513,18 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
char *be_path = libxl__device_backend_path(gc, dev);
char *fe_path = libxl__device_frontend_path(gc, dev);
xs_transaction_t t = 0;
- int rc = 0;
+ int rc;
+
+ for (;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
- do {
- t = xs_transaction_start(CTX->xsh);
libxl__xs_path_cleanup(gc, t, fe_path);
libxl__xs_path_cleanup(gc, t, be_path);
- rc = !xs_transaction_end(CTX->xsh, t, 0);
- } while (rc && errno == EAGAIN);
- if (rc) {
- LOGE(ERROR, "unable to finish transaction");
- goto out;
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc < 0) goto out;
}
libxl__device_destroy_tapdisk(gc, be_path);
@@ -993,29 +994,17 @@ error:
static void device_hotplug_done(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
- char *be_path = libxl__device_backend_path(gc, aodev->dev);
- char *fe_path = libxl__device_frontend_path(gc, aodev->dev);
- xs_transaction_t t = 0;
int rc;
device_hotplug_clean(gc, aodev);
/* Clean xenstore if it's a disconnection */
if (aodev->action == DEVICE_DISCONNECT) {
- for (;;) {
- rc = libxl__xs_transaction_start(gc, &t);
- if (rc) goto out;
-
- libxl__xs_path_cleanup(gc, t, fe_path);
- libxl__xs_path_cleanup(gc, t, be_path);
-
- rc = libxl__xs_transaction_commit(gc, &t);
- if (!rc) break;
- if (rc < 0) goto out;
- }
+ rc = libxl__device_destroy(gc, aodev->dev);
+ if (!aodev->rc)
+ aodev->rc = rc;
}
-out:
aodev->callback(egc, aodev);
return;
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
2012-08-01 16:24 ` [PATCH 01/11] libxl: unify libxl__device_destroy and device_hotplug_done Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:16 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 03/11] libxl: fix device counting race in libxl__devices_destroy Ian Jackson
` (8 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Roger Pau Monne
Receive POLLHUP on the bootloader master pty is not an error.
Hopefully it means that the bootloader has exited and therefore the
pty slave side has no process group any more. (At least NetBSD
indicates POLLHUP on the master in this case.)
So send the bootloader SIGTERM; if it has already exited then this has
no effect (except that on some versions of NetBSD it erroneously
returns ESRCH and we print a harmless warning) and we will then
collect the bootloader's exit status and be satisfied.
However, we remember that we have done this so that if we got POLLHUP
for some other reason than that the bootloader exited we report
something resembling a useful message.
In order to implement this we need to provide a way for users of
datacopier to handle POLLHUP rather than treating it as fatal.
We rename bootloader_abort to bootloader_stop since it now no longer
only applies to error situations.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
-
Changes in v4:
* Track whether we sent SIGTERM due to POLLHUP so we can report
messages properly.
Changes in v3:
* datacopier provides new interface for handling POLLHUP
* Do not ignore errors on the xenconsole pty
* Rename bootloader_abort.
---
tools/libxl/libxl_aoutils.c | 23 +++++++++++++++++++++++
tools/libxl/libxl_bootloader.c | 39 +++++++++++++++++++++++++++++----------
tools/libxl/libxl_internal.h | 7 +++++--
3 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 99972a2..4bd5484 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -97,11 +97,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
}
+static int datacopier_pollhup_handled(libxl__egc *egc,
+ libxl__datacopier_state *dc,
+ short revents, int onwrite)
+{
+ STATE_AO_GC(dc->ao);
+
+ if (dc->callback_pollhup && (revents & POLLHUP)) {
+ LOG(DEBUG, "received POLLHUP on %s during copy of %s",
+ onwrite ? dc->writewhat : dc->readwhat,
+ dc->copywhat);
+ libxl__datacopier_kill(dc);
+ dc->callback(egc, dc, onwrite, -1);
+ return 1;
+ }
+ return 0;
+}
+
static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
int fd, short events, short revents) {
libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
STATE_AO_GC(dc->ao);
+ if (datacopier_pollhup_handled(egc, dc, revents, 0))
+ return;
+
if (revents & ~POLLIN) {
LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
" on %s during copy of %s", revents, dc->readwhat, dc->copywhat);
@@ -163,6 +183,9 @@ static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, towrite);
STATE_AO_GC(dc->ao);
+ if (datacopier_pollhup_handled(egc, dc, revents, 1))
+ return;
+
if (revents & ~POLLOUT) {
LOG(ERROR, "unexpected poll event 0x%x (should be POLLOUT)"
" on %s during copy of %s", revents, dc->writewhat, dc->copywhat);
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index ef5a91b..bfc1b56 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -215,6 +215,7 @@ void libxl__bootloader_init(libxl__bootloader_state *bl)
libxl__domaindeathcheck_init(&bl->deathcheck);
bl->keystrokes.ao = bl->ao; libxl__datacopier_init(&bl->keystrokes);
bl->display.ao = bl->ao; libxl__datacopier_init(&bl->display);
+ bl->got_pollhup = 0;
}
static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
@@ -275,7 +276,7 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
}
/* might be called at any time, provided it's init'd */
-static void bootloader_abort(libxl__egc *egc,
+static void bootloader_stop(libxl__egc *egc,
libxl__bootloader_state *bl, int rc)
{
STATE_AO_GC(bl->ao);
@@ -285,8 +286,8 @@ static void bootloader_abort(libxl__egc *egc,
libxl__datacopier_kill(&bl->display);
if (libxl__ev_child_inuse(&bl->child)) {
r = kill(bl->child.pid, SIGTERM);
- if (r) LOGE(WARN, "after failure, failed to kill bootloader [%lu]",
- (unsigned long)bl->child.pid);
+ if (r) LOGE(WARN, "%sfailed to kill bootloader [%lu]",
+ rc ? "after failure, " : "", (unsigned long)bl->child.pid);
}
bl->rc = rc;
}
@@ -508,7 +509,10 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
bl->keystrokes.copywhat =
GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
- bl->keystrokes.callback = bootloader_keystrokes_copyfail;
+ bl->keystrokes.callback = bootloader_keystrokes_copyfail;
+ bl->keystrokes.callback_pollhup = bootloader_keystrokes_copyfail;
+ /* pollhup gets called with errnoval==-1 which is not otherwise
+ * possible since errnos are nonnegative, so it's unambiguous */
rc = libxl__datacopier_start(&bl->keystrokes);
if (rc) goto out;
@@ -516,7 +520,8 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
bl->display.maxsz = BOOTLOADER_BUF_IN;
bl->display.copywhat =
GCSPRINTF("bootloader output for domain %"PRIu32, bl->domid);
- bl->display.callback = bootloader_display_copyfail;
+ bl->display.callback = bootloader_display_copyfail;
+ bl->display.callback_pollhup = bootloader_display_copyfail;
rc = libxl__datacopier_start(&bl->display);
if (rc) goto out;
@@ -562,30 +567,42 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
/* perhaps one of these will be called, but perhaps not */
static void bootloader_copyfail(libxl__egc *egc, const char *which,
- libxl__bootloader_state *bl, int onwrite, int errnoval)
+ libxl__bootloader_state *bl, int ondisplay, int onwrite, int errnoval)
{
STATE_AO_GC(bl->ao);
+ int rc = ERROR_FAIL;
+
+ if (errnoval==-1) {
+ /* POLLHUP */
+ if (!!ondisplay != !!onwrite) {
+ rc = 0;
+ bl->got_pollhup = 1;
+ } else {
+ LOG(ERROR, "unexpected POLLHUP on %s", which);
+ }
+ }
if (!onwrite && !errnoval)
LOG(ERROR, "unexpected eof copying %s", which);
- bootloader_abort(egc, bl, ERROR_FAIL);
+
+ bootloader_stop(egc, bl, rc);
}
static void bootloader_keystrokes_copyfail(libxl__egc *egc,
libxl__datacopier_state *dc, int onwrite, int errnoval)
{
libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
- bootloader_copyfail(egc, "bootloader input", bl, onwrite, errnoval);
+ bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, errnoval);
}
static void bootloader_display_copyfail(libxl__egc *egc,
libxl__datacopier_state *dc, int onwrite, int errnoval)
{
libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, display);
- bootloader_copyfail(egc, "bootloader output", bl, onwrite, errnoval);
+ bootloader_copyfail(egc, "bootloader output", bl, 1, onwrite, errnoval);
}
static void bootloader_domaindeath(libxl__egc *egc, libxl__domaindeathcheck *dc)
{
libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, deathcheck);
- bootloader_abort(egc, bl, ERROR_FAIL);
+ bootloader_stop(egc, bl, ERROR_FAIL);
}
static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
@@ -599,6 +616,8 @@ static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
libxl__datacopier_kill(&bl->display);
if (status) {
+ if (bl->got_pollhup && WIFSIGNALED(status) && WTERMSIG(status)==SIGTERM)
+ LOG(ERROR, "got POLLHUP, sent SIGTERM");
LOG(ERROR, "bootloader failed - consult logfile %s", bl->logfile);
libxl_report_child_exitstatus(CTX, XTL_ERROR, "bootloader",
pid, status);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 691b4f6..c57503f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2070,7 +2070,9 @@ typedef struct libxl__datacopier_buf libxl__datacopier_buf;
* errnoval==0 means we got eof and all data was written
* errnoval!=0 means we had a read error, logged
* onwrite==-1 means some other internal failure, errnoval not valid, logged
- * in all cases copier is killed before calling this callback */
+ * If we get POLLHUP, we call callback_pollhup(..., onwrite, -1);
+ * or if callback_pollhup==0 this is an internal failure, as above.
+ * In all cases copier is killed before calling this callback */
typedef void libxl__datacopier_callback(libxl__egc *egc,
libxl__datacopier_state *dc, int onwrite, int errnoval);
@@ -2089,6 +2091,7 @@ struct libxl__datacopier_state {
const char *copywhat, *readwhat, *writewhat; /* for error msgs */
FILE *log; /* gets a copy of everything */
libxl__datacopier_callback *callback;
+ libxl__datacopier_callback *callback_pollhup;
/* remaining fields are private to datacopier */
libxl__ev_fd toread, towrite;
ssize_t used;
@@ -2273,7 +2276,7 @@ struct libxl__bootloader_state {
int nargs, argsspace;
const char **args;
libxl__datacopier_state keystrokes, display;
- int rc;
+ int rc, got_pollhup;
};
_hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP
2012-08-01 16:24 ` [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP Ian Jackson
@ 2012-08-02 10:16 ` Ian Campbell
2012-08-02 11:18 ` Ian Jackson
0 siblings, 1 reply; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 10:16 UTC (permalink / raw)
To: Ian Jackson; +Cc: Roger Pau Monne, xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> Receive POLLHUP on the bootloader master pty is not an error.
> Hopefully it means that the bootloader has exited and therefore the
> pty slave side has no process group any more. (At least NetBSD
> indicates POLLHUP on the master in this case.)
>
> So send the bootloader SIGTERM; if it has already exited then this has
> no effect (except that on some versions of NetBSD it erroneously
> returns ESRCH and we print a harmless warning) and we will then
> collect the bootloader's exit status and be satisfied.
>
> However, we remember that we have done this so that if we got POLLHUP
> for some other reason than that the bootloader exited we report
> something resembling a useful message.
>
> In order to implement this we need to provide a way for users of
> datacopier to handle POLLHUP rather than treating it as fatal.
>
> We rename bootloader_abort to bootloader_stop since it now no longer
> only applies to error situations.
>
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> -
> Changes in v4:
> * Track whether we sent SIGTERM due to POLLHUP so we can report
> messages properly.
>
> Changes in v3:
> * datacopier provides new interface for handling POLLHUP
> * Do not ignore errors on the xenconsole pty
> * Rename bootloader_abort.
> ---
> tools/libxl/libxl_aoutils.c | 23 +++++++++++++++++++++++
> tools/libxl/libxl_bootloader.c | 39 +++++++++++++++++++++++++++++----------
> tools/libxl/libxl_internal.h | 7 +++++--
> 3 files changed, 57 insertions(+), 12 deletions(-)
>
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 99972a2..4bd5484 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -97,11 +97,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
> LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> }
>
> +static int datacopier_pollhup_handled(libxl__egc *egc,
> + libxl__datacopier_state *dc,
> + short revents, int onwrite)
> +{
> + STATE_AO_GC(dc->ao);
> +
> + if (dc->callback_pollhup && (revents & POLLHUP)) {
> + LOG(DEBUG, "received POLLHUP on %s during copy of %s",
> + onwrite ? dc->writewhat : dc->readwhat,
> + dc->copywhat);
> + libxl__datacopier_kill(dc);
> + dc->callback(egc, dc, onwrite, -1);
You've forgotten to make this ->callback_pollhup as discussed last time.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP
2012-08-02 10:16 ` Ian Campbell
@ 2012-08-02 11:18 ` Ian Jackson
0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2012-08-02 11:18 UTC (permalink / raw)
To: Ian Campbell; +Cc: Roger Pau Monne, xen-devel@lists.xen.org
Ian Campbell writes ("Re: [Xen-devel] [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP"):
> On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> > Receive POLLHUP on the bootloader master pty is not an error.
> > Hopefully it means that the bootloader has exited and therefore the
> > pty slave side has no process group any more. (At least NetBSD
> > indicates POLLHUP on the master in this case.)
...
> > +static int datacopier_pollhup_handled(libxl__egc *egc,
> > + libxl__datacopier_state *dc,
> > + short revents, int onwrite)
> > +{
> > + STATE_AO_GC(dc->ao);
> > +
> > + if (dc->callback_pollhup && (revents & POLLHUP)) {
> > + LOG(DEBUG, "received POLLHUP on %s during copy of %s",
> > + onwrite ? dc->writewhat : dc->readwhat,
> > + dc->copywhat);
> > + libxl__datacopier_kill(dc);
> > + dc->callback(egc, dc, onwrite, -1);
>
> You've forgotten to make this ->callback_pollhup as discussed last time.
So I have. And this didn't show up in my testing because the callers
all either set ->callback_pollhup==0 or ==->callback.
I have fixed this in my tree.
Ian.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 03/11] libxl: fix device counting race in libxl__devices_destroy
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
2012-08-01 16:24 ` [PATCH 01/11] libxl: unify libxl__device_destroy and device_hotplug_done Ian Jackson
2012-08-01 16:24 ` [PATCH 02/11] libxl: react correctly to bootloader pty master POLLHUP Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:18 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 04/11] libxl: fix formatting of DEFINE_DEVICES_ADD Ian Jackson
` (7 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell, Ian Jackson, Roger Pau Monne
Don't have a fixed number of devices in the aodevs array, and instead
size it depending on the devices present in xenstore. Somewhat
formalise the multiple device addition/removal machinery to make this
clearer and easier to do.
As a side-effect we fix a few "lost thread of control" bug which would
occur if there were no devices of a particular kind. (Various if
statements which checked for there being no devices have become
redundant, but are retained to avoid making the patch bigger.)
Specifically:
* Users of libxl__ao_devices are no longer expected to know in
advance how many device operations they are going to do. Instead
they can initiate them one at a time, between bracketing calls to
"begin" and "prepared".
* The array of aodevs used for this is dynamically sized; to support
this it's an array of pointers rather than of structs.
* Users of libxl__ao_devices are presented with a more opaque interface.
They are are no longer expected to, themselves,
- look into the array of aodevs (this is now private)
- know that the individual addition/removal completions are
handled by libxl__ao_devices_callback (this callback function
is now a private function for the multidev machinery)
- ever deal with populating the contents of an aodevs
* The doc comments relating to some of the members of
libxl__ao_device are clarified. (And the member `aodevs' is moved
to put it with the other members with the same status.)
* The multidev machinery allocates an aodev to represent the
operation of preparing all of the other operations. See
the comment in libxl__multidev_begin.
A wrinkle is that the functions are called "multidev" but the structs
are called "libxl__ao_devices" and "aodevs". I have given these
functions this name to distinguish them from "libxl__ao_device" and
"aodev" and so forth by more than just the use of the plural "s"
suffix.
In the next patch we will rename the structs.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@eu.citrix.com>
-
Changes in v4:
* Actually honour errors in rc argument to libxl__multidev_prepared.
* Fix the doc comment for libxl__add_*.
* In comments, consistently use "multidev" not "multidevs".
Changes in v3:
* New multidev interfaces - extensive changes.
---
tools/libxl/libxl_create.c | 8 +-
tools/libxl/libxl_device.c | 129 +++++++++++++++++++-----------------------
tools/libxl/libxl_dm.c | 8 +-
tools/libxl/libxl_internal.h | 75 +++++++++++++++----------
4 files changed, 111 insertions(+), 109 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index aafacd8..3265d69 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -909,10 +909,10 @@ static void domcreate_rebuild_done(libxl__egc *egc,
store_libxl_entry(gc, domid, &d_config->b_info);
- dcs->aodevs.size = d_config->num_disks;
+ libxl__multidev_begin(ao, &dcs->aodevs);
dcs->aodevs.callback = domcreate_launch_dm;
- libxl__prepare_ao_devices(ao, &dcs->aodevs);
libxl__add_disks(egc, ao, domid, 0, d_config, &dcs->aodevs);
+ libxl__multidev_prepared(egc, &dcs->aodevs, 0);
return;
@@ -1039,10 +1039,10 @@ static void domcreate_devmodel_started(libxl__egc *egc,
/* Plug nic interfaces */
if (d_config->num_nics > 0) {
/* Attach nics */
- dcs->aodevs.size = d_config->num_nics;
+ libxl__multidev_begin(ao, &dcs->aodevs);
dcs->aodevs.callback = domcreate_attach_pci;
- libxl__prepare_ao_devices(ao, &dcs->aodevs);
libxl__add_nics(egc, ao, domid, 0, d_config, &dcs->aodevs);
+ libxl__multidev_prepared(egc, &dcs->aodevs, 0);
return;
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 3658bd1..544a861 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -58,50 +58,6 @@ int libxl__parse_backend_path(libxl__gc *gc,
return libxl__device_kind_from_string(strkind, &dev->backend_kind);
}
-static int libxl__num_devices(libxl__gc *gc, uint32_t domid)
-{
- char *path;
- unsigned int num_kinds, num_devs;
- char **kinds = NULL, **devs = NULL;
- int i, j, rc = 0;
- libxl__device dev;
- libxl__device_kind kind;
- int numdevs = 0;
-
- path = GCSPRINTF("/local/domain/%d/device", domid);
- kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
- if (!kinds) {
- if (errno != ENOENT) {
- LOGE(ERROR, "unable to get xenstore device listing %s", path);
- rc = ERROR_FAIL;
- goto out;
- }
- num_kinds = 0;
- }
- for (i = 0; i < num_kinds; i++) {
- if (libxl__device_kind_from_string(kinds[i], &kind))
- continue;
- if (kind == LIBXL__DEVICE_KIND_CONSOLE)
- continue;
-
- path = GCSPRINTF("/local/domain/%d/device/%s", domid, kinds[i]);
- devs = libxl__xs_directory(gc, XBT_NULL, path, &num_devs);
- if (!devs)
- continue;
- for (j = 0; j < num_devs; j++) {
- path = GCSPRINTF("/local/domain/%d/device/%s/%s/backend",
- domid, kinds[i], devs[j]);
- path = libxl__xs_read(gc, XBT_NULL, path);
- if (path && libxl__parse_backend_path(gc, path, &dev) == 0) {
- numdevs++;
- }
- }
- }
-out:
- if (rc) return rc;
- return numdevs;
-}
-
int libxl__nic_type(libxl__gc *gc, libxl__device *dev, libxl_nic_type *nictype)
{
char *snictype, *be_path;
@@ -445,40 +401,81 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
libxl__ev_child_init(&aodev->child);
}
-void libxl__prepare_ao_devices(libxl__ao *ao, libxl__ao_devices *aodevs)
+/* multidev */
+
+void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices *aodevs)
{
AO_GC;
- GCNEW_ARRAY(aodevs->array, aodevs->size);
- for (int i = 0; i < aodevs->size; i++) {
- aodevs->array[i].aodevs = aodevs;
- libxl__prepare_ao_device(ao, &aodevs->array[i]);
+ aodevs->ao = ao;
+ aodevs->array = 0;
+ aodevs->used = aodevs->allocd = 0;
+
+ /* We allocate an aodev to represent the operation of preparing
+ * all of the other operations. This operation is completed when
+ * we have started all the others (ie, when the user calls
+ * _prepared). That arranges automatically that
+ * (i) we do not think we have finished even if one of the
+ * operations completes while we are still preparing
+ * (ii) if we are starting zero operations, we do still
+ * make the callback as soon as we know this fact
+ * (iii) we have a nice consistent way to deal with any
+ * error that might occur while deciding what to initiate
+ */
+ aodevs->preparation = libxl__multidev_prepare(aodevs);
+}
+
+static void multidev_one_callback(libxl__egc *egc, libxl__ao_device *aodev);
+
+libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices *aodevs) {
+ STATE_AO_GC(aodevs->ao);
+ libxl__ao_device *aodev;
+
+ GCNEW(aodev);
+ aodev->aodevs = aodevs;
+ aodev->callback = multidev_one_callback;
+ libxl__prepare_ao_device(ao, aodev);
+
+ if (aodevs->used >= aodevs->allocd) {
+ aodevs->allocd = aodevs->used * 2 + 5;
+ GCREALLOC_ARRAY(aodevs->array, aodevs->allocd);
}
+ aodevs->array[aodevs->used++] = aodev;
+
+ return aodev;
}
-void libxl__ao_devices_callback(libxl__egc *egc, libxl__ao_device *aodev)
+static void multidev_one_callback(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
libxl__ao_devices *aodevs = aodev->aodevs;
int i, error = 0;
aodev->active = 0;
- for (i = 0; i < aodevs->size; i++) {
- if (aodevs->array[i].active)
+
+ for (i = 0; i < aodevs->used; i++) {
+ if (aodevs->array[i]->active)
return;
- if (aodevs->array[i].rc)
- error = aodevs->array[i].rc;
+ if (aodevs->array[i]->rc)
+ error = aodevs->array[i]->rc;
}
aodevs->callback(egc, aodevs, error);
return;
}
+void libxl__multidev_prepared(libxl__egc *egc, libxl__ao_devices *aodevs,
+ int rc)
+{
+ aodevs->preparation->rc = rc;
+ multidev_one_callback(egc, aodevs->preparation);
+}
+
/******************************************************************************/
/* Macro for defining the functions that will add a bunch of disks when
- * inside an async op.
+ * inside an async op with multidev.
* This macro is added to prevent repetition of code.
*
* The following functions are defined:
@@ -495,9 +492,9 @@ void libxl__ao_devices_callback(libxl__egc *egc, libxl__ao_device *aodev)
int i; \
int end = start + d_config->num_##type##s; \
for (i = start; i < end; i++) { \
- aodevs->array[i].callback = libxl__ao_devices_callback; \
+ libxl__ao_device *aodev = libxl__multidev_prepare(aodevs); \
libxl__device_##type##_add(egc, domid, &d_config->type##s[i-start],\
- &aodevs->array[i]); \
+ aodev); \
} \
}
@@ -546,20 +543,13 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
char *path;
unsigned int num_kinds, num_dev_xsentries;
char **kinds = NULL, **devs = NULL;
- int i, j, numdev = 0, rc = 0;
+ int i, j, rc = 0;
libxl__device *dev;
libxl__ao_devices *aodevs = &drs->aodevs;
libxl__ao_device *aodev;
libxl__device_kind kind;
- aodevs->size = libxl__num_devices(gc, drs->domid);
- if (aodevs->size < 0) {
- LOG(ERROR, "unable to get number of devices for domain %u", drs->domid);
- rc = aodevs->size;
- goto out;
- }
-
- libxl__prepare_ao_devices(drs->ao, aodevs);
+ libxl__multidev_begin(ao, aodevs);
aodevs->callback = devices_remove_callback;
path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
@@ -597,13 +587,11 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
libxl__device_destroy(gc, dev);
continue;
}
- aodev = &aodevs->array[numdev];
+ aodev = libxl__multidev_prepare(aodevs);
aodev->action = DEVICE_DISCONNECT;
aodev->dev = dev;
- aodev->callback = libxl__ao_devices_callback;
aodev->force = drs->force;
libxl__initiate_device_remove(egc, aodev);
- numdev++;
}
}
}
@@ -625,8 +613,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
}
out:
- if (!numdev) drs->callback(egc, drs, rc);
- return;
+ libxl__multidev_prepared(egc, aodevs, rc);
}
/* Callbacks for device related operations */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f2e9572..177642b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -856,10 +856,10 @@ retry_transaction:
if (errno == EAGAIN)
goto retry_transaction;
- sdss->aodevs.size = dm_config->num_disks;
+ libxl__multidev_begin(ao, &sdss->aodevs);
sdss->aodevs.callback = spawn_stub_launch_dm;
- libxl__prepare_ao_devices(ao, &sdss->aodevs);
libxl__add_disks(egc, ao, dm_domid, 0, dm_config, &sdss->aodevs);
+ libxl__multidev_prepared(egc, &sdss->aodevs, 0);
free(args);
return;
@@ -982,10 +982,10 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
if (rc) goto out;
if (d_config->num_nics > 0) {
- sdss->aodevs.size = d_config->num_nics;
+ libxl__multidev_begin(ao, &sdss->aodevs);
sdss->aodevs.callback = stubdom_pvqemu_cb;
- libxl__prepare_ao_devices(ao, &sdss->aodevs);
libxl__add_nics(egc, ao, dm_domid, 0, d_config, &sdss->aodevs);
+ libxl__multidev_prepared(egc, &sdss->aodevs, 0);
return;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c57503f..a5978b0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1810,20 +1810,6 @@ typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
*/
_hidden void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev);
-/* Prepare a bunch of devices for addition/removal. Every ao_device in
- * ao_devices is set to 'active', and the ao_device 'base' field is set to
- * the one pointed by aodevs.
- */
-_hidden void libxl__prepare_ao_devices(libxl__ao *ao,
- libxl__ao_devices *aodevs);
-
-/* Generic callback to use when adding/removing several devices, this will
- * check if the given aodev is the last one, and call the callback in the
- * parent libxl__ao_devices struct, passing the appropriate error if found.
- */
-_hidden void libxl__ao_devices_callback(libxl__egc *egc,
- libxl__ao_device *aodev);
-
struct libxl__ao_device {
/* filled in by user */
libxl__ao *ao;
@@ -1831,32 +1817,60 @@ struct libxl__ao_device {
libxl__device *dev;
int force;
libxl__device_callback *callback;
- /* private for implementation */
- int active;
+ /* return value, zeroed by user on entry, is valid on callback */
int rc;
+ /* private for multidev */
+ int active;
+ libxl__ao_devices *aodevs; /* reference to the containing multidev */
+ /* private for add/remove implementation */
libxl__ev_devstate backend_ds;
/* Bodge for Qemu devices, also used for timeout of hotplug execution */
libxl__ev_time timeout;
- /* Used internally to have a reference to the upper libxl__ao_devices
- * struct when present */
- libxl__ao_devices *aodevs;
/* device hotplug execution */
const char *what;
int num_exec;
libxl__ev_child child;
};
-/* Helper struct to simply the plug/unplug of multiple devices at the same
- * time.
- *
- * This structure holds several devices, and the callback is only called
- * when all the devices inside of the array have finished.
- */
+/*
+ * Multiple devices "multidev" handling.
+ *
+ * Firstly, you should
+ * libxl__multidev_begin
+ * multidev->callback = ...
+ * Then zero or more times
+ * libxl__multidev_prepare
+ * libal__initiate_device_{remove/addition}.
+ * Finally, once
+ * libxl__multidev_prepared
+ * which will result (perhaps reentrantly) in one call to callback().
+ */
+
+/* Starts preparing to add/remove a bunch of devices. */
+_hidden void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices*);
+
+/* Prepares to add/remove one of many devices. Returns a libxl__ao_device
+ * which has had libxl__prepare_ao_device called, and which has also
+ * had ->callback set. The user should not mess with aodev->callback. */
+_hidden libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices*);
+
+/* Notifies the multidev machinery that we have now finished preparing
+ * and initiating devices. multidev->callback may then be called as
+ * soon as there are no prepared but not completed operations
+ * outstanding, perhaps reentrantly. If rc!=0 (error should have been
+ * logged) multidev->callback will get a non-zero rc.
+ * callback may be set by the user at any point before prepared. */
+_hidden void libxl__multidev_prepared(libxl__egc*, libxl__ao_devices*, int rc);
+
typedef void libxl__devices_callback(libxl__egc*, libxl__ao_devices*, int rc);
struct libxl__ao_devices {
- libxl__ao_device *array;
- int size;
+ /* set by user: */
libxl__devices_callback *callback;
+ /* for private use by libxl__...ao_devices... machinery: */
+ libxl__ao *ao;
+ libxl__ao_device **array;
+ int used, allocd;
+ libxl__ao_device *preparation;
};
/*
@@ -2366,10 +2380,11 @@ _hidden void libxl__devices_destroy(libxl__egc *egc,
libxl__devices_remove_state *drs);
/* Helper function to add a bunch of disks. This should be used when
- * the caller is inside an async op. "devices" will NOT be prepared by this
- * function, so the caller must make sure to call _prepare before calling this
- * function. The start parameter contains the position inside the aodevs array
- * that should be used to store the state of this devices.
+ * the caller is inside an async op. "devices" will NOT be prepared by
+ * this function, so the caller must make sure to call
+ * libxl__multidev_begin before calling this function. The start
+ * parameter contains the position inside the aodevs array that should
+ * be used to store the state of this devices.
*
* The "callback" will be called for each device, and the user is responsible
* for calling libxl__ao_device_check_last on the callback.
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 04/11] libxl: fix formatting of DEFINE_DEVICES_ADD
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (2 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 03/11] libxl: fix device counting race in libxl__devices_destroy Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:18 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 05/11] libxl: abolish useless `start' parameter to libxl__add_* Ian Jackson
` (6 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
These lines were exactly 80 columns wide, which produces hideous wrap
damage in an 80 column emacs. Reformat using emacs's C-c \,
which puts the \ in column 72 (by default) where possible.
Whitespace change only.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_device.c | 26 +++++++++++++-------------
1 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 544a861..319f0e8 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -483,19 +483,19 @@ void libxl__multidev_prepared(libxl__egc *egc, libxl__ao_devices *aodevs,
* libxl__add_nics
*/
-#define DEFINE_DEVICES_ADD(type) \
- void libxl__add_##type##s(libxl__egc *egc, libxl__ao *ao, uint32_t domid, \
- int start, libxl_domain_config *d_config, \
- libxl__ao_devices *aodevs) \
- { \
- AO_GC; \
- int i; \
- int end = start + d_config->num_##type##s; \
- for (i = start; i < end; i++) { \
- libxl__ao_device *aodev = libxl__multidev_prepare(aodevs); \
- libxl__device_##type##_add(egc, domid, &d_config->type##s[i-start],\
- aodev); \
- } \
+#define DEFINE_DEVICES_ADD(type) \
+ void libxl__add_##type##s(libxl__egc *egc, libxl__ao *ao, uint32_t domid, \
+ int start, libxl_domain_config *d_config, \
+ libxl__ao_devices *aodevs) \
+ { \
+ AO_GC; \
+ int i; \
+ int end = start + d_config->num_##type##s; \
+ for (i = start; i < end; i++) { \
+ libxl__ao_device *aodev = libxl__multidev_prepare(aodevs); \
+ libxl__device_##type##_add(egc, domid, &d_config->type##s[i-start], \
+ aodev); \
+ } \
}
DEFINE_DEVICES_ADD(disk)
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 05/11] libxl: abolish useless `start' parameter to libxl__add_*
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (3 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 04/11] libxl: fix formatting of DEFINE_DEVICES_ADD Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:19 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 06/11] libxl: rename aodevs to multidev Ian Jackson
` (5 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
0 is always passed for this parameter and the code doesn't, actually,
use it, now.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_create.c | 4 ++--
tools/libxl/libxl_device.c | 7 +++----
tools/libxl/libxl_dm.c | 4 ++--
tools/libxl/libxl_internal.h | 8 +++-----
4 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 3265d69..5275373 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -911,7 +911,7 @@ static void domcreate_rebuild_done(libxl__egc *egc,
libxl__multidev_begin(ao, &dcs->aodevs);
dcs->aodevs.callback = domcreate_launch_dm;
- libxl__add_disks(egc, ao, domid, 0, d_config, &dcs->aodevs);
+ libxl__add_disks(egc, ao, domid, d_config, &dcs->aodevs);
libxl__multidev_prepared(egc, &dcs->aodevs, 0);
return;
@@ -1041,7 +1041,7 @@ static void domcreate_devmodel_started(libxl__egc *egc,
/* Attach nics */
libxl__multidev_begin(ao, &dcs->aodevs);
dcs->aodevs.callback = domcreate_attach_pci;
- libxl__add_nics(egc, ao, domid, 0, d_config, &dcs->aodevs);
+ libxl__add_nics(egc, ao, domid, d_config, &dcs->aodevs);
libxl__multidev_prepared(egc, &dcs->aodevs, 0);
return;
}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 319f0e8..41d527b 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -485,15 +485,14 @@ void libxl__multidev_prepared(libxl__egc *egc, libxl__ao_devices *aodevs,
#define DEFINE_DEVICES_ADD(type) \
void libxl__add_##type##s(libxl__egc *egc, libxl__ao *ao, uint32_t domid, \
- int start, libxl_domain_config *d_config, \
+ libxl_domain_config *d_config, \
libxl__ao_devices *aodevs) \
{ \
AO_GC; \
int i; \
- int end = start + d_config->num_##type##s; \
- for (i = start; i < end; i++) { \
+ for (i = 0; i < d_config->num_##type##s; i++) { \
libxl__ao_device *aodev = libxl__multidev_prepare(aodevs); \
- libxl__device_##type##_add(egc, domid, &d_config->type##s[i-start], \
+ libxl__device_##type##_add(egc, domid, &d_config->type##s[i], \
aodev); \
} \
}
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 177642b..66aa45e 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -858,7 +858,7 @@ retry_transaction:
libxl__multidev_begin(ao, &sdss->aodevs);
sdss->aodevs.callback = spawn_stub_launch_dm;
- libxl__add_disks(egc, ao, dm_domid, 0, dm_config, &sdss->aodevs);
+ libxl__add_disks(egc, ao, dm_domid, dm_config, &sdss->aodevs);
libxl__multidev_prepared(egc, &sdss->aodevs, 0);
free(args);
@@ -984,7 +984,7 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
if (d_config->num_nics > 0) {
libxl__multidev_begin(ao, &sdss->aodevs);
sdss->aodevs.callback = stubdom_pvqemu_cb;
- libxl__add_nics(egc, ao, dm_domid, 0, d_config, &sdss->aodevs);
+ libxl__add_nics(egc, ao, dm_domid, d_config, &sdss->aodevs);
libxl__multidev_prepared(egc, &sdss->aodevs, 0);
return;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a5978b0..450dbe5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2382,19 +2382,17 @@ _hidden void libxl__devices_destroy(libxl__egc *egc,
/* Helper function to add a bunch of disks. This should be used when
* the caller is inside an async op. "devices" will NOT be prepared by
* this function, so the caller must make sure to call
- * libxl__multidev_begin before calling this function. The start
- * parameter contains the position inside the aodevs array that should
- * be used to store the state of this devices.
+ * libxl__multidev_begin before calling this function.
*
* The "callback" will be called for each device, and the user is responsible
* for calling libxl__ao_device_check_last on the callback.
*/
_hidden void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
- int start, libxl_domain_config *d_config,
+ libxl_domain_config *d_config,
libxl__ao_devices *aodevs);
_hidden void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
- int start, libxl_domain_config *d_config,
+ libxl_domain_config *d_config,
libxl__ao_devices *aodevs);
/*----- device model creation -----*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 05/11] libxl: abolish useless `start' parameter to libxl__add_*
2012-08-01 16:24 ` [PATCH 05/11] libxl: abolish useless `start' parameter to libxl__add_* Ian Jackson
@ 2012-08-02 10:19 ` Ian Campbell
0 siblings, 0 replies; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 10:19 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> 0 is always passed for this parameter and the code doesn't, actually,
> use it, now.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> { \
> AO_GC; \
> int i; \
> - int end = start + d_config->num_##type##s; \
This definition of end is pretty dodgy, glad to be rid of it.
> - for (i = start; i < end; i++) { \
> + for (i = 0; i < d_config->num_##type##s; i++) { \
> libxl__ao_device *aodev = libxl__multidev_prepare(aodevs); \
> - libxl__device_##type##_add(egc, domid, &d_config->type##s[i-start], \
> + libxl__device_##type##_add(egc, domid, &d_config->type##s[i], \
> aodev); \
> } \
> }
Ian.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 06/11] libxl: rename aodevs to multidev
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (4 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 05/11] libxl: abolish useless `start' parameter to libxl__add_* Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:20 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 07/11] libxl: do not blunder on if bootloader fails (again) Ian Jackson
` (4 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
To be consistent with the new function naming, rename
libxl__ao_devices to libxl__multidev and all variables aodevs to
multidev.
No functional change.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_create.c | 30 +++++++++---------
tools/libxl/libxl_device.c | 68 +++++++++++++++++++++---------------------
tools/libxl/libxl_dm.c | 30 +++++++++---------
tools/libxl/libxl_internal.h | 26 ++++++++--------
4 files changed, 77 insertions(+), 77 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 5275373..5f0d26f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -599,10 +599,10 @@ static void domcreate_bootloader_done(libxl__egc *egc,
libxl__bootloader_state *bl,
int rc);
-static void domcreate_launch_dm(libxl__egc *egc, libxl__ao_devices *aodevs,
+static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
-static void domcreate_attach_pci(libxl__egc *egc, libxl__ao_devices *aodevs,
+static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
static void domcreate_console_available(libxl__egc *egc,
@@ -909,10 +909,10 @@ static void domcreate_rebuild_done(libxl__egc *egc,
store_libxl_entry(gc, domid, &d_config->b_info);
- libxl__multidev_begin(ao, &dcs->aodevs);
- dcs->aodevs.callback = domcreate_launch_dm;
- libxl__add_disks(egc, ao, domid, d_config, &dcs->aodevs);
- libxl__multidev_prepared(egc, &dcs->aodevs, 0);
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_launch_dm;
+ libxl__add_disks(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
return;
@@ -921,10 +921,10 @@ static void domcreate_rebuild_done(libxl__egc *egc,
domcreate_complete(egc, dcs, ret);
}
-static void domcreate_launch_dm(libxl__egc *egc, libxl__ao_devices *aodevs,
+static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
int ret)
{
- libxl__domain_create_state *dcs = CONTAINER_OF(aodevs, *dcs, aodevs);
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
STATE_AO_GC(dcs->ao);
int i;
@@ -1039,14 +1039,14 @@ static void domcreate_devmodel_started(libxl__egc *egc,
/* Plug nic interfaces */
if (d_config->num_nics > 0) {
/* Attach nics */
- libxl__multidev_begin(ao, &dcs->aodevs);
- dcs->aodevs.callback = domcreate_attach_pci;
- libxl__add_nics(egc, ao, domid, d_config, &dcs->aodevs);
- libxl__multidev_prepared(egc, &dcs->aodevs, 0);
+ libxl__multidev_begin(ao, &dcs->multidev);
+ dcs->multidev.callback = domcreate_attach_pci;
+ libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev);
+ libxl__multidev_prepared(egc, &dcs->multidev, 0);
return;
}
- domcreate_attach_pci(egc, &dcs->aodevs, 0);
+ domcreate_attach_pci(egc, &dcs->multidev, 0);
return;
error_out:
@@ -1054,10 +1054,10 @@ error_out:
domcreate_complete(egc, dcs, ret);
}
-static void domcreate_attach_pci(libxl__egc *egc, libxl__ao_devices *aodevs,
+static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
int ret)
{
- libxl__domain_create_state *dcs = CONTAINER_OF(aodevs, *dcs, aodevs);
+ libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
STATE_AO_GC(dcs->ao);
int i;
libxl_ctx *ctx = CTX;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 41d527b..84fa06c 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -403,13 +403,13 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
/* multidev */
-void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices *aodevs)
+void libxl__multidev_begin(libxl__ao *ao, libxl__multidev *multidev)
{
AO_GC;
- aodevs->ao = ao;
- aodevs->array = 0;
- aodevs->used = aodevs->allocd = 0;
+ multidev->ao = ao;
+ multidev->array = 0;
+ multidev->used = multidev->allocd = 0;
/* We allocate an aodev to represent the operation of preparing
* all of the other operations. This operation is completed when
@@ -422,25 +422,25 @@ void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices *aodevs)
* (iii) we have a nice consistent way to deal with any
* error that might occur while deciding what to initiate
*/
- aodevs->preparation = libxl__multidev_prepare(aodevs);
+ multidev->preparation = libxl__multidev_prepare(multidev);
}
static void multidev_one_callback(libxl__egc *egc, libxl__ao_device *aodev);
-libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices *aodevs) {
- STATE_AO_GC(aodevs->ao);
+libxl__ao_device *libxl__multidev_prepare(libxl__multidev *multidev) {
+ STATE_AO_GC(multidev->ao);
libxl__ao_device *aodev;
GCNEW(aodev);
- aodev->aodevs = aodevs;
+ aodev->multidev = multidev;
aodev->callback = multidev_one_callback;
libxl__prepare_ao_device(ao, aodev);
- if (aodevs->used >= aodevs->allocd) {
- aodevs->allocd = aodevs->used * 2 + 5;
- GCREALLOC_ARRAY(aodevs->array, aodevs->allocd);
+ if (multidev->used >= multidev->allocd) {
+ multidev->allocd = multidev->used * 2 + 5;
+ GCREALLOC_ARRAY(multidev->array, multidev->allocd);
}
- aodevs->array[aodevs->used++] = aodev;
+ multidev->array[multidev->used++] = aodev;
return aodev;
}
@@ -448,28 +448,28 @@ libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices *aodevs) {
static void multidev_one_callback(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
- libxl__ao_devices *aodevs = aodev->aodevs;
+ libxl__multidev *multidev = aodev->multidev;
int i, error = 0;
aodev->active = 0;
- for (i = 0; i < aodevs->used; i++) {
- if (aodevs->array[i]->active)
+ for (i = 0; i < multidev->used; i++) {
+ if (multidev->array[i]->active)
return;
- if (aodevs->array[i]->rc)
- error = aodevs->array[i]->rc;
+ if (multidev->array[i]->rc)
+ error = multidev->array[i]->rc;
}
- aodevs->callback(egc, aodevs, error);
+ multidev->callback(egc, multidev, error);
return;
}
-void libxl__multidev_prepared(libxl__egc *egc, libxl__ao_devices *aodevs,
- int rc)
+void libxl__multidev_prepared(libxl__egc *egc,
+ libxl__multidev *multidev, int rc)
{
- aodevs->preparation->rc = rc;
- multidev_one_callback(egc, aodevs->preparation);
+ multidev->preparation->rc = rc;
+ multidev_one_callback(egc, multidev->preparation);
}
/******************************************************************************/
@@ -486,12 +486,12 @@ void libxl__multidev_prepared(libxl__egc *egc, libxl__ao_devices *aodevs,
#define DEFINE_DEVICES_ADD(type) \
void libxl__add_##type##s(libxl__egc *egc, libxl__ao *ao, uint32_t domid, \
libxl_domain_config *d_config, \
- libxl__ao_devices *aodevs) \
+ libxl__multidev *multidev) \
{ \
AO_GC; \
int i; \
for (i = 0; i < d_config->num_##type##s; i++) { \
- libxl__ao_device *aodev = libxl__multidev_prepare(aodevs); \
+ libxl__ao_device *aodev = libxl__multidev_prepare(multidev); \
libxl__device_##type##_add(egc, domid, &d_config->type##s[i], \
aodev); \
} \
@@ -531,8 +531,8 @@ out:
/* Callback for device destruction */
-static void devices_remove_callback(libxl__egc *egc, libxl__ao_devices *aodevs,
- int rc);
+static void devices_remove_callback(libxl__egc *egc,
+ libxl__multidev *multidev, int rc);
void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
{
@@ -544,12 +544,12 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
char **kinds = NULL, **devs = NULL;
int i, j, rc = 0;
libxl__device *dev;
- libxl__ao_devices *aodevs = &drs->aodevs;
+ libxl__multidev *multidev = &drs->multidev;
libxl__ao_device *aodev;
libxl__device_kind kind;
- libxl__multidev_begin(ao, aodevs);
- aodevs->callback = devices_remove_callback;
+ libxl__multidev_begin(ao, multidev);
+ multidev->callback = devices_remove_callback;
path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
@@ -586,7 +586,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
libxl__device_destroy(gc, dev);
continue;
}
- aodev = libxl__multidev_prepare(aodevs);
+ aodev = libxl__multidev_prepare(multidev);
aodev->action = DEVICE_DISCONNECT;
aodev->dev = dev;
aodev->force = drs->force;
@@ -612,7 +612,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
}
out:
- libxl__multidev_prepared(egc, aodevs, rc);
+ libxl__multidev_prepared(egc, multidev, rc);
}
/* Callbacks for device related operations */
@@ -1002,10 +1002,10 @@ static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev)
assert(!libxl__ev_child_inuse(&aodev->child));
}
-static void devices_remove_callback(libxl__egc *egc, libxl__ao_devices *aodevs,
- int rc)
+static void devices_remove_callback(libxl__egc *egc,
+ libxl__multidev *multidev, int rc)
{
- libxl__devices_remove_state *drs = CONTAINER_OF(aodevs, *drs, aodevs);
+ libxl__devices_remove_state *drs = CONTAINER_OF(multidev, *drs, multidev);
STATE_AO_GC(drs->ao);
drs->callback(egc, drs, rc);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 66aa45e..0c0084f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -714,10 +714,10 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
int rc);
static void spawn_stub_launch_dm(libxl__egc *egc,
- libxl__ao_devices *aodevs, int ret);
+ libxl__multidev *aodevs, int ret);
static void stubdom_pvqemu_cb(libxl__egc *egc,
- libxl__ao_devices *aodevs,
+ libxl__multidev *aodevs,
int rc);
static void spaw_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
@@ -856,10 +856,10 @@ retry_transaction:
if (errno == EAGAIN)
goto retry_transaction;
- libxl__multidev_begin(ao, &sdss->aodevs);
- sdss->aodevs.callback = spawn_stub_launch_dm;
- libxl__add_disks(egc, ao, dm_domid, dm_config, &sdss->aodevs);
- libxl__multidev_prepared(egc, &sdss->aodevs, 0);
+ libxl__multidev_begin(ao, &sdss->multidev);
+ sdss->multidev.callback = spawn_stub_launch_dm;
+ libxl__add_disks(egc, ao, dm_domid, dm_config, &sdss->multidev);
+ libxl__multidev_prepared(egc, &sdss->multidev, 0);
free(args);
return;
@@ -872,9 +872,9 @@ out:
}
static void spawn_stub_launch_dm(libxl__egc *egc,
- libxl__ao_devices *aodevs, int ret)
+ libxl__multidev *multidev, int ret)
{
- libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(aodevs, *sdss, aodevs);
+ libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(multidev, *sdss, multidev);
STATE_AO_GC(sdss->dm.spawn.ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
int i, num_console = STUBDOM_SPECIAL_CONSOLES;
@@ -982,22 +982,22 @@ static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
if (rc) goto out;
if (d_config->num_nics > 0) {
- libxl__multidev_begin(ao, &sdss->aodevs);
- sdss->aodevs.callback = stubdom_pvqemu_cb;
- libxl__add_nics(egc, ao, dm_domid, d_config, &sdss->aodevs);
- libxl__multidev_prepared(egc, &sdss->aodevs, 0);
+ libxl__multidev_begin(ao, &sdss->multidev);
+ sdss->multidev.callback = stubdom_pvqemu_cb;
+ libxl__add_nics(egc, ao, dm_domid, d_config, &sdss->multidev);
+ libxl__multidev_prepared(egc, &sdss->multidev, 0);
return;
}
out:
- stubdom_pvqemu_cb(egc, &sdss->aodevs, rc);
+ stubdom_pvqemu_cb(egc, &sdss->multidev, rc);
}
static void stubdom_pvqemu_cb(libxl__egc *egc,
- libxl__ao_devices *aodevs,
+ libxl__multidev *multidev,
int rc)
{
- libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(aodevs, *sdss, aodevs);
+ libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(multidev, *sdss, multidev);
STATE_AO_GC(sdss->dm.spawn.ao);
uint32_t dm_domid = sdss->pvqemu.guest_domid;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 450dbe5..9315ae0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1791,7 +1791,7 @@ typedef enum {
} libxl__device_action;
typedef struct libxl__ao_device libxl__ao_device;
-typedef struct libxl__ao_devices libxl__ao_devices;
+typedef struct libxl__multidev libxl__multidev;
typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
/* This functions sets the necessary libxl__ao_device struct values to use
@@ -1821,7 +1821,7 @@ struct libxl__ao_device {
int rc;
/* private for multidev */
int active;
- libxl__ao_devices *aodevs; /* reference to the containing multidev */
+ libxl__multidev *multidev; /* reference to the containing multidev */
/* private for add/remove implementation */
libxl__ev_devstate backend_ds;
/* Bodge for Qemu devices, also used for timeout of hotplug execution */
@@ -1847,12 +1847,12 @@ struct libxl__ao_device {
*/
/* Starts preparing to add/remove a bunch of devices. */
-_hidden void libxl__multidev_begin(libxl__ao *ao, libxl__ao_devices*);
+_hidden void libxl__multidev_begin(libxl__ao *ao, libxl__multidev*);
/* Prepares to add/remove one of many devices. Returns a libxl__ao_device
* which has had libxl__prepare_ao_device called, and which has also
* had ->callback set. The user should not mess with aodev->callback. */
-_hidden libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices*);
+_hidden libxl__ao_device *libxl__multidev_prepare(libxl__multidev*);
/* Notifies the multidev machinery that we have now finished preparing
* and initiating devices. multidev->callback may then be called as
@@ -1860,10 +1860,10 @@ _hidden libxl__ao_device *libxl__multidev_prepare(libxl__ao_devices*);
* outstanding, perhaps reentrantly. If rc!=0 (error should have been
* logged) multidev->callback will get a non-zero rc.
* callback may be set by the user at any point before prepared. */
-_hidden void libxl__multidev_prepared(libxl__egc*, libxl__ao_devices*, int rc);
+_hidden void libxl__multidev_prepared(libxl__egc*, libxl__multidev*, int rc);
-typedef void libxl__devices_callback(libxl__egc*, libxl__ao_devices*, int rc);
-struct libxl__ao_devices {
+typedef void libxl__devices_callback(libxl__egc*, libxl__multidev*, int rc);
+struct libxl__multidev {
/* set by user: */
libxl__devices_callback *callback;
/* for private use by libxl__...ao_devices... machinery: */
@@ -2336,7 +2336,7 @@ struct libxl__devices_remove_state {
libxl__devices_remove_callback *callback;
int force; /* libxl_device_TYPE_destroy rather than _remove */
/* private */
- libxl__ao_devices aodevs;
+ libxl__multidev multidev;
int num_devices;
};
@@ -2380,7 +2380,7 @@ _hidden void libxl__devices_destroy(libxl__egc *egc,
libxl__devices_remove_state *drs);
/* Helper function to add a bunch of disks. This should be used when
- * the caller is inside an async op. "devices" will NOT be prepared by
+ * the caller is inside an async op. "multidev" will NOT be prepared by
* this function, so the caller must make sure to call
* libxl__multidev_begin before calling this function.
*
@@ -2389,11 +2389,11 @@ _hidden void libxl__devices_destroy(libxl__egc *egc,
*/
_hidden void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
libxl_domain_config *d_config,
- libxl__ao_devices *aodevs);
+ libxl__multidev *multidev);
_hidden void libxl__add_nics(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
libxl_domain_config *d_config,
- libxl__ao_devices *aodevs);
+ libxl__multidev *multidev);
/*----- device model creation -----*/
@@ -2429,7 +2429,7 @@ typedef struct {
libxl__domain_build_state dm_state;
libxl__dm_spawn_state pvqemu;
libxl__destroy_domid_state dis;
- libxl__ao_devices aodevs;
+ libxl__multidev multidev;
} libxl__stub_dm_spawn_state;
_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
@@ -2461,7 +2461,7 @@ struct libxl__domain_create_state {
libxl__save_helper_state shs;
/* necessary if the domain creation failed and we have to destroy it */
libxl__domain_destroy_state dds;
- libxl__ao_devices aodevs;
+ libxl__multidev multidev;
};
/*----- Domain suspend (save) functions -----*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 07/11] libxl: do not blunder on if bootloader fails (again)
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (5 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 06/11] libxl: rename aodevs to multidev Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:22 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 08/11] Debugging machinery for synthesising POLLHUP Ian Jackson
` (3 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Do not lose the rc value passed to bootloader_callback. Do not lose
the rc value from the bl when the local disk detach succeeds.
While we're here rationalise the use of bl->rc to make things clearer.
Set it to zero at the start and always update it conditionally; copy
it into bootloader_callback's argument each time.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_bootloader.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index bfc1b56..e103ee9 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -206,6 +206,7 @@ static int parse_bootloader_result(libxl__egc *egc,
void libxl__bootloader_init(libxl__bootloader_state *bl)
{
assert(bl->ao);
+ bl->rc = 0;
bl->dls.diskpath = NULL;
bl->openpty.ao = bl->ao;
bl->dls.ao = bl->ao;
@@ -255,6 +256,9 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
int rc)
{
+ if (!bl->rc)
+ bl->rc = rc;
+
bootloader_cleanup(egc, bl);
bl->dls.callback = bootloader_local_detached_cb;
@@ -270,9 +274,11 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
if (rc) {
LOG(ERROR, "unable to detach locally attached disk");
+ if (!bl->rc)
+ bl->rc = rc;
}
- bl->callback(egc, bl, rc);
+ bl->callback(egc, bl, bl->rc);
}
/* might be called at any time, provided it's init'd */
@@ -289,7 +295,8 @@ static void bootloader_stop(libxl__egc *egc,
if (r) LOGE(WARN, "%sfailed to kill bootloader [%lu]",
rc ? "after failure, " : "", (unsigned long)bl->child.pid);
}
- bl->rc = rc;
+ if (!bl->rc)
+ bl->rc = rc;
}
/*----- main flow of control -----*/
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 07/11] libxl: do not blunder on if bootloader fails (again)
2012-08-01 16:24 ` [PATCH 07/11] libxl: do not blunder on if bootloader fails (again) Ian Jackson
@ 2012-08-02 10:22 ` Ian Campbell
0 siblings, 0 replies; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 10:22 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> Do not lose the rc value passed to bootloader_callback. Do not lose
> the rc value from the bl when the local disk detach succeeds.
>
> While we're here rationalise the use of bl->rc to make things clearer.
> Set it to zero at the start and always update it conditionally; copy
> it into bootloader_callback's argument each time.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_bootloader.c | 11 +++++++++--
> 1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index bfc1b56..e103ee9 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
> @@ -206,6 +206,7 @@ static int parse_bootloader_result(libxl__egc *egc,
> void libxl__bootloader_init(libxl__bootloader_state *bl)
> {
> assert(bl->ao);
> + bl->rc = 0;
> bl->dls.diskpath = NULL;
> bl->openpty.ao = bl->ao;
> bl->dls.ao = bl->ao;
> @@ -255,6 +256,9 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
> static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
> int rc)
> {
> + if (!bl->rc)
> + bl->rc = rc;
> +
> bootloader_cleanup(egc, bl);
>
> bl->dls.callback = bootloader_local_detached_cb;
> @@ -270,9 +274,11 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
>
> if (rc) {
> LOG(ERROR, "unable to detach locally attached disk");
> + if (!bl->rc)
> + bl->rc = rc;
> }
>
> - bl->callback(egc, bl, rc);
> + bl->callback(egc, bl, bl->rc);
> }
>
> /* might be called at any time, provided it's init'd */
> @@ -289,7 +295,8 @@ static void bootloader_stop(libxl__egc *egc,
> if (r) LOGE(WARN, "%sfailed to kill bootloader [%lu]",
> rc ? "after failure, " : "", (unsigned long)bl->child.pid);
> }
> - bl->rc = rc;
> + if (!bl->rc)
> + bl->rc = rc;
> }
>
> /*----- main flow of control -----*/
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 08/11] Debugging machinery for synthesising POLLHUP
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (6 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 07/11] libxl: do not blunder on if bootloader fails (again) Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-01 16:24 ` [PATCH 09/11] libxl: remus: mark TODOs more clearly Ian Jackson
` (2 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
---
tools/libxl/libxl_bootloader.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index e103ee9..2e65383 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -447,6 +447,19 @@ static void bootloader_disk_attached_cb(libxl__egc *egc,
bootloader_callback(egc, bl, rc);
}
+static int tst_blfd;
+static void tst_sigh(int dummy) {
+ int r, e = errno;
+ int p[2];
+ write(2,"tst_sigh\n",9);
+ r = pipe(p); assert(!r);
+ close(p[1]);
+if (getenv("TST_EXTRADUP")) dup(tst_blfd);
+ dup2(p[0], tst_blfd);
+ close(p[0]);
+ errno = e;
+}
+
static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
{
libxl__bootloader_state *bl = CONTAINER_OF(op, *bl, openpty);
@@ -503,6 +516,9 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
int bootloader_master = libxl__carefd_fd(bl->ptys[0].master);
int xenconsole_master = libxl__carefd_fd(bl->ptys[1].master);
+tst_blfd = bootloader_master;
+signal(SIGUSR2,tst_sigh);
+
libxl_fd_set_nonblock(CTX, bootloader_master, 1);
libxl_fd_set_nonblock(CTX, xenconsole_master, 1);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 09/11] libxl: remus: mark TODOs more clearly
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (7 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 08/11] Debugging machinery for synthesising POLLHUP Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:23 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 10/11] libxl: remove an unused numainfo parameter Ian Jackson
2012-08-01 16:24 ` [PATCH 11/11] libxl: -Wunused-parameter Ian Jackson
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Change the TODOs in the remus code to "REMUS TODO" which will make
them easier to grep for later. AIUI all of these are essential for
use of remus in production.
Also add a new TODO and a new assert, to check rc on entry to
remus_checkpoint_dm_saved.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_dom.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d749983..06d5e4f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1110,7 +1110,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
static int libxl__remus_domain_suspend_callback(void *data)
{
- /* TODO: Issue disk and network checkpoint reqs. */
+ /* REMUS TODO: Issue disk and network checkpoint reqs. */
return libxl__domain_suspend_common_callback(data);
}
@@ -1124,7 +1124,7 @@ static int libxl__remus_domain_resume_callback(void *data)
if (libxl_domain_resume(CTX, dss->domid, /* Fast Suspend */1))
return 0;
- /* TODO: Deal with disk. Start a new network output buffer */
+ /* REMUS TODO: Deal with disk. Start a new network output buffer */
return 1;
}
@@ -1151,8 +1151,9 @@ static void libxl__remus_domain_checkpoint_callback(void *data)
static void remus_checkpoint_dm_saved(libxl__egc *egc,
libxl__domain_suspend_state *dss, int rc)
{
- /* TODO: Wait for disk and memory ack, release network buffer */
- /* TODO: make this asynchronous */
+ /* REMUS TODO: Wait for disk and memory ack, release network buffer */
+ /* REMUS TODO: make this asynchronous */
+ assert(!rc); /* REMUS TODO handle this error properly */
usleep(dss->interval * 1000);
libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, 1);
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 09/11] libxl: remus: mark TODOs more clearly
2012-08-01 16:24 ` [PATCH 09/11] libxl: remus: mark TODOs more clearly Ian Jackson
@ 2012-08-02 10:23 ` Ian Campbell
2012-08-02 12:18 ` Shriram Rajagopalan
0 siblings, 1 reply; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 10:23 UTC (permalink / raw)
To: Ian Jackson; +Cc: Shriram Rajagopalan, xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> Change the TODOs in the remus code to "REMUS TODO" which will make
> them easier to grep for later. AIUI all of these are essential for
> use of remus in production.
>
> Also add a new TODO and a new assert, to check rc on entry to
> remus_checkpoint_dm_saved.
CCing Shriram in the hops of getting some actual code here (for 4.3 most
likely).
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxl/libxl_dom.c | 9 +++++----
> 1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index d749983..06d5e4f 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1110,7 +1110,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
>
> static int libxl__remus_domain_suspend_callback(void *data)
> {
> - /* TODO: Issue disk and network checkpoint reqs. */
> + /* REMUS TODO: Issue disk and network checkpoint reqs. */
> return libxl__domain_suspend_common_callback(data);
> }
>
> @@ -1124,7 +1124,7 @@ static int libxl__remus_domain_resume_callback(void *data)
> if (libxl_domain_resume(CTX, dss->domid, /* Fast Suspend */1))
> return 0;
>
> - /* TODO: Deal with disk. Start a new network output buffer */
> + /* REMUS TODO: Deal with disk. Start a new network output buffer */
> return 1;
> }
>
> @@ -1151,8 +1151,9 @@ static void libxl__remus_domain_checkpoint_callback(void *data)
> static void remus_checkpoint_dm_saved(libxl__egc *egc,
> libxl__domain_suspend_state *dss, int rc)
> {
> - /* TODO: Wait for disk and memory ack, release network buffer */
> - /* TODO: make this asynchronous */
> + /* REMUS TODO: Wait for disk and memory ack, release network buffer */
> + /* REMUS TODO: make this asynchronous */
> + assert(!rc); /* REMUS TODO handle this error properly */
> usleep(dss->interval * 1000);
> libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, 1);
> }
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 09/11] libxl: remus: mark TODOs more clearly
2012-08-02 10:23 ` Ian Campbell
@ 2012-08-02 12:18 ` Shriram Rajagopalan
0 siblings, 0 replies; 28+ messages in thread
From: Shriram Rajagopalan @ 2012-08-02 12:18 UTC (permalink / raw)
To: Ian Campbell; +Cc: Ian Jackson, xen-devel@lists.xen.org
[-- Attachment #1.1: Type: text/plain, Size: 2358 bytes --]
On Thu, Aug 2, 2012 at 6:23 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:
> On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> > Change the TODOs in the remus code to "REMUS TODO" which will make
> > them easier to grep for later. AIUI all of these are essential for
> > use of remus in production.
> >
> > Also add a new TODO and a new assert, to check rc on entry to
> > remus_checkpoint_dm_saved.
>
> CCing Shriram in the hops of getting some actual code here (for 4.3 most
> likely).
>
> >
> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
>
Thanks I saw this yesterday.
shriram
> > ---
> > tools/libxl/libxl_dom.c | 9 +++++----
> > 1 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index d749983..06d5e4f 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -1110,7 +1110,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t
> **buf,
> >
> > static int libxl__remus_domain_suspend_callback(void *data)
> > {
> > - /* TODO: Issue disk and network checkpoint reqs. */
> > + /* REMUS TODO: Issue disk and network checkpoint reqs. */
> > return libxl__domain_suspend_common_callback(data);
> > }
> >
> > @@ -1124,7 +1124,7 @@ static int
> libxl__remus_domain_resume_callback(void *data)
> > if (libxl_domain_resume(CTX, dss->domid, /* Fast Suspend */1))
> > return 0;
> >
> > - /* TODO: Deal with disk. Start a new network output buffer */
> > + /* REMUS TODO: Deal with disk. Start a new network output buffer */
> > return 1;
> > }
> >
> > @@ -1151,8 +1151,9 @@ static void
> libxl__remus_domain_checkpoint_callback(void *data)
> > static void remus_checkpoint_dm_saved(libxl__egc *egc,
> > libxl__domain_suspend_state *dss,
> int rc)
> > {
> > - /* TODO: Wait for disk and memory ack, release network buffer */
> > - /* TODO: make this asynchronous */
> > + /* REMUS TODO: Wait for disk and memory ack, release network buffer
> */
> > + /* REMUS TODO: make this asynchronous */
> > + assert(!rc); /* REMUS TODO handle this error properly */
> > usleep(dss->interval * 1000);
> > libxl__xc_domain_saverestore_async_callback_done(egc, &dss->shs, 1);
> > }
>
>
>
[-- Attachment #1.2: Type: text/html, Size: 3317 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 10/11] libxl: remove an unused numainfo parameter
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (8 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 09/11] libxl: remus: mark TODOs more clearly Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 10:24 ` Ian Campbell
2012-08-01 16:24 ` [PATCH 11/11] libxl: -Wunused-parameter Ian Jackson
10 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_numa.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_numa.c b/tools/libxl/libxl_numa.c
index 5301ec4..2c8e59f 100644
--- a/tools/libxl/libxl_numa.c
+++ b/tools/libxl/libxl_numa.c
@@ -231,7 +231,7 @@ static int nodemap_to_nr_vcpus(libxl__gc *gc, libxl_cputopology *tinfo,
* candidates with just one node).
*/
static int count_cpus_per_node(libxl_cputopology *tinfo, int nr_cpus,
- libxl_numainfo *ninfo, int nr_nodes)
+ int nr_nodes)
{
int cpus_per_node = 0;
int j, i;
@@ -340,7 +340,7 @@ int libxl__get_numa_candidate(libxl__gc *gc,
if (!min_nodes) {
int cpus_per_node;
- cpus_per_node = count_cpus_per_node(tinfo, nr_cpus, ninfo, nr_nodes);
+ cpus_per_node = count_cpus_per_node(tinfo, nr_cpus, nr_nodes);
if (cpus_per_node == 0)
min_nodes = 1;
else
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 10/11] libxl: remove an unused numainfo parameter
2012-08-01 16:24 ` [PATCH 10/11] libxl: remove an unused numainfo parameter Ian Jackson
@ 2012-08-02 10:24 ` Ian Campbell
2012-08-02 11:00 ` Dario Faggioli
0 siblings, 1 reply; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 10:24 UTC (permalink / raw)
To: Ian Jackson; +Cc: Dario Faggioli, xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
CCing Dario.
> ---
> tools/libxl/libxl_numa.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl_numa.c b/tools/libxl/libxl_numa.c
> index 5301ec4..2c8e59f 100644
> --- a/tools/libxl/libxl_numa.c
> +++ b/tools/libxl/libxl_numa.c
> @@ -231,7 +231,7 @@ static int nodemap_to_nr_vcpus(libxl__gc *gc, libxl_cputopology *tinfo,
> * candidates with just one node).
> */
> static int count_cpus_per_node(libxl_cputopology *tinfo, int nr_cpus,
> - libxl_numainfo *ninfo, int nr_nodes)
> + int nr_nodes)
> {
> int cpus_per_node = 0;
> int j, i;
> @@ -340,7 +340,7 @@ int libxl__get_numa_candidate(libxl__gc *gc,
> if (!min_nodes) {
> int cpus_per_node;
>
> - cpus_per_node = count_cpus_per_node(tinfo, nr_cpus, ninfo, nr_nodes);
> + cpus_per_node = count_cpus_per_node(tinfo, nr_cpus, nr_nodes);
> if (cpus_per_node == 0)
> min_nodes = 1;
> else
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 10/11] libxl: remove an unused numainfo parameter
2012-08-02 10:24 ` Ian Campbell
@ 2012-08-02 11:00 ` Dario Faggioli
0 siblings, 0 replies; 28+ messages in thread
From: Dario Faggioli @ 2012-08-02 11:00 UTC (permalink / raw)
To: Ian Campbell; +Cc: Ian Jackson, xen-devel@lists.xen.org
[-- Attachment #1.1: Type: text/plain, Size: 671 bytes --]
On Thu, 2012-08-02 at 11:24 +0100, Ian Campbell wrote:
> On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> CCing Dario.
>
Uups, sorry, I guess it was a stale from a previous version I forgot to
kill. Thanks!
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 11/11] libxl: -Wunused-parameter
2012-08-01 16:24 [PATCH v4 00/11] libxl: Assorted bugfixes Ian Jackson
` (9 preceding siblings ...)
2012-08-01 16:24 ` [PATCH 10/11] libxl: remove an unused numainfo parameter Ian Jackson
@ 2012-08-01 16:24 ` Ian Jackson
2012-08-02 8:59 ` Ian Campbell
2012-08-02 10:41 ` Ian Campbell
10 siblings, 2 replies; 28+ messages in thread
From: Ian Jackson @ 2012-08-01 16:24 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Ian Campbell
*** DO NOT APPLY ***
We have recently had a couple of bugs which basically involved
ignoring the rc parameter to a callback function. I thought I would
try -Wunused-parameter. Here are the results.
I found three further problems:
* libxl_wait_for_free_memory takes a domid parameter but its
semantics don't seem to call for that. This function is
going to have a big warning put on it for 4.2 and that
should happen soon.
* qmp_synchronous_send has an `ask_timeout' parameter which is
ignored.
* The autogenerated function libxl_event_init_type ignores the type
parameter.
Things I needed to do to get the rest of the code to compile:
* Remove one harmless unused parameter from an internal function.
(Earlier in this series.)
* Add an assert to make the error handling in the broken remus code
slightly less broken. (Earlier in this series.)
* Provide machinery in the Makefile for passing different CFLAGS to
libxl as opposed to xl and libxlu. The flex- and bison-generated
files in libxlu can't be compiled with -Wunused-parameter.
* Define a new helper macro
#define USE(var) ((void)(var))
and use it 43 times. The pattern is something like
USE(egc);
in a function which takes egc but doesn't need it. If the
parameter is later used, this is harmless. In functions
which are placeholders the USE statement should be placed in the
middle of the function where the parameter would be used if the
function is changed later, so that the USE gets deleted by the
patch introducing the implementation.
* Define a new helper macro for use only in other macros
#define MAYBE_UNUSED __attribute__((unused))
and use it in 10 different places.
* Define new macros for helping declare common types of callback
functions. For example:
#define EV_XSWATCH_CALLBACK_PARAMS(egc, watch, wpath, epath) \
libxl__egc *egc MAYBE_UNUSED, \
libxl__ev_xswatch *watch MAYBE_UNUSED, \
const char *wpath MAYBE_UNUSED, \
const char *epath MAYBE_UNUSED
which is used like this:
-static void some_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
- const char *wpath, const char *epath)
+static void some_callback(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, watch, wpath, epath))
{
... now we use (or not) egc, watch, wpath, etc. or not as we like
This somewhat resembles a Traditional K&R C typeless function
definition. The types of the parameters are actually defined
for the compiler of course, along with the information that
the parameters might be unused.
There are 4 macros of this kind with 22 call sites.
IMO on the cost (65 places in ordinary code where we have to write
something somewhat ugly) is worth the benefit (finding, if we had
deployed this right away, around 6 bugs). But it's arguable.
*** DO NOT APPLY ***
Anyway, this patch must not be applied right now because it causes the
build to fail.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
---
tools/libxl/Makefile | 4 +++-
tools/libxl/libxl.c | 29 +++++++++++++++++++++++++----
tools/libxl/libxl_aoutils.c | 8 ++++----
tools/libxl/libxl_blktap2.c | 1 +
tools/libxl/libxl_bootloader.c | 6 ++++++
tools/libxl/libxl_create.c | 2 ++
tools/libxl/libxl_device.c | 9 +++++----
tools/libxl/libxl_dm.c | 4 ++++
tools/libxl/libxl_dom.c | 8 ++++----
tools/libxl/libxl_event.c | 22 +++++++++++++---------
tools/libxl/libxl_exec.c | 8 ++++----
tools/libxl/libxl_fork.c | 1 +
tools/libxl/libxl_internal.h | 24 ++++++++++++++++++++++--
tools/libxl/libxl_pci.c | 6 ++++++
tools/libxl/libxl_qmp.c | 17 +++++++++--------
tools/libxl/libxl_save_callout.c | 4 ++--
tools/libxl/libxl_utils.c | 2 ++
17 files changed, 113 insertions(+), 42 deletions(-)
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 424a7ee..7d1ebf9 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -13,6 +13,8 @@ XLUMINOR = 0
CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
-Wno-declaration-after-statement -Wformat-nonliteral
+CFLAGS_FOR_LIBXL = -Wunused-parameter
+
CFLAGS += -I. -fPIC
ifeq ($(CONFIG_Linux),y)
@@ -71,7 +73,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
-$(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl) -include $(XEN_ROOT)/tools/config.h
+$(LIBXL_OBJS): CFLAGS += $(CFLAGS_FOR_LIBXL) $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl) -include $(XEN_ROOT)/tools/config.h
AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
_libxl_save_msgs_callout.h _libxl_save_msgs_helper.h
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 00ddc0e..3ad83ea 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -28,6 +28,8 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
struct stat stat_buf;
int rc;
+ USE(flags);
+
if (version != LIBXL_VERSION) { rc = ERROR_VERSION; goto out; }
ctx = malloc(sizeof(*ctx));
@@ -700,6 +702,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
libxl__domain_suspend_state *dss;
int rc;
+ USE(recv_fd); /* TODO get rid of this and actually use it! */
+
libxl_domain_type type = libxl__domain_type(gc, domid);
if (type == LIBXL_DOMAIN_TYPE_INVALID) {
rc = ERROR_FAIL;
@@ -979,8 +983,9 @@ static void domain_death_occurred(libxl__egc *egc,
LIBXL_TAILQ_INSERT_HEAD(&CTX->death_reported, evg, entry);
}
-static void domain_death_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
- const char *wpath, const char *epath) {
+static void domain_death_xswatch_callback(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, watch, wpath, epath))
+{
EGC_GC;
libxl_evgen_domain_death *evg;
uint32_t domid;
@@ -1137,8 +1142,9 @@ void libxl_evdisable_domain_death(libxl_ctx *ctx,
GC_FREE;
}
-static void disk_eject_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch *w,
- const char *wpath, const char *epath) {
+static void disk_eject_xswatch_callback(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, w, wpath, epath))
+{
EGC_GC;
libxl_evgen_disk_eject *evg = (void*)w;
char *backend;
@@ -2524,6 +2530,8 @@ static int libxl__device_from_nic(libxl__gc *gc, uint32_t domid,
libxl_device_nic *nic,
libxl__device *device)
{
+ USE(gc);
+
device->backend_devid = nic->devid;
device->backend_domid = nic->backend_domid;
device->backend_kind = LIBXL__DEVICE_KIND_VIF;
@@ -2902,6 +2910,9 @@ out:
int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb)
{
+ USE(gc);
+
+ USE(vkb);
return 0;
}
@@ -2909,6 +2920,7 @@ static int libxl__device_from_vkb(libxl__gc *gc, uint32_t domid,
libxl_device_vkb *vkb,
libxl__device *device)
{
+ USE(gc);
device->backend_devid = vkb->devid;
device->backend_domid = vkb->backend_domid;
device->backend_kind = LIBXL__DEVICE_KIND_VKBD;
@@ -2990,6 +3002,7 @@ out:
int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb)
{
+ USE(gc);
libxl_defbool_setdefault(&vfb->vnc.enable, true);
if (libxl_defbool_val(vfb->vnc.enable)) {
if (!vfb->vnc.listen) {
@@ -3010,6 +3023,7 @@ static int libxl__device_from_vfb(libxl__gc *gc, uint32_t domid,
libxl_device_vfb *vfb,
libxl__device *device)
{
+ USE(gc);
device->backend_devid = vfb->devid;
device->backend_domid = vfb->backend_domid;
device->backend_kind = LIBXL__DEVICE_KIND_VFB;
@@ -3936,8 +3950,13 @@ libxl_scheduler libxl_get_scheduler(libxl_ctx *ctx)
static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid,
const libxl_domain_sched_params *scinfo)
{
+ USE(gc);
+
/* Currently, the ARINC 653 scheduler does not take any domain-specific
configuration, so we simply return success. */
+ USE(domid);
+ USE(scinfo);
+
return 0;
}
@@ -4385,6 +4404,8 @@ int libxl_xen_console_read_line(libxl_ctx *ctx,
void libxl_xen_console_read_finish(libxl_ctx *ctx,
libxl_xen_console_reader *cr)
{
+ USE(ctx);
+
free(cr->buffer);
free(cr);
}
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 4bd5484..e91dc9c 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -114,8 +114,8 @@ static int datacopier_pollhup_handled(libxl__egc *egc,
return 0;
}
-static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
- int fd, short events, short revents) {
+static void datacopier_readable(EV_FD_CALLBACK_PARAMS
+ (egc, ev, fd, events, revents)) {
libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
STATE_AO_GC(dc->ao);
@@ -178,8 +178,8 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
datacopier_check_state(egc, dc);
}
-static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
- int fd, short events, short revents) {
+static void datacopier_writable(EV_FD_CALLBACK_PARAMS(
+ egc, ev, fd, events, revents)) {
libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, towrite);
STATE_AO_GC(dc->ao);
diff --git a/tools/libxl/libxl_blktap2.c b/tools/libxl/libxl_blktap2.c
index 2c40182..660a669 100644
--- a/tools/libxl/libxl_blktap2.c
+++ b/tools/libxl/libxl_blktap2.c
@@ -19,6 +19,7 @@
int libxl__blktap_enabled(libxl__gc *gc)
{
+ USE(gc);
const char *msg;
return !tap_ctl_check(&msg);
}
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 2e65383..bfae328 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -88,6 +88,7 @@ static void make_bootloader_args(libxl__gc *gc, libxl__bootloader_state *bl,
static int setup_xenconsoled_pty(libxl__egc *egc, libxl__bootloader_state *bl,
char *slave_path, size_t slave_path_len)
{
+ USE(egc);
STATE_AO_GC(bl->ao);
struct termios termattr;
int r, rc;
@@ -141,6 +142,7 @@ static const char *bootloader_result_command(libxl__gc *gc, const char *buf,
static int parse_bootloader_result(libxl__egc *egc,
libxl__bootloader_state *bl)
{
+ USE(egc);
STATE_AO_GC(bl->ao);
char buf[PATH_MAX*2];
FILE *f = 0;
@@ -221,6 +223,7 @@ void libxl__bootloader_init(libxl__bootloader_state *bl)
static void bootloader_cleanup(libxl__egc *egc, libxl__bootloader_state *bl)
{
+ USE(egc);
STATE_AO_GC(bl->ao);
int i;
@@ -256,6 +259,8 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
int rc)
{
+ USE(egc);
+
if (!bl->rc)
bl->rc = rc;
@@ -285,6 +290,7 @@ static void bootloader_local_detached_cb(libxl__egc *egc,
static void bootloader_stop(libxl__egc *egc,
libxl__bootloader_state *bl, int rc)
{
+ USE(egc);
STATE_AO_GC(bl->ao);
int r;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 5f0d26f..5d56d67 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -62,6 +62,8 @@ void libxl_domain_config_dispose(libxl_domain_config *d_config)
int libxl__domain_create_info_setdefault(libxl__gc *gc,
libxl_domain_create_info *c_info)
{
+ USE(gc);
+
if (!c_info->type)
return ERROR_INVAL;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 84fa06c..85df199 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -44,6 +44,8 @@ int libxl__parse_backend_path(libxl__gc *gc,
const char *path,
libxl__device *dev)
{
+ USE(gc);
+
/* /local/domain/<domid>/backend/<kind>/<domid>/<devid> */
char strkind[16]; /* Longest is actually "console" */
int rc = sscanf(path, "/local/domain/%d/backend/%15[^/]/%u/%d",
@@ -795,8 +797,7 @@ out:
return;
}
-static void device_qemu_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+static void device_qemu_timeout(EV_TIME_CALLBACK_PARAMS(egc, ev, requested_abs))
{
libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
STATE_AO_GC(aodev->ao);
@@ -918,8 +919,8 @@ out:
return;
}
-static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+static void device_hotplug_timeout_cb(EV_TIME_CALLBACK_PARAMS
+ (egc, ev, requested_abs))
{
libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
STATE_AO_GC(aodev->ao);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0c0084f..6995306 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -640,6 +640,7 @@ static int libxl__vfb_and_vkb_from_hvm_guest_config(libxl__gc *gc,
libxl_device_vfb *vfb,
libxl_device_vkb *vkb)
{
+ USE(gc);
const libxl_domain_build_info *b_info = &guest_config->b_info;
if (b_info->type != LIBXL_DOMAIN_TYPE_HVM)
@@ -1177,6 +1178,7 @@ static void device_model_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
{
libxl__dm_spawn_state *dmss = CONTAINER_OF(spawn, *dmss, spawn);
STATE_AO_GC(spawn->ao);
+ USE(egc);
if (!xsdata)
return;
@@ -1262,6 +1264,7 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
int nr_vfbs, libxl_device_vfb *vfbs,
int nr_disks, libxl_device_disk *disks)
{
+ USE(gc);
int i, ret = 0;
/*
@@ -1283,6 +1286,7 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
}
if (nr_vfbs > 0) {
+ USE(vfbs);
ret = 1;
goto out;
}
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 06d5e4f..4a36652 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -756,8 +756,8 @@ void libxl__domain_suspend_common_switch_qemu_logdirty
switch_logdirty_done(egc,dss,-1);
}
-static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+static void switch_logdirty_timeout(EV_TIME_CALLBACK_PARAMS
+ (egc, ev, requested_abs))
{
libxl__domain_suspend_state *dss = CONTAINER_OF(ev, *dss, logdirty.timeout);
STATE_AO_GC(dss->ao);
@@ -765,8 +765,8 @@ static void switch_logdirty_timeout(libxl__egc *egc, libxl__ev_time *ev,
switch_logdirty_done(egc,dss,-1);
}
-static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch *watch,
- const char *watch_path, const char *event_path)
+static void switch_logdirty_xswatch(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, watch, wpath, epath))
{
libxl__domain_suspend_state *dss =
CONTAINER_OF(watch, *dss, logdirty.watch);
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 1af64c8..296303c 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -197,6 +197,8 @@ static void time_done_debug(libxl__gc *gc, const char *func,
"ev_time=%p done rc=%d .func=%p infinite=%d abs=%lu.%06lu",
ev, rc, ev->func, ev->infinite,
(unsigned long)ev->abs.tv_sec, (unsigned long)ev->abs.tv_usec);
+#else
+ USE(gc); USE(func); USE(ev); USE(rc);
#endif
}
@@ -381,8 +383,7 @@ static void libxl__set_watch_slot_contents(libxl__ev_watch_slot *slot,
slot->empty.sle_next = (void*)w;
}
-static void watchfd_callback(libxl__egc *egc, libxl__ev_fd *ev,
- int fd, short events, short revents)
+static void watchfd_callback(EV_FD_CALLBACK_PARAMS(egc,ev, fd,events,revents))
{
EGC_GC;
@@ -571,8 +572,8 @@ void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch *w)
* waiting for device state
*/
-static void devstate_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
- const char *watch_path, const char *event_path)
+static void devstate_watch_callback(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, watch, watch_path, event_path))
{
EGC_GC;
libxl__ev_devstate *ds = CONTAINER_OF(watch, *ds, watch);
@@ -605,8 +606,7 @@ static void devstate_watch_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
ds->callback(egc, ds, rc);
}
-static void devstate_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+static void devstate_timeout(EV_TIME_CALLBACK_PARAMS(egc, ev, requested_abs))
{
EGC_GC;
libxl__ev_devstate *ds = CONTAINER_OF(ev, *ds, timeout);
@@ -662,8 +662,8 @@ int libxl__ev_devstate_wait(libxl__gc *gc, libxl__ev_devstate *ds,
* futile.
*/
-static void domaindeathcheck_callback(libxl__egc *egc, libxl__ev_xswatch *w,
- const char *watch_path, const char *event_path)
+static void domaindeathcheck_callback(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, w, watch_path, event_path))
{
libxl__domaindeathcheck *dc = CONTAINER_OF(w, *dc, watch);
EGC_GC;
@@ -1019,6 +1019,7 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
assert(fd == ev->fd);
revents &= ev->events;
+ USE(events); /* we use our own idea of what we asked for */
if (revents)
ev->func(egc, ev, fd, ev->events, revents);
@@ -1151,6 +1152,8 @@ void libxl__event_occurred(libxl__egc *egc, libxl_event *event)
void libxl_event_free(libxl_ctx *ctx, libxl_event *event)
{
+ USE(ctx);
+
/* Exceptionally, this function may be called from libxl, with ctx==0 */
libxl_event_dispose(event);
free(event);
@@ -1645,7 +1648,8 @@ int libxl__ao_inprogress(libxl__ao *ao,
* for how. But we want to copy *how. So we have this dummy function
* whose address is stored in callback if the app passed how==NULL. */
static void dummy_asyncprogress_callback_ignore
- (libxl_ctx *ctx, libxl_event *ev, void *for_callback) { }
+ (libxl_ctx *ctx, libxl_event *ev, void *for_callback)
+ { USE(ctx); USE(ev); USE(for_callback); }
void libxl__ao_progress_gethow(libxl_asyncprogress_how *in_state,
const libxl_asyncprogress_how *from_app) {
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 0477386..ed6b44e 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -280,6 +280,7 @@ void libxl__spawn_init(libxl__spawn_state *ss)
int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
{
+ USE(egc);
STATE_AO_GC(ss->ao);
int r;
pid_t child;
@@ -387,8 +388,7 @@ static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss)
spawn_detach(gc, ss);
}
-static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
+static void spawn_timeout(EV_TIME_CALLBACK_PARAMS(egc, ev, requested_abs))
{
/* Before event, was Attached. */
EGC_GC;
@@ -397,8 +397,8 @@ static void spawn_timeout(libxl__egc *egc, libxl__ev_time *ev,
spawn_fail(egc, ss); /* must be last */
}
-static void spawn_watch_event(libxl__egc *egc, libxl__ev_xswatch *xsw,
- const char *watch_path, const char *event_path)
+static void spawn_watch_event(EV_XSWATCH_CALLBACK_PARAMS
+ (egc, xsw, wpath, epath))
{
/* On entry, is Attached. */
EGC_GC;
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 044ddad..0379604 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -157,6 +157,7 @@ int libxl__carefd_fd(const libxl__carefd *cf)
static void sigchld_handler(int signo)
{
+ USE(signo);
int e = libxl__self_pipe_wakeup(sigchld_owner->sigchld_selfpipe[1]);
assert(!e); /* errors are probably EBADF, very bad */
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9315ae0..2687382 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -95,6 +95,8 @@
#define DISABLE_UDEV_PATH "libxl/disable_udev"
#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
+#define USE(var) ((void)(var))
+#define MAYBE_UNUSED __attribute__((unused))
#define LIBXL__LOGGING_ENABLED
@@ -147,6 +149,14 @@ typedef void libxl__ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev,
* It is not permitted to listen for the same or overlapping events
* on the same fd using multiple different libxl__ev_fd's.
*/
+
+/* Declare your callback functions with this helper and you avoid unused
+ * parameter warnings (and don't have to list all the types either): */
+#define EV_FD_CALLBACK_PARAMS(egc, ev, fd, events, revents) \
+ libxl__egc *egc MAYBE_UNUSED, libxl__ev_fd *ev MAYBE_UNUSED, \
+ int fd MAYBE_UNUSED, \
+ short events MAYBE_UNUSED, short revents MAYBE_UNUSED
+
struct libxl__ev_fd {
/* caller should include this in their own struct */
/* read-only for caller, who may read only when registered: */
@@ -162,6 +172,11 @@ struct libxl__ev_fd {
typedef struct libxl__ev_time libxl__ev_time;
typedef void libxl__ev_time_callback(libxl__egc *egc, libxl__ev_time *ev,
const struct timeval *requested_abs);
+
+#define EV_TIME_CALLBACK_PARAMS(egc, ev, requested_abs) \
+ libxl__egc *egc MAYBE_UNUSED, libxl__ev_time *ev MAYBE_UNUSED, \
+ const struct timeval *requested_abs MAYBE_UNUSED
+
struct libxl__ev_time {
/* caller should include this in their own struct */
/* read-only for caller, who may read only when registered: */
@@ -174,8 +189,13 @@ struct libxl__ev_time {
};
typedef struct libxl__ev_xswatch libxl__ev_xswatch;
-typedef void libxl__ev_xswatch_callback(libxl__egc *egc, libxl__ev_xswatch*,
- const char *watch_path, const char *event_path);
+typedef void libxl__ev_xswatch_callback(libxl__egc *egc,
+ libxl__ev_xswatch *watch, const char *watch_path, const char *event_path);
+
+#define EV_XSWATCH_CALLBACK_PARAMS(egc, watch, wpath, epath) \
+ libxl__egc *egc MAYBE_UNUSED, libxl__ev_xswatch *watch MAYBE_UNUSED, \
+ const char *wpath MAYBE_UNUSED, const char *epath MAYBE_UNUSED
+
struct libxl__ev_xswatch {
/* caller should include this in their own struct */
/* read-only for caller, who may read only when registered: */
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 9c92ae6..a094965 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -800,6 +800,10 @@ static int pci_ins_check(libxl__gc *gc, uint32_t domid, const char *state, void
{
char *orig_state = priv;
+ USE(gc);
+ USE(domid);
+ USE(priv);
+
if ( !strcmp(state, "pci-insert-failed") )
return -1;
if ( !strcmp(state, "pci-inserted") )
@@ -1007,6 +1011,8 @@ static int libxl__device_pci_reset(libxl__gc *gc, unsigned int domain, unsigned
int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci *pci)
{
+ USE(gc);
+ USE(pci);
return 0;
}
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index e33b130..c354c71 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -46,6 +46,10 @@
typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
const libxl__json_object *tree,
void *opaque);
+#define QMP_CALLBACK_PARAMS(qmp, tree, opaque) \
+ libxl__qmp_handler *qmp MAYBE_UNUSED, \
+ const libxl__json_object *tree MAYBE_UNUSED, \
+ void *opaque MAYBE_UNUSED
typedef struct qmp_request_context {
int rc;
@@ -109,9 +113,7 @@ static int store_serial_port_info(libxl__qmp_handler *qmp,
return ret;
}
-static int register_serials_chardev_callback(libxl__qmp_handler *qmp,
- const libxl__json_object *o,
- void *unused)
+static int register_serials_chardev_callback(QMP_CALLBACK_PARAMS(qmp,o,unused))
{
const libxl__json_object *obj = NULL;
const libxl__json_object *label = NULL;
@@ -165,9 +167,7 @@ static int qmp_write_domain_console_item(libxl__gc *gc, int domid,
return libxl__xs_write(gc, XBT_NULL, path, "%s", value);
}
-static int qmp_register_vnc_callback(libxl__qmp_handler *qmp,
- const libxl__json_object *o,
- void *unused)
+static int qmp_register_vnc_callback(QMP_CALLBACK_PARAMS(qmp,o,unused))
{
GC_INIT(qmp->ctx);
const libxl__json_object *obj;
@@ -203,8 +203,7 @@ out:
return rc;
}
-static int qmp_capabilities_callback(libxl__qmp_handler *qmp,
- const libxl__json_object *o, void *unused)
+static int qmp_capabilities_callback(QMP_CALLBACK_PARAMS(qmp,o,unused))
{
qmp->connected = true;
@@ -228,6 +227,8 @@ static int enable_qmp_capabilities(libxl__qmp_handler *qmp)
static libxl__qmp_message_type qmp_response_type(libxl__qmp_handler *qmp,
const libxl__json_object *o)
{
+ USE(qmp);
+
libxl__qmp_message_type type;
libxl__json_map_node *node = NULL;
int i = 0;
diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
index 078b7ee..78bb67e 100644
--- a/tools/libxl/libxl_save_callout.c
+++ b/tools/libxl/libxl_save_callout.c
@@ -252,8 +252,8 @@ static void helper_failed(libxl__egc *egc, libxl__save_helper_state *shs,
(unsigned long)shs->child.pid);
}
-static void helper_stdout_readable(libxl__egc *egc, libxl__ev_fd *ev,
- int fd, short events, short revents)
+static void helper_stdout_readable(EV_FD_CALLBACK_PARAMS
+ (egc, ev, fd, events, revents))
{
libxl__save_helper_state *shs = CONTAINER_OF(ev, *shs, readable);
STATE_AO_GC(shs->ao);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f7b44a0..a7c34a9 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -230,6 +230,7 @@ out:
int libxl_string_to_backend(libxl_ctx *ctx, char *s, libxl_disk_backend *backend)
{
+ USE(ctx);
char *p;
int rc = 0;
@@ -513,6 +514,7 @@ void libxl_bitmap_dispose(libxl_bitmap *map)
void libxl_bitmap_copy(libxl_ctx *ctx, libxl_bitmap *dptr,
const libxl_bitmap *sptr)
{
+ USE(ctx);
int sz;
assert(dptr->size == sptr->size);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 11/11] libxl: -Wunused-parameter
2012-08-01 16:24 ` [PATCH 11/11] libxl: -Wunused-parameter Ian Jackson
@ 2012-08-02 8:59 ` Ian Campbell
2012-08-02 14:09 ` Ian Jackson
2012-08-02 10:41 ` Ian Campbell
1 sibling, 1 reply; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 8:59 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
>
> * The autogenerated function libxl_event_init_type ignores the type
> parameter.
Your wish etc...
8<------------------------------------------
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1343897835 -3600
# Node ID 5bbb555747204f9b1926741416f79ab6b8b02361
# Parent 5feb45a76581091bd267eecccb078afb91db0b8c
libxl: idl: always initialise the KeyedEnum keyvar in the member init function
Previously we only initialised it if an explicit keyvar_init_val was given but
not if the default was implicitly 0.
In the generated code this only changes the unused libxl_event_init_type
function:
void libxl_event_init_type(libxl_event *p, libxl_event_type type)
{
+ assert(!p->type);
+ p->type = type;
switch (p->type) {
case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
break;
However I think it is wrong that this function is unused, this and
libxl_event_init should be used by libxl__event_new. As it happens both are
just memset to zero but for correctness we should use the init functions (in
case the IDL changes).
In the generator we also need to properly handle init_var == 0 which the
current if statements incorrectly treat as False. This doesn't actually have
any impact on the generated code.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 5feb45a76581 -r 5bbb55574720 tools/libxl/gentypes.py
--- a/tools/libxl/gentypes.py Thu Aug 02 09:26:36 2012 +0100
+++ b/tools/libxl/gentypes.py Thu Aug 02 09:57:15 2012 +0100
@@ -162,17 +162,20 @@ def libxl_C_type_member_init(ty, field):
ku.keyvar.type.make_arg(ku.keyvar.name))
s += "{\n"
- if ku.keyvar.init_val:
+ if ku.keyvar.init_val is not None:
init_val = ku.keyvar.init_val
- elif ku.keyvar.type.init_val:
+ elif ku.keyvar.type.init_val is not None:
init_val = ku.keyvar.type.init_val
else:
init_val = None
+ (nparent,fexpr) = ty.member(ty.pass_arg("p"), ku.keyvar, isref=True)
if init_val is not None:
- (nparent,fexpr) = ty.member(ty.pass_arg("p"), ku.keyvar, isref=True)
s += " assert(%s == %s);\n" % (fexpr, init_val)
- s += " %s = %s;\n" % (fexpr, ku.keyvar.name)
+ else:
+ s += " assert(!%s);\n" % (fexpr)
+ s += " %s = %s;\n" % (fexpr, ku.keyvar.name)
+
(nparent,fexpr) = ty.member(ty.pass_arg("p"), field, isref=True)
s += _libxl_C_type_init(ku, fexpr, parent=nparent, subinit=True)
s += "}\n"
diff -r 5feb45a76581 -r 5bbb55574720 tools/libxl/libxl_event.c
--- a/tools/libxl/libxl_event.c Thu Aug 02 09:26:36 2012 +0100
+++ b/tools/libxl/libxl_event.c Thu Aug 02 09:57:15 2012 +0100
@@ -1163,7 +1163,10 @@ libxl_event *libxl__event_new(libxl__egc
libxl_event *ev;
ev = libxl__zalloc(NOGC,sizeof(*ev));
- ev->type = type;
+
+ libxl_event_init(ev);
+ libxl_event_init_type(ev, type);
+
ev->domid = domid;
return ev;
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 11/11] libxl: -Wunused-parameter
2012-08-01 16:24 ` [PATCH 11/11] libxl: -Wunused-parameter Ian Jackson
2012-08-02 8:59 ` Ian Campbell
@ 2012-08-02 10:41 ` Ian Campbell
2012-08-02 14:32 ` Ian Jackson
1 sibling, 1 reply; 28+ messages in thread
From: Ian Campbell @ 2012-08-02 10:41 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel@lists.xen.org
On Wed, 2012-08-01 at 17:24 +0100, Ian Jackson wrote:
> *** DO NOT APPLY ***
>
> We have recently had a couple of bugs which basically involved
> ignoring the rc parameter to a callback function. I thought I would
> try -Wunused-parameter. Here are the results.
>
> I found three further problems:
>
> * libxl_wait_for_free_memory takes a domid parameter but its
> semantics don't seem to call for that. This function is
> going to have a big warning put on it for 4.2 and that
> should happen soon.
>
> * qmp_synchronous_send has an `ask_timeout' parameter which is
> ignored.
>
> * The autogenerated function libxl_event_init_type ignores the type
> parameter.
>
> Things I needed to do to get the rest of the code to compile:
>
> * Remove one harmless unused parameter from an internal function.
> (Earlier in this series.)
>
> * Add an assert to make the error handling in the broken remus code
> slightly less broken. (Earlier in this series.)
>
> * Provide machinery in the Makefile for passing different CFLAGS to
> libxl as opposed to xl and libxlu. The flex- and bison-generated
> files in libxlu can't be compiled with -Wunused-parameter.
>
> * Define a new helper macro
> #define USE(var) ((void)(var))
> and use it 43 times. The pattern is something like
> USE(egc);
> in a function which takes egc but doesn't need it. If the
> parameter is later used, this is harmless. In functions
> which are placeholders the USE statement should be placed in the
> middle of the function where the parameter would be used if the
> function is changed later, so that the USE gets deleted by the
> patch introducing the implementation.
>
> * Define a new helper macro for use only in other macros
> #define MAYBE_UNUSED __attribute__((unused))
> and use it in 10 different places.
>
> * Define new macros for helping declare common types of callback
> functions. For example:
>
> #define EV_XSWATCH_CALLBACK_PARAMS(egc, watch, wpath, epath) \
> libxl__egc *egc MAYBE_UNUSED, \
> libxl__ev_xswatch *watch MAYBE_UNUSED, \
> const char *wpath MAYBE_UNUSED, \
> const char *epath MAYBE_UNUSED
>
> which is used like this:
>
> -static void some_callback(libxl__egc *egc, libxl__ev_xswatch *watch,
> - const char *wpath, const char *epath)
> +static void some_callback(EV_XSWATCH_CALLBACK_PARAMS
> + (egc, watch, wpath, epath))
> {
> ... now we use (or not) egc, watch, wpath, etc. or not as we like
>
> This somewhat resembles a Traditional K&R C typeless function
> definition. The types of the parameters are actually defined
> for the compiler of course, along with the information that
> the parameters might be unused.
>
> There are 4 macros of this kind with 22 call sites.
>
> IMO on the cost (65 places in ordinary code where we have to write
> something somewhat ugly) is worth the benefit (finding, if we had
> deployed this right away, around 6 bugs). But it's arguable.
>
> *** DO NOT APPLY ***
>
> Anyway, this patch must not be applied right now because it causes the
> build to fail.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>
> ---
> tools/libxl/Makefile | 4 +++-
> tools/libxl/libxl.c | 29 +++++++++++++++++++++++++----
> tools/libxl/libxl_aoutils.c | 8 ++++----
> tools/libxl/libxl_blktap2.c | 1 +
> tools/libxl/libxl_bootloader.c | 6 ++++++
> tools/libxl/libxl_create.c | 2 ++
> tools/libxl/libxl_device.c | 9 +++++----
> tools/libxl/libxl_dm.c | 4 ++++
> tools/libxl/libxl_dom.c | 8 ++++----
> tools/libxl/libxl_event.c | 22 +++++++++++++---------
> tools/libxl/libxl_exec.c | 8 ++++----
> tools/libxl/libxl_fork.c | 1 +
> tools/libxl/libxl_internal.h | 24 ++++++++++++++++++++++--
> tools/libxl/libxl_pci.c | 6 ++++++
> tools/libxl/libxl_qmp.c | 17 +++++++++--------
> tools/libxl/libxl_save_callout.c | 4 ++--
> tools/libxl/libxl_utils.c | 2 ++
> 17 files changed, 113 insertions(+), 42 deletions(-)
I'm not entirely sure how I feel about this patch generally (it's quite
a bit of mess, but the bugs it would have found are real). It's also
quite a lot of churn for 4.2.
On the other hand we are likely to want to backport lots of libxl fixes
for 4.2.1 (I was actually considering an exception to the "no new
features" rule for 4.2.1 for xm parity causing patches) and having this
in 4.2 would make that cleaner.
I guess I come down (just) on the side of taking this, when it is baked.
> @@ -700,6 +702,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
> libxl__domain_suspend_state *dss;
> int rc;
>
> + USE(recv_fd); /* TODO get rid of this and actually use it! */
You've only just introduced TODO REMUS...
> @@ -1019,6 +1019,7 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
>
> assert(fd == ev->fd);
> revents &= ev->events;
> + USE(events); /* we use our own idea of what we asked for */
What is the point of this argument then?
Is getting an event we weren't expecting a log-worthy occurrence?
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index 9c92ae6..a094965 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -800,6 +800,10 @@ static int pci_ins_check(libxl__gc *gc, uint32_t domid, const char *state, void
> {
> char *orig_state = priv;
>
> + USE(gc);
> + USE(domid);
> + USE(priv);
You actually use priv above.
Ian.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 11/11] libxl: -Wunused-parameter
2012-08-02 10:41 ` Ian Campbell
@ 2012-08-02 14:32 ` Ian Jackson
0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2012-08-02 14:32 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xen.org
Ian Campbell writes ("Re: [PATCH 11/11] libxl: -Wunused-parameter"):
> I'm not entirely sure how I feel about this patch generally (it's quite
> a bit of mess, but the bugs it would have found are real). It's also
> quite a lot of churn for 4.2.
Yes.
> On the other hand we are likely to want to backport lots of libxl fixes
> for 4.2.1 (I was actually considering an exception to the "no new
> features" rule for 4.2.1 for xm parity causing patches) and having this
> in 4.2 would make that cleaner.
Indeed.
> I guess I come down (just) on the side of taking this, when it is baked.
OK. Having slept on it I think overall this is an improvement and
will help us in the future.
> > @@ -700,6 +702,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
> > libxl__domain_suspend_state *dss;
> > int rc;
> >
> > + USE(recv_fd); /* TODO get rid of this and actually use it! */
>
> You've only just introduced TODO REMUS...
Point.
> > @@ -1019,6 +1019,7 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
> >
> > assert(fd == ev->fd);
> > revents &= ev->events;
> > + USE(events); /* we use our own idea of what we asked for */
>
> What is the point of this argument then?
It's there for consistency with poll(2)'s API.
> Is getting an event we weren't expecting a log-worthy occurrence?
events might be different from those we requested because events might
be "in flight" from the application's call to poll, to us, while we
register/deregister them. So this is not a logworthy event. I guess
this property of the registration API is not documented and should be
(although it amounts only to a relaxation from the point of view of
the application).
Also I found a mistake in a related comment so I will fix these
comments in another patch.
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 9c92ae6..a094965 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -800,6 +800,10 @@ static int pci_ins_check(libxl__gc *gc, uint32_t domid, const char *state, void
> > {
> > char *orig_state = priv;
> >
> > + USE(gc);
> > + USE(domid);
> > + USE(priv);
>
> You actually use priv above.
Fixed.
Ian.
^ permalink raw reply [flat|nested] 28+ messages in thread