From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH] libxl: introduce asynchronous execution API Date: Wed, 30 Apr 2014 16:40:25 +0200 Message-ID: <53610B59.6050601@citrix.com> References: <1398403588-23619-1-git-send-email-yanghy@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <1398403588-23619-1-git-send-email-yanghy@cn.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Yang Hongyang , xen-devel@lists.xen.org Cc: ian.campbell@citrix.com, wency@cn.fujitsu.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, yunhong.jiang@intel.com, ian.jackson@eu.citrix.com, eddie.dong@intel.com, rshriram@cs.ubc.ca, laijs@cn.fujitsu.com List-Id: xen-devel@lists.xenproject.org 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=E9 > = > Signed-off-by: Lai Jiangshan > Signed-off-by: Wen Congyang > Signed-off-by: Yang Hongyang > --- > 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 =3D 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 =3D 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 =3D &aes->child; > + char ** const args =3D 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 =3D libxl__ev_child_fork(gc, child, async_exec_done); > + if (pid =3D=3D -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.