[PATCH 0/9] libxl: poll: Avoid fd deregistration race POLLNVAL

[PATCH 0/9] libxl: poll: Avoid fd deregistration race POLLNVAL

[Ian Jackson]

The first three patches in this series fix a race which I think is the
cause of a bug reported by Jim Fehlig (thanks, Jim).  They are IMO
backport candidates.

The next 5 provide a test case which reproduces the bug.  I actually
developed these patches in the opposite order, and have verified that
the without the fix, the test case fails the POLLNVAL assertion as
expected.

The final patch is a fix to another libxl event test.

Thanks,
Ian.

[PATCH 1/9] libxl: poll: Make libxl__poller_get have only one success return path

[Ian Jackson]

In preparation for doing some more work on successful exit.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jim Fehlig <jfehlig@suse.com>
---
 tools/libxl/libxl_event.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 9072df4..b332dd7 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1627,15 +1627,14 @@ libxl__poller *libxl__poller_get(libxl__gc *gc)
     libxl__poller *p = LIBXL_LIST_FIRST(&CTX->pollers_idle);
     if (p) {
         LIBXL_LIST_REMOVE(p, entry);
-        return p;
-    }
-
-    p = libxl__zalloc(NOGC, sizeof(*p));
+    } else {
+        p = libxl__zalloc(NOGC, sizeof(*p));
 
-    rc = libxl__poller_init(gc, p);
-    if (rc) {
-        free(p);
-        return NULL;
+        rc = libxl__poller_init(gc, p);
+        if (rc) {
+            free(p);
+            return NULL;
+        }
     }
 
     return p;
-- 
1.7.10.4

Re: [PATCH 1/9] libxl: poll: Make libxl__poller_get have only one success return path

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:49PM +0100, Ian Jackson wrote:
> In preparation for doing some more work on successful exit.
> 
> No functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 2/9] libxl: poll: Use poller_get and poller_put for poller_app

[Ian Jackson]

This makes the code more regular.  We are going to want to do some
more work in poller_get and poller_put, which work also wants to be
done for poller_app.

Two very minor functional changes:

 * We call malloc an extra time since poller_app is now a pointer

 * ERROR_FAIL on poller_get failing for poller_app is generated in
   libxl_ctx_init rather than passed through by libxl_poller_init
   from libxl__pipe_nonblock.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jim Fehlig <jfehlig@suse.com>
---
 tools/libxl/libxl.c          |   11 ++++++++---
 tools/libxl/libxl_event.c    |    4 ++--
 tools/libxl/libxl_internal.h |    2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e9a2d26..97a60cf 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -59,6 +59,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 
     ctx->osevent_hooks = 0;
 
+    ctx->poller_app = 0;
     LIBXL_LIST_INIT(&ctx->pollers_event);
     LIBXL_LIST_INIT(&ctx->pollers_idle);
 
@@ -103,8 +104,11 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     rc = libxl__atfork_init(ctx);
     if (rc) goto out;
 
-    rc = libxl__poller_init(gc, &ctx->poller_app);
-    if (rc) goto out;
+    ctx->poller_app = libxl__poller_get(gc);
+    if (!ctx->poller_app) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
 
     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {
@@ -183,7 +187,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
     if (ctx->xsh) xs_daemon_close(ctx->xsh);
     if (ctx->xce) xc_evtchn_close(ctx->xce);
 
-    libxl__poller_dispose(&ctx->poller_app);
+    libxl__poller_put(ctx, ctx->poller_app);
+    ctx->poller_app = NULL;
     assert(LIBXL_LIST_EMPTY(&ctx->pollers_event));
     libxl__poller *poller, *poller_tmp;
     LIBXL_LIST_FOREACH_SAFE(poller, &ctx->pollers_idle, entry, poller_tmp) {
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index b332dd7..942e02c 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1138,7 +1138,7 @@ int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
 {
     EGC_INIT(ctx);
     CTX_LOCK;
-    int rc = beforepoll_internal(gc, &ctx->poller_app,
+    int rc = beforepoll_internal(gc, ctx->poller_app,
                                  nfds_io, fds, timeout_upd, now);
     CTX_UNLOCK;
     EGC_FREE;
@@ -1291,7 +1291,7 @@ void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
 {
     EGC_INIT(ctx);
     CTX_LOCK;
-    afterpoll_internal(egc, &ctx->poller_app, nfds, fds, now);
+    afterpoll_internal(egc, ctx->poller_app, nfds, fds, now);
     CTX_UNLOCK;
     EGC_FREE;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d52589e..a33725e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -401,7 +401,7 @@ struct libxl__ctx {
       /* See the comment for OSEVENT_HOOK_INTERN in libxl_event.c
        * for restrictions on the use of the osevent fields. */
 
-    libxl__poller poller_app; /* libxl_osevent_beforepoll and _afterpoll */
+    libxl__poller *poller_app; /* libxl_osevent_beforepoll and _afterpoll */
     LIBXL_LIST_HEAD(, libxl__poller) pollers_event, pollers_idle;
 
     LIBXL_SLIST_HEAD(libxl__osevent_hook_nexi, libxl__osevent_hook_nexus)
-- 
1.7.10.4

Re: [PATCH 2/9] libxl: poll: Use poller_get and poller_put for poller_app

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:50PM +0100, Ian Jackson wrote:
> This makes the code more regular.  We are going to want to do some
> more work in poller_get and poller_put, which work also wants to be
> done for poller_app.
> 
> Two very minor functional changes:
> 
>  * We call malloc an extra time since poller_app is now a pointer
> 
>  * ERROR_FAIL on poller_get failing for poller_app is generated in
>    libxl_ctx_init rather than passed through by libxl_poller_init
>    from libxl__pipe_nonblock.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 3/9] libxl: poll: Avoid fd deregistration race POLLNVAL crash

[Ian Jackson]

It can happen that an fd is deregistered, and closed, and then a new
fd opened, and reregistered, all while another thread is in poll().

If this happens poll might report POLLNVAL, but the event loop would
think that the fd was supposed to have been valid, and then fail an
assertion:
  libxl_event.c:1183: afterpoll_check_fd: Assertion `poller->fds_changed || !(fds[slot].revents & 0x020)' failed.

We can't simply ignore POLLNVAL because if we have bugs which cause
messed-up fds, it is a serious problem which we really need to detect.

Instead, add extra tracking to spot when this possibility arises, and
abort on POLLNVAL if we are sure that it is unexpected.

Reported-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Jim Fehlig <jfehlig@suse.com>
---
 tools/libxl/libxl.c          |    2 ++
 tools/libxl/libxl_event.c    |   12 +++++++++++-
 tools/libxl/libxl_internal.h |   13 +++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 97a60cf..6297a94 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -62,6 +62,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     ctx->poller_app = 0;
     LIBXL_LIST_INIT(&ctx->pollers_event);
     LIBXL_LIST_INIT(&ctx->pollers_idle);
+    LIBXL_LIST_INIT(&ctx->pollers_fds_changed);
 
     LIBXL_LIST_INIT(&ctx->efds);
     LIBXL_TAILQ_INIT(&ctx->etimes);
@@ -190,6 +191,7 @@ int libxl_ctx_free(libxl_ctx *ctx)
     libxl__poller_put(ctx, ctx->poller_app);
     ctx->poller_app = NULL;
     assert(LIBXL_LIST_EMPTY(&ctx->pollers_event));
+    assert(LIBXL_LIST_EMPTY(&ctx->pollers_fds_changed));
     libxl__poller *poller, *poller_tmp;
     LIBXL_LIST_FOREACH_SAFE(poller, &ctx->pollers_idle, entry, poller_tmp) {
         libxl__poller_dispose(poller);
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 942e02c..8acecfa 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -225,6 +225,7 @@ int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev, short events)
 void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev)
 {
     CTX_LOCK;
+    libxl__poller *poller;
 
     if (!libxl__ev_fd_isregistered(ev)) {
         DBG("ev_fd=%p deregister unregistered",ev);
@@ -237,6 +238,9 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev)
     LIBXL_LIST_REMOVE(ev, entry);
     ev->fd = -1;
 
+    LIBXL_LIST_FOREACH(poller, &CTX->pollers_fds_changed, fds_changed_entry)
+        poller->fds_changed = 1;
+
  out:
     CTX_UNLOCK;
 }
@@ -1110,6 +1114,8 @@ static int beforepoll_internal(libxl__gc *gc, libxl__poller *poller,
 
     *nfds_io = used;
 
+    poller->fds_changed = 0;
+
     libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
     if (etime) {
         int our_timeout;
@@ -1174,7 +1180,7 @@ static int afterpoll_check_fd(libxl__poller *poller,
             /* again, stale slot entry */
             continue;
 
-        assert(!(fds[slot].revents & POLLNVAL));
+        assert(poller->fds_changed || !(fds[slot].revents & POLLNVAL));
 
         /* we mask in case requested events have changed */
         int slot_revents = fds[slot].revents & events;
@@ -1601,6 +1607,7 @@ int libxl__poller_init(libxl__gc *gc, libxl__poller *p)
     int rc;
     p->fd_polls = 0;
     p->fd_rindices = 0;
+    p->fds_changed = 0;
 
     rc = libxl__pipe_nonblock(CTX, p->wakeup_pipe);
     if (rc) goto out;
@@ -1637,12 +1644,15 @@ libxl__poller *libxl__poller_get(libxl__gc *gc)
         }
     }
 
+    LIBXL_LIST_INSERT_HEAD(&CTX->pollers_fds_changed, p,
+                           fds_changed_entry);
     return p;
 }
 
 void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p)
 {
     if (!p) return;
+    LIBXL_LIST_REMOVE(p, fds_changed_entry);
     LIBXL_LIST_INSERT_HEAD(&ctx->pollers_idle, p, entry);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a33725e..5b7a9f6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -362,6 +362,18 @@ struct libxl__poller {
     int (*fd_rindices)[3]; /* see libxl_event.c:beforepoll_internal */
 
     int wakeup_pipe[2]; /* 0 means no fd allocated */
+
+    /*
+     * We also use the poller to record whether any fds have been
+     * deregistered since we entered poll.  Each poller which is not
+     * idle is on the list pollers_fds_changed.  fds_changed is
+     * cleared by beforepoll, and tested by afterpoll.  Whenever an fd
+     * event is deregistered, we set the fds_changed of all non-idle
+     * pollers.  So afterpoll can tell whether any POLLNVAL is
+     * plausibly due to an fd being closed and reopened.
+     */
+    LIBXL_LIST_ENTRY(libxl__poller) fds_changed_entry;
+    bool fds_changed;
 };
 
 struct libxl__gc {
@@ -403,6 +415,7 @@ struct libxl__ctx {
 
     libxl__poller *poller_app; /* libxl_osevent_beforepoll and _afterpoll */
     LIBXL_LIST_HEAD(, libxl__poller) pollers_event, pollers_idle;
+    LIBXL_LIST_HEAD(, libxl__poller) pollers_fds_changed;
 
     LIBXL_SLIST_HEAD(libxl__osevent_hook_nexi, libxl__osevent_hook_nexus)
         hook_fd_nexi_idle, hook_timeout_nexi_idle;
-- 
1.7.10.4

Re: [PATCH 3/9] libxl: poll: Avoid fd deregistration race POLLNVAL crash

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:51PM +0100, Ian Jackson wrote:
> It can happen that an fd is deregistered, and closed, and then a new
> fd opened, and reregistered, all while another thread is in poll().
> 
> If this happens poll might report POLLNVAL, but the event loop would
> think that the fd was supposed to have been valid, and then fail an
> assertion:
>   libxl_event.c:1183: afterpoll_check_fd: Assertion `poller->fds_changed || !(fds[slot].revents & 0x020)' failed.
> 
> We can't simply ignore POLLNVAL because if we have bugs which cause
> messed-up fds, it is a serious problem which we really need to detect.
> 
> Instead, add extra tracking to spot when this possibility arises, and
> abort on POLLNVAL if we are sure that it is unexpected.
> 
> Reported-by: Jim Fehlig <jfehlig@suse.com>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Jim Fehlig <jfehlig@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 4/9] libxl: event tests: Improve Makefile doc comment

[Ian Jackson]

Including the explanation of how to run these tests.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/Makefile |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index cc9c152..44a4da7 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -109,7 +109,13 @@ LIBXL_TESTS += timedereg
 # "outside libxl" file is compiled exactly like a piece of application
 # code.  They must share information via explicit libxl entrypoints.
 # Unlike proper parts of libxl, it is permissible for libxl_test_FOO.c
-# to use private global variables for its state.
+# to use private global variables for its state.  Note that all the
+# "inside" parts are compiled into a single test library, so their
+# symbol names must be unique.
+#
+# To run these tests, either use LD_PRELOAD to get libxenlight_test.so
+# loaded, or rename it to libxenlight.so so it is the target of the
+# appropriate symlinks.
 
 LIBXL_TEST_OBJS += $(foreach t, $(LIBXL_TESTS),libxl_test_$t.o)
 TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS),test_$t.o) test_common.o
-- 
1.7.10.4

Re: [PATCH 4/9] libxl: event tests: Improve Makefile doc comment

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:52PM +0100, Ian Jackson wrote:
> Including the explanation of how to run these tests.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/Makefile |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index cc9c152..44a4da7 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -109,7 +109,13 @@ LIBXL_TESTS += timedereg
>  # "outside libxl" file is compiled exactly like a piece of application
>  # code.  They must share information via explicit libxl entrypoints.
>  # Unlike proper parts of libxl, it is permissible for libxl_test_FOO.c
> -# to use private global variables for its state.
> +# to use private global variables for its state.  Note that all the
> +# "inside" parts are compiled into a single test library, so their
> +# symbol names must be unique.
> +#
> +# To run these tests, either use LD_PRELOAD to get libxenlight_test.so
> +# loaded, or rename it to libxenlight.so so it is the target of the
> +# appropriate symlinks.
>  
>  LIBXL_TEST_OBJS += $(foreach t, $(LIBXL_TESTS),libxl_test_$t.o)
>  TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS),test_$t.o) test_common.o
> -- 
> 1.7.10.4

[PATCH 5/9] libxl: event tests: Contemplate separate tests

[Ian Jackson]

Split LIBXL_TESTS into two variables, each of which gets all of
LIBXL_TESTS, so that we can have tests which do use generic test
helper inside functions, rather than test-specific ones.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/Makefile |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 44a4da7..512b0e1 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -100,6 +100,9 @@ LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
 LIBXL_TESTS += timedereg
+LIBXL_TESTS_PROGS = $(LIBXL_TESTS)
+LIBXL_TESTS_INSIDE = $(LIBXL_TESTS)
+
 # Each entry FOO in LIBXL_TESTS has two main .c files:
 #   libxl_test_FOO.c  "inside libxl" code to support the test case
 #   test_FOO.c        "outside libxl" code to exercise the test case
@@ -117,9 +120,9 @@ LIBXL_TESTS += timedereg
 # loaded, or rename it to libxenlight.so so it is the target of the
 # appropriate symlinks.
 
-LIBXL_TEST_OBJS += $(foreach t, $(LIBXL_TESTS),libxl_test_$t.o)
-TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS),test_$t.o) test_common.o
-TEST_PROGS += $(foreach t, $(LIBXL_TESTS),test_$t)
+LIBXL_TEST_OBJS += $(foreach t, $(LIBXL_TESTS_INSIDE),libxl_test_$t.o)
+TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS_PROGS),test_$t.o) test_common.o
+TEST_PROGS += $(foreach t, $(LIBXL_TESTS_PROGS),test_$t)
 
 $(LIBXL_OBJS) $(LIBXL_TEST_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h
 
-- 
1.7.10.4

Re: [PATCH 5/9] libxl: event tests: Contemplate separate tests

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:53PM +0100, Ian Jackson wrote:
> Split LIBXL_TESTS into two variables, each of which gets all of
> LIBXL_TESTS, so that we can have tests which do use generic test
> helper inside functions, rather than test-specific ones.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/Makefile |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 44a4da7..512b0e1 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -100,6 +100,9 @@ LIBXL_OBJS += libxl_genid.o
>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>  
>  LIBXL_TESTS += timedereg
> +LIBXL_TESTS_PROGS = $(LIBXL_TESTS)
> +LIBXL_TESTS_INSIDE = $(LIBXL_TESTS)
> +
>  # Each entry FOO in LIBXL_TESTS has two main .c files:
>  #   libxl_test_FOO.c  "inside libxl" code to support the test case
>  #   test_FOO.c        "outside libxl" code to exercise the test case
> @@ -117,9 +120,9 @@ LIBXL_TESTS += timedereg
>  # loaded, or rename it to libxenlight.so so it is the target of the
>  # appropriate symlinks.
>  
> -LIBXL_TEST_OBJS += $(foreach t, $(LIBXL_TESTS),libxl_test_$t.o)
> -TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS),test_$t.o) test_common.o
> -TEST_PROGS += $(foreach t, $(LIBXL_TESTS),test_$t)
> +LIBXL_TEST_OBJS += $(foreach t, $(LIBXL_TESTS_INSIDE),libxl_test_$t.o)
> +TEST_PROG_OBJS += $(foreach t, $(LIBXL_TESTS_PROGS),test_$t.o) test_common.o
> +TEST_PROGS += $(foreach t, $(LIBXL_TESTS_PROGS),test_$t)
>  
>  $(LIBXL_OBJS) $(LIBXL_TEST_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h
>  
> -- 
> 1.7.10.4

[PATCH 6/9] libxl: event tests: Provide libxl_test_fdevent

[Ian Jackson]

We are going to use this shortly.  But, it is nicely self-contained.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/Makefile             |    2 +-
 tools/libxl/libxl_test_fdevent.c |   79 ++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_test_fdevent.h |   12 ++++++
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_test_fdevent.c
 create mode 100644 tools/libxl/libxl_test_fdevent.h

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 512b0e1..b92809c 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -101,7 +101,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
 LIBXL_TESTS += timedereg
 LIBXL_TESTS_PROGS = $(LIBXL_TESTS)
-LIBXL_TESTS_INSIDE = $(LIBXL_TESTS)
+LIBXL_TESTS_INSIDE = $(LIBXL_TESTS) fdevent
 
 # Each entry FOO in LIBXL_TESTS has two main .c files:
 #   libxl_test_FOO.c  "inside libxl" code to support the test case
diff --git a/tools/libxl/libxl_test_fdevent.c b/tools/libxl/libxl_test_fdevent.c
new file mode 100644
index 0000000..2d875d9
--- /dev/null
+++ b/tools/libxl/libxl_test_fdevent.c
@@ -0,0 +1,79 @@
+/*
+ * fdevent test helpr for the libxl event system
+ */
+
+#include "libxl_internal.h"
+
+#include "libxl_test_fdevent.h"
+
+typedef struct {
+    libxl__ao *ao;
+    libxl__ev_fd fd;
+    libxl__ao_abortable abrt;
+} libxl__test_fdevent;
+
+static void fdevent_complete(libxl__egc *egc, libxl__test_fdevent *tfe,
+                             int rc);
+
+static void tfe_init(libxl__test_fdevent *tfe, libxl__ao *ao)
+{
+    tfe->ao = ao;
+    libxl__ev_fd_init(&tfe->fd);
+    libxl__ao_abortable_init(&tfe->abrt);
+}
+
+static void tfe_cleanup(libxl__gc *gc, libxl__test_fdevent *tfe)
+{
+    libxl__ev_fd_deregister(gc, &tfe->fd);
+    libxl__ao_abortable_deregister(&tfe->abrt);
+}
+
+static void tfe_fd_cb(libxl__egc *egc, libxl__ev_fd *ev,
+                      int fd, short events, short revents)
+{
+    libxl__test_fdevent *tfe = CONTAINER_OF(ev,*tfe,fd);
+    STATE_AO_GC(tfe->ao);
+    fdevent_complete(egc, tfe, 0);
+}
+
+static void tfe_abrt_cb(libxl__egc *egc, libxl__ao_abortable *abrt,
+                        int rc)
+{
+    libxl__test_fdevent *tfe = CONTAINER_OF(abrt,*tfe,abrt);
+    STATE_AO_GC(tfe->ao);
+    fdevent_complete(egc, tfe, rc);
+}
+
+static void fdevent_complete(libxl__egc *egc, libxl__test_fdevent *tfe,
+                             int rc)
+{
+    STATE_AO_GC(tfe->ao);
+    tfe_cleanup(gc, tfe);
+    libxl__ao_complete(egc, ao, rc);
+}
+
+int libxl_test_fdevent(libxl_ctx *ctx, int fd, short events,
+                       libxl_asyncop_how *ao_how)
+{
+    int rc;
+    libxl__test_fdevent *tfe;
+
+    AO_CREATE(ctx, 0, ao_how);
+    GCNEW(tfe);
+
+    tfe_init(tfe, ao);
+
+    rc = libxl__ev_fd_register(gc, &tfe->fd, tfe_fd_cb, fd, events);
+    if (rc) goto out;
+
+    tfe->abrt.ao = ao;
+    tfe->abrt.callback = tfe_abrt_cb;
+    rc = libxl__ao_abortable_register(&tfe->abrt);
+    if (rc) goto out;
+
+    return AO_INPROGRESS;
+
+ out:
+    tfe_cleanup(gc, tfe);
+    return AO_CREATE_FAIL(rc);
+}
diff --git a/tools/libxl/libxl_test_fdevent.h b/tools/libxl/libxl_test_fdevent.h
new file mode 100644
index 0000000..82a307e
--- /dev/null
+++ b/tools/libxl/libxl_test_fdevent.h
@@ -0,0 +1,12 @@
+#ifndef TEST_FDEVENT_H
+#define TEST_FDEVENT_H
+
+#include <pthread.h>
+
+int libxl_test_fdevent(libxl_ctx *ctx, int fd, short events,
+                       libxl_asyncop_how *ao_how)
+                       LIBXL_EXTERNAL_CALLERS_ONLY;
+/* This operation waits for one of the poll events to occur on fd, and
+ * then completes successfully.  (Or, it can be aborted.) */
+
+#endif /*TEST_FDEVENT_H*/
-- 
1.7.10.4

Re: [PATCH 6/9] libxl: event tests: Provide libxl_test_fdevent

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:54PM +0100, Ian Jackson wrote:
> We are going to use this shortly.  But, it is nicely self-contained.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 7/9] libxl: event tests: Provide miniature poll machinery

[Ian Jackson]

This allows a test case to have a poll-based event loop with exact
control of the sequence of operations.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/test_common.c |   44 +++++++++++++++++++++++++++++++++++++++++++-
 tools/libxl/test_common.h |   15 +++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/test_common.c b/tools/libxl/test_common.c
index 83b94eb..c6bbbab 100644
--- a/tools/libxl/test_common.c
+++ b/tools/libxl/test_common.c
@@ -12,4 +12,46 @@ void test_common_setup(int level)
 
     int rc = libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, logger);
     assert(!rc);
-}    
+}
+
+struct timeval now;
+
+void test_common_get_now(void)
+{
+    int r = gettimeofday(&now, 0);  assert(!r);
+}
+
+int poll_nfds, poll_nfds_allocd;
+struct pollfd *poll_fds;
+int poll_timeout;
+
+void test_common_beforepoll(void)
+{
+    for (;;) {
+        test_common_get_now();
+
+        poll_timeout = -1;
+        poll_nfds = poll_nfds_allocd;
+        int rc = libxl_osevent_beforepoll(ctx, &poll_nfds, poll_fds,
+                                          &poll_timeout, now);
+        if (!rc) return;
+        assert(rc == ERROR_BUFFERFULL);
+
+        assert(poll_nfds > poll_nfds_allocd);
+        poll_fds = realloc(poll_fds, poll_nfds * sizeof(poll_fds[0]));
+        assert(poll_fds);
+        poll_nfds_allocd = poll_nfds;
+    }
+}
+
+void test_common_dopoll(void) {
+    errno = 0;
+    int r = poll(poll_fds, poll_nfds, poll_timeout);
+    fprintf(stderr, "poll: r=%d errno=%s\n", r, strerror(errno));
+}
+
+void test_common_afterpoll(void)
+{
+    test_common_get_now();
+    libxl_osevent_afterpoll(ctx, poll_nfds, poll_fds, now);
+}
diff --git a/tools/libxl/test_common.h b/tools/libxl/test_common.h
index 8b2471e..10c7166 100644
--- a/tools/libxl/test_common.h
+++ b/tools/libxl/test_common.h
@@ -6,9 +6,24 @@
 #include <assert.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 
 void test_common_setup(int level);
 
 extern libxl_ctx *ctx;
 
+void test_common_get_now(void);
+
+extern struct timeval now;
+
+void test_common_beforepoll(void);
+void test_common_dopoll(void);
+void test_common_afterpoll(void);
+
+extern int poll_nfds, poll_nfds_allocd;
+extern struct pollfd *poll_fds;
+extern int poll_timeout;
+
 #endif /*TEST_COMMON_H*/
-- 
1.7.10.4

Re: [PATCH 7/9] libxl: event tests: Provide miniature poll machinery

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:55PM +0100, Ian Jackson wrote:
> This allows a test case to have a poll-based event loop with exact
> control of the sequence of operations.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 8/9] libxl: event tests: Introduce `fdderegrace' test

[Ian Jackson]

This exercises the potential race between fd deregistration and
poll().  (Because we have control of the individual steps, we can do
the whole test in a single thread and ensure that the pessimal order
is always reached.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 .gitignore                     |    1 +
 tools/libxl/Makefile           |    2 +-
 tools/libxl/test_fdderegrace.c |   56 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/test_fdderegrace.c

diff --git a/.gitignore b/.gitignore
index 3f42ded..464f3f4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -289,6 +289,7 @@ tools/libxl/testidl.c
 tools/libxl/*.pyc
 tools/libxl/libxl-save-helper
 tools/libxl/test_timedereg
+tools/libxl/test_fdderegrace
 tools/libxl/xen-init-dom0
 tools/blktap2/control/tap-ctl
 tools/firmware/etherboot/eb-roms.h
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index b92809c..95195c5 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -100,7 +100,7 @@ LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
 LIBXL_TESTS += timedereg
-LIBXL_TESTS_PROGS = $(LIBXL_TESTS)
+LIBXL_TESTS_PROGS = $(LIBXL_TESTS) fdderegrace
 LIBXL_TESTS_INSIDE = $(LIBXL_TESTS) fdevent
 
 # Each entry FOO in LIBXL_TESTS has two main .c files:
diff --git a/tools/libxl/test_fdderegrace.c b/tools/libxl/test_fdderegrace.c
new file mode 100644
index 0000000..b644d7a
--- /dev/null
+++ b/tools/libxl/test_fdderegrace.c
@@ -0,0 +1,56 @@
+#include "test_common.h"
+#include "libxl_test_fdevent.h"
+
+int main(int argc, char **argv) {
+    int rc, i;
+    libxl_asyncop_how how;
+    libxl_event *event;
+
+    test_common_setup(XTL_DEBUG);
+
+    how.callback = NULL;
+    how.u.for_event = 1;
+
+    int fd = open("/dev/null", O_RDONLY);
+    assert(fd > 0);
+
+    rc = libxl_test_fdevent(ctx, fd, POLLIN, &how);
+    assert(!rc);
+
+    test_common_beforepoll();
+
+    rc = libxl_ao_abort(ctx, &how);
+    assert(!rc);
+
+    rc = libxl_event_check(ctx, &event, LIBXL_EVENTMASK_ALL, 0,0);
+    assert(!rc);
+    assert(event);
+    assert(event->for_user = how.u.for_event);
+    assert(event->type == LIBXL_EVENT_TYPE_OPERATION_COMPLETE);
+    assert(event->u.operation_complete.rc == ERROR_ABORTED);
+
+    close(fd);
+
+    test_common_dopoll();
+
+    for (i=0; i<poll_nfds; i++) {
+        if (poll_fds[i].fd == fd && (poll_fds[i].revents & POLLNVAL)) {
+            fprintf(stderr, "POLLNVAL on fd=%d in slot i=%d as expected\n",
+                    fd, i);
+            goto found;
+        }
+    }
+    abort();
+ found:;
+
+    int fd2 = open("/dev/null", O_RDONLY);
+    assert(fd2 == fd);
+
+    how.u.for_event++;
+    rc = libxl_test_fdevent(ctx, fd, POLLIN, &how);
+    assert(!rc);
+
+    test_common_afterpoll();
+
+    fprintf(stderr, "complete\n");
+}
-- 
1.7.10.4

Re: [PATCH 8/9] libxl: event tests: Introduce `fdderegrace' test

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:56PM +0100, Ian Jackson wrote:
> This exercises the potential race between fd deregistration and
> poll().  (Because we have control of the individual steps, we can do
> the whole test in a single thread and ensure that the pessimal order
> is always reached.)
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

[PATCH 9/9] libxl: event tests: test_timedereg: Fix rc handling

[Ian Jackson]

In 31c836f4 "libxl: events: Permit timeouts to signal ao abort",
timeout callbacks take an extra rc argument.

In that patch the wrong assertion is made about the rc in
test_timedereg's `occurs' callback.  Fix this to make the test pass
again.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_test_timedereg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_test_timedereg.c b/tools/libxl/libxl_test_timedereg.c
index c464663..a567db6 100644
--- a/tools/libxl/libxl_test_timedereg.c
+++ b/tools/libxl/libxl_test_timedereg.c
@@ -67,7 +67,7 @@ static void occurs(libxl__egc *egc, libxl__ev_time *ev,
     int off = ev - &et[0][0];
     LOG(DEBUG,"occurs[%d][%d] seq=%d rc=%d", off/NTIMES, off%NTIMES, seq, rc);
 
-    assert(!rc);
+    assert(rc == ERROR_TIMEDOUT);
 
     switch (seq) {
     case 0:
-- 
1.7.10.4

Re: [PATCH 9/9] libxl: event tests: test_timedereg: Fix rc handling

[Wei Liu]

On Thu, Jul 09, 2015 at 06:47:57PM +0100, Ian Jackson wrote:
> In 31c836f4 "libxl: events: Permit timeouts to signal ao abort",
> timeout callbacks take an extra rc argument.
> 
> In that patch the wrong assertion is made about the rc in
> test_timedereg's `occurs' callback.  Fix this to make the test pass
> again.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/libxl_test_timedereg.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_test_timedereg.c b/tools/libxl/libxl_test_timedereg.c
> index c464663..a567db6 100644
> --- a/tools/libxl/libxl_test_timedereg.c
> +++ b/tools/libxl/libxl_test_timedereg.c
> @@ -67,7 +67,7 @@ static void occurs(libxl__egc *egc, libxl__ev_time *ev,
>      int off = ev - &et[0][0];
>      LOG(DEBUG,"occurs[%d][%d] seq=%d rc=%d", off/NTIMES, off%NTIMES, seq, rc);
>  
> -    assert(!rc);
> +    assert(rc == ERROR_TIMEDOUT);
>  
>      switch (seq) {
>      case 0:
> -- 
> 1.7.10.4

Re: [PATCH 9/9] libxl: event tests: test_timedereg: Fix rc handling [and 1 more messages]

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 9/9] libxl: event tests: test_timedereg: Fix rc handling"):
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks for the review.

Jim Fehlig writes ("Re: [Xen-devel] [PATCH 0/9] libxl: poll: Avoid fd deregistration race	POLLNVAL"):
> FYI, I ran these patches in my test setup over the weekend and never
> encountered the bug.  So for patches 1-3
>      Tested-by: Jim Fehlig <jfehlig@suse.com>

And, thanks for the report and the testing.  I have pushed all 9 to
staging for 4.6, and queued the first three for backport.

Ian.

Re: [PATCH 0/9] libxl: poll: Avoid fd deregistration race POLLNVAL

[Jim Fehlig]

Ian Jackson wrote:
> The first three patches in this series fix a race which I think is the
> cause of a bug reported by Jim Fehlig (thanks, Jim).  They are IMO
> backport candidates.
>   

FYI, I ran these patches in my test setup over the weekend and never
encountered the bug.  So for patches 1-3

     Tested-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

Re: [PATCH 0/9] libxl: poll: Avoid fd deregistration race POLLNVAL

[Ian Jackson]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH 0/9] libxl: poll: Avoid fd deregistration race	POLLNVAL"):
> Ian Jackson wrote:
> > The first three patches in this series fix a race which I think is the
> > cause of a bug reported by Jim Fehlig (thanks, Jim).  They are IMO
> > backport candidates.
> 
> FYI, I ran these patches in my test setup over the weekend and never
> encountered the bug.  So for patches 1-3
> 
>      Tested-by: Jim Fehlig <jfehlig@suse.com>

Thanks for all your valuable testing!

Ian.