Re: [PATCH 10/29] libxl: events: Make timeout and async exec setup take an ao, not a gc

[Wen Congyang <wency@cn.fujitsu.com>]

On 02/11/2015 04:09 AM, Ian Jackson wrote:
> Change the timeout setup functions to take a libxl__ao, not a
> libxl__gc.  This is going to be needed for ao cancellation, because
> timeouts are going to be a main hook for ao cancellation - so the
> timeouts need to be associated with an ao.
> 
> This means that timeouts can only occur as part of a long-running
> libxl function (but this is of course correct, as libxl shouldn't have
> any global timeouts, and indeed all the call sites have an ao).
> 
> Also remove the gc parameter from libxl__async_exec_start.  It can
> just use the gc from the ao supplied in the aes.
> 
> All the callers follow the obvious patterns and therefore supply the
> ao's gc to libxl__async_exec_start and the timeout setup functions.
> There is therefore no functional change in this patch.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> CC: Lai Jiangshan <laijs@cn.fujitsu.com>

libxl__async_exec_start() related modifications look fine to me.

> ---
> v2: This patch split off from "Permit timeouts to signal cancellation".
>     Rebased; consequently, deal with libxl__async_exec_start.
>     CC'd authors of the libxl__async_exec_* functions.
> ---
>  tools/libxl/libxl_aoutils.c         |    8 +++++---
>  tools/libxl/libxl_device.c          |    4 ++--
>  tools/libxl/libxl_dom.c             |    8 ++++----
>  tools/libxl/libxl_event.c           |    6 ++++--
>  tools/libxl/libxl_internal.h        |    6 +++---
>  tools/libxl/libxl_remus_disk_drbd.c |    2 +-
>  tools/libxl/libxl_test_timedereg.c  |    9 +++++----
>  7 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 44dc222..754e2d1 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -46,7 +46,7 @@ int libxl__xswait_start(libxl__gc *gc, libxl__xswait_state *xswa)
>  {
>      int rc;
>  
> -    rc = libxl__ev_time_register_rel(gc, &xswa->time_ev,
> +    rc = libxl__ev_time_register_rel(xswa->ao, &xswa->time_ev,
>                                       xswait_timeout_callback, xswa->timeout_ms);
>      if (rc) goto err;
>  
> @@ -496,16 +496,18 @@ void libxl__async_exec_init(libxl__async_exec_state *aes)
>      libxl__ev_child_init(&aes->child);
>  }
>  
> -int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> +int libxl__async_exec_start(libxl__async_exec_state *aes)
>  {
>      pid_t pid;
>  
>      /* Convenience aliases */
> +    libxl__ao *ao = aes->ao;
> +    AO_GC;
>      libxl__ev_child *const child = &aes->child;
>      char ** const args = aes->args;
>  
>      /* Set execution timeout */
> -    if (libxl__ev_time_register_rel(gc, &aes->time,
> +    if (libxl__ev_time_register_rel(ao, &aes->time,
>                                      async_exec_timeout,
>                                      aes->timeout_ms)) {
>          LOG(ERROR, "unable to register timeout for executing: %s", aes->what);
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 0455134..c80749f 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -808,7 +808,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
>               * TODO: 4.2 Bodge due to QEMU, see comment on top of
>               * libxl__initiate_device_remove in libxl_internal.h
>               */
> -            rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
> +            rc = libxl__ev_time_register_rel(ao, &aodev->timeout,
>                                               device_qemu_timeout,
>                                               LIBXL_QEMU_BODGE_TIMEOUT * 1000);
>              if (rc) {
> @@ -1034,7 +1034,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>      aes->stdfds[1] = 2;
>      aes->stdfds[2] = -1;
>  
> -    rc = libxl__async_exec_start(gc, aes);
> +    rc = libxl__async_exec_start(aes);
>      if (rc)
>          goto out;
>  
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 90877d6..e292cb3 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -980,7 +980,7 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty
>                                  switch_logdirty_xswatch, lds->ret_path);
>      if (rc) goto out;
>  
> -    rc = libxl__ev_time_register_rel(gc, &lds->timeout,
> +    rc = libxl__ev_time_register_rel(ao, &lds->timeout,
>                                  switch_logdirty_timeout, 10*1000);
>      if (rc) goto out;
>  
> @@ -1260,7 +1260,7 @@ static void domain_suspend_callback_common(libxl__egc *egc,
>          rc = libxl__ev_evtchn_wait(gc, &dss->guest_evtchn);
>          if (rc) goto err;
>  
> -        rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
> +        rc = libxl__ev_time_register_rel(ao, &dss->guest_timeout,
>                                           suspend_common_wait_guest_timeout,
>                                           60*1000);
>          if (rc) goto err;
> @@ -1391,7 +1391,7 @@ static void domain_suspend_common_wait_guest(libxl__egc *egc,
>                                      "@releaseDomain");
>      if (rc) goto err;
>  
> -    rc = libxl__ev_time_register_rel(gc, &dss->guest_timeout,
> +    rc = libxl__ev_time_register_rel(ao, &dss->guest_timeout,
>                                       suspend_common_wait_guest_timeout,
>                                       60*1000);
>      if (rc) goto err;
> @@ -1751,7 +1751,7 @@ static void remus_devices_commit_cb(libxl__egc *egc,
>       */
>  
>      /* Set checkpoint interval timeout */
> -    rc = libxl__ev_time_register_rel(gc, &dss->checkpoint_timeout,
> +    rc = libxl__ev_time_register_rel(ao, &dss->checkpoint_timeout,
>                                       remus_next_checkpoint,
>                                       dss->interval);
>  
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index fb6daeb..1a97cf8 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -309,10 +309,11 @@ static void time_done_debug(libxl__gc *gc, const char *func,
>  #endif
>  }
>  
> -int libxl__ev_time_register_abs(libxl__gc *gc, libxl__ev_time *ev,
> +int libxl__ev_time_register_abs(libxl__ao *ao, libxl__ev_time *ev,
>                                  libxl__ev_time_callback *func,
>                                  struct timeval absolute)
>  {
> +    AO_GC;
>      int rc;
>  
>      CTX_LOCK;
> @@ -333,10 +334,11 @@ int libxl__ev_time_register_abs(libxl__gc *gc, libxl__ev_time *ev,
>  }
>  
>  
> -int libxl__ev_time_register_rel(libxl__gc *gc, libxl__ev_time *ev,
> +int libxl__ev_time_register_rel(libxl__ao *ao, libxl__ev_time *ev,
>                                  libxl__ev_time_callback *func,
>                                  int milliseconds /* as for poll(2) */)
>  {
> +    AO_GC;
>      struct timeval absolute;
>      int rc;
>  
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6bb208c..b615fc5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -770,10 +770,10 @@ static inline void libxl__ev_fd_init(libxl__ev_fd *efd)
>  static inline int libxl__ev_fd_isregistered(const libxl__ev_fd *efd)
>                      { return efd->fd >= 0; }
>  
> -_hidden int libxl__ev_time_register_rel(libxl__gc*, libxl__ev_time *ev_out,
> +_hidden int libxl__ev_time_register_rel(libxl__ao*, libxl__ev_time *ev_out,
>                                          libxl__ev_time_callback*,
>                                          int milliseconds /* as for poll(2) */);
> -_hidden int libxl__ev_time_register_abs(libxl__gc*, libxl__ev_time *ev_out,
> +_hidden int libxl__ev_time_register_abs(libxl__ao*, libxl__ev_time *ev_out,
>                                          libxl__ev_time_callback*,
>                                          struct timeval);
>  _hidden int libxl__ev_time_modify_rel(libxl__gc*, libxl__ev_time *ev,
> @@ -2108,7 +2108,7 @@ struct libxl__async_exec_state {
>  };
>  
>  void libxl__async_exec_init(libxl__async_exec_state *aes);
> -int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes);
> +int libxl__async_exec_start(libxl__async_exec_state *aes);
>  bool libxl__async_exec_inuse(const libxl__async_exec_state *aes);
>  
>  /*----- device addition/removal -----*/
> diff --git a/tools/libxl/libxl_remus_disk_drbd.c b/tools/libxl/libxl_remus_disk_drbd.c
> index afe9b61..5e0c9a6 100644
> --- a/tools/libxl/libxl_remus_disk_drbd.c
> +++ b/tools/libxl/libxl_remus_disk_drbd.c
> @@ -120,7 +120,7 @@ static void match_async_exec(libxl__egc *egc, libxl__remus_device *dev)
>      aes->stdfds[1] = -1;
>      aes->stdfds[2] = -1;
>  
> -    rc = libxl__async_exec_start(gc, aes);
> +    rc = libxl__async_exec_start(aes);
>      if (rc)
>          goto out;
>  
> diff --git a/tools/libxl/libxl_test_timedereg.c b/tools/libxl/libxl_test_timedereg.c
> index a44639f..e2cc27d 100644
> --- a/tools/libxl/libxl_test_timedereg.c
> +++ b/tools/libxl/libxl_test_timedereg.c
> @@ -30,12 +30,13 @@ static int seq;
>  static void occurs(libxl__egc *egc, libxl__ev_time *ev,
>                     const struct timeval *requested_abs);
>  
> -static void regs(libxl__gc *gc, int j)
> +static void regs(libxl__ao *ao, int j)
>  {
> +    AO_GC;
>      int rc, i;
>      LOG(DEBUG,"regs(%d)", j);
>      for (i=0; i<NTIMES; i++) {
> -        rc = libxl__ev_time_register_rel(gc, &et[j][i], occurs, ms[j][i]);
> +        rc = libxl__ev_time_register_rel(ao, &et[j][i], occurs, ms[j][i]);
>          assert(!rc);
>      }    
>  }
> @@ -52,7 +53,7 @@ int libxl_test_timedereg(libxl_ctx *ctx, libxl_asyncop_how *ao_how)
>          libxl__ev_time_init(&et[1][i]);
>      }
>  
> -    regs(gc, 0);
> +    regs(ao, 0);
>  
>      return AO_INPROGRESS;
>  }
> @@ -71,7 +72,7 @@ static void occurs(libxl__egc *egc, libxl__ev_time *ev,
>          assert(ev == &et[0][1]);
>          libxl__ev_time_deregister(gc, &et[0][0]);
>          libxl__ev_time_deregister(gc, &et[0][2]);
> -        regs(gc, 1);
> +        regs(tao, 1);
>          libxl__ev_time_deregister(gc, &et[0][1]);
>          break;
>  
>