* [PATCH] libxl: introduce asynchronous execution API
@ 2014-04-25 5:26 Yang Hongyang
2014-04-30 14:40 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Yang Hongyang @ 2014-04-25 5:26 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, wency, stefano.stabellini, andrew.cooper3,
yunhong.jiang, ian.jackson, eddie.dong, rshriram, roger.pau,
laijs
1.introduce asynchronous execution API:
libxl__async_exec_init
libxl__async_exec_start
libxl__async_exec_inuse
2.use the async exec API to execute device hotplug scripts
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
---
tools/libxl/libxl_aoutils.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_device.c | 78 +++++++++++---------------------------
tools/libxl/libxl_internal.h | 32 ++++++++++++++--
3 files changed, 139 insertions(+), 60 deletions(-)
diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 1c9eb9e..6215a3d 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -451,3 +451,92 @@ int libxl__openptys(libxl__openpty_state *op,
return rc;
}
+static void async_exec_timeout(libxl__egc *egc,
+ libxl__ev_time *ev,
+ const struct timeval *requested_abs)
+{
+ libxl__async_exec_state *aes = CONTAINER_OF(ev, *aes, time);
+ STATE_AO_GC(aes->ao);
+
+ libxl__ev_time_deregister(gc, &aes->time);
+
+ assert(libxl__ev_child_inuse(&aes->child));
+ LOG(DEBUG, "killing execution of %s because of timeout", aes->what);
+
+ if (kill(aes->child.pid, SIGKILL)) {
+ LOGEV(ERROR, errno, "unable to kill %s [%ld]",
+ aes->what, (unsigned long)aes->child.pid);
+ }
+
+ return;
+}
+
+static void async_exec_done(libxl__egc *egc,
+ libxl__ev_child *child,
+ pid_t pid, int status)
+{
+ libxl__async_exec_state *aes = CONTAINER_OF(child, *aes, child);
+ STATE_AO_GC(aes->ao);
+
+ libxl__ev_time_deregister(gc, &aes->time);
+
+ if (status) {
+ libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
+ aes->what, pid, status);
+ }
+
+ aes->callback(egc, aes, status);
+}
+
+void libxl__async_exec_init(libxl__async_exec_state *aes)
+{
+ libxl__ev_time_init(&aes->time);
+ libxl__ev_child_init(&aes->child);
+}
+
+int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
+{
+ pid_t pid;
+
+ /* Convenience aliases */
+ libxl__ev_child *const child = &aes->child;
+ char ** const args = aes->args;
+
+ /* Set execution timeout */
+ if (libxl__ev_time_register_rel(gc, &aes->time,
+ async_exec_timeout,
+ aes->timeout_ms)) {
+ LOG(ERROR, "unable to register timeout for executing: %s", aes->what);
+ goto errout;
+ }
+
+ LOG(DEBUG, "asynchronously execute: %s ", aes->what);
+
+ /* Fork and exec */
+ pid = libxl__ev_child_fork(gc, child, async_exec_done);
+ if (pid == -1) {
+ LOG(ERROR, "unable to fork");
+ goto errout;
+ }
+
+ if (!pid) {
+ /* child */
+ libxl__exec(gc, aes->stdfds[0], aes->stdfds[1],
+ aes->stdfds[2], args[0], args, aes->env);
+ /* notreached */
+ abort();
+ }
+
+ return 0;
+
+errout:
+ return ERROR_FAIL;
+}
+
+bool libxl__async_exec_inuse(const libxl__async_exec_state *aes)
+{
+ bool time_inuse = libxl__ev_time_isregistered(&aes->time);
+ bool child_inuse = libxl__ev_child_inuse(&aes->child);
+ assert(time_inuse == child_inuse);
+ return child_inuse;
+}
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index fa99f77..90ae564 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -430,7 +430,7 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
aodev->rc = 0;
aodev->dev = NULL;
aodev->num_exec = 0;
- /* Initialize timer for QEMU Bodge and hotplug execution */
+ /* Initialize timer for QEMU Bodge */
libxl__ev_time_init(&aodev->timeout);
/*
* Initialize xs_watch, because it's not used on all possible
@@ -440,7 +440,7 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
aodev->active = 1;
/* We init this here because we might call device_hotplug_done
* without actually calling any hotplug script */
- libxl__ev_child_init(&aodev->child);
+ libxl__async_exec_init(&aodev->aes);
}
/* multidev */
@@ -707,12 +707,9 @@ static void device_backend_cleanup(libxl__gc *gc,
static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
-static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs);
-
static void device_hotplug_child_death_cb(libxl__egc *egc,
- libxl__ev_child *child,
- pid_t pid, int status);
+ libxl__async_exec_state *aes,
+ int status);
static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
const struct timeval *requested_abs);
@@ -953,11 +950,11 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
{
STATE_AO_GC(aodev->ao);
+ libxl__async_exec_state *aes = &aodev->aes;
char *be_path = libxl__device_backend_path(gc, aodev->dev);
char **args = NULL, **env = NULL;
int rc = 0;
int hotplug, nullfd = -1;
- pid_t pid;
uint32_t domid;
/*
@@ -1009,16 +1006,6 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
goto out;
}
- /* Set hotplug timeout */
- rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
- device_hotplug_timeout_cb,
- LIBXL_HOTPLUG_TIMEOUT * 1000);
- if (rc) {
- LOG(ERROR, "unable to register timeout for hotplug device %s", be_path);
- goto out;
- }
-
- aodev->what = GCSPRINTF("%s %s", args[0], args[1]);
LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
nullfd = open("/dev/null", O_RDONLY);
@@ -1028,23 +1015,22 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
goto out;
}
- /* fork and execute hotplug script */
- pid = libxl__ev_child_fork(gc, &aodev->child, device_hotplug_child_death_cb);
- if (pid == -1) {
- LOG(ERROR, "unable to fork");
- rc = ERROR_FAIL;
+ aes->ao = ao;
+ aes->what = GCSPRINTF("%s %s", args[0], args[1]);
+ aes->env = env;
+ aes->args = args;
+ aes->callback = device_hotplug_child_death_cb;
+ aes->timeout_ms = LIBXL_HOTPLUG_TIMEOUT * 1000;
+ aes->stdfds[0] = nullfd;
+ aes->stdfds[1] = 2;
+ aes->stdfds[2] = -1;
+
+ rc = libxl__async_exec_start(gc, aes);
+ if (rc)
goto out;
- }
-
- if (!pid) {
- /* child */
- libxl__exec(gc, nullfd, 2, -1, args[0], args, env);
- /* notreached */
- abort();
- }
close(nullfd);
- assert(libxl__ev_child_inuse(&aodev->child));
+ assert(libxl__async_exec_inuse(&aodev->aes));
return;
@@ -1055,29 +1041,11 @@ out:
return;
}
-static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
- const struct timeval *requested_abs)
-{
- libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
- STATE_AO_GC(aodev->ao);
-
- libxl__ev_time_deregister(gc, &aodev->timeout);
-
- assert(libxl__ev_child_inuse(&aodev->child));
- LOG(DEBUG, "killing hotplug script %s because of timeout", aodev->what);
- if (kill(aodev->child.pid, SIGKILL)) {
- LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]",
- aodev->what, (unsigned long)aodev->child.pid);
- }
-
- return;
-}
-
static void device_hotplug_child_death_cb(libxl__egc *egc,
- libxl__ev_child *child,
- pid_t pid, int status)
+ libxl__async_exec_state *aes,
+ int status)
{
- libxl__ao_device *aodev = CONTAINER_OF(child, *aodev, child);
+ libxl__ao_device *aodev = CONTAINER_OF(aes, *aodev, aes);
STATE_AO_GC(aodev->ao);
char *be_path = libxl__device_backend_path(gc, aodev->dev);
char *hotplug_error;
@@ -1085,8 +1053,6 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
device_hotplug_clean(gc, aodev);
if (status) {
- libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
- aodev->what, pid, status);
hotplug_error = libxl__xs_read(gc, XBT_NULL,
GCSPRINTF("%s/hotplug-error", be_path));
if (hotplug_error)
@@ -1178,7 +1144,7 @@ static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev)
/* Clean events and check reentrancy */
libxl__ev_time_deregister(gc, &aodev->timeout);
libxl__ev_xswatch_deregister(gc, &aodev->xs_watch);
- assert(!libxl__ev_child_inuse(&aodev->child));
+ assert(!libxl__async_exec_inuse(&aodev->aes));
}
static void devices_remove_callback(libxl__egc *egc,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c2b73c4..3b9acc5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2030,6 +2030,31 @@ _hidden const char *libxl__xen_script_dir_path(void);
_hidden const char *libxl__lock_dir_path(void);
_hidden const char *libxl__run_dir_path(void);
+/*----- asynchronous function -----*/
+typedef struct libxl__async_exec_state libxl__async_exec_state;
+
+typedef void libxl__async_exec_callback(libxl__egc *egc,
+ libxl__async_exec_state *aes, int status);
+
+struct libxl__async_exec_state {
+ /* caller must fill these in */
+ libxl__ao *ao;
+ const char *what; /* for error msgs, what we're execute */
+ char **env; /* execution environment */
+ char **args; /* execution arguments */
+ libxl__async_exec_callback *callback;
+ int timeout_ms;
+ int stdfds[3];
+
+ /* private */
+ libxl__ev_time time;
+ libxl__ev_child child;
+};
+
+void libxl__async_exec_init(libxl__async_exec_state *aes);
+int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes);
+bool libxl__async_exec_inuse(const libxl__async_exec_state *aes);
+
/*----- device addition/removal -----*/
typedef struct libxl__ao_device libxl__ao_device;
@@ -2066,14 +2091,13 @@ struct libxl__ao_device {
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 */
+ /* Bodge for Qemu devices */
libxl__ev_time timeout;
/* xenstore watch for backend path of driver domains */
libxl__ev_xswatch xs_watch;
- /* device hotplug execution */
- const char *what;
int num_exec;
- libxl__ev_child child;
+ /* for calling hotplug scripts */
+ libxl__async_exec_state aes;
};
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] libxl: introduce asynchronous execution API
2014-04-25 5:26 [PATCH] libxl: introduce asynchronous execution API Yang Hongyang
@ 2014-04-30 14:40 ` Roger Pau Monné
2014-05-02 16:23 ` [PATCH] libxl: introduce asynchronous execution API [and 1 more messages] Ian Jackson
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monné @ 2014-04-30 14:40 UTC (permalink / raw)
To: Yang Hongyang, xen-devel
Cc: ian.campbell, wency, stefano.stabellini, andrew.cooper3,
yunhong.jiang, ian.jackson, eddie.dong, rshriram, laijs
On 25/04/14 07:26, Yang Hongyang wrote:
> 1.introduce asynchronous execution API:
> libxl__async_exec_init
> libxl__async_exec_start
> libxl__async_exec_inuse
> 2.use the async exec API to execute device hotplug scripts
Thanks, the patch looks fine to me, just a couple of comments on style
nits (but it can be applied as-is).
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> ---
> tools/libxl/libxl_aoutils.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_device.c | 78 +++++++++++---------------------------
> tools/libxl/libxl_internal.h | 32 ++++++++++++++--
> 3 files changed, 139 insertions(+), 60 deletions(-)
>
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 1c9eb9e..6215a3d 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -451,3 +451,92 @@ int libxl__openptys(libxl__openpty_state *op,
> return rc;
> }
>
> +static void async_exec_timeout(libxl__egc *egc,
> + libxl__ev_time *ev,
> + const struct timeval *requested_abs)
> +{
> + libxl__async_exec_state *aes = CONTAINER_OF(ev, *aes, time);
> + STATE_AO_GC(aes->ao);
> +
> + libxl__ev_time_deregister(gc, &aes->time);
> +
> + assert(libxl__ev_child_inuse(&aes->child));
> + LOG(DEBUG, "killing execution of %s because of timeout", aes->what);
I would make the message above an ERROR, rather than DEBUG (I know it's
a DEBUG message right now, but I think the message is relevant enough to
be printed as an error).
> +
> + if (kill(aes->child.pid, SIGKILL)) {
> + LOGEV(ERROR, errno, "unable to kill %s [%ld]",
> + aes->what, (unsigned long)aes->child.pid);
> + }
> +
> + return;
> +}
> +
> +static void async_exec_done(libxl__egc *egc,
> + libxl__ev_child *child,
> + pid_t pid, int status)
> +{
> + libxl__async_exec_state *aes = CONTAINER_OF(child, *aes, child);
> + STATE_AO_GC(aes->ao);
> +
> + libxl__ev_time_deregister(gc, &aes->time);
> +
> + if (status) {
> + libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
> + aes->what, pid, status);
> + }
> +
> + aes->callback(egc, aes, status);
> +}
> +
> +void libxl__async_exec_init(libxl__async_exec_state *aes)
> +{
> + libxl__ev_time_init(&aes->time);
> + libxl__ev_child_init(&aes->child);
> +}
> +
> +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> +{
> + pid_t pid;
> +
> + /* Convenience aliases */
> + libxl__ev_child *const child = &aes->child;
> + char ** const args = aes->args;
> +
> + /* Set execution timeout */
> + if (libxl__ev_time_register_rel(gc, &aes->time,
> + async_exec_timeout,
> + aes->timeout_ms)) {
> + LOG(ERROR, "unable to register timeout for executing: %s", aes->what);
> + goto errout;
> + }
> +
> + LOG(DEBUG, "asynchronously execute: %s ", aes->what);
> +
> + /* Fork and exec */
> + pid = libxl__ev_child_fork(gc, child, async_exec_done);
> + if (pid == -1) {
> + LOG(ERROR, "unable to fork");
> + goto errout;
> + }
> +
> + if (!pid) {
> + /* child */
> + libxl__exec(gc, aes->stdfds[0], aes->stdfds[1],
> + aes->stdfds[2], args[0], args, aes->env);
> + /* notreached */
> + abort();
> + }
> +
> + return 0;
> +
> +errout:
Those kind of labels are usually just called "error" inside of libxl.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] libxl: introduce asynchronous execution API [and 1 more messages]
2014-04-30 14:40 ` Roger Pau Monné
@ 2014-05-02 16:23 ` Ian Jackson
0 siblings, 0 replies; 3+ messages in thread
From: Ian Jackson @ 2014-05-02 16:23 UTC (permalink / raw)
To: Roger Pau Monné, Yang Hongyang
Cc: ian.campbell, wency, stefano.stabellini, andrew.cooper3,
yunhong.jiang, eddie.dong, xen-devel, rshriram, laijs
Yang Hongyang writes ("[PATCH] libxl: introduce asynchronous execution API"):
> 1.introduce asynchronous execution API:
> libxl__async_exec_init
> libxl__async_exec_start
> libxl__async_exec_inuse
> 2.use the async exec API to execute device hotplug scripts
Thanks, this is very good. Exactly what I meant.
I have only some very small nits to comment on:
> +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> +{
...
> + LOG(DEBUG, "asynchronously execute: %s ", aes->what);
I would write "forking to execute" in the message.
> static void devices_remove_callback(libxl__egc *egc,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c2b73c4..3b9acc5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2030,6 +2030,31 @@ _hidden const char *libxl__xen_script_dir_path(void);
> _hidden const char *libxl__lock_dir_path(void);
> _hidden const char *libxl__run_dir_path(void);
>
> +/*----- asynchronous function -----*/
> +typedef struct libxl__async_exec_state libxl__async_exec_state;
I think this comment needs a blank line after it, and a better name.
I suggest "subprocess execution with timeout".
> +typedef void libxl__async_exec_callback(libxl__egc *egc,
> + libxl__async_exec_state *aes, int status);
> +
> +struct libxl__async_exec_state {
> + /* caller must fill these in */
> + libxl__ao *ao;
> + const char *what; /* for error msgs, what we're execute */
"what we're executing"
> + char **env; /* execution environment */
> + char **args; /* execution arguments */
> + libxl__async_exec_callback *callback;
> + int timeout_ms;
> + int stdfds[3];
I would reorder these so that all the ones which are just the same as
arguments to libxl__exec are together. And then add a comment
/* caller must fill in; as for libxl__exec */
for that section.
Roger Pau Monné writes ("Re: [PATCH] libxl: introduce asynchronous execution API"):
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
....
> > diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> > index 1c9eb9e..6215a3d 100644
> > --- a/tools/libxl/libxl_aoutils.c
> > +++ b/tools/libxl/libxl_aoutils.c
...
> > +static void async_exec_timeout(libxl__egc *egc,
> > + libxl__ev_time *ev,
> > + const struct timeval *requested_abs)
> > +{
...
> > + assert(libxl__ev_child_inuse(&aes->child));
> > + LOG(DEBUG, "killing execution of %s because of timeout", aes->what);
>
> I would make the message above an ERROR, rather than DEBUG (I know it's
> a DEBUG message right now, but I think the message is relevant enough to
> be printed as an error).
I think I agree.
> > +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> > +{
...
> > + if (libxl__ev_time_register_rel(gc, &aes->time,
> > + async_exec_timeout,
> > + aes->timeout_ms)) {
> > + LOG(ERROR, "unable to register timeout for executing: %s", aes->what);
> > + goto errout;
...
> > +errout:
>
> Those kind of labels are usually just called "error" inside of libxl.
Actually, they are normally called "out". See eg
libxl__datacopier_start, libxl__openptys, in that same file.
It should be called "out" here too.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-05-02 16:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-25 5:26 [PATCH] libxl: introduce asynchronous execution API Yang Hongyang
2014-04-30 14:40 ` Roger Pau Monné
2014-05-02 16:23 ` [PATCH] libxl: introduce asynchronous execution API [and 1 more messages] Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.