Re: [PATCH 07/19] libxc: suspend: Rename, improve xc_suspend_evtchn_init

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/03/14 14:56, Ian Jackson wrote:
> xc_suspend_evtchn_init expects to eat the first event on the xce.  If
> the xce is used for any other purpose then this can break.  Document
> this fact and rename the function to xc_suspend_evtchn_init_exclusive.
> (I haven't checked the call sites for improper shared use of the xce.)
>
> Provide a corresponding xc_suspend_evtchn_init_sane which doesn't try
> to eat an event, and instead leaves the caller the ability to
> demultiplex.
>
> Also document that xc_await_suspend needs exclusive use of the xce.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/libxc/xc_suspend.c                           |   21 ++++++++++++++++----
>  tools/libxc/xenguest.h                             |   13 +++++++++++-
>  tools/libxl/libxl_dom.c                            |    2 +-
>  tools/misc/xen-hptool.c                            |    2 +-
>  .../python/xen/lowlevel/checkpoint/libcheckpoint.c |    2 +-
>  tools/xcutils/xc_save.c                            |    2 +-
>  6 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/tools/libxc/xc_suspend.c b/tools/libxc/xc_suspend.c
> index 1ace411..7968a44 100644
> --- a/tools/libxc/xc_suspend.c
> +++ b/tools/libxc/xc_suspend.c
> @@ -14,6 +14,8 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>   */
>  
> +#include <assert.h>
> +

This appears unused - is it debugging which slipped in?

~Andrew

>  #include "xc_private.h"
>  #include "xenguest.h"
>  
> @@ -102,7 +104,7 @@ int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int
>      return unlock_suspend_event(xch, domid);
>  }
>  
> -int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port)
>  {
>      int rc, suspend_evtchn = -1;
>  
> @@ -121,9 +123,6 @@ int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int por
>          goto cleanup;
>      }
>  
> -    /* event channel is pending immediately after binding */
> -    xc_await_suspend(xch, xce, suspend_evtchn);
> -
>      return suspend_evtchn;
>  
>  cleanup:
> @@ -132,3 +131,17 @@ cleanup:
>  
>      return -1;
>  }
> +
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port)
> +{
> +    int suspend_evtchn;
> +
> +    suspend_evtchn = xc_suspend_evtchn_init_sane(xch, xce, domid, port);
> +    if (suspend_evtchn < 0)
> +        return suspend_evtchn;
> +
> +    /* event channel is pending immediately after binding */
> +    xc_await_suspend(xch, xce, suspend_evtchn);
> +
> +    return suspend_evtchn;
> +}
> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
> index a0e30e1..ce5456c 100644
> --- a/tools/libxc/xenguest.h
> +++ b/tools/libxc/xenguest.h
> @@ -256,10 +256,21 @@ int xc_hvm_build_target_mem(xc_interface *xch,
>  
>  int xc_suspend_evtchn_release(xc_interface *xch, xc_evtchn *xce, int domid, int suspend_evtchn);
>  
> -int xc_suspend_evtchn_init(xc_interface *xch, xc_evtchn *xce, int domid, int port);
> +/**
> + * This function eats the initial notification.
> + * xce must not be used for anything else
> + */
> +int xc_suspend_evtchn_init_exclusive(xc_interface *xch, xc_evtchn *xce, int domid, int port);
>  
> +/* xce must not be used for anything else */
>  int xc_await_suspend(xc_interface *xch, xc_evtchn *xce, int suspend_evtchn);
>  
> +/**
> + * The port will be signaled immediately after this call
> + * The caller should check the domain status and look for the next event
> + */
> +int xc_suspend_evtchn_init_sane(xc_interface *xch, xc_evtchn *xce, int domid, int port);
> +
>  int xc_get_bit_size(xc_interface *xch,
>                      const char *image_name, const char *cmdline,
>                      const char *features, int *type);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 55f74b2..4b42856 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1358,7 +1358,7 @@ void libxl__domain_suspend(libxl__egc *egc, libxl__domain_suspend_state *dss)
>  
>          if (port >= 0) {
>              dss->suspend_eventchn =
> -                xc_suspend_evtchn_init(CTX->xch, dss->xce, dss->domid, port);
> +                xc_suspend_evtchn_init_exclusive(CTX->xch, dss->xce, dss->domid, port);
>  
>              if (dss->suspend_eventchn < 0)
>                  LOG(WARN, "Suspend event channel initialization failed");
> diff --git a/tools/misc/xen-hptool.c b/tools/misc/xen-hptool.c
> index 24c3e95..db76f79 100644
> --- a/tools/misc/xen-hptool.c
> +++ b/tools/misc/xen-hptool.c
> @@ -112,7 +112,7 @@ static int suspend_guest(xc_interface *xch, xc_evtchn *xce, int domid, int *evtc
>          fprintf(stderr, "DOM%d: No suspend port, try live migration\n", domid);
>          goto failed;
>      }
> -    suspend_evtchn = xc_suspend_evtchn_init(xch, xce, domid, port);
> +    suspend_evtchn = xc_suspend_evtchn_init_exclusive(xch, xce, domid, port);
>      if (suspend_evtchn < 0)
>      {
>          fprintf(stderr, "Suspend evtchn initialization failed\n");
> diff --git a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> index 01c0d47..817d272 100644
> --- a/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> +++ b/tools/python/xen/lowlevel/checkpoint/libcheckpoint.c
> @@ -360,7 +360,7 @@ static int setup_suspend_evtchn(checkpoint_state* s)
>      return -1;
>    }
>  
> -  s->suspend_evtchn = xc_suspend_evtchn_init(s->xch, s->xce, s->domid, port);
> +  s->suspend_evtchn = xc_suspend_evtchn_init_exclusive(s->xch, s->xce, s->domid, port);
>    if (s->suspend_evtchn < 0) {
>        s->errstr = "failed to bind suspend event channel";
>        return -1;
> diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
> index e34bd2c..974f706 100644
> --- a/tools/xcutils/xc_save.c
> +++ b/tools/xcutils/xc_save.c
> @@ -202,7 +202,7 @@ main(int argc, char **argv)
>          else
>          {
>              si.suspend_evtchn =
> -              xc_suspend_evtchn_init(si.xch, si.xce, si.domid, port);
> +              xc_suspend_evtchn_init_exclusive(si.xch, si.xce, si.domid, port);
>  
>              if (si.suspend_evtchn < 0)
>                  warnx("suspend event channel initialization failed, "