Re: [Xen-devel] [PATCH] libxl: wait for the ack when issuing power control requests

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Oct 01, 2019 at 12:12:59PM +0200, Roger Pau Monne wrote:
> Currently only suspend power control requests wait for an ack from the
> domain, while power off or reboot requests simply write the command to
> xenstore and exit.
> 
> Introduce a 1 minute wait for the domain to acknowledge the request, or
> else return an error. The suspend code is slightly modified to use the
> new infrastructure added, but shouldn't have any functional change.
> 
> Fix the ocaml bindings and also provide a backwards compatible
> interface for the reboot and poweroff libxl API functions.
> 
> Reported-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> I believe applying this patch is not going to cause regressions in
> osstest, albeit FreeBSD doesn't acknowledge poweroff/reboot requests
> in the currently tested versions, it will shutdown in less than one
> minute, and thus the toolstack won't complain because the control node
> is going to be removed from xenstore.
> ---
>  tools/libxl/libxl.h                  | 23 +++++++-
>  tools/libxl/libxl_dom_suspend.c      | 11 ++--
>  tools/libxl/libxl_domain.c           | 81 ++++++++++++++++++++--------
>  tools/libxl/libxl_internal.h         |  7 ++-
>  tools/ocaml/libs/xl/xenlight_stubs.c | 18 ++++---
>  tools/xl/xl_vmcontrol.c              |  4 +-
>  6 files changed, 102 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index f711cfc750..03ea5ca740 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h

In libxl.h, can you update the comment of LIBXL_HAVE_FN_USING_QMP_ASYNC
to add the _shutdown and _reboot?
(The define name might not reflect the reality, but I think the comment
describe what the define is for. It was introduced by edaa631ddce.)

> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> index 0dd5b7ffa9..058f3c77ec 100644
> --- a/tools/libxl/libxl_domain.c
> +++ b/tools/libxl/libxl_domain.c
> @@ -763,49 +763,86 @@ char * libxl__domain_pvcontrol_read(libxl__gc *gc, 
> xs_transaction_t t,
>      return libxl__xs_read(gc, t, shutdown_path);
>  }
>  
> -int libxl__domain_pvcontrol_write(libxl__gc *gc, xs_transaction_t t,
> -                                  uint32_t domid, const char *cmd)
> +int libxl__domain_pvcontrol(libxl__egc *egc, libxl__xswait_state *pvcontrol,
> +                            domid_t domid, const char *cmd)
>  {
> +    STATE_AO_GC(pvcontrol->ao);
>      const char *shutdown_path;
> +    int rc;
> +
> +    rc = libxl__domain_pvcontrol_available(gc, domid);
> +    if (rc < 0)
> +        return rc;
>  
>      shutdown_path = libxl__domain_pvcontrol_xspath(gc, domid);
>      if (!shutdown_path)
>          return ERROR_FAIL;
>  
> -    return libxl__xs_printf(gc, t, shutdown_path, "%s", cmd);
> +    rc = libxl__xs_printf(gc, XBT_NULL, shutdown_path, "%s", cmd);
> +    if (rc)
> +        return rc;
> +
> +    pvcontrol->path = shutdown_path;
> +    pvcontrol->what = GCSPRINTF("guest acknowledgement of %s request", cmd);
> +    pvcontrol->timeout_ms = 60 * 1000;
> +    libxl__xswait_start(gc, pvcontrol);

Shouldn't we watch shutdown_path before we write to it? Otherwise, we
might never see the acknowledgement by the guest.
But I don't know if xswait to a read the first time it is setup.

Also, you need to check the return value of libxl__xswait_start.

> +
> +    return 0;
>  }
>  
> -static int libxl__domain_pvcontrol(libxl__gc *gc, uint32_t domid,
> -                                   const char *cmd)
> +static bool pvcontrol_acked(const char *state)
>  {
> -    int ret;
> +    if (!state || !strcmp(state,""))
> +        return true;
> +
> +    return false;
> +}
> +
> +static void pvcontrol_cb(libxl__egc *egc, libxl__xswait_state *xswa, int rc,
> +                         const char *state)

Can you move pvcontrol_cb after libxl_domain_shutdown and
libxl_domain_reboot ? In general, the callback of a function should be
after the function that set the callback.
See "ASYNCHRONOUS/LONG-RUNNING OPERATIONS" for a more detail
explanation.

> +{
> +    STATE_AO_GC(xswa->ao);
> +
> +    if (!rc && !pvcontrol_acked(state))
> +        return;
>  
> -    ret = libxl__domain_pvcontrol_available(gc, domid);
> -    if (ret < 0)
> -        return ret;
> +    libxl__xswait_stop(gc, xswa);
>  
> -    if (!ret)
> -        return ERROR_NOPARAVIRT;
> +    if (rc)
> +        LOG(ERROR, "guest didn't acknowledge control request: %d", rc);
>  
> -    return libxl__domain_pvcontrol_write(gc, XBT_NULL, domid, cmd);
> +    libxl__ao_complete(egc, ao, rc);
>  }
>  
> -int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid)
> +
> +int libxl_domain_shutdown(libxl_ctx *ctx, uint32_t domid,
> +                          const libxl_asyncop_how *ao_how)
>  {
> -    GC_INIT(ctx);
> +    AO_CREATE(ctx, domid, ao_how);
> +    libxl__xswait_state *pvcontrol;
>      int ret;
> -    ret = libxl__domain_pvcontrol(gc, domid, "poweroff");
> -    GC_FREE;
> -    return ret;
> +
> +    GCNEW(pvcontrol);
> +    pvcontrol->ao = ao;
> +    pvcontrol->callback = pvcontrol_cb;
> +    ret = libxl__domain_pvcontrol(egc, pvcontrol, domid, "poweroff");

libxl__domain_pvcontrol should return a libxl error (I haven't check),
so it should be `rc' here, not ret.

> +
> +    return ret ? AO_CREATE_FAIL(ret) : AO_INPROGRESS;
>  }
>  
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
> b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 37b046df63..ff16b8710b 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -551,32 +551,38 @@ value stub_libxl_domain_create_restore(value ctx, value 
> domain_config, value par
>       CAMLreturn(Val_int(c_domid));
>  }
>  
> -value stub_libxl_domain_shutdown(value ctx, value domid)
> +value stub_libxl_domain_shutdown(value ctx, value domid, value async, value 
> unit)

You also need to change the ocaml prototype ;-). They are declared in
the *.ml and *.mli file.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel