Mailing List Archive

[PATCH 14/15] libxl: New API for providing OS events to libxl
We provide a new set of functions and related structures
libxl_osevent_*
which are to be used by event-driven applications to receive
information from libxl about which fds libxl is interested in, and
what timeouts libxl is waiting for, and to pass back to libxl
information about which fds are readable/writeable etc., and which
timeouts have occurred. Ie, low-level events.

In this patch, this new machinery is still all unused. Callers will
appear in the next patch in the series, which introduces a new API for
applications to receive high-level events about actual domains etc.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.c | 25 ++
tools/libxl/libxl.h | 6 +
tools/libxl/libxl_event.c | 711 ++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_event.h | 199 ++++++++++++
tools/libxl/libxl_internal.h | 216 +++++++++++++-
6 files changed, 1155 insertions(+), 4 deletions(-)
create mode 100644 tools/libxl/libxl_event.c
create mode 100644 tools/libxl/libxl_event.h

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 4e0f3fb..3d575b8 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -38,7 +38,7 @@ LIBXL_LIBS += -lyajl
LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
libxl_internal.o libxl_utils.o libxl_uuid.o libxl_json.o \
- libxl_qmp.o $(LIBXL_OBJS-y)
+ libxl_qmp.o libxl_event.o $(LIBXL_OBJS-y)
LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o

$(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3a8cfe3..58f280c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -60,6 +60,16 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
* only as an initialiser, not as an expression. */
memcpy(&ctx->lock, &mutex_value, sizeof(ctx->lock));

+ ctx->osevent_hooks = 0;
+
+ ctx->fd_beforepolled = 0;
+ LIBXL_LIST_INIT(&ctx->efds);
+ LIBXL_TAILQ_INIT(&ctx->etimes);
+
+ ctx->watch_slots = 0;
+ LIBXL_SLIST_INIT(&ctx->watch_freeslots);
+ libxl__ev_fd_init(&ctx->watch_efd);
+
if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
"failed to stat %s", XENSTORE_PID_FILE);
@@ -89,10 +99,25 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,

int libxl_ctx_free(libxl_ctx *ctx)
{
+ int i;
+ GC_INIT(ctx);
+
if (!ctx) return 0;
+
+ for (i = 0; i < ctx->watch_nslots; i++)
+ assert(!libxl__watch_slot_contents(gc, i));
+ libxl__ev_fd_deregister(gc, &ctx->watch_efd);
+
+ assert(LIBXL_LIST_EMPTY(&ctx->efds));
+ assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
+
if (ctx->xch) xc_interface_close(ctx->xch);
libxl_version_info_dispose(&ctx->version_info);
if (ctx->xsh) xs_daemon_close(ctx->xsh);
+
+ free(ctx->fd_beforepolled);
+ free(ctx->watch_slots);
+
return 0;
}

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 289dc85..654a5b0 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -137,6 +137,7 @@
#include <xen/sysctl.h>

#include <libxl_uuid.h>
+#include <_libxl_list.h>

typedef uint8_t libxl_mac[6];
#define LIBXL_MAC_FMT "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx"
@@ -222,6 +223,9 @@ enum {
ERROR_BADFAIL = -7,
ERROR_GUEST_TIMEDOUT = -8,
ERROR_TIMEDOUT = -9,
+ ERROR_NOT_READY = -10,
+ ERROR_OSEVENT_REG_FAIL = -11,
+ ERROR_BUFFERFULL = -12,
};

#define LIBXL_VERSION 0
@@ -635,6 +639,8 @@ const char *libxl_lock_dir_path(void);
const char *libxl_run_dir_path(void);
const char *libxl_xenpaging_dir_path(void);

+#include <libxl_event.h>
+
#endif /* LIBXL_H */

/*
diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
new file mode 100644
index 0000000..8d4dbf6
--- /dev/null
+++ b/tools/libxl/libxl_event.c
@@ -0,0 +1,711 @@
+/*
+ * Copyright (C) 2011 Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+/*
+ * Internal event machinery for use by other parts of libxl
+ */
+
+#include <poll.h>
+
+#include "libxl_internal.h"
+
+/*
+ * The counter osevent_in_hook is used to ensure that the application
+ * honours the reentrancy restriction documented in libxl_event.h.
+ *
+ * The application's registration hooks should be called ONLY via
+ * these macros, with the ctx locked. Likewise all the "occurred"
+ * entrypoints from the application should assert(!in_hook);
+ */
+#define OSEVENT_HOOK_INTERN(defval, hookname, ...) \
+ (CTX->osevent_hooks \
+ ? (CTX->osevent_in_hook++, \
+ CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__), \
+ CTX->osevent_in_hook--) \
+ : defval)
+
+#define OSEVENT_HOOK(hookname,...) \
+ OSEVENT_HOOK_INTERN(0, hookname, __VA_ARGS__)
+
+#define OSEVENT_HOOK_VOID(hookname,...) \
+ OSEVENT_HOOK_INTERN((void)0, hookname, __VA_ARGS__)
+
+/*
+ * fd events
+ */
+
+int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
+ libxl__ev_fd_callback *func,
+ int fd, short events) {
+ int rc;
+
+ assert(fd >= 0);
+
+ CTX_LOCK;
+
+ rc = OSEVENT_HOOK(fd_register, fd, &ev->for_app_reg, events, ev);
+ if (rc) goto out;
+
+ LIBXL_LIST_INSERT_HEAD(&CTX->efds, ev, entry);
+
+ ev->fd = fd;
+ ev->events = events;
+ ev->in_beforepolled = -1;
+ ev->func = func;
+
+ rc = 0;
+
+ out:
+ CTX_UNLOCK;
+ return rc;
+}
+
+int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev, short events) {
+ int rc;
+
+ CTX_LOCK;
+ assert(libxl__ev_fd_isregistered(ev));
+
+ rc = OSEVENT_HOOK(fd_modify, ev->fd, &ev->for_app_reg, events);
+ if (rc) goto out;
+
+ ev->events = events;
+
+ rc = 0;
+ out:
+ CTX_UNLOCK;
+ return rc;
+}
+
+void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev) {
+ CTX_LOCK;
+
+ if (!libxl__ev_fd_isregistered(ev))
+ goto out;
+
+ OSEVENT_HOOK_VOID(fd_deregister, ev->fd, ev->for_app_reg);
+ LIBXL_LIST_REMOVE(ev, entry);
+ ev->fd = -1;
+
+ if (ev->in_beforepolled >= 0 &&
+ ev->in_beforepolled < CTX->fd_beforepolled_used)
+ /* remove stale reference */
+ CTX->fd_beforepolled[ev->in_beforepolled] = NULL;
+
+ out:
+ CTX_UNLOCK;
+}
+
+/*
+ * timeouts
+ */
+
+
+int libxl__gettimeofday(libxl__gc *gc, struct timeval *now_r) {
+ int rc = gettimeofday(now_r, 0);
+ if (rc) {
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR, "gettimeofday failed");
+ return ERROR_FAIL;
+ }
+ return 0;
+}
+
+static int time_rel_to_abs(libxl__gc *gc, int ms, struct timeval *abs_out) {
+ int rc;
+ struct timeval additional = {
+ .tv_sec = ms / 1000,
+ .tv_usec = (ms % 1000) * 1000
+ };
+ struct timeval now;
+
+ rc = libxl__gettimeofday(gc, &now);
+ if (rc) return rc;
+
+ timeradd(&now, &additional, abs_out);
+ return 0;
+}
+
+static void time_insert_finite(libxl__gc *gc, libxl__ev_time *ev) {
+ libxl__ev_time *evsearch;
+ LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch, ,
+ timercmp(&ev->abs, &evsearch->abs, >));
+ ev->infinite = 0;
+}
+
+static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev,
+ struct timeval abs) {
+ int rc;
+
+ rc = OSEVENT_HOOK(timeout_register, &ev->for_app_reg, abs, ev);
+ if (rc) return rc;
+
+ ev->infinite = 0;
+ ev->abs = abs;
+ time_insert_finite(gc, ev);
+
+ return 0;
+}
+
+static void time_deregister(libxl__gc *gc, libxl__ev_time *ev) {
+ if (!ev->infinite) {
+ OSEVENT_HOOK_VOID(timeout_deregister, &ev->for_app_reg);
+ LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
+ }
+}
+
+
+int libxl__ev_time_register_abs(libxl__gc *gc, libxl__ev_time *ev,
+ libxl__ev_time_callback *func,
+ struct timeval abs) {
+ int rc;
+
+ CTX_LOCK;
+
+ rc = time_register_finite(gc, ev, abs);
+ if (rc) goto out;
+
+ ev->func = func;
+
+ rc = 0;
+ out:
+ CTX_UNLOCK;
+ return rc;
+}
+
+
+int libxl__ev_time_register_rel(libxl__gc *gc, libxl__ev_time *ev,
+ libxl__ev_time_callback *func,
+ int milliseconds /* as for poll(2) */) {
+ struct timeval abs;
+ int rc;
+
+ CTX_LOCK;
+
+ if (milliseconds < 0) {
+ ev->infinite = 1;
+ } else {
+ rc = time_rel_to_abs(gc, milliseconds, &abs);
+ if (rc) goto out;
+
+ rc = time_register_finite(gc, ev, abs);
+ if (rc) goto out;
+ }
+
+ ev->func = func;
+ rc = 0;
+
+ out:
+ CTX_UNLOCK;
+ return 0;
+}
+
+int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
+ struct timeval abs) {
+ int rc;
+
+ CTX_LOCK;
+
+ assert(libxl__ev_time_isregistered(ev));
+
+ if (ev->infinite) {
+ rc = time_register_finite(gc, ev, abs);
+ if (rc) goto out;
+ } else {
+ rc = OSEVENT_HOOK(timeout_modify, &ev->for_app_reg, abs);
+ if (rc) goto out;
+
+ LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
+ ev->abs = abs;
+ time_insert_finite(gc, ev);
+ }
+
+ rc = 0;
+ out:
+ CTX_UNLOCK;
+ return rc;
+}
+
+int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev,
+ int milliseconds) {
+ struct timeval abs;
+ int rc;
+
+ CTX_LOCK;
+
+ assert(libxl__ev_time_isregistered(ev));
+
+ if (milliseconds < 0) {
+ time_deregister(gc, ev);
+ ev->infinite = 1;
+ rc = 0;
+ goto out;
+ }
+
+ rc = time_rel_to_abs(gc, milliseconds, &abs);
+ if (rc) goto out;
+
+ rc = libxl__ev_time_modify_abs(gc, ev, abs);
+ if (rc) goto out;
+
+ rc = 0;
+ out:
+ CTX_UNLOCK;
+ return 0;
+}
+
+void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev) {
+ CTX_LOCK;
+
+ if (!libxl__ev_time_isregistered(ev))
+ goto out;
+
+ time_deregister(gc, ev);
+ ev->func = 0;
+
+ out:
+ CTX_UNLOCK;
+ return;
+}
+
+
+/*
+ * xenstore watches
+ */
+
+libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum) {
+ libxl__ev_watch_slot *slot = &CTX->watch_slots[slotnum];
+ libxl__ev_watch_slot *slotcontents = LIBXL_SLIST_NEXT(slot, empty);
+
+ if (slotcontents == NULL ||
+ ((uintptr_t)slotcontents >= (uintptr_t)CTX->watch_slots &&
+ (uintptr_t)slotcontents < (uintptr_t)(CTX->watch_slots +
+ CTX->watch_nslots)))
+ /* An empty slot has either a NULL pointer (end of the
+ * free list), or a pointer to another entry in the array.
+ * So we can do a bounds check to distinguish empty from
+ * full slots.
+ */
+ /* We need to do the comparisons as uintptr_t because
+ * comparing pointers which are not in the same object is
+ * undefined behaviour; if the compiler managed to figure
+ * out that watch_slots[0..watch_nslots-1] is all of the
+ * whole array object it could prove that the above bounds
+ * check was always true if it was legal, and remove it!
+ *
+ * uintptr_t because even on a machine with signed
+ * pointers, objects do not cross zero; whereas on
+ * machines with unsigned pointers, they may cross
+ * 0x8bazillion.
+ */
+ return NULL;
+
+ /* see comment near libxl__ev_watch_slot definition */
+ return (void*)slotcontents;
+}
+
+static void watchfd_callback(libxl__gc *gc, libxl__ev_fd *ev,
+ int fd, short events, short revents) {
+ for (;;) {
+ char **event = xs_check_watch(CTX->xsh);
+ if (!event) {
+ if (errno == EAGAIN) break;
+ if (errno == EINTR) continue;
+ LIBXL__EVENT_DISASTER(gc, "cannot check/read watches", errno, 0);
+ return;
+ }
+
+ const char *epath = event[0];
+ const char *token = event[1];
+ int slotnum;
+ uint32_t counterval;
+ int rc = sscanf(token, "%d/%"SCNx32, &slotnum, &counterval);
+ if (rc != 2) {
+ LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+ "watch epath=%s token=%s: failed to parse token",
+ epath, token);
+ /* oh well */
+ goto ignore;
+ }
+ if (slotnum < 0 || slotnum >= CTX->watch_nslots) {
+ /* perhaps in the future we will make the watchslots array shrink */
+ LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, "watch epath=%s token=%s:"
+ " slotnum %d out of range [.0,%d>",
+ epath, token, slotnum, CTX->watch_nslots);
+ goto ignore;
+ }
+
+ libxl__ev_xswatch *w = libxl__watch_slot_contents(gc, slotnum);
+
+ if (!w) {
+ LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
+ "watch epath=%s token=%s: empty slot",
+ epath, token);
+ goto ignore;
+ }
+
+ if (w->counterval != counterval) {
+ LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
+ "watch epath=%s token=%s: counter != %"PRIx32,
+ epath, token, w->counterval);
+ goto ignore;
+ }
+
+ /* Now it's possible, though unlikely, that this was an event
+ * from a previous use of the same slot with the same counterval.
+ *
+ * In that case either:
+ * - the event path is a child of the watch path, in
+ * which case this watch would really have generated this
+ * event if it had been registered soon enough and we are
+ * OK to give this possibly-spurious event to the caller; or
+ * - it is not, in which case we must suppress it as the
+ * caller should not see events for unrelated paths.
+ *
+ * See also docs/misc/xenstore.txt.
+ */
+ size_t epathlen = strlen(epath);
+ size_t wpathlen = strlen(w->path);
+ if (epathlen < wpathlen ||
+ memcmp(epath, w->path, wpathlen) ||
+ (epathlen > wpathlen && epath[wpathlen] != '/')) {
+ LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
+ "watch epath=%s token=%s: not child of wpath=%s",
+ epath, token, w->path);
+ goto ignore;
+ }
+
+ /* At last, we have checked everything! */
+ LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
+ "watch event: epath=%s token=%s wpath=%s w=%p",
+ epath, token, w->path, w);
+ w->callback(gc, w, w->path, epath);
+
+ ignore:
+ free(event);
+ }
+}
+
+static char *watch_token(libxl__gc *gc, int slotnum, uint32_t counterval) {
+ return libxl__sprintf(gc, "%d/%"PRIx32, slotnum, counterval);
+}
+
+int libxl__ev_xswatch_register(libxl__gc *gc, libxl__ev_xswatch *w,
+ libxl__ev_xswatch_callback *func,
+ const char *path /* copied */) {
+ libxl__ev_watch_slot *use = NULL;
+ char *path_copy = NULL;
+ int rc;
+
+ CTX_LOCK;
+
+ if (!libxl__ev_fd_isregistered(&CTX->watch_efd)) {
+ rc = libxl__ev_fd_register(gc, &CTX->watch_efd, watchfd_callback,
+ xs_fileno(CTX->xsh), POLLIN);
+ if (rc) goto out_rc;
+ }
+
+ if (LIBXL_SLIST_EMPTY(&CTX->watch_freeslots)) {
+ /* Free list is empty so there is not in fact a linked
+ * free list in the array and we can safely realloc it */
+ int newarraysize = (CTX->watch_nslots + 1) << 2;
+ int i;
+ libxl__ev_watch_slot *newarray =
+ realloc(CTX->watch_slots, sizeof(*newarray) * newarraysize);
+ if (!newarray) goto out_nomem;
+ for (i=CTX->watch_nslots; i<newarraysize; i++)
+ LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots,
+ &newarray[i], empty);
+ CTX->watch_slots = newarray;
+ CTX->watch_nslots = newarraysize;
+ }
+ use = LIBXL_SLIST_FIRST(&CTX->watch_freeslots);
+ assert(use);
+ LIBXL_SLIST_REMOVE_HEAD(&CTX->watch_freeslots, empty);
+
+ path_copy = strdup(path);
+ if (!path_copy) goto out_nomem;
+
+ int slotnum = use - CTX->watch_slots;
+ w->counterval = CTX->watch_counter++;
+
+ if (!xs_watch(CTX->xsh, path, watch_token(gc, slotnum, w->counterval))) {
+ LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
+ "create watch for path %s", path);
+ rc = ERROR_FAIL;
+ goto out_rc;
+ }
+
+ w->slotnum = slotnum;
+ w->path = path_copy;
+ w->callback = func;
+ /* we look a bit behind the curtain of LIBXL_SLIST, to explictly
+ * assign to the pointer that's the next link. See the comment
+ * by the definitionn of libxl__ev_watch_slot */
+ use->empty.sle_next = (void*)w;
+
+ CTX_UNLOCK;
+ return 0;
+
+ out_nomem:
+ rc = ERROR_NOMEM;
+ out_rc:
+ if (use)
+ LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, use, empty);
+ if (path_copy)
+ free(path_copy);
+ CTX_UNLOCK;
+ return rc;
+}
+
+void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch *w) {
+ /* it is legal to deregister from within _callback */
+ CTX_LOCK;
+
+ if (w->slotnum >= 0) {
+ char *token = watch_token(gc, w->slotnum, w->counterval);
+ if (!xs_unwatch(CTX->xsh, w->path, token))
+ /* Oh well, we will just get watch events forever more
+ * and ignore them. But we should complain to the log. */
+ LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
+ "remove watch for path %s", w->path);
+
+ libxl__ev_watch_slot *slot = &CTX->watch_slots[w->slotnum];
+ LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, slot, empty);
+ }
+
+ free(w->path);
+ w->path = NULL;
+
+ CTX_UNLOCK;
+}
+
+/*
+ * osevent poll
+ */
+
+int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
+ struct pollfd *fds, int *timeout_upd,
+ struct timeval now) {
+ libxl__ev_fd *efd;
+ int i;
+
+ /*
+ * In order to be able to efficiently find the libxl__ev_fd
+ * for a struct poll during _afterpoll, we maintain a shadow
+ * data structure in CTX->fd_beforepolled: each slot in
+ * the fds array corresponds to a slot in fd_beforepolled.
+ */
+
+ GC_INIT(ctx);
+ CTX_LOCK;
+
+ if (*nfds_io) {
+ /*
+ * As an optimisation, we don't touch fd_beforepolled_used
+ * if *nfds_io is zero on entry, since in that case the
+ * caller just wanted to know how big an array to give us.
+ *
+ * If !*nfds_io, the unconditional parts below are guaranteed
+ * not to mess with fd_beforepolled... or any in_beforepolled.
+ */
+
+ /* Remove all the old references into beforepolled */
+ for (i = 0; i < CTX->fd_beforepolled_used; i++) {
+ efd = CTX->fd_beforepolled[i];
+ if (efd) {
+ assert(efd->in_beforepolled == i);
+ efd->in_beforepolled = -1;
+ CTX->fd_beforepolled[i] = NULL;
+ }
+ }
+ CTX->fd_beforepolled_used = 0;
+
+ /* make sure our array is as big as *nfds_io */
+ if (CTX->fd_beforepolled_allocd < *nfds_io) {
+ assert(*nfds_io < INT_MAX / sizeof(libxl__ev_fd*) / 2);
+ libxl__ev_fd **newarray =
+ realloc(CTX->fd_beforepolled, sizeof(*newarray) * *nfds_io);
+ if (!newarray)
+ return ERROR_NOMEM;
+ CTX->fd_beforepolled = newarray;
+ CTX->fd_beforepolled_allocd = *nfds_io;
+ }
+ }
+
+ int used = 0;
+ LIBXL_LIST_FOREACH(efd, &CTX->efds, entry) {
+ if (used < *nfds_io) {
+ fds[used].fd = efd->fd;
+ fds[used].events = efd->events;
+ fds[used].revents = 0;
+ CTX->fd_beforepolled[used] = efd;
+ efd->in_beforepolled = used;
+ }
+ used++;
+ }
+ int rc = used <= *nfds_io ? 0 : ERROR_BUFFERFULL;
+
+ if (*nfds_io) {
+ CTX->fd_beforepolled_used = used;
+ }
+
+ *nfds_io = used;
+
+ libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
+ if (etime) {
+ int our_timeout;
+ struct timeval rel;
+ static struct timeval zero;
+
+ timersub(&etime->abs, &now, &rel);
+
+ if (timercmp(&rel, &zero, <)) {
+ our_timeout = 0;
+ } else if (rel.tv_sec >= 2000000) {
+ our_timeout = 2000000000;
+ } else {
+ our_timeout = rel.tv_sec * 1000 + (rel.tv_usec + 999) / 1000;
+ }
+ if (*timeout_upd < 0 || our_timeout < *timeout_upd)
+ *timeout_upd = our_timeout;
+ }
+
+ CTX_UNLOCK;
+ GC_FREE;
+ return rc;
+}
+
+void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
+ struct timeval now) {
+ int i;
+ GC_INIT(ctx);
+ CTX_LOCK;
+
+ assert(nfds <= CTX->fd_beforepolled_used);
+
+ for (i = 0; i < nfds; i++) {
+ if (!fds[i].revents)
+ continue;
+
+ libxl__ev_fd *efd = CTX->fd_beforepolled[i];
+ if (!efd)
+ continue;
+
+ assert(efd->in_beforepolled == i);
+ assert(fds[i].fd == efd->fd);
+
+ int revents = fds[i].revents & efd->events;
+ if (!revents)
+ continue;
+
+ efd->func(gc, efd, efd->fd, efd->events, revents);
+ }
+
+ for (;;) {
+ libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
+ if (!etime)
+ break;
+
+ assert(!etime->infinite);
+
+ if (timercmp(&etime->abs, &now, >))
+ break;
+
+ time_deregister(gc, etime);
+
+ etime->func(gc, etime, &etime->abs);
+ }
+
+ CTX_UNLOCK;
+ GC_FREE;
+}
+
+
+/*
+ * osevent hook and callback machinery
+ */
+
+void libxl_osevent_register_hooks(libxl_ctx *ctx,
+ const libxl_osevent_hooks *hooks,
+ void *user) {
+ GC_INIT(ctx);
+ CTX_LOCK;
+ ctx->osevent_hooks = hooks;
+ ctx->osevent_user = user;
+ CTX_UNLOCK;
+ GC_FREE;
+}
+
+
+void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
+ int fd, short events, short revents) {
+ libxl__ev_fd *ev = for_libxl;
+
+ GC_INIT(ctx);
+ CTX_LOCK;
+ assert(!CTX->osevent_in_hook);
+
+ assert(fd == ev->fd);
+ revents &= ev->events;
+ if (revents)
+ ev->func(gc, ev, fd, ev->events, revents);
+
+ CTX_UNLOCK;
+ GC_FREE;
+}
+
+void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl) {
+ libxl__ev_time *ev = for_libxl;
+
+ GC_INIT(ctx);
+ CTX_LOCK;
+ assert(!CTX->osevent_in_hook);
+
+ assert(!ev->infinite);
+ LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
+ ev->func(gc, ev, &ev->abs);
+
+ CTX_UNLOCK;
+ GC_FREE;
+}
+
+void libxl__event_disaster(libxl__gc *gc, const char *msg, int errnoval,
+ libxl_event_type type /* may be 0 */,
+ const char *file, int line, const char *func) {
+ libxl__log(CTX, XTL_CRITICAL, errnoval, file, line, func,
+ "DISASTER in event loop: %s%s%s%s",
+ msg,
+ type ? " (relates to event type " : "",
+ type ? libxl_event_type_to_string(type) : "",
+ type ? ")" : "");
+
+ /*
+ * FIXME: This should call the "disaster" hook supplied to
+ * libxl_event_register_callbacks, which will be introduced in the
+ * next patch.
+ */
+
+ const char verybad[] =
+ "DISASTER in event loop not handled by libxl application";
+ LIBXL__LOG(CTX, XTL_CRITICAL, verybad);
+ fprintf(stderr, "libxl: fatal error, exiting program: %s\n", verybad);
+ exit(-1);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
new file mode 100644
index 0000000..25efbdf
--- /dev/null
+++ b/tools/libxl/libxl_event.h
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2011 Citrix Ltd.
+ * Author Ian Jackson <ian.jackson@eu.citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#ifndef LIBXL_EVENT_H
+#define LIBXL_EVENT_H
+
+#include <libxl.h>
+
+
+/*======================================================================*/
+
+/*
+ * OS event handling - passing low-level OS events to libxl
+ *
+ * Event-driven programs must use these facilities to allow libxl
+ * to become aware of readability/writeability of file descriptors
+ * and the occurrence of timeouts.
+ *
+ * There are two approaches available. The first is appropriate for
+ * simple programs handling reasonably small numbers of domains:
+ *
+ * for (;;) {
+ * libxl_osevent_beforepoll(...)
+ * poll();
+ * libxl_osevent_afterpoll(...);
+ * for (;;) {
+ * r=libxl_event_check(...);
+ * if (r==LIBXL_NOT_READY) break;
+ * if (r) handle failure;
+ * do something with the event;
+ * }
+ * }
+ *
+ * The second approach uses libxl_osevent_register_hooks and is
+ * suitable for programs which are already using a callback-based
+ * event library.
+ *
+ * An application may freely mix the two styles of interaction.
+ */
+
+struct pollfd;
+
+int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
+ struct pollfd *fds, int *timeout_upd,
+ struct timeval now);
+ /* The caller should provide beforepoll with some space for libxl's
+ * fds, and tell libxl how much space is available by setting *nfds_io.
+ * fds points to the start of this space (and fds may be a pointer into
+ * a larger array, for example, if the application has some fds of
+ * its own that it is interested in).
+ *
+ * On return *nfds_io will in any case have been updated by libxl
+ * according to how many fds libxl wants to poll on.
+ *
+ * If the space was sufficient, libxl fills in fds[0..<new
+ * *nfds_io>] suitably for poll(2), updates *timeout_upd if needed,
+ * and returns ok.
+ *
+ * If space was insufficient, fds[0..<old *nfds_io>] is undefined on
+ * return; *nfds_io on return will be greater than the value on
+ * entry; *timeout_upd may or may not have been updated; and
+ * libxl_osevent_beforepoll returns ERROR_BUFERFULL. In this case
+ * the application needs to make more space (enough space for
+ * *nfds_io struct pollfd) and then call beforepoll again, before
+ * entering poll(2). Typically this will involve calling realloc.
+ *
+ * The application may call beforepoll with fds==NULL and
+ * *nfds_io==0 in order to find out how much space is needed.
+ *
+ * *timeout_upd is as for poll(2): it's in milliseconds, and
+ * negative values mean no timeout (infinity).
+ * libxl_osevent_beforepoll will only reduce the timeout, naturally.
+ */
+void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
+ struct timeval now);
+ /* nfds and fds[0..nfds] must be from the most recent call to
+ * _beforepoll, as modified by poll.
+ *
+ * This function actually performs all of the IO and other actions,
+ * and generates events (libxl_event), which are implied by either
+ * (a) the time of day or (b) both (i) the returned information from
+ * _beforepoll, and (ii) the results from poll specified in
+ * fds[0..nfds-1]. Generated events can then be retrieved by
+ * libxl_event_check.
+ */
+
+
+typedef struct libxl_osevent_hooks {
+ int (*fd_register)(void *user, int fd, void **for_app_registration_out,
+ short events, void *for_libxl);
+ int (*fd_modify)(void *user, int fd, void **for_app_registration_update,
+ short events);
+ void (*fd_deregister)(void *user, int fd, void *for_app_registration);
+ int (*timeout_register)(void *user, void **for_app_registration_out,
+ struct timeval abs, void *for_libxl);
+ int (*timeout_modify)(void *user, void **for_app_registration_update,
+ struct timeval abs);
+ void (*timeout_deregister)(void *user, void *for_app_registration_io);
+} libxl_osevent_hooks;
+
+void libxl_osevent_register_hooks(libxl_ctx *ctx,
+ const libxl_osevent_hooks *hooks,
+ void *user);
+ /* The application which calls register_fd_hooks promises to
+ * maintain a register of fds and timeouts that libxl is interested
+ * in, and make calls into libxl (libxl_osevent_occurred_*)
+ * when those fd events and timeouts occur. This is more efficient
+ * than _beforepoll/_afterpoll if there are many fds (which can
+ * happen if the same libxl application is managing many domains).
+ *
+ * For an fd event, events is as for poll(). register or modify may
+ * be called with events==0, in which case it must still work
+ * normally, just not generate any events.
+ *
+ * For a timeout event, milliseconds is as for poll().
+ * Specifically, negative values of milliseconds mean NO TIMEOUT.
+ * This is used by libxl to temporarily disable a timeout.
+ *
+ * If the register or modify hook succeeds it may update
+ * *for_app_registration_out/_update and must then return 0.
+ * On entry to register, *for_app_registration_out is always NULL.
+ *
+ * A registration or modification hook may fail, in which case it
+ * must leave the registration state of the fd or timeout unchanged.
+ * It may then either return ERROR_OSEVENT_REG_FAIL or any positive
+ * int. The value returned will be passed up through libxl and
+ * eventually returned back to the application. When register
+ * fails, any value stored into *for_registration_out is ignored by
+ * libxl; when modify fails, any changed value stored into
+ * *for_registration_update is honoured by libxl and will be passed
+ * to future modify or deregister calls.
+ *
+ * libxl will only attempt to register one callback for any one fd.
+ * libxl will remember the value stored in *for_app_registration_io
+ * by a successful call to register or modify and pass it into
+ * subsequent calls to modify or deregister.
+ *
+ * register_fd_hooks may be called only once for each libxl_ctx.
+ * libxl may make calls to register/modify/deregister from within
+ * any libxl function (indeed, it will usually call register from
+ * register_event_hooks). Conversely, the application MUST NOT make
+ * the event occurrence calls (libxl_osevent_occurred_*) into libxl
+ * reentrantly from within libxl (for example, from within the
+ * register/modify functions).
+ *
+ * Lock hierarchy: the register/modify/deregister functions may be
+ * called with locks held. These locks (the "libxl internal locks")
+ * are inside the libxl_ctx. Therefore, if those register functions
+ * acquire any locks of their own ("caller register locks") outside
+ * libxl, to avoid deadlock one of the following must hold for each
+ * such caller register lock:
+ * (a) "acquire libxl internal locks before caller register lock":
+ * No libxl function may be called with the caller register
+ * lock held.
+ * (b) "acquire caller register lock before libxl internal locks":
+ * No libxl function may be called _without_ the caller
+ * register lock held.
+ * Of these we would normally recommend (a).
+ *
+ * The value *hooks is not copied and must outlast the libxl_ctx.
+ */
+
+/* It is NOT legal to call _occurred_ reentrantly within any libxl
+ * function. Specifically it is NOT legal to call it from within
+ * a register callback. Conversely, libxl MAY call register/deregister
+ * from within libxl_event_registered_call_*.
+ */
+
+void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
+ int fd, short events, short revents);
+
+void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
+ /* Implicitly, on entry to this function the timeout has been
+ * deregistered. If _occurred_timeout is called, libxl will not
+ * call timeout_deregister; if it wants to requeue the timeout it
+ * will call timeout_register again.
+ */
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d015c7c..88e7dbb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -24,6 +24,9 @@
#include <stdlib.h>
#include <string.h>
#include <pthread.h>
+#include <inttypes.h>
+#include <assert.h>
+#include <sys/poll.h>

#include <xs.h>
#include <xenctrl.h>
@@ -91,6 +94,66 @@ _hidden void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,

/* these functions preserve errno (saving and restoring) */

+typedef struct libxl__gc libxl__gc;
+
+typedef struct libxl__ev_fd libxl__ev_fd;
+typedef void libxl__ev_fd_callback(libxl__gc *gc, libxl__ev_fd *ev,
+ int fd, short events, short revents);
+struct libxl__ev_fd {
+ /* all private for libxl__ev_fd... */
+ LIBXL_LIST_ENTRY(libxl__ev_fd) entry;
+ int fd;
+ short events;
+ int in_beforepolled; /* -1 means not in fd_beforepolled */
+ void *for_app_reg;
+ libxl__ev_fd_callback *func;
+};
+
+
+typedef struct libxl__ev_time libxl__ev_time;
+typedef void libxl__ev_time_callback(libxl__gc *gc, libxl__ev_time *ev,
+ const struct timeval *requested_abs);
+struct libxl__ev_time {
+ /* all private for libxl__ev_time... */
+ int infinite; /* not registered in list or with app if infinite */
+ LIBXL_TAILQ_ENTRY(libxl__ev_time) entry;
+ struct timeval abs;
+ void *for_app_reg;
+ libxl__ev_time_callback *func;
+};
+
+typedef struct libxl__ev_xswatch libxl__ev_xswatch;
+typedef void libxl__ev_xswatch_callback(libxl__gc *gc, libxl__ev_xswatch*,
+ const char *watch_path, const char *event_path);
+struct libxl__ev_xswatch {
+ /* caller should include this in their own struct */
+ /* contents are private to xswatch_register */
+ int slotnum;
+ uint32_t counterval;
+ char *path;
+ libxl__ev_xswatch_callback *callback;
+};
+
+/*
+ * An entry in the watch_slots table is either:
+ * 1. an entry in the free list, ie NULL or pointer to next free list entry
+ * 2. an pointer to a libxl__ev_xswatch
+ *
+ * But we don't want to use unions or type-punning because the
+ * compiler might "prove" that our code is wrong and misoptimise it.
+ *
+ * The rules say that all struct pointers have identical
+ * representation and alignment requirements (C99+TC1+TC2 6.2.5p26) so
+ * what we do is simply declare our array as containing only the free
+ * list pointers, and explicitly convert from and to our actual
+ * xswatch pointers when we store and retrieve them.
+ */
+typedef struct libxl__ev_watch_slot {
+ LIBXL_SLIST_ENTRY(struct libxl__ev_watch_slot) empty;
+} libxl__ev_watch_slot;
+
+libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum);
+
struct libxl__ctx {
xentoollog_logger *lg;
xc_interface *xch;
@@ -108,6 +171,23 @@ struct libxl__ctx {
* documented in the libxl public interface.
*/

+ int osevent_in_hook;
+ const libxl_osevent_hooks *osevent_hooks;
+ void *osevent_user;
+ /* See the comment for OSEVENT_HOOK_INTERN in libxl_event.c
+ * for restrictions on the use of the osevent fields. */
+
+ int fd_beforepolled_allocd, fd_beforepolled_used;
+ libxl__ev_fd **fd_beforepolled; /* see libxl_osevent_beforepoll */
+ LIBXL_LIST_HEAD(, libxl__ev_fd) efds;
+ LIBXL_TAILQ_HEAD(, libxl__ev_time) etimes;
+
+ libxl__ev_watch_slot *watch_slots;
+ int watch_nslots;
+ LIBXL_SLIST_HEAD(, libxl__ev_watch_slot) watch_freeslots;
+ uint32_t watch_counter; /* helps disambiguate slot reuse */
+ libxl__ev_fd watch_efd;
+
/* for callers who reap children willy-nilly; caller must only
* set this after libxl_init and before any other call - or
* may leave them untouched */
@@ -138,12 +218,12 @@ typedef struct {

#define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))

-typedef struct {
+struct libxl__gc {
/* mini-GC */
int alloc_maxsize;
void **alloc_ptrs;
libxl_ctx *owner;
-} libxl__gc;
+};

#define LIBXL_INIT_GC(gc,ctx) do{ \
(gc).alloc_maxsize = 0; \
@@ -209,9 +289,137 @@ _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t,
_hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
const char *path, unsigned int *nb);
/* On error: returns NULL, sets errno (no logging) */
-
_hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);

+
+/*
+ * Event generation functions provided by the libxl event core to the
+ * rest of libxl. Implemented in terms of _beforepoll/_afterpoll
+ * and/or the fd registration machinery, as provided by the
+ * application.
+ *
+ * Semantics are similar to those of the fd and timeout registration
+ * functions provided to libxl_osevent_register_hooks.
+ *
+ * Non-0 returns from libxl__ev_{modify,deregister} have already been
+ * logged by the core and should be returned unmodified to libxl's
+ * caller; NB that they may be valid libxl error codes but they may
+ * also be positive numbers supplied by the caller.
+ *
+ * In each case, there is a libxl__ev_FOO structure which can be in
+ * one of three states:
+ *
+ * Undefined - Might contain anything. All-bits-zero is
+ * an undefined state.
+ *
+ * Idle - Struct contents are defined enough to pass to any
+ * libxl__ev_FOO function but not registered and
+ * callback will not be called. The struct does not
+ * contain references to any allocated resources so
+ * can be thrown away.
+ *
+ * Active - Request for events has been registered and events
+ * may be generated. _deregister must be called to
+ * reclaim resources.
+ *
+ * These functions are provided for each kind of event KIND:
+ *
+ * int libxl__ev_KIND_register(libxl__gc *gc, libxl__ev_KIND *GEN,
+ * libxl__ev_KIND_callback *FUNC,
+ * DETAILS);
+ * On entry *GEN must be in state Undefined or Idle.
+ * Returns a libxl error code; on error return *GEN is Idle.
+ * On successful return *GEN is Active and FUNC wil be
+ * called by the event machinery in future. FUNC will
+ * not be called from within the call to _register.
+ *
+ * void libxl__ev_KIND_deregister(libxl__gc *gc, libxl__ev_KIND *GEN_upd);
+ * On entry *GEN must be in state Active or Idle.
+ * On return it is Idle. (Idempotent.)
+ *
+ * void libxl__ev_KIND_init(libxl__ev_KIND *GEN);
+ * Provided for initialising an Undefined KIND.
+ * On entry *GEN must be in state Idle or Undefined.
+ * On return it is Idle. (Idempotent.)
+ *
+ * int libxl__ev_KIND_isregistered(const libxl__ev_KIND *GEN);
+ * On entry *GEN must be Idle or Active.
+ * Returns nonzero if it is Active, zero otherwise.
+ * Cannot fail.
+ *
+ * int libxl__ev_KiND_modify(libxl__gc*, libxl__ev_KIND *GEN,
+ * DETAILS);
+ * Only provided for some kinds of generator.
+ * On entry *GEN must be Active and on return, whether successful
+ * or not, it will be Active.
+ * Returns a libxl error code; on error the modification
+ * is not effective.
+ *
+ * All of these functions are fully threadsafe and may be called by
+ * general code in libxl even from within event callback FUNCs.
+ */
+
+
+_hidden int libxl__ev_fd_register(libxl__gc*, libxl__ev_fd *ev_out,
+ libxl__ev_fd_callback*,
+ int fd, short events /* as for poll(2) */);
+_hidden int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
+ short events);
+_hidden void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
+static inline void libxl__ev_fd_init(libxl__ev_fd *efd)
+ { efd->fd = -1; }
+static inline int libxl__ev_fd_isregistered(libxl__ev_fd *efd)
+ { return efd->fd >= 0; }
+
+_hidden int libxl__ev_time_register_rel(libxl__gc*, 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,
+ libxl__ev_time_callback*,
+ struct timeval);
+_hidden int libxl__ev_time_modify_rel(libxl__gc*, libxl__ev_time *ev,
+ int milliseconds /* as for poll(2) */);
+_hidden int libxl__ev_time_modify_abs(libxl__gc*, libxl__ev_time *ev,
+ struct timeval);
+_hidden void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
+static inline void libxl__ev_time_init(libxl__ev_time *ev)
+ { ev->func = 0; }
+static inline int libxl__ev_time_isregistered(libxl__ev_time *ev)
+ { return !!ev->func; }
+
+
+_hidden int libxl__ev_xswatch_register(libxl__gc*, libxl__ev_xswatch *xsw_out,
+ libxl__ev_xswatch_callback*,
+ const char *path /* copied */);
+_hidden void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch*);
+
+static inline void libxl__ev_xswatch_init(libxl__ev_xswatch *xswatch_out)
+ { xswatch_out->slotnum = -1; }
+static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw)
+ { return xw->slotnum >= 0; }
+
+
+
+_hidden void libxl__event_disaster(libxl__gc*, const char *msg, int errnoval,
+ libxl_event_type type /* may be 0 */,
+ const char *file, int line,
+ const char *func);
+ /*
+ * In general, call this via the macro LIBXL__EVENT_DISASTER.
+ *
+ * Event-generating functions may call this if they might have
+ * wanted to generate an event (either an internal one ie a
+ * libxl__ev_FOO_callback or an application event), but are
+ * prevented from doing so due to eg lack of memory.
+ *
+ * NB that this function may return and the caller isn't supposed to
+ * then crash, although it may fail (and henceforth leave things in
+ * a state where many or all calls fail).
+ */
+#define LIBXL__EVENT_DISASTER(gc, msg, errnoval, type) \
+ libxl__event_disaster(gc, msg, errnoval, type, __FILE__, __LINE__, __func__)
+
+
/* from xl_dom */
_hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
_hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
@@ -536,6 +744,8 @@ _hidden int libxl__parse_mac(const char *s, libxl_mac mac);
/* compare mac address @a and @b. 0 if the same, -ve if a<b and +ve if a>b */
_hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b);

+_hidden int libxl__gettimeofday(libxl__gc *gc, struct timeval *now_r);
+
#define STRINGIFY(x) #x
#define TOSTRING(x) STRINGIFY(x)

--
1.7.2.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Mon, 2011-12-05 at 18:10 +0000, Ian Jackson wrote:

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 289dc85..654a5b0 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -137,6 +137,7 @@
> #include <xen/sysctl.h>
>
> #include <libxl_uuid.h>
> +#include <_libxl_list.h>

Do we expose the use of these to users anywhere? I've failed to spot it
if so (at least in this patch). If it is deliberate then #3 needs to
install this header.

> [...]
> @@ -635,6 +639,8 @@ const char *libxl_lock_dir_path(void);
> const char *libxl_run_dir_path(void);
> const char *libxl_xenpaging_dir_path(void);
>
> +#include <libxl_event.h>
> +

Putting this at the end is a bit odd, I don't really object though. I
suppose it depends on stuff defined in this header and putting it half
way through is even more odd.

> #endif /* LIBXL_H */
>
> /*
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> new file mode 100644
> index 0000000..8d4dbf6
> --- /dev/null
> +++ b/tools/libxl/libxl_event.c
> @@ -0,0 +1,711 @@
> +/*
> + * Copyright (C) 2011 Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.

I've only just noticed this but it is present in some other libxl files
too. There is no LICENSE file in tools/libxl:
$ find -name LICENSE
./tools/ioemu-remote/LICENSE
./tools/ioemu-remote/tcg/LICENSE
./tools/ocaml/LICENSE
./xen/tools/figlet/LICENSE

I suspect this is a cut-and-paste-o, probably from tools/ocaml?

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +/*
> + * Internal event machinery for use by other parts of libxl
> + */
> +
> +#include <poll.h>
> +
> +#include "libxl_internal.h"
> +
> +/*
> + * The counter osevent_in_hook is used to ensure that the application
> + * honours the reentrancy restriction documented in libxl_event.h.
> + *
> + * The application's registration hooks should be called ONLY via
> + * these macros, with the ctx locked. Likewise all the "occurred"
> + * entrypoints from the application should assert(!in_hook);

In RFC speak you mean MUST rather than should both times here, right?

> + */
> +#define OSEVENT_HOOK_INTERN(defval, hookname, ...) \
> + (CTX->osevent_hooks \
> + ? (CTX->osevent_in_hook++, \
> + CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__), \
> + CTX->osevent_in_hook--) \
> + : defval)
> +
> +#define OSEVENT_HOOK(hookname,...) \
> + OSEVENT_HOOK_INTERN(0, hookname, __VA_ARGS__)
> +
> +#define OSEVENT_HOOK_VOID(hookname,...) \
> + OSEVENT_HOOK_INTERN((void)0, hookname, __VA_ARGS__)
> +
> +/*
> + * fd events
> + */
> +
> +int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
> + libxl__ev_fd_callback *func,
> + int fd, short events) {

Strictly speaking CODING_STYLE requires the { to be on the next line.

> + int rc;
> +
> + assert(fd >= 0);
> +
> + CTX_LOCK;
> +
> + rc = OSEVENT_HOOK(fd_register, fd, &ev->for_app_reg, events, ev);
> + if (rc) goto out;
> +
> + LIBXL_LIST_INSERT_HEAD(&CTX->efds, ev, entry);
> +
> + ev->fd = fd;
> + ev->events = events;
> + ev->in_beforepolled = -1;
> + ev->func = func;

Even though this is all locked correctly seeing the ev initialised after
it is already on the list has tweaked my "what's up" instinct such that
I've had to look twice both times I've looked at this patch.

> +
> + rc = 0;
> +
> + out:
> + CTX_UNLOCK;
> + return rc;
> +}
> +
[...]
> +int libxl__ev_time_register_rel(libxl__gc *gc, libxl__ev_time *ev,
> + libxl__ev_time_callback *func,
> + int milliseconds /* as for poll(2) */) {
> + struct timeval abs;
> + int rc;
> +
> + CTX_LOCK;
> +
> + if (milliseconds < 0) {
> + ev->infinite = 1;

diff has inconveniently chosen to present me with the implementation
before the interface. /me scurries off to read libxl_events.h. OK I see
why this == infinite now (it even tells me 5 lines before, oh well).

> + } else {
> + rc = time_rel_to_abs(gc, milliseconds, &abs);
> + if (rc) goto out;
> +
> + rc = time_register_finite(gc, ev, abs);
> + if (rc) goto out;
> + }
> +
> + ev->func = func;
> + rc = 0;
> +
> + out:
> + CTX_UNLOCK;
> + return 0;

You mean "return rc" here.

> +}
> +
> +int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
> + struct timeval abs) {
> + int rc;
> +
> + CTX_LOCK;
> +
> + assert(libxl__ev_time_isregistered(ev));

Why is there no deregister here? Is libxl__ev_time_modify_rel the only
caller?

> +
> + if (ev->infinite) {
> + rc = time_register_finite(gc, ev, abs);
> + if (rc) goto out;
> + } else {
> + rc = OSEVENT_HOOK(timeout_modify, &ev->for_app_reg, abs);
> + if (rc) goto out;
> +
> + LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> + ev->abs = abs;
> + time_insert_finite(gc, ev);
> + }
> +
> + rc = 0;
> + out:
> + CTX_UNLOCK;
> + return rc;
> +}
> +
> +int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev,
> + int milliseconds) {
> + struct timeval abs;
> + int rc;
> +
> + CTX_LOCK;
> +
> + assert(libxl__ev_time_isregistered(ev));
> +
> + if (milliseconds < 0) {
> + time_deregister(gc, ev);
> + ev->infinite = 1;
> + rc = 0;
> + goto out;
> + }
> +
> + rc = time_rel_to_abs(gc, milliseconds, &abs);
> + if (rc) goto out;
> +
> + rc = libxl__ev_time_modify_abs(gc, ev, abs);
> + if (rc) goto out;
> +
> + rc = 0;
> + out:
> + CTX_UNLOCK;
> + return 0;

> [...]
> +
> +/*
> + * xenstore watches
> + */
> +[...]
> +
> +static void watchfd_callback(libxl__gc *gc, libxl__ev_fd *ev,
> + int fd, short events, short revents) {
> + for (;;) {
> + char **event = xs_check_watch(CTX->xsh);
> + if (!event) {
> + if (errno == EAGAIN) break;
> + if (errno == EINTR) continue;
> + LIBXL__EVENT_DISASTER(gc, "cannot check/read watches", errno, 0);
> + return;
> + }
> +
> + const char *epath = event[0];
> + const char *token = event[1];
> + int slotnum;
> + uint32_t counterval;
> + int rc = sscanf(token, "%d/%"SCNx32, &slotnum, &counterval);

How have I never come across the SCNxxx counterpart to PRIxxx before!

> + if (rc != 2) {
> + LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> + "watch epath=%s token=%s: failed to parse token",
> + epath, token);
> + /* oh well */
> + goto ignore;
> + }
> + if (slotnum < 0 || slotnum >= CTX->watch_nslots) {
> + /* perhaps in the future we will make the watchslots array shrink */
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, "watch epath=%s token=%s:"
> + " slotnum %d out of range [.0,%d>",
> + epath, token, slotnum, CTX->watch_nslots);
> + goto ignore;
> + }
> +
> + libxl__ev_xswatch *w = libxl__watch_slot_contents(gc, slotnum);
> +
> + if (!w) {
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch epath=%s token=%s: empty slot",
> + epath, token);
> + goto ignore;
> + }
> +
> + if (w->counterval != counterval) {
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch epath=%s token=%s: counter != %"PRIx32,
> + epath, token, w->counterval);
> + goto ignore;
> + }
> +
> + /* Now it's possible, though unlikely, that this was an event
> + * from a previous use of the same slot with the same counterval.
> + *
> + * In that case either:
> + * - the event path is a child of the watch path, in
> + * which case this watch would really have generated this
> + * event if it had been registered soon enough and we are
> + * OK to give this possibly-spurious event to the caller; or
> + * - it is not, in which case we must suppress it as the
> + * caller should not see events for unrelated paths.
> + *
> + * See also docs/misc/xenstore.txt.
> + */
> + size_t epathlen = strlen(epath);
> + size_t wpathlen = strlen(w->path);
> + if (epathlen < wpathlen ||
> + memcmp(epath, w->path, wpathlen) ||
> + (epathlen > wpathlen && epath[wpathlen] != '/')) {
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch epath=%s token=%s: not child of wpath=%s",
> + epath, token, w->path);

It seems like this is worthy of a helper function of its own. Possibly
in libxenstore itself?

> + goto ignore;
> + }
> +
> + /* At last, we have checked everything! */

Huzzah!

> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch event: epath=%s token=%s wpath=%s w=%p",
> + epath, token, w->path, w);

Aside: At some point we might want to have a way to configure classes of
debug log message on/off. I was just wondering if this message might be
a bit spammy but I suspect it is ok.

> + w->callback(gc, w, w->path, epath);
> +
> + ignore:
> + free(event);
> + }
> +}
[...]
> +int libxl__ev_xswatch_register(libxl__gc *gc, libxl__ev_xswatch *w,
> + libxl__ev_xswatch_callback *func,
> + const char *path /* copied */) {
> + libxl__ev_watch_slot *use = NULL;
> + char *path_copy = NULL;
> + int rc;
> +
> + CTX_LOCK;
> +
> + if (!libxl__ev_fd_isregistered(&CTX->watch_efd)) {
> + rc = libxl__ev_fd_register(gc, &CTX->watch_efd, watchfd_callback,
> + xs_fileno(CTX->xsh), POLLIN);
> + if (rc) goto out_rc;
> + }
> +
> + if (LIBXL_SLIST_EMPTY(&CTX->watch_freeslots)) {
> + /* Free list is empty so there is not in fact a linked
> + * free list in the array and we can safely realloc it */
> + int newarraysize = (CTX->watch_nslots + 1) << 2;
> + int i;
> + libxl__ev_watch_slot *newarray =
> + realloc(CTX->watch_slots, sizeof(*newarray) * newarraysize);
> + if (!newarray) goto out_nomem;
> + for (i=CTX->watch_nslots; i<newarraysize; i++)
> + LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots,
> + &newarray[i], empty);
> + CTX->watch_slots = newarray;
> + CTX->watch_nslots = newarraysize;
> + }
> + use = LIBXL_SLIST_FIRST(&CTX->watch_freeslots);
> + assert(use);
> + LIBXL_SLIST_REMOVE_HEAD(&CTX->watch_freeslots, empty);

I presume this is removing "use" from the list. It seems odd to refer to
that element of the list as HEAD and FIRST interchangeably since it
doesn't make this obvious but I guess we got this API from elsewhere.

There's no get-and-return function which combines the two operations?

> +
> + path_copy = strdup(path);
> + if (!path_copy) goto out_nomem;
> +
> + int slotnum = use - CTX->watch_slots;
> + w->counterval = CTX->watch_counter++;
> +
> + if (!xs_watch(CTX->xsh, path, watch_token(gc, slotnum, w->counterval))) {
> + LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
> + "create watch for path %s", path);
> + rc = ERROR_FAIL;
> + goto out_rc;
> + }
> +
> + w->slotnum = slotnum;
> + w->path = path_copy;
> + w->callback = func;
> + /* we look a bit behind the curtain of LIBXL_SLIST, to explictly

explicitly

> + * assign to the pointer that's the next link. See the comment
> + * by the definitionn of libxl__ev_watch_slot */

definition.

I think this behind the curtain stuff would be better encapsulated in a
macro up somewhere near the comment and libxl__ev_watch_slot.

> + use->empty.sle_next = (void*)w;


> +
> + CTX_UNLOCK;
> + return 0;
> +
> + out_nomem:
> + rc = ERROR_NOMEM;
> + out_rc:
> + if (use)
> + LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, use, empty);
> + if (path_copy)
> + free(path_copy);
> + CTX_UNLOCK;
> + return rc;
> +}
> +
> +void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch *w) {
> + /* it is legal to deregister from within _callback */

and CTX_LOCK is recursive so this is ok.

> + CTX_LOCK;
> +
> + if (w->slotnum >= 0) {
> + char *token = watch_token(gc, w->slotnum, w->counterval);
> + if (!xs_unwatch(CTX->xsh, w->path, token))
> + /* Oh well, we will just get watch events forever more
> + * and ignore them. But we should complain to the log. */
> + LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
> + "remove watch for path %s", w->path);

Is it possible to also unwatch an unexpected watch at the point it
fires, IOW to try again later? I havn'et looked at the potential failure
cases of xs_unwatch so perhaps once it has failed there is no point in
trying again.

> +
> + libxl__ev_watch_slot *slot = &CTX->watch_slots[w->slotnum];
> + LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, slot, empty);
> + }
> +
> + free(w->path);
> + w->path = NULL;
> +
> + CTX_UNLOCK;
> +}
> +
> +/*
> + * osevent poll
> + */

This seems like a good place to stop for the day. I'll pickup the rest
tomorrow.

[...]


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
> [...snip already reviewed bits...]
> +/*
> + * osevent poll
> + */
> +
> +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> + struct pollfd *fds, int *timeout_upd,
> + struct timeval now) {
> + libxl__ev_fd *efd;
> + int i;
> +
> + /*
> + * In order to be able to efficiently find the libxl__ev_fd
> + * for a struct poll during _afterpoll, we maintain a shadow
> + * data structure in CTX->fd_beforepolled: each slot in
> + * the fds array corresponds to a slot in fd_beforepolled.
> + */
> +
> + GC_INIT(ctx);
> + CTX_LOCK;
> +
> + if (*nfds_io) {
> + /*
> + * As an optimisation, we don't touch fd_beforepolled_used
> + * if *nfds_io is zero on entry, since in that case the
> + * caller just wanted to know how big an array to give us.
> + *
> + * If !*nfds_io, the unconditional parts below are guaranteed
> + * not to mess with fd_beforepolled... or any in_beforepolled.
> + */
> +
> + /* Remove all the old references into beforepolled */
> + for (i = 0; i < CTX->fd_beforepolled_used; i++) {
> + efd = CTX->fd_beforepolled[i];
> + if (efd) {
> + assert(efd->in_beforepolled == i);
> + efd->in_beforepolled = -1;
> + CTX->fd_beforepolled[i] = NULL;
> + }
> + }
> + CTX->fd_beforepolled_used = 0;
> +
> + /* make sure our array is as big as *nfds_io */
> + if (CTX->fd_beforepolled_allocd < *nfds_io) {
> + assert(*nfds_io < INT_MAX / sizeof(libxl__ev_fd*) / 2);

What is the /2 for?

> + libxl__ev_fd **newarray =
> + realloc(CTX->fd_beforepolled, sizeof(*newarray) * *nfds_io);
> + if (!newarray)
> + return ERROR_NOMEM;

Need to CTX_UNLOCK here.

> + CTX->fd_beforepolled = newarray;
> + CTX->fd_beforepolled_allocd = *nfds_io;
> + }
> + }
> +
> + int used = 0;
> + LIBXL_LIST_FOREACH(efd, &CTX->efds, entry) {
> + if (used < *nfds_io) {
> + fds[used].fd = efd->fd;
> + fds[used].events = efd->events;
> + fds[used].revents = 0;
> + CTX->fd_beforepolled[used] = efd;
> + efd->in_beforepolled = used;
> + }
> + used++;
> + }
> + int rc = used <= *nfds_io ? 0 : ERROR_BUFFERFULL;
> +
> + if (*nfds_io) {
> + CTX->fd_beforepolled_used = used;
> + }
> +
> + *nfds_io = used;
> +
> + libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
> + if (etime) {
> + int our_timeout;
> + struct timeval rel;
> + static struct timeval zero;
> +
> + timersub(&etime->abs, &now, &rel);
> +
> + if (timercmp(&rel, &zero, <)) {
> + our_timeout = 0;
> + } else if (rel.tv_sec >= 2000000) {
> + our_timeout = 2000000000;
> + } else {
> + our_timeout = rel.tv_sec * 1000 + (rel.tv_usec + 999) / 1000;
> + }
> + if (*timeout_upd < 0 || our_timeout < *timeout_upd)
> + *timeout_upd = our_timeout;
> + }
> +
> + CTX_UNLOCK;
> + GC_FREE;
> + return rc;
> +}
> +
> +void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
> + struct timeval now) {
> + int i;
> + GC_INIT(ctx);
> + CTX_LOCK;
> +
> + assert(nfds <= CTX->fd_beforepolled_used);
> +
> + for (i = 0; i < nfds; i++) {
> + if (!fds[i].revents)
> + continue;
> +
> + libxl__ev_fd *efd = CTX->fd_beforepolled[i];
> + if (!efd)
> + continue;

Would this be a bug? If we've set it up for polling how can it be NULL?

> +
> + assert(efd->in_beforepolled == i);
> + assert(fds[i].fd == efd->fd);
> +
> + int revents = fds[i].revents & efd->events;
> + if (!revents)
> + continue;
> +
> + efd->func(gc, efd, efd->fd, efd->events, revents);
> + }
> +
> + for (;;) {
> + libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
> + if (!etime)
> + break;
> +
> + assert(!etime->infinite);
> +
> + if (timercmp(&etime->abs, &now, >))
> + break;
> +
> + time_deregister(gc, etime);
> +
> + etime->func(gc, etime, &etime->abs);
> + }
> +
> + CTX_UNLOCK;
> + GC_FREE;
> +}
> +
> +
> +/*
> + * osevent hook and callback machinery
> + */
> +
> +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> + const libxl_osevent_hooks *hooks,
> + void *user) {
> + GC_INIT(ctx);

I nearly said, "there's no gc used in this function" but actually it is
artfully concealed in CTX_LOCK.

I wonder if CTX_LOCK should take the context, e.g. either CTX_LOCK(CTX)
or CTX_LOCK(ctx)?

Another alternative would be to have CTX_INIT to parallel GC_INIT which
creates a local *ctx instead of having CTX. This would also avoid the
need to s/CTX/ctx/ if you make an internal function into an external one
and vice versa.

> + CTX_LOCK;
> + ctx->osevent_hooks = hooks;
> + ctx->osevent_user = user;
> + CTX_UNLOCK;
> + GC_FREE;
> +}
> +
> +
[...]
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> new file mode 100644
> index 0000000..25efbdf
> --- /dev/null
> +++ b/tools/libxl/libxl_event.h
> @@ -0,0 +1,199 @@
[...]
> +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> + struct pollfd *fds, int *timeout_upd,
> + struct timeval now);
> + /* The caller should provide beforepoll with some space for libxl's
> + * fds, and tell libxl how much space is available by setting *nfds_io.
> + * fds points to the start of this space (and fds may be a pointer into
> + * a larger array, for example, if the application has some fds of
> + * its own that it is interested in).
> + *
> + * On return *nfds_io will in any case have been updated by libxl
> + * according to how many fds libxl wants to poll on.
> + *
> + * If the space was sufficient, libxl fills in fds[0..<new
> + * *nfds_io>] suitably for poll(2), updates *timeout_upd if needed,
> + * and returns ok.
> + *
> + * If space was insufficient, fds[0..<old *nfds_io>] is undefined on
> + * return; *nfds_io on return will be greater than the value on
> + * entry; *timeout_upd may or may not have been updated; and
> + * libxl_osevent_beforepoll returns ERROR_BUFERFULL. In this case

ERROR_BUFFERFULL
[...]

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d015c7c..88e7dbb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
[...]
> @@ -138,12 +218,12 @@ typedef struct {
>
> #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
>
> -typedef struct {
> +struct libxl__gc {
> /* mini-GC */
> int alloc_maxsize;
> void **alloc_ptrs;
> libxl_ctx *owner;
> -} libxl__gc;
> +};

Is this an unrelated change which has snuck in? I'd hack expected an
equivalent change to GC_INIT if not.

[...]
> --
> 1.7.2.5

Phew!



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
Ian Campbell writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> On Mon, 2011-12-05 at 18:10 +0000, Ian Jackson wrote:
> > #include <libxl_uuid.h>
> > +#include <_libxl_list.h>
>
> Do we expose the use of these to users anywhere? I've failed to spot it
> if so (at least in this patch). If it is deliberate then #3 needs to
> install this header.

Oh. Yes, it is deliberate. There is a list link in libxl_event.
I will arrange to install the header.

> > +#include <libxl_event.h>
>
> Putting this at the end is a bit odd, I don't really object though. I
> suppose it depends on stuff defined in this header and putting it half
> way through is even more odd.

Exactly. This seems the best approach.

> > @@ -0,0 +1,711 @@
> > +/*
> > + * Copyright (C) 2011 Citrix Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU Lesser General Public License as published
> > + * by the Free Software Foundation; version 2.1 only. with the special
> > + * exception on linking described in file LICENSE.
>
> I've only just noticed this but it is present in some other libxl files
> too. There is no LICENSE file in tools/libxl:
> $ find -name LICENSE
> ./tools/ioemu-remote/LICENSE
> ./tools/ioemu-remote/tcg/LICENSE
> ./tools/ocaml/LICENSE
> ./xen/tools/figlet/LICENSE
>
> I suspect this is a cut-and-paste-o, probably from tools/ocaml?

All the existing files in libxl/ mention this nonexistent file
LICENCE. I think we should fix this in a separate patch. I'd argue
that my copying of the existing text isn't making the situation worse.

> > + * The application's registration hooks should be called ONLY via
> > + * these macros, with the ctx locked. Likewise all the "occurred"
> > + * entrypoints from the application should assert(!in_hook);
>
> In RFC speak you mean MUST rather than should both times here, right?

In the immortal words of RFC2181:

3. Terminology

This memo does not use the oft used expressions MUST, SHOULD, MAY, or
their negative forms. In some sections it may seem that a
specification is worded mildly, and hence some may infer that the
specification is optional. That is not correct. Anywhere that this
memo suggests that some action should be carried out, or must be
carried out, or that some behaviour is acceptable, or not, that is to
be considered as a fundamental aspect of this specification,
regardless of the specific words used. If some behaviour or action
is truly optional, that will be clearly specified by the text.

If you think this is confusing I can change it to "must"...

> > +int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
> > + libxl__ev_fd_callback *func,
> > + int fd, short events) {
>
> Strictly speaking CODING_STYLE requires the { to be on the next line.

Oh, I probably have a lot of those. Damn. I'll try to remember to
fix this up with some seddery somehow.

> > + LIBXL_LIST_INSERT_HEAD(&CTX->efds, ev, entry);
> > +
> > + ev->fd = fd;
> > + ev->events = events;
> > + ev->in_beforepolled = -1;
> > + ev->func = func;
>
> Even though this is all locked correctly seeing the ev initialised after
> it is already on the list has tweaked my "what's up" instinct such that
> I've had to look twice both times I've looked at this patch.

I'll swap it round.

> > + out:
> > + CTX_UNLOCK;
> > + return 0;
>
> You mean "return rc" here.

So I do. Fixed here and in libxl__ev_time_modify_rel.

> > +int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
> > + struct timeval abs) {
> > + int rc;
> > +
> > + CTX_LOCK;
> > +
> > + assert(libxl__ev_time_isregistered(ev));
>
> Why is there no deregister here? Is libxl__ev_time_modify_rel the only
> caller?

It's currently the only caller but other bits of libxl are entitled to
call it. It's part of the facilities supplied to the rest of libxl.

There is no deregister because (a) we don't want to do that until
we've called the hook, if necessary, in case the hook fails and (b) we
have to explicitly deal with the two different cases:
1. existing event was infinite timeout, so not on queue, just
call register_finite which will do everything needed
2. existing event _wasn't_ infinite timeout, first call modify
hook, then fiddle about with the queue

> > + const char *epath = event[0];
> > + const char *token = event[1];
> > + int slotnum;
> > + uint32_t counterval;
> > + int rc = sscanf(token, "%d/%"SCNx32, &slotnum, &counterval);
>
> How have I never come across the SCNxxx counterpart to PRIxxx before!

Hardly anyone ever uses scanf.

> > + /* Now it's possible, though unlikely, that this was an event
> > + * from a previous use of the same slot with the same counterval.
> > + *
> > + * In that case either:
> > + * - the event path is a child of the watch path, in
> > + * which case this watch would really have generated this
> > + * event if it had been registered soon enough and we are
> > + * OK to give this possibly-spurious event to the caller; or
> > + * - it is not, in which case we must suppress it as the
> > + * caller should not see events for unrelated paths.
> > + *
> > + * See also docs/misc/xenstore.txt.
> > + */
> > + size_t epathlen = strlen(epath);
> > + size_t wpathlen = strlen(w->path);
> > + if (epathlen < wpathlen ||
> > + memcmp(epath, w->path, wpathlen) ||
> > + (epathlen > wpathlen && epath[wpathlen] != '/')) {
> > + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> > + "watch epath=%s token=%s: not child of wpath=%s",
> > + epath, token, w->path);
>
> It seems like this is worthy of a helper function of its own. Possibly
> in libxenstore itself?

You mean "int xs_path_is_subpath_p(const char *parent, const char *child)" ?

Possibly. I wouldn't be opposed to putting those 5 lines in
libxenstore it there but AFAIK only this place in libxl needs it.

> > + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> > + "watch event: epath=%s token=%s wpath=%s w=%p",
> > + epath, token, w->path, w);
>
> Aside: At some point we might want to have a way to configure classes of
> debug log message on/off. I was just wondering if this message might be
> a bit spammy but I suspect it is ok.

I don't think there will be that many of these. If so then yes we may
need a more sophisticated debug framework.

> > + use = LIBXL_SLIST_FIRST(&CTX->watch_freeslots);
> > + assert(use);
> > + LIBXL_SLIST_REMOVE_HEAD(&CTX->watch_freeslots, empty);
>
> I presume this is removing "use" from the list. It seems odd to refer to
> that element of the list as HEAD and FIRST interchangeably since it
> doesn't make this obvious but I guess we got this API from elsewhere.

Yes, FreeBSD.

> There's no get-and-return function which combines the two operations?

Not in this pile of macros, I'm afraid.

> > + w->slotnum = slotnum;
> > + w->path = path_copy;
> > + w->callback = func;
> > + /* we look a bit behind the curtain of LIBXL_SLIST, to explictly
>
> explicitly

Fixed.

> > + * assign to the pointer that's the next link. See the comment
> > + * by the definitionn of libxl__ev_watch_slot */
>
> definition.

Fixed.

> I think this behind the curtain stuff would be better encapsulated in a
> macro up somewhere near the comment and libxl__ev_watch_slot.
>
> > + use->empty.sle_next = (void*)w;

How about:

static void libxl__set_watch_slot_contents(libxl__ev_watch_slot *slot,
libxl__ev_xswatch *w) {
/* we look a bit behind the curtain of LIBXL_SLIST, to explicitly
* assign to the pointer that's the next link. See the comment
* by the definition of libxl__ev_watch_slot */
slot->empty.sle_next = (void*)w;
}

Just below the the definition of libxl__watch_slot_contents. That
puts it near the other big comment and it's essentially the sister
function.

> > + CTX_LOCK;
> > +
> > + if (w->slotnum >= 0) {
> > + char *token = watch_token(gc, w->slotnum, w->counterval);
> > + if (!xs_unwatch(CTX->xsh, w->path, token))
> > + /* Oh well, we will just get watch events forever more
> > + * and ignore them. But we should complain to the log. */
> > + LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
> > + "remove watch for path %s", w->path);
>
> Is it possible to also unwatch an unexpected watch at the point it
> fires, IOW to try again later? I havn'et looked at the potential failure
> cases of xs_unwatch so perhaps once it has failed there is no point in
> trying again.

Offhand I think it can fail because:
- communication with xenstore has broken down, in which case trying
again is pretty pointless
- xenstore didn't recognise the watch (ie we or xenstore have a bug)
in which case trying to remove it is not sensible
I'm also not sure exactly about the race implications. What if you
add and remove watches in different threads simultaneously ?

So I think this is probably best - and it's probably best not to do
additional complicated flailing when either (a) things are already
going wrong or (b) we just got a harmless lost race.

> This seems like a good place to stop for the day. I'll pickup the rest
> tomorrow.

Heh. I should go to the pub :-).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Thu, 2011-12-08 at 19:53 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> > > @@ -0,0 +1,711 @@
> > > +/*
> > > + * Copyright (C) 2011 Citrix Ltd.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU Lesser General Public License as published
> > > + * by the Free Software Foundation; version 2.1 only. with the special
> > > + * exception on linking described in file LICENSE.
> >
> > I've only just noticed this but it is present in some other libxl files
> > too. There is no LICENSE file in tools/libxl:
> > $ find -name LICENSE
> > ./tools/ioemu-remote/LICENSE
> > ./tools/ioemu-remote/tcg/LICENSE
> > ./tools/ocaml/LICENSE
> > ./xen/tools/figlet/LICENSE
> >
> > I suspect this is a cut-and-paste-o, probably from tools/ocaml?
>
> All the existing files in libxl/ mention this nonexistent file
> LICENCE. I think we should fix this in a separate patch. I'd argue
> that my copying of the existing text isn't making the situation worse.

Sure, I didn't mean to suggest you needed to fix this in this series, I
just happened to observe it here.

> > > + * The application's registration hooks should be called ONLY via
> > > + * these macros, with the ctx locked. Likewise all the "occurred"
> > > + * entrypoints from the application should assert(!in_hook);
> >
> > In RFC speak you mean MUST rather than should both times here, right?
>
> In the immortal words of RFC2181:
>
> 3. Terminology
>
> This memo does not use the oft used expressions MUST, SHOULD, MAY, or
> their negative forms. In some sections it may seem that a
> specification is worded mildly, and hence some may infer that the
> specification is optional. That is not correct. Anywhere that this
> memo suggests that some action should be carried out, or must be
> carried out, or that some behaviour is acceptable, or not, that is to
> be considered as a fundamental aspect of this specification,
> regardless of the specific words used. If some behaviour or action
> is truly optional, that will be clearly specified by the text.
>
> If you think this is confusing I can change it to "must"...

I was mainly asking just to clarify my own understanding, as you point
out we don't use those "oft used expressions" in our comments.

> > > +int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
> > > + libxl__ev_fd_callback *func,
> > > + int fd, short events) {
> >
> > Strictly speaking CODING_STYLE requires the { to be on the next line.
>
> Oh, I probably have a lot of those. Damn.

Yeah, I refrained from commenting every time ;-)

> I'll try to remember to
> fix this up with some seddery somehow.

[...]
> > > +int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
> > > + struct timeval abs) {
> > > + int rc;
> > > +
> > > + CTX_LOCK;
> > > +
> > > + assert(libxl__ev_time_isregistered(ev));
> >
> > Why is there no deregister here? Is libxl__ev_time_modify_rel the only
> > caller?
>
> It's currently the only caller but other bits of libxl are entitled to
> call it. It's part of the facilities supplied to the rest of libxl.
>
> There is no deregister because (a) we don't want to do that until
> we've called the hook, if necessary, in case the hook fails and (b) we
> have to explicitly deal with the two different cases:
> 1. existing event was infinite timeout, so not on queue, just
> call register_finite which will do everything needed
> 2. existing event _wasn't_ infinite timeout, first call modify
> hook, then fiddle about with the queue

Thanks, I was confused by the presence of a register/insert without a
corresponding unregister/delete but I see now that in one case the ev
isn't registered by definition (the infinite->finite case) and the other
you do actually remove it from the list (I missed that somehow).

[...]

> > > + /* Now it's possible, though unlikely, that this was an event
> > > + * from a previous use of the same slot with the same counterval.
> > > + *
> > > + * In that case either:
> > > + * - the event path is a child of the watch path, in
> > > + * which case this watch would really have generated this
> > > + * event if it had been registered soon enough and we are
> > > + * OK to give this possibly-spurious event to the caller; or
> > > + * - it is not, in which case we must suppress it as the
> > > + * caller should not see events for unrelated paths.
> > > + *
> > > + * See also docs/misc/xenstore.txt.
> > > + */
> > > + size_t epathlen = strlen(epath);
> > > + size_t wpathlen = strlen(w->path);
> > > + if (epathlen < wpathlen ||
> > > + memcmp(epath, w->path, wpathlen) ||
> > > + (epathlen > wpathlen && epath[wpathlen] != '/')) {
> > > + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> > > + "watch epath=%s token=%s: not child of wpath=%s",
> > > + epath, token, w->path);
> >
> > It seems like this is worthy of a helper function of its own. Possibly
> > in libxenstore itself?
>
> You mean "int xs_path_is_subpath_p(const char *parent, const char *child)" ?
>
> Possibly. I wouldn't be opposed to putting those 5 lines in
> libxenstore it there but AFAIK only this place in libxl needs it.

Wouldn't most users of libxenstore doing watches need something like
this (and probably either open code it or erroneously omit it)?

Regardless of where it goes moving that logic into a helper function
will make it clearer what is going on, both by having a descriptive name
and allowing the logic to be a bit more spaced out / commented, the last
clause in particular is slightly subtle.

> > > + use = LIBXL_SLIST_FIRST(&CTX->watch_freeslots);
> > > + assert(use);
> > > + LIBXL_SLIST_REMOVE_HEAD(&CTX->watch_freeslots, empty);
> >
> > I presume this is removing "use" from the list. It seems odd to refer to
> > that element of the list as HEAD and FIRST interchangeably since it
> > doesn't make this obvious but I guess we got this API from elsewhere.
>
> Yes, FreeBSD.

Sorry, I knew that, I was trying to say "we got this API from elsewhere
so I guess we have to live with its quirks".

> > I think this behind the curtain stuff would be better encapsulated in a
> > macro up somewhere near the comment and libxl__ev_watch_slot.
> >
> > > + use->empty.sle_next = (void*)w;
>
> How about:
>
> static void libxl__set_watch_slot_contents(libxl__ev_watch_slot *slot,
> libxl__ev_xswatch *w) {
> /* we look a bit behind the curtain of LIBXL_SLIST, to explicitly
> * assign to the pointer that's the next link. See the comment
> * by the definition of libxl__ev_watch_slot */
> slot->empty.sle_next = (void*)w;
> }
>
> Just below the the definition of libxl__watch_slot_contents. That
> puts it near the other big comment and it's essentially the sister
> function.

Works for me.

>
> > > + CTX_LOCK;
> > > +
> > > + if (w->slotnum >= 0) {
> > > + char *token = watch_token(gc, w->slotnum, w->counterval);
> > > + if (!xs_unwatch(CTX->xsh, w->path, token))
> > > + /* Oh well, we will just get watch events forever more
> > > + * and ignore them. But we should complain to the log. */
> > > + LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
> > > + "remove watch for path %s", w->path);
> >
> > Is it possible to also unwatch an unexpected watch at the point it
> > fires, IOW to try again later? I havn'et looked at the potential failure
> > cases of xs_unwatch so perhaps once it has failed there is no point in
> > trying again.
>
> Offhand I think it can fail because:
> - communication with xenstore has broken down, in which case trying
> again is pretty pointless
> - xenstore didn't recognise the watch (ie we or xenstore have a bug)
> in which case trying to remove it is not sensible
> I'm also not sure exactly about the race implications. What if you
> add and remove watches in different threads simultaneously ?

We are holding a lock at this point, I'm not sure if this is just
because of the associated datastructure frobbing or if their is an
(implicit or explicit) policy of holding the lock when adding or
removing watches.

> So I think this is probably best - and it's probably best not to do
> additional complicated flailing when either (a) things are already
> going wrong or (b) we just got a harmless lost race.

I'm convinced, thanks.

> > This seems like a good place to stop for the day. I'll pickup the rest
> > tomorrow.
>
> Heh. I should go to the pub :-).

Always a good call.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Mon, 5 Dec 2011, Ian Jackson wrote:
> We provide a new set of functions and related structures
> libxl_osevent_*
> which are to be used by event-driven applications to receive
> information from libxl about which fds libxl is interested in, and
> what timeouts libxl is waiting for, and to pass back to libxl
> information about which fds are readable/writeable etc., and which
> timeouts have occurred. Ie, low-level events.
>
> In this patch, this new machinery is still all unused. Callers will
> appear in the next patch in the series, which introduces a new API for
> applications to receive high-level events about actual domains etc.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> tools/libxl/Makefile | 2 +-
> tools/libxl/libxl.c | 25 ++
> tools/libxl/libxl.h | 6 +
> tools/libxl/libxl_event.c | 711 ++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_event.h | 199 ++++++++++++
> tools/libxl/libxl_internal.h | 216 +++++++++++++-
> 6 files changed, 1155 insertions(+), 4 deletions(-)
> create mode 100644 tools/libxl/libxl_event.c
> create mode 100644 tools/libxl/libxl_event.h
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 4e0f3fb..3d575b8 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -38,7 +38,7 @@ LIBXL_LIBS += -lyajl
> LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
> libxl_dom.o libxl_exec.o libxl_xshelp.o libxl_device.o \
> libxl_internal.o libxl_utils.o libxl_uuid.o libxl_json.o \
> - libxl_qmp.o $(LIBXL_OBJS-y)
> + libxl_qmp.o libxl_event.o $(LIBXL_OBJS-y)
> LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>
> $(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl)
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 3a8cfe3..58f280c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -60,6 +60,16 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
> * only as an initialiser, not as an expression. */
> memcpy(&ctx->lock, &mutex_value, sizeof(ctx->lock));
>
> + ctx->osevent_hooks = 0;
> +
> + ctx->fd_beforepolled = 0;
> + LIBXL_LIST_INIT(&ctx->efds);
> + LIBXL_TAILQ_INIT(&ctx->etimes);
> +
> + ctx->watch_slots = 0;
> + LIBXL_SLIST_INIT(&ctx->watch_freeslots);
> + libxl__ev_fd_init(&ctx->watch_efd);
> +
> if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
> "failed to stat %s", XENSTORE_PID_FILE);
> @@ -89,10 +99,25 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>
> int libxl_ctx_free(libxl_ctx *ctx)
> {
> + int i;
> + GC_INIT(ctx);
> +
> if (!ctx) return 0;
> +
> + for (i = 0; i < ctx->watch_nslots; i++)
> + assert(!libxl__watch_slot_contents(gc, i));
> + libxl__ev_fd_deregister(gc, &ctx->watch_efd);
> +
> + assert(LIBXL_LIST_EMPTY(&ctx->efds));
> + assert(LIBXL_TAILQ_EMPTY(&ctx->etimes));
> +
> if (ctx->xch) xc_interface_close(ctx->xch);
> libxl_version_info_dispose(&ctx->version_info);
> if (ctx->xsh) xs_daemon_close(ctx->xsh);
> +
> + free(ctx->fd_beforepolled);
> + free(ctx->watch_slots);
> +
> return 0;
> }
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 289dc85..654a5b0 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -137,6 +137,7 @@
> #include <xen/sysctl.h>
>
> #include <libxl_uuid.h>
> +#include <_libxl_list.h>
>
> typedef uint8_t libxl_mac[6];
> #define LIBXL_MAC_FMT "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx"
> @@ -222,6 +223,9 @@ enum {
> ERROR_BADFAIL = -7,
> ERROR_GUEST_TIMEDOUT = -8,
> ERROR_TIMEDOUT = -9,
> + ERROR_NOT_READY = -10,
> + ERROR_OSEVENT_REG_FAIL = -11,
> + ERROR_BUFFERFULL = -12,
> };
>
> #define LIBXL_VERSION 0
> @@ -635,6 +639,8 @@ const char *libxl_lock_dir_path(void);
> const char *libxl_run_dir_path(void);
> const char *libxl_xenpaging_dir_path(void);
>
> +#include <libxl_event.h>
> +
> #endif /* LIBXL_H */
>
> /*
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> new file mode 100644
> index 0000000..8d4dbf6
> --- /dev/null
> +++ b/tools/libxl/libxl_event.c
> @@ -0,0 +1,711 @@
> +/*
> + * Copyright (C) 2011 Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +/*
> + * Internal event machinery for use by other parts of libxl
> + */
> +
> +#include <poll.h>
> +
> +#include "libxl_internal.h"
> +
> +/*
> + * The counter osevent_in_hook is used to ensure that the application
> + * honours the reentrancy restriction documented in libxl_event.h.
> + *
> + * The application's registration hooks should be called ONLY via
> + * these macros, with the ctx locked. Likewise all the "occurred"
> + * entrypoints from the application should assert(!in_hook);
> + */
> +#define OSEVENT_HOOK_INTERN(defval, hookname, ...) \
> + (CTX->osevent_hooks \
> + ? (CTX->osevent_in_hook++, \
> + CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__), \
> + CTX->osevent_in_hook--) \
> + : defval)
> +
> +#define OSEVENT_HOOK(hookname,...) \
> + OSEVENT_HOOK_INTERN(0, hookname, __VA_ARGS__)
> +
> +#define OSEVENT_HOOK_VOID(hookname,...) \
> + OSEVENT_HOOK_INTERN((void)0, hookname, __VA_ARGS__)

Is there any reasons why we cannot use static inline functions here?


> +/*
> + * fd events
> + */
> +
> +int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
> + libxl__ev_fd_callback *func,
> + int fd, short events) {
> + int rc;
> +
> + assert(fd >= 0);
> +
> + CTX_LOCK;
> +
> + rc = OSEVENT_HOOK(fd_register, fd, &ev->for_app_reg, events, ev);
> + if (rc) goto out;
> +
> + LIBXL_LIST_INSERT_HEAD(&CTX->efds, ev, entry);
> +
> + ev->fd = fd;
> + ev->events = events;
> + ev->in_beforepolled = -1;
> + ev->func = func;
> +
> + rc = 0;
> +
> + out:
> + CTX_UNLOCK;
> + return rc;
> +}
> +
> +int libxl__ev_fd_modify(libxl__gc *gc, libxl__ev_fd *ev, short events) {
> + int rc;
> +
> + CTX_LOCK;
> + assert(libxl__ev_fd_isregistered(ev));
> +
> + rc = OSEVENT_HOOK(fd_modify, ev->fd, &ev->for_app_reg, events);
> + if (rc) goto out;
> +
> + ev->events = events;
> +
> + rc = 0;
> + out:
> + CTX_UNLOCK;
> + return rc;
> +}
> +
> +void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev) {
> + CTX_LOCK;
> +
> + if (!libxl__ev_fd_isregistered(ev))
> + goto out;
> +
> + OSEVENT_HOOK_VOID(fd_deregister, ev->fd, ev->for_app_reg);
> + LIBXL_LIST_REMOVE(ev, entry);
> + ev->fd = -1;
> +
> + if (ev->in_beforepolled >= 0 &&
> + ev->in_beforepolled < CTX->fd_beforepolled_used)
> + /* remove stale reference */
> + CTX->fd_beforepolled[ev->in_beforepolled] = NULL;
> +
> + out:
> + CTX_UNLOCK;
> +}
> +
> +/*
> + * timeouts
> + */
> +
> +
> +int libxl__gettimeofday(libxl__gc *gc, struct timeval *now_r) {
> + int rc = gettimeofday(now_r, 0);
> + if (rc) {
> + LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR, "gettimeofday failed");
> + return ERROR_FAIL;
> + }
> + return 0;
> +}
> +
> +static int time_rel_to_abs(libxl__gc *gc, int ms, struct timeval *abs_out) {
> + int rc;
> + struct timeval additional = {
> + .tv_sec = ms / 1000,
> + .tv_usec = (ms % 1000) * 1000
> + };
> + struct timeval now;
> +
> + rc = libxl__gettimeofday(gc, &now);
> + if (rc) return rc;
> +
> + timeradd(&now, &additional, abs_out);
> + return 0;
> +}
> +
> +static void time_insert_finite(libxl__gc *gc, libxl__ev_time *ev) {
> + libxl__ev_time *evsearch;
> + LIBXL_TAILQ_INSERT_SORTED(&CTX->etimes, entry, ev, evsearch, ,
> + timercmp(&ev->abs, &evsearch->abs, >));
> + ev->infinite = 0;
> +}
> +
> +static int time_register_finite(libxl__gc *gc, libxl__ev_time *ev,
> + struct timeval abs) {
> + int rc;
> +
> + rc = OSEVENT_HOOK(timeout_register, &ev->for_app_reg, abs, ev);
> + if (rc) return rc;
> +
> + ev->infinite = 0;
> + ev->abs = abs;
> + time_insert_finite(gc, ev);
> +
> + return 0;
> +}
> +
> +static void time_deregister(libxl__gc *gc, libxl__ev_time *ev) {
> + if (!ev->infinite) {
> + OSEVENT_HOOK_VOID(timeout_deregister, &ev->for_app_reg);
> + LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> + }
> +}
> +
> +
> +int libxl__ev_time_register_abs(libxl__gc *gc, libxl__ev_time *ev,
> + libxl__ev_time_callback *func,
> + struct timeval abs) {
> + int rc;
> +
> + CTX_LOCK;
> +
> + rc = time_register_finite(gc, ev, abs);
> + if (rc) goto out;
> +
> + ev->func = func;
> +
> + rc = 0;
> + out:
> + CTX_UNLOCK;
> + return rc;
> +}
> +
> +
> +int libxl__ev_time_register_rel(libxl__gc *gc, libxl__ev_time *ev,
> + libxl__ev_time_callback *func,
> + int milliseconds /* as for poll(2) */) {
> + struct timeval abs;
> + int rc;
> +
> + CTX_LOCK;
> +
> + if (milliseconds < 0) {
> + ev->infinite = 1;
> + } else {
> + rc = time_rel_to_abs(gc, milliseconds, &abs);
> + if (rc) goto out;
> +
> + rc = time_register_finite(gc, ev, abs);
> + if (rc) goto out;
> + }
> +
> + ev->func = func;
> + rc = 0;
> +
> + out:
> + CTX_UNLOCK;
> + return 0;
> +}
> +
> +int libxl__ev_time_modify_abs(libxl__gc *gc, libxl__ev_time *ev,
> + struct timeval abs) {
> + int rc;
> +
> + CTX_LOCK;
> +
> + assert(libxl__ev_time_isregistered(ev));
> +
> + if (ev->infinite) {
> + rc = time_register_finite(gc, ev, abs);
> + if (rc) goto out;
> + } else {
> + rc = OSEVENT_HOOK(timeout_modify, &ev->for_app_reg, abs);
> + if (rc) goto out;
> +
> + LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> + ev->abs = abs;
> + time_insert_finite(gc, ev);
> + }
> +
> + rc = 0;
> + out:
> + CTX_UNLOCK;
> + return rc;
> +}
> +
> +int libxl__ev_time_modify_rel(libxl__gc *gc, libxl__ev_time *ev,
> + int milliseconds) {
> + struct timeval abs;
> + int rc;
> +
> + CTX_LOCK;
> +
> + assert(libxl__ev_time_isregistered(ev));
> +
> + if (milliseconds < 0) {
> + time_deregister(gc, ev);
> + ev->infinite = 1;
> + rc = 0;
> + goto out;
> + }
> +
> + rc = time_rel_to_abs(gc, milliseconds, &abs);
> + if (rc) goto out;
> +
> + rc = libxl__ev_time_modify_abs(gc, ev, abs);
> + if (rc) goto out;
> +
> + rc = 0;
> + out:
> + CTX_UNLOCK;
> + return 0;
> +}
> +
> +void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev) {
> + CTX_LOCK;
> +
> + if (!libxl__ev_time_isregistered(ev))
> + goto out;
> +
> + time_deregister(gc, ev);
> + ev->func = 0;
> +
> + out:
> + CTX_UNLOCK;
> + return;
> +}
> +
> +
> +/*
> + * xenstore watches
> + */
> +
> +libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum) {
> + libxl__ev_watch_slot *slot = &CTX->watch_slots[slotnum];
> + libxl__ev_watch_slot *slotcontents = LIBXL_SLIST_NEXT(slot, empty);
> +
> + if (slotcontents == NULL ||
> + ((uintptr_t)slotcontents >= (uintptr_t)CTX->watch_slots &&
> + (uintptr_t)slotcontents < (uintptr_t)(CTX->watch_slots +
> + CTX->watch_nslots)))
> + /* An empty slot has either a NULL pointer (end of the
> + * free list), or a pointer to another entry in the array.
> + * So we can do a bounds check to distinguish empty from
> + * full slots.
> + */
> + /* We need to do the comparisons as uintptr_t because
> + * comparing pointers which are not in the same object is
> + * undefined behaviour; if the compiler managed to figure
> + * out that watch_slots[0..watch_nslots-1] is all of the
> + * whole array object it could prove that the above bounds
> + * check was always true if it was legal, and remove it!
> + *
> + * uintptr_t because even on a machine with signed
> + * pointers, objects do not cross zero; whereas on
> + * machines with unsigned pointers, they may cross
> + * 0x8bazillion.
> + */
> + return NULL;
> +
> + /* see comment near libxl__ev_watch_slot definition */
> + return (void*)slotcontents;
> +}
> +
> +static void watchfd_callback(libxl__gc *gc, libxl__ev_fd *ev,
> + int fd, short events, short revents) {
> + for (;;) {
> + char **event = xs_check_watch(CTX->xsh);
> + if (!event) {
> + if (errno == EAGAIN) break;
> + if (errno == EINTR) continue;
> + LIBXL__EVENT_DISASTER(gc, "cannot check/read watches", errno, 0);
> + return;
> + }
> +
> + const char *epath = event[0];
> + const char *token = event[1];
> + int slotnum;
> + uint32_t counterval;

OK, this is the last time I am going to point this out, but epath,
token, etc, should be declared above, at the beginning of the block.


> + int rc = sscanf(token, "%d/%"SCNx32, &slotnum, &counterval);
> + if (rc != 2) {
> + LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> + "watch epath=%s token=%s: failed to parse token",
> + epath, token);
> + /* oh well */
> + goto ignore;
> + }
> + if (slotnum < 0 || slotnum >= CTX->watch_nslots) {
> + /* perhaps in the future we will make the watchslots array shrink */
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, "watch epath=%s token=%s:"
> + " slotnum %d out of range [.0,%d>",
> + epath, token, slotnum, CTX->watch_nslots);
> + goto ignore;
> + }
> +
> + libxl__ev_xswatch *w = libxl__watch_slot_contents(gc, slotnum);
> +
> + if (!w) {
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch epath=%s token=%s: empty slot",
> + epath, token);
> + goto ignore;
> + }
> +
> + if (w->counterval != counterval) {
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch epath=%s token=%s: counter != %"PRIx32,
> + epath, token, w->counterval);
> + goto ignore;
> + }
> +
> + /* Now it's possible, though unlikely, that this was an event
> + * from a previous use of the same slot with the same counterval.
> + *
> + * In that case either:
> + * - the event path is a child of the watch path, in
> + * which case this watch would really have generated this
> + * event if it had been registered soon enough and we are
> + * OK to give this possibly-spurious event to the caller; or
> + * - it is not, in which case we must suppress it as the
> + * caller should not see events for unrelated paths.
> + *
> + * See also docs/misc/xenstore.txt.
> + */
> + size_t epathlen = strlen(epath);
> + size_t wpathlen = strlen(w->path);
> + if (epathlen < wpathlen ||
> + memcmp(epath, w->path, wpathlen) ||
> + (epathlen > wpathlen && epath[wpathlen] != '/')) {
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch epath=%s token=%s: not child of wpath=%s",
> + epath, token, w->path);
> + goto ignore;
> + }
> +
> + /* At last, we have checked everything! */
> + LIBXL__LOG(CTX, LIBXL__LOG_DEBUG,
> + "watch event: epath=%s token=%s wpath=%s w=%p",
> + epath, token, w->path, w);
> + w->callback(gc, w, w->path, epath);
> +
> + ignore:
> + free(event);
> + }
> +}
> +
> +static char *watch_token(libxl__gc *gc, int slotnum, uint32_t counterval) {
> + return libxl__sprintf(gc, "%d/%"PRIx32, slotnum, counterval);
> +}
> +
> +int libxl__ev_xswatch_register(libxl__gc *gc, libxl__ev_xswatch *w,
> + libxl__ev_xswatch_callback *func,
> + const char *path /* copied */) {
> + libxl__ev_watch_slot *use = NULL;
> + char *path_copy = NULL;
> + int rc;
> +
> + CTX_LOCK;
> +
> + if (!libxl__ev_fd_isregistered(&CTX->watch_efd)) {
> + rc = libxl__ev_fd_register(gc, &CTX->watch_efd, watchfd_callback,
> + xs_fileno(CTX->xsh), POLLIN);
> + if (rc) goto out_rc;
> + }
> +
> + if (LIBXL_SLIST_EMPTY(&CTX->watch_freeslots)) {
> + /* Free list is empty so there is not in fact a linked
> + * free list in the array and we can safely realloc it */
> + int newarraysize = (CTX->watch_nslots + 1) << 2;
> + int i;
> + libxl__ev_watch_slot *newarray =
> + realloc(CTX->watch_slots, sizeof(*newarray) * newarraysize);
> + if (!newarray) goto out_nomem;
> + for (i=CTX->watch_nslots; i<newarraysize; i++)

does not follow the CODING_STYLE, it should be

for (i = CTX->watch_nslots; i < newarraysize; i++)


> + LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots,
> + &newarray[i], empty);
> + CTX->watch_slots = newarray;
> + CTX->watch_nslots = newarraysize;
> + }

would it make sense to move this code in its own allocate_watch_slots
function?


> + use = LIBXL_SLIST_FIRST(&CTX->watch_freeslots);
> + assert(use);
> + LIBXL_SLIST_REMOVE_HEAD(&CTX->watch_freeslots, empty);
> +
> + path_copy = strdup(path);
> + if (!path_copy) goto out_nomem;
> +
> + int slotnum = use - CTX->watch_slots;
> + w->counterval = CTX->watch_counter++;
> +
> + if (!xs_watch(CTX->xsh, path, watch_token(gc, slotnum, w->counterval))) {
> + LIBXL__LOG_ERRNOVAL(CTX, LIBXL__LOG_ERROR, errno,
> + "create watch for path %s", path);
> + rc = ERROR_FAIL;
> + goto out_rc;
> + }
> +
> + w->slotnum = slotnum;
> + w->path = path_copy;
> + w->callback = func;
> + /* we look a bit behind the curtain of LIBXL_SLIST, to explictly
> + * assign to the pointer that's the next link. See the comment
> + * by the definitionn of libxl__ev_watch_slot */
> + use->empty.sle_next = (void*)w;
> +
> + CTX_UNLOCK;
> + return 0;
> +
> + out_nomem:
> + rc = ERROR_NOMEM;
> + out_rc:
> + if (use)
> + LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots, use, empty);
> +
> + free(w->path);
> + w->path = NULL;
> +
> + CTX_UNLOCK;
> +}
> +
> +/*
> + * osevent poll
> + */
> +
> +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> + struct pollfd *fds, int *timeout_upd,
> + struct timeval now) {
> + libxl__ev_fd *efd;
> + int i;
> +
> + /*
> + * In order to be able to efficiently find the libxl__ev_fd
> + * for a struct poll during _afterpoll, we maintain a shadow
> + * data structure in CTX->fd_beforepolled: each slot in
> + * the fds array corresponds to a slot in fd_beforepolled.
> + */
> +
> + GC_INIT(ctx);
> + CTX_LOCK;
> +
> + if (*nfds_io) {
> + /*
> + * As an optimisation, we don't touch fd_beforepolled_used
> + * if *nfds_io is zero on entry, since in that case the
> + * caller just wanted to know how big an array to give us.
> + *
> + * If !*nfds_io, the unconditional parts below are guaranteed
> + * not to mess with fd_beforepolled... or any in_beforepolled.
> + */
> +
> + /* Remove all the old references into beforepolled */
> + for (i = 0; i < CTX->fd_beforepolled_used; i++) {
> + efd = CTX->fd_beforepolled[i];
> + if (efd) {
> + assert(efd->in_beforepolled == i);
> + efd->in_beforepolled = -1;
> + CTX->fd_beforepolled[i] = NULL;
> + }
> + }
> + CTX->fd_beforepolled_used = 0;
> +
> + /* make sure our array is as big as *nfds_io */
> + if (CTX->fd_beforepolled_allocd < *nfds_io) {
> + assert(*nfds_io < INT_MAX / sizeof(libxl__ev_fd*) / 2);
> + libxl__ev_fd **newarray =
> + realloc(CTX->fd_beforepolled, sizeof(*newarray) * *nfds_io);
> + if (!newarray)
> + return ERROR_NOMEM;
> + CTX->fd_beforepolled = newarray;
> + CTX->fd_beforepolled_allocd = *nfds_io;
> + }
> + }
> +
> + int used = 0;
> + LIBXL_LIST_FOREACH(efd, &CTX->efds, entry) {
> + if (used < *nfds_io) {
> + fds[used].fd = efd->fd;
> + fds[used].events = efd->events;
> + fds[used].revents = 0;
> + CTX->fd_beforepolled[used] = efd;
> + efd->in_beforepolled = used;
> + }
> + used++;
> + }
> + int rc = used <= *nfds_io ? 0 : ERROR_BUFFERFULL;
> +
> + if (*nfds_io) {
> + CTX->fd_beforepolled_used = used;
> + }
> +
> + *nfds_io = used;
> +
> + libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
> + if (etime) {
> + int our_timeout;
> + struct timeval rel;
> + static struct timeval zero;
> +
> + timersub(&etime->abs, &now, &rel);
> +
> + if (timercmp(&rel, &zero, <)) {
> + our_timeout = 0;
> + } else if (rel.tv_sec >= 2000000) {
> + our_timeout = 2000000000;
> + } else {
> + our_timeout = rel.tv_sec * 1000 + (rel.tv_usec + 999) / 1000;
> + }
> + if (*timeout_upd < 0 || our_timeout < *timeout_upd)
> + *timeout_upd = our_timeout;
> + }
> +
> + CTX_UNLOCK;
> + GC_FREE;
> + return rc;
> +}
> +
> +void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
> + struct timeval now) {
> + int i;
> + GC_INIT(ctx);
> + CTX_LOCK;
> +
> + assert(nfds <= CTX->fd_beforepolled_used);
> +
> + for (i = 0; i < nfds; i++) {
> + if (!fds[i].revents)
> + continue;
> +
> + libxl__ev_fd *efd = CTX->fd_beforepolled[i];
> + if (!efd)
> + continue;
> +
> + assert(efd->in_beforepolled == i);
> + assert(fds[i].fd == efd->fd);
> +
> + int revents = fds[i].revents & efd->events;
> + if (!revents)
> + continue;
> +
> + efd->func(gc, efd, efd->fd, efd->events, revents);
> + }
> +
> + for (;;) {
> + libxl__ev_time *etime = LIBXL_TAILQ_FIRST(&CTX->etimes);
> + if (!etime)
> + break;
> +
> + assert(!etime->infinite);
> +
> + if (timercmp(&etime->abs, &now, >))
> + break;
> +
> + time_deregister(gc, etime);
> +
> + etime->func(gc, etime, &etime->abs);
> + }
> +
> + CTX_UNLOCK;
> + GC_FREE;
> +}
> +
> +
> +/*
> + * osevent hook and callback machinery
> + */
> +
> +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> + const libxl_osevent_hooks *hooks,
> + void *user) {
> + GC_INIT(ctx);
> + CTX_LOCK;
> + ctx->osevent_hooks = hooks;
> + ctx->osevent_user = user;
> + CTX_UNLOCK;
> + GC_FREE;
> +}
> +
> +
> +void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
> + int fd, short events, short revents) {
> + libxl__ev_fd *ev = for_libxl;
> +
> + GC_INIT(ctx);
> + CTX_LOCK;
> + assert(!CTX->osevent_in_hook);
> +
> + assert(fd == ev->fd);
> + revents &= ev->events;
> + if (revents)
> + ev->func(gc, ev, fd, ev->events, revents);
> +
> + CTX_UNLOCK;
> + GC_FREE;
> +}
> +
> +void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl) {
> + libxl__ev_time *ev = for_libxl;
> +
> + GC_INIT(ctx);
> + CTX_LOCK;
> + assert(!CTX->osevent_in_hook);
> +
> + assert(!ev->infinite);
> + LIBXL_TAILQ_REMOVE(&CTX->etimes, ev, entry);
> + ev->func(gc, ev, &ev->abs);
> +
> + CTX_UNLOCK;
> + GC_FREE;
> +}
> +
> +void libxl__event_disaster(libxl__gc *gc, const char *msg, int errnoval,
> + libxl_event_type type /* may be 0 */,
> + const char *file, int line, const char *func) {
> + libxl__log(CTX, XTL_CRITICAL, errnoval, file, line, func,
> + "DISASTER in event loop: %s%s%s%s",
> + msg,
> + type ? " (relates to event type " : "",
> + type ? libxl_event_type_to_string(type) : "",
> + type ? ")" : "");
> +
> + /*
> + * FIXME: This should call the "disaster" hook supplied to
> + * libxl_event_register_callbacks, which will be introduced in the
> + * next patch.
> + */
> +
> + const char verybad[] =
> + "DISASTER in event loop not handled by libxl application";
> + LIBXL__LOG(CTX, XTL_CRITICAL, verybad);
> + fprintf(stderr, "libxl: fatal error, exiting program: %s\n", verybad);
> + exit(-1);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> new file mode 100644
> index 0000000..25efbdf
> --- /dev/null
> +++ b/tools/libxl/libxl_event.h
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2011 Citrix Ltd.
> + * Author Ian Jackson <ian.jackson@eu.citrix.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#ifndef LIBXL_EVENT_H
> +#define LIBXL_EVENT_H
> +
> +#include <libxl.h>
> +
> +
> +/*======================================================================*/
> +
> +/*
> + * OS event handling - passing low-level OS events to libxl
> + *
> + * Event-driven programs must use these facilities to allow libxl
> + * to become aware of readability/writeability of file descriptors
> + * and the occurrence of timeouts.
> + *
> + * There are two approaches available. The first is appropriate for
> + * simple programs handling reasonably small numbers of domains:
> + *
> + * for (;;) {
> + * libxl_osevent_beforepoll(...)
> + * poll();
> + * libxl_osevent_afterpoll(...);
> + * for (;;) {
> + * r=libxl_event_check(...);
> + * if (r==LIBXL_NOT_READY) break;
> + * if (r) handle failure;
> + * do something with the event;
> + * }
> + * }
> + *
> + * The second approach uses libxl_osevent_register_hooks and is
> + * suitable for programs which are already using a callback-based
> + * event library.
> + *
> + * An application may freely mix the two styles of interaction.
> + */
> +
> +struct pollfd;
> +
> +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> + struct pollfd *fds, int *timeout_upd,
> + struct timeval now);
> + /* The caller should provide beforepoll with some space for libxl's
> + * fds, and tell libxl how much space is available by setting *nfds_io.
> + * fds points to the start of this space (and fds may be a pointer into
> + * a larger array, for example, if the application has some fds of
> + * its own that it is interested in).
> + *
> + * On return *nfds_io will in any case have been updated by libxl
> + * according to how many fds libxl wants to poll on.
> + *
> + * If the space was sufficient, libxl fills in fds[0..<new
> + * *nfds_io>] suitably for poll(2), updates *timeout_upd if needed,
> + * and returns ok.
> + *
> + * If space was insufficient, fds[0..<old *nfds_io>] is undefined on
> + * return; *nfds_io on return will be greater than the value on
> + * entry; *timeout_upd may or may not have been updated; and
> + * libxl_osevent_beforepoll returns ERROR_BUFERFULL. In this case
> + * the application needs to make more space (enough space for
> + * *nfds_io struct pollfd) and then call beforepoll again, before
> + * entering poll(2). Typically this will involve calling realloc.
> + *
> + * The application may call beforepoll with fds==NULL and
> + * *nfds_io==0 in order to find out how much space is needed.
> + *
> + * *timeout_upd is as for poll(2): it's in milliseconds, and
> + * negative values mean no timeout (infinity).
> + * libxl_osevent_beforepoll will only reduce the timeout, naturally.
> + */

so far we have always added the comment on a function above the
declaration of the function; we should keep doing it for consistency


> +void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
> + struct timeval now);
> + /* nfds and fds[0..nfds] must be from the most recent call to
> + * _beforepoll, as modified by poll.
> + *
> + * This function actually performs all of the IO and other actions,
> + * and generates events (libxl_event), which are implied by either
> + * (a) the time of day or (b) both (i) the returned information from
> + * _beforepoll, and (ii) the results from poll specified in
> + * fds[0..nfds-1]. Generated events can then be retrieved by
> + * libxl_event_check.
> + */
> +

I see that the implementation of libxl_event_check is actually missing
from this patch.
Is this patch supposed to compiled, even without the actual libxl_event
generation? Or are the two patches have to be committed together?


> +typedef struct libxl_osevent_hooks {
> + int (*fd_register)(void *uselibxl_event_check.r, int fd, void **for_app_registration_out,
> + short events, void *for_libxl);
> + int (*fd_modify)(void *user, int fd, void **for_app_registration_update,
> + short events);
> + void (*fd_deregister)(void *user, int fd, void *for_app_registration);
> + int (*timeout_register)(void *user, void **for_app_registration_out,
> + struct timeval abs, void *for_libxl);
> + int (*timeout_modify)(void *user, void **for_app_registration_update,
> + struct timeval abs);
> + void (*timeout_deregister)(void *user, void *for_app_registration_io);
> +} libxl_osevent_hooks;
> +
> +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> + const libxl_osevent_hooks *hooks,
> + void *user);
> + /* The application which calls register_fd_hooks promises to
> + * maintain a register of fds and timeouts that libxl is interested
> + * in, and make calls into libxl (libxl_osevent_occurred_*)
> + * when those fd events and timeouts occur. This is more efficient
> + * than _beforepoll/_afterpoll if there are many fds (which can
> + * happen if the same libxl application is managing many domains).
> + *
> + * For an fd event, events is as for poll(). register or modify may
> + * be called with events==0, in which case it must still work
> + * normally, just not generate any events.
> + *
> + * For a timeout event, milliseconds is as for poll().
> + * Specifically, negative values of milliseconds mean NO TIMEOUT.
> + * This is used by libxl to temporarily disable a timeout.
> + *
> + * If the register or modify hook succeeds it may update
> + * *for_app_registration_out/_update and must then return 0.
> + * On entry to register, *for_app_registration_out is always NULL.
> + *
> + * A registration or modification hook may fail, in which case it
> + * must leave the registration state of the fd or timeout unchanged.
> + * It may then either return ERROR_OSEVENT_REG_FAIL or any positive
> + * int. The value returned will be passed up through libxl and
> + * eventually returned back to the application. When register
> + * fails, any value stored into *for_registration_out is ignored by
> + * libxl; when modify fails, any changed value stored into
> + * *for_registration_update is honoured by libxl and will be passed
> + * to future modify or deregister calls.
> + *
> + * libxl will only attempt to register one callback for any one fd.
> + * libxl will remember the value stored in *for_app_registration_io
> + * by a successful call to register or modify and pass it into
> + * subsequent calls to modify or deregister.
> + *
> + * register_fd_hooks may be called only once for each libxl_ctx.
> + * libxl may make calls to register/modify/deregister from within
> + * any libxl function (indeed, it will usually call register from
> + * register_event_hooks). Conversely, the application MUST NOT make
> + * the event occurrence calls (libxl_osevent_occurred_*) into libxl
> + * reentrantly from within libxl (for example, from within the
> + * register/modify functions).
> + *
> + * Lock hierarchy: the register/modify/deregister functions may be
> + * called with locks held. These locks (the "libxl internal locks")
> + * are inside the libxl_ctx. Therefore, if those register functions
> + * acquire any locks of their own ("caller register locks") outside
> + * libxl, to avoid deadlock one of the following must hold for each
> + * such caller register lock:
> + * (a) "acquire libxl internal locks before caller register lock":
> + * No libxl function may be called with the caller register
> + * lock held.
> + * (b) "acquire caller register lock before libxl internal locks":
> + * No libxl function may be called _without_ the caller
> + * register lock held.
> + * Of these we would normally recommend (a).
> + *
> + * The value *hooks is not copied and must outlast the libxl_ctx.
> + */

while this description is very verbose, it doesn't contain informations
on:

- what is void *user;
- what is "const libxl_osevent_hooks *hooks", in particular the role of
each of these function pointers and the description of their
arguments.

If I am a user of the library, how am I supposed to pass as user? and as
hooks?
I think these few lines should go first, then the description of the
contract.


> +/* It is NOT legal to call _occurred_ reentrantly within any libxl
> + * function. Specifically it is NOT legal to call it from within
> + * a register callback. Conversely, libxl MAY call register/deregister
> + * from within libxl_event_registered_call_*.
> + */
> +
> +void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
> + int fd, short events, short revents);
> +
> +void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
> + /* Implicitly, on entry to this function the timeout has been
> + * deregistered. If _occurred_timeout is called, libxl will not
> + * call timeout_deregister; if it wants to requeue the timeout it
> + * will call timeout_register again.
> + */
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d015c7c..88e7dbb 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -24,6 +24,9 @@
> #include <stdlib.h>
> #include <string.h>
> #include <pthread.h>
> +#include <inttypes.h>
> +#include <assert.h>
> +#include <sys/poll.h>
>
> #include <xs.h>
> #include <xenctrl.h>
> @@ -91,6 +94,66 @@ _hidden void libxl__log(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
>
> /* these functions preserve errno (saving and restoring) */
>
> +typedef struct libxl__gc libxl__gc;
> +
> +typedef struct libxl__ev_fd libxl__ev_fd;
> +typedef void libxl__ev_fd_callback(libxl__gc *gc, libxl__ev_fd *ev,
> + int fd, short events, short revents);
> +struct libxl__ev_fd {
> + /* all private for libxl__ev_fd... */
> + LIBXL_LIST_ENTRY(libxl__ev_fd) entry;
> + int fd;
> + short events;
> + int in_beforepolled; /* -1 means not in fd_beforepolled */
> + void *for_app_reg;
> + libxl__ev_fd_callback *func;
> +};
> +
> +
> +typedef struct libxl__ev_time libxl__ev_time;
> +typedef void libxl__ev_time_callback(libxl__gc *gc, libxl__ev_time *ev,
> + const struct timeval *requested_abs);
> +struct libxl__ev_time {
> + /* all private for libxl__ev_time... */
> + int infinite; /* not registered in list or with app if infinite */
> + LIBXL_TAILQ_ENTRY(libxl__ev_time) entry;
> + struct timeval abs;
> + void *for_app_reg;
> + libxl__ev_time_callback *func;
> +};
> +
> +typedef struct libxl__ev_xswatch libxl__ev_xswatch;
> +typedef void libxl__ev_xswatch_callback(libxl__gc *gc, libxl__ev_xswatch*,
> + const char *watch_path, const char *event_path);
> +struct libxl__ev_xswatch {
> + /* caller should include this in their own struct */
> + /* contents are private to xswatch_register */
> + int slotnum;
> + uint32_t counterval;
> + char *path;
> + libxl__ev_xswatch_callback *callback;
> +};
> +
> +/*
> + * An entry in the watch_slots table is either:
> + * 1. an entry in the free list, ie NULL or pointer to next free list entry
> + * 2. an pointer to a libxl__ev_xswatch
> + *
> + * But we don't want to use unions or type-punning because the
> + * compiler might "prove" that our code is wrong and misoptimise it.
> + *
> + * The rules say that all struct pointers have identical
> + * representation and alignment requirements (C99+TC1+TC2 6.2.5p26) so
> + * what we do is simply declare our array as containing only the free
> + * list pointers, and explicitly convert from and to our actual
> + * xswatch pointers when we store and retrieve them.
> + */
> +typedef struct libxl__ev_watch_slot {
> + LIBXL_SLIST_ENTRY(struct libxl__ev_watch_slot) empty;
> +} libxl__ev_watch_slot;
> +
> +libxl__ev_xswatch *libxl__watch_slot_contents(libxl__gc *gc, int slotnum);
> +
> struct libxl__ctx {
> xentoollog_logger *lg;
> xc_interface *xch;
> @@ -108,6 +171,23 @@ struct libxl__ctx {
> * documented in the libxl public interface.
> */
>
> + int osevent_in_hook;
> + const libxl_osevent_hooks *osevent_hooks;
> + void *osevent_user;
> + /* See the comment for OSEVENT_HOOK_INTERN in libxl_event.c
> + * for restrictions on the use of the osevent fields. */
> +
> + int fd_beforepolled_allocd, fd_beforepolled_used;
> + libxl__ev_fd **fd_beforepolled; /* see libxl_osevent_beforepoll */
> + LIBXL_LIST_HEAD(, libxl__ev_fd) efds;
> + LIBXL_TAILQ_HEAD(, libxl__ev_time) etimes;
> +
> + libxl__ev_watch_slot *watch_slots;
> + int watch_nslots;
> + LIBXL_SLIST_HEAD(, libxl__ev_watch_slot) watch_freeslots;
> + uint32_t watch_counter; /* helps disambiguate slot reuse */
> + libxl__ev_fd watch_efd;
> +
> /* for callers who reap children willy-nilly; caller must only
> * set this after libxl_init and before any other call - or
> * may leave them untouched */
> @@ -138,12 +218,12 @@ typedef struct {
>
> #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
>
> -typedef struct {
> +struct libxl__gc {
> /* mini-GC */
> int alloc_maxsize;
> void **alloc_ptrs;
> libxl_ctx *owner;
> -} libxl__gc;
> +};
>
> #define LIBXL_INIT_GC(gc,ctx) do{ \
> (gc).alloc_maxsize = 0; \
> @@ -209,9 +289,137 @@ _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t,
> _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
> const char *path, unsigned int *nb);
> /* On error: returns NULL, sets errno (no logging) */
> -
> _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
>
> +
> +/*
> + * Event generation functions provided by the libxl event core to the
> + * rest of libxl. Implemented in terms of _beforepoll/_afterpoll
> + * and/or the fd registration machinery, as provided by the
> + * application.
> + *
> + * Semantics are similar to those of the fd and timeout registration
> + * functions provided to libxl_osevent_register_hooks.
> + *
> + * Non-0 returns from libxl__ev_{modify,deregister} have already been
> + * logged by the core and should be returned unmodified to libxl's
> + * caller; NB that they may be valid libxl error codes but they may
> + * also be positive numbers supplied by the caller.
> + *
> + * In each case, there is a libxl__ev_FOO structure which can be in
> + * one of three states:
> + *
> + * Undefined - Might contain anything. All-bits-zero is
> + * an undefined state.
> + *
> + * Idle - Struct contents are defined enough to pass to any
> + * libxl__ev_FOO function but not registered and
> + * callback will not be called. The struct does not
> + * contain references to any allocated resources so
> + * can be thrown away.
> + *
> + * Active - Request for events has been registered and events
> + * may be generated. _deregister must be called to
> + * reclaim resources.
> + *
> + * These functions are provided for each kind of event KIND:
> + *
> + * int libxl__ev_KIND_register(libxl__gc *gc, libxl__ev_KIND *GEN,
> + * libxl__ev_KIND_callback *FUNC,
> + * DETAILS);
> + * On entry *GEN must be in state Undefined or Idle.
> + * Returns a libxl error code; on error return *GEN is Idle.
> + * On successful return *GEN is Active and FUNC wil be
> + * called by the event machinery in future. FUNC will
> + * not be called from within the call to _register.
> + *
> + * void libxl__ev_KIND_deregister(libxl__gc *gc, libxl__ev_KIND *GEN_upd);
> + * On entry *GEN must be in state Active or Idle.
> + * On return it is Idle. (Idempotent.)
> + *
> + * void libxl__ev_KIND_init(libxl__ev_KIND *GEN);
> + * Provided for initialising an Undefined KIND.
> + * On entry *GEN must be in state Idle or Undefined.
> + * On return it is Idle. (Idempotent.)
> + *
> + * int libxl__ev_KIND_isregistered(const libxl__ev_KIND *GEN);
> + * On entry *GEN must be Idle or Active.
> + * Returns nonzero if it is Active, zero otherwise.
> + * Cannot fail.
> + *
> + * int libxl__ev_KiND_modify(libxl__gc*, libxl__ev_KIND *GEN,
> + * DETAILS);
> + * Only provided for some kinds of generator.
> + * On entry *GEN must be Active and on return, whether successful
> + * or not, it will be Active.
> + * Returns a libxl error code; on error the modification
> + * is not effective.
> + *
> + * All of these functions are fully threadsafe and may be called by
> + * general code in libxl even from within event callback FUNCs.
> + */
> +
> +
> +_hidden int libxl__ev_fd_register(libxl__gc*, libxl__ev_fd *ev_out,
> + libxl__ev_fd_callback*,
> + int fd, short events /* as for poll(2) */);
> +_hidden int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
> + short events);
> +_hidden void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
> +static inline void libxl__ev_fd_init(libxl__ev_fd *efd)
> + { efd->fd = -1; }
> +static inline int libxl__ev_fd_isregistered(libxl__ev_fd *efd)
> + { return efd->fd >= 0; }
> +
> +_hidden int libxl__ev_time_register_rel(libxl__gc*, 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,
> + libxl__ev_time_callback*,
> + struct timeval);
> +_hidden int libxl__ev_time_modify_rel(libxl__gc*, libxl__ev_time *ev,
> + int milliseconds /* as for poll(2) */);
> +_hidden int libxl__ev_time_modify_abs(libxl__gc*, libxl__ev_time *ev,
> + struct timeval);
> +_hidden void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
> +static inline void libxl__ev_time_init(libxl__ev_time *ev)
> + { ev->func = 0; }
> +static inline int libxl__ev_time_isregistered(libxl__ev_time *ev)
> + { return !!ev->func; }
> +
> +
> +_hidden int libxl__ev_xswatch_register(libxl__gc*, libxl__ev_xswatch *xsw_out,
> + libxl__ev_xswatch_callback*,
> + const char *path /* copied */);
> +_hidden void libxl__ev_xswatch_deregister(libxl__gc *gc, libxl__ev_xswatch*);
> +
> +static inline void libxl__ev_xswatch_init(libxl__ev_xswatch *xswatch_out)
> + { xswatch_out->slotnum = -1; }
> +static inline int libxl__ev_xswatch_isregistered(const libxl__ev_xswatch *xw)
> + { return xw->slotnum >= 0; }
> +
> +
> +
> +_hidden void libxl__event_disaster(libxl__gc*, const char *msg, int errnoval,
> + libxl_event_type type /* may be 0 */,
> + const char *file, int line,
> + const char *func);
> + /*
> + * In general, call this via the macro LIBXL__EVENT_DISASTER.
> + *
> + * Event-generating functions may call this if they might have
> + * wanted to generate an event (either an internal one ie a
> + * libxl__ev_FOO_callback or an application event), but are
> + * prevented from doing so due to eg lack of memory.
> + *
> + * NB that this function may return and the caller isn't supposed to
> + * then crash, although it may fail (and henceforth leave things in
> + * a state where many or all calls fail).
> + */
> +#define LIBXL__EVENT_DISASTER(gc, msg, errnoval, type) \
> + libxl__event_disaster(gc, msg, errnoval, type, __FILE__, __LINE__, __func__)

any reason why this shouldn't be a static inline?

> +
> /* from xl_dom */
> _hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
> _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
> @@ -536,6 +744,8 @@ _hidden int libxl__parse_mac(const char *s, libxl_mac mac);
> /* compare mac address @a and @b. 0 if the same, -ve if a<b and +ve if a>b */
> _hidden int libxl__compare_macs(libxl_mac *a, libxl_mac *b);
>
> +_hidden int libxl__gettimeofday(libxl__gc *gc, struct timeval *now_r);
> +
> #define STRINGIFY(x) #x
> #define TOSTRING(x) STRINGIFY(x)
>
> --
> 1.7.2.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
Firstly, Stefano, please trim your quotes. You don't need to quote
the whole zillion-line patch. Just quote the bits that are relevant.
Otherwise people may miss your comments as they page through trying to
find them.


Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> On Mon, 5 Dec 2011, Ian Jackson wrote:
> > +#define OSEVENT_HOOK_INTERN(defval, hookname, ...) \
> > + (CTX->osevent_hooks \
> > + ? (CTX->osevent_in_hook++, \
> > + CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__), \
> > + CTX->osevent_in_hook--) \
> > + : defval)
> > +
> > +#define OSEVENT_HOOK(hookname,...) \
> > + OSEVENT_HOOK_INTERN(0, hookname, __VA_ARGS__)
> > +
> > +#define OSEVENT_HOOK_VOID(hookname,...) \
> > + OSEVENT_HOOK_INTERN((void)0, hookname, __VA_ARGS__)
>
> Is there any reasons why we cannot use static inline functions here?

Yes, I'm afraid so. The types of the hooks vary, and even if it
weren't for that, C doesn't have Lisp's "apply".


> > + for (i=CTX->watch_nslots; i<newarraysize; i++)
>
> does not follow the CODING_STYLE, it should be
>
> for (i = CTX->watch_nslots; i < newarraysize; i++)

Fixed, thanks.


> > + LIBXL_SLIST_INSERT_HEAD(&CTX->watch_freeslots,
> > + &newarray[i], empty);
> > + CTX->watch_slots = newarray;
> > + CTX->watch_nslots = newarraysize;
> > + }
>
> would it make sense to move this code in its own allocate_watch_slots
> function?
>
> > +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> > + struct pollfd *fds, int *timeout_upd,
> > + struct timeval now);
> > + /* The caller should provide beforepoll with some space for libxl's
> > + * fds, and tell libxl how much space is available by setting *nfds_io.
... [ many lines of commentary ]
> > + * libxl_osevent_beforepoll will only reduce the timeout, naturally.
> > + */
>
> so far we have always added the comment on a function above the
> declaration of the function; we should keep doing it for consistency

I disagree. That comment style involves either:

1. Repeating the declaration at the top of the comment (or worse,
repeating the information in the declaration but in a different
format, doxygen-style); or

2. Writing a comment which contains almost entirely forward references

This is not too bad if the comment is a one-liner. But with a big
comment like this, you end up with something like:

/* The caller should provide beforepoll with some space for libxl's
* fds, and tell libxl how much space is available by setting *nfds_io.
* fds points to the start of this space (and fds may be a pointer into
* a larger array, for example, if the application has some fds of
* its own that it is interested in).
*
* On return *nfds_io will in any case have been updated by libxl
* according to how many fds libxl wants to poll on.
*
* If the space was sufficient, libxl fills in fds[0..<new
* *nfds_io>] suitably for poll(2), updates *timeout_upd if needed,
* and returns ok.
*
* If space was insufficient, fds[0..<old *nfds_io>] is undefined on
* return; *nfds_io on return will be greater than the value on
* entry; *timeout_upd may or may not have been updated; and
* libxl_osevent_beforepoll returns ERROR_BUFERFULL. In this case
* the application needs to make more space (enough space for
* *nfds_io struct pollfd) and then call beforepoll again, before
* entering poll(2). Typically this will involve calling realloc.
*
* The application may call beforepoll with fds==NULL and
* *nfds_io==0 in order to find out how much space is needed.
*
* *timeout_upd is as for poll(2): it's in milliseconds, and
* negative values mean no timeout (infinity).
* libxl_osevent_beforepoll will only reduce the timeout, naturally.
*/
int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
struct pollfd *fds, int *timeout_upd,
struct timeval now);

This is very disjointed if you try to read it. You start with

/* The caller should provide beforepoll with some space for libxl's
* fds, and tell libxl how much space is available by setting *nfds_io.
* fds points to the start of this space (and fds may be a pointer into
* a larger array, for example, if the application has some fds of
* its own that it is interested in).

and the natural reaction is "What caller?? What is this beforepoll and
*nfds_io of which you speak?? What on earth are we talking about??"

The alternative, repeating the declaration, violates the principle
that things should be said (and defined) in the code if that's
possible, rather than having the primary reference be a comment. It
also violates the principle of trying to avoid putting the same
information in two places.

If you want consistency then we should change the coding style to put
comment about a function or object declaration after the prototype or
declaration.


> > +void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct pollfd *fds,
> > + struct timeval now);
> > + /* nfds and fds[0..nfds] must be from the most recent call to
> > + * _beforepoll, as modified by poll.
> > + *
> > + * This function actually performs all of the IO and other actions,
> > + * and generates events (libxl_event), which are implied by either
> > + * (a) the time of day or (b) both (i) the returned information from
> > + * _beforepoll, and (ii) the results from poll specified in
> > + * fds[0..nfds-1]. Generated events can then be retrieved by
> > + * libxl_event_check.
> > + */
> > +
>
> I see that the implementation of libxl_event_check is actually missing
> from this patch.
> Is this patch supposed to compiled, even without the actual libxl_event
> generation? Or are the two patches have to be committed together?

libxl_event_check is not referred to by the code in this patch. It is
introduced in 15/15. The comment is indeed not 100% true in this
patch but it seemed better to provide here a comment explaining how
things are going to be rather than writing

* This function performs all of the IO and other actions,
* but this does not currently have any visible effect outside
* libxl.

in this patch and removing it in the next one.

My series compiles, and indeed is supposed to work, after each patch.


> > +typedef struct libxl_osevent_hooks {
> > + int (*fd_register)(void *uselibxl_event_check.r, int fd, void **for_app_registration_out,
> > + short events, void *for_libxl);
> > + int (*fd_modify)(void *user, int fd, void **for_app_registration_update,
> > + short events);
> > + void (*fd_deregister)(void *user, int fd, void *for_app_registration);
> > + int (*timeout_register)(void *user, void **for_app_registration_out,
> > + struct timeval abs, void *for_libxl);
> > + int (*timeout_modify)(void *user, void **for_app_registration_update,
> > + struct timeval abs);
> > + void (*timeout_deregister)(void *user, void *for_app_registration_io);
> > +} libxl_osevent_hooks;
> > +
> > +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> > + const libxl_osevent_hooks *hooks,
> > + void *user);
...
> while this description is very verbose, it doesn't contain informations
> on:
>
> - what is void *user;

I would have thought this was obvious. Every situation in C where a
function pointer is passed needs also to pass a void* (or the
equivalent) so that some context or dynamic information from the
original caller can be passed to the inner called function. So user
is stored by libxl and passed to the hooks.

> - what is "const libxl_osevent_hooks *hooks", in particular the role of
> each of these function pointers and the description of their
> arguments.

The role of these function pointers is this:

> > + /* The application which calls register_fd_hooks promises to
> > + * maintain a register of fds and timeouts that libxl is interested
> > + * in, and make calls into libxl (libxl_osevent_occurred_*)
> > + * when those fd events and timeouts occur. This is more efficient
> > + * than _beforepoll/_afterpoll if there are many fds (which can
> > + * happen if the same libxl application is managing many domains).

Ie, this is how libxl tells the application what fds and timeouts
libxl is interested in.

> If I am a user of the library, how am I supposed to pass as user? and as
> hooks?
> I think these few lines should go first, then the description of the
> contract.

Did you spot this comment, earlier ?

> > + * There are two approaches available. The first is appropriate for
> > + * simple programs handling reasonably small numbers of domains:
> > + *
...
> > + * The second approach uses libxl_osevent_register_hooks and is
> > + * suitable for programs which are already using a callback-based
> > + * event library.

libxl_osevent_hooks is for this second approach. If you know what a
callback-based event library is - particularly if you actually have
one in front of you - all of this should be obvious. If you don't
know what a callback-based event library is, then you don't have one
and you don't want to use this interface.

To help make it clear to you, here is an example of a callback-based
event library:

http://www.chiark.greenend.org.uk/doc/liboop-doc/html/ref.html
http://www.chiark.greenend.org.uk/doc/liboop-doc/html/

(Copy of the docs as installed on my colo as apparently the upstream
website has gone away.)


> > +#define LIBXL__EVENT_DISASTER(gc, msg, errnoval, type) \
> > + libxl__event_disaster(gc, msg, errnoval, type, __FILE__, __LINE__, __func__)
>
> any reason why this shouldn't be a static inline?

__FILE__, __LINE__, __func__


Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Fri, 2011-12-09 at 15:44 +0000, Stefano Stabellini wrote:
[...]
> > tools/libxl/Makefile | 2 +-
> > tools/libxl/libxl.c | 25 ++
> > tools/libxl/libxl.h | 6 +
> > tools/libxl/libxl_event.c | 711 ++++++++++++++++++++++++++++++++++++++++++
> > tools/libxl/libxl_event.h | 199 ++++++++++++
> > tools/libxl/libxl_internal.h | 216 +++++++++++++-

Please do trim the thousand lines of patch which you aren't commenting
on. Just including the hunk or function you are commenting on is
sufficient for context and snipping appropriately makes makes it far
easier to find the wheat in the chafe.

(this applies to everyone, this is just the latest one to annoy me ;-))

Ian.

[...snip...]


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
Ian Campbell writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> On Thu, 2011-12-08 at 19:53 +0000, Ian Jackson wrote:
> > All the existing files in libxl/ mention this nonexistent file
> > LICENCE. I think we should fix this in a separate patch. I'd argue
> > that my copying of the existing text isn't making the situation worse.
>
> Sure, I didn't mean to suggest you needed to fix this in this series, I
> just happened to observe it here.

Right. Yes.

> > > > +int libxl__ev_fd_register(libxl__gc *gc, libxl__ev_fd *ev,
> > > > + libxl__ev_fd_callback *func,
> > > > + int fd, short events) {
> > >
> > > Strictly speaking CODING_STYLE requires the { to be on the next line.
> >
> > Oh, I probably have a lot of those. Damn.
>
> Yeah, I refrained from commenting every time ;-)

Fixed.

> > You mean "int xs_path_is_subpath_p(const char *parent, const char *child)" ?
...
> Wouldn't most users of libxenstore doing watches need something like
> this (and probably either open code it or erroneously omit it)?

Many. If you are careful enough with your tokens you might not need
to.

> Regardless of where it goes moving that logic into a helper function
> will make it clearer what is going on, both by having a descriptive name
> and allowing the logic to be a bit more spaced out / commented, the last
> clause in particular is slightly subtle.

OK, I have split this out into a function in libxenstore (in a
separate patch):

/* Returns true if child is either equal to parent, or a node underneath
* parent; or false otherwise. Done by string comparison, so relative and
* absolute pathnames never in a parent/child relationship by this
* definition. Cannot fail.
*/
bool xs_path_is_subpath(const char *parent, const char *child);

You're right, the resulting code is clearer I think.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Fri, 9 Dec 2011, Ian Jackson wrote:
> Firstly, Stefano, please trim your quotes. You don't need to quote
> the whole zillion-line patch. Just quote the bits that are relevant.
> Otherwise people may miss your comments as they page through trying to
> find them.

I personally prefer to keep the full quote so that I can search for
references without having to go back to the other email. However I do
understand that some people don't like this so I'll trim my comment to
your patches in the future.


> > > +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> > > + struct pollfd *fds, int *timeout_upd,
> > > + struct timeval now);
> > > + /* The caller should provide beforepoll with some space for libxl's
> > > + * fds, and tell libxl how much space is available by setting *nfds_io.
> ... [ many lines of commentary ]
> > > + * libxl_osevent_beforepoll will only reduce the timeout, naturally.
> > > + */
> >
> > so far we have always added the comment on a function above the
> > declaration of the function; we should keep doing it for consistency
>
> I disagree. That comment style involves either:
>
> 1. Repeating the declaration at the top of the comment (or worse,
> repeating the information in the declaration but in a different
> format, doxygen-style); or
>
> 2. Writing a comment which contains almost entirely forward references
>
> This is not too bad if the comment is a one-liner. But with a big
> comment like this, you end up with something like:
>
> /* The caller should provide beforepoll with some space for libxl's
> * fds, and tell libxl how much space is available by setting *nfds_io.
> * fds points to the start of this space (and fds may be a pointer into
> * a larger array, for example, if the application has some fds of
> * its own that it is interested in).
> *
> * On return *nfds_io will in any case have been updated by libxl
> * according to how many fds libxl wants to poll on.
> *
> * If the space was sufficient, libxl fills in fds[0..<new
> * *nfds_io>] suitably for poll(2), updates *timeout_upd if needed,
> * and returns ok.
> *
> * If space was insufficient, fds[0..<old *nfds_io>] is undefined on
> * return; *nfds_io on return will be greater than the value on
> * entry; *timeout_upd may or may not have been updated; and
> * libxl_osevent_beforepoll returns ERROR_BUFERFULL. In this case
> * the application needs to make more space (enough space for
> * *nfds_io struct pollfd) and then call beforepoll again, before
> * entering poll(2). Typically this will involve calling realloc.
> *
> * The application may call beforepoll with fds==NULL and
> * *nfds_io==0 in order to find out how much space is needed.
> *
> * *timeout_upd is as for poll(2): it's in milliseconds, and
> * negative values mean no timeout (infinity).
> * libxl_osevent_beforepoll will only reduce the timeout, naturally.
> */
> int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> struct pollfd *fds, int *timeout_upd,
> struct timeval now);
>
> This is very disjointed if you try to read it. You start with
>
> /* The caller should provide beforepoll with some space for libxl's
> * fds, and tell libxl how much space is available by setting *nfds_io.
> * fds points to the start of this space (and fds may be a pointer into
> * a larger array, for example, if the application has some fds of
> * its own that it is interested in).
>
> and the natural reaction is "What caller?? What is this beforepoll and
> *nfds_io of which you speak?? What on earth are we talking about??"
>
> The alternative, repeating the declaration, violates the principle
> that things should be said (and defined) in the code if that's
> possible, rather than having the primary reference be a comment. It
> also violates the principle of trying to avoid putting the same
> information in two places.

I would prefer having a brief explanation of what the parameters are
before the function. At least a few times this what my eyes where
looking for reading this patch.
See for example arch/x86/include/asm/paravirt_types.h in the linux
kernel: the long description of the MACROs is before the MACROs.


> If you want consistency then we should change the coding style to put
> comment about a function or object declaration after the prototype or
> declaration.

I disagree.


> > I see that the implementation of libxl_event_check is actually missing
> > from this patch.
> > Is this patch supposed to compiled, even without the actual libxl_event
> > generation? Or are the two patches have to be committed together?
>
> libxl_event_check is not referred to by the code in this patch. It is
> introduced in 15/15. The comment is indeed not 100% true in this
> patch but it seemed better to provide here a comment explaining how
> things are going to be rather than writing
>
> * This function performs all of the IO and other actions,
> * but this does not currently have any visible effect outside
> * libxl.
>
> in this patch and removing it in the next one.
>
> My series compiles, and indeed is supposed to work, after each patch.

Yes, I think it is better this way.


> > > +typedef struct libxl_osevent_hooks {
> > > + int (*fd_register)(void *uselibxl_event_check.r, int fd, void **for_app_registration_out,
> > > + short events, void *for_libxl);
> > > + int (*fd_modify)(void *user, int fd, void **for_app_registration_update,
> > > + short events);
> > > + void (*fd_deregister)(void *user, int fd, void *for_app_registration);
> > > + int (*timeout_register)(void *user, void **for_app_registration_out,
> > > + struct timeval abs, void *for_libxl);
> > > + int (*timeout_modify)(void *user, void **for_app_registration_update,
> > > + struct timeval abs);
> > > + void (*timeout_deregister)(void *user, void *for_app_registration_io);
> > > +} libxl_osevent_hooks;
> > > +
> > > +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> > > + const libxl_osevent_hooks *hooks,
> > > + void *user);
> ...
> > while this description is very verbose, it doesn't contain informations
> > on:
> >
> > - what is void *user;
>
> I would have thought this was obvious. Every situation in C where a
> function pointer is passed needs also to pass a void* (or the
> equivalent) so that some context or dynamic information from the
> original caller can be passed to the inner called function. So user
> is stored by libxl and passed to the hooks.

It might be obvious but it is not written anywhere. Considering the
level of details you went through in the following paragraph, I am
surprised that you left to guessing what this parameter is for. Better
be pedantic than leaving things to imagination.


> > - what is "const libxl_osevent_hooks *hooks", in particular the role of
> > each of these function pointers and the description of their
> > arguments.
>
> The role of these function pointers is this:
>
> > > + /* The application which calls register_fd_hooks promises to
> > > + * maintain a register of fds and timeouts that libxl is interested
> > > + * in, and make calls into libxl (libxl_osevent_occurred_*)
> > > + * when those fd events and timeouts occur. This is more efficient
> > > + * than _beforepoll/_afterpoll if there are many fds (which can
> > > + * happen if the same libxl application is managing many domains).
>
> Ie, this is how libxl tells the application what fds and timeouts
> libxl is interested in.
>
> > If I am a user of the library, how am I supposed to pass as user? and as
> > hooks?
> > I think these few lines should go first, then the description of the
> > contract.
>
> Did you spot this comment, earlier ?

That comment explains what the application promises, not what the
parameters are. However I know what you mean: from that paragraph and
from the name of the parameters it is easy to guess what they are for.
Still I would rather avoid leaving anything to guessing.


> > > + * The second approach uses libxl_osevent_register_hooks and is
> > > + * suitable for programs which are already using a callback-based
> > > + * event library.
>
> libxl_osevent_hooks is for this second approach. If you know what a
> callback-based event library is - particularly if you actually have
> one in front of you - all of this should be obvious. If you don't
> know what a callback-based event library is, then you don't have one
> and you don't want to use this interface.

I disagree. Even if it is the first time one sees a callback-based event
library, one should be able to use it without trouble. If it is too
difficult, maybe it is not designed in the best possible way.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> On Fri, 9 Dec 2011, Ian Jackson wrote:
> > Firstly, Stefano, please trim your quotes. You don't need to quote
> > the whole zillion-line patch. Just quote the bits that are relevant.
> > Otherwise people may miss your comments as they page through trying to
> > find them.
>
> I personally prefer to keep the full quote so that I can search for
> references without having to go back to the other email. However I do
> understand that some people don't like this so I'll trim my comment to
> your patches in the future.

Everyone on the list needs to be able to read these review comments,
not just the author of the original patch. I see Ian C agrees with
me. The standard approach should always be to trim the patch to the
relevant parts only.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Fri, 2011-12-09 at 16:57 +0000, Stefano Stabellini wrote:
> On Fri, 9 Dec 2011, Ian Jackson wrote:
> > Firstly, Stefano, please trim your quotes. You don't need to quote
> > the whole zillion-line patch. Just quote the bits that are relevant.
> > Otherwise people may miss your comments as they page through trying to
> > find them.
>
> I personally prefer to keep the full quote so that I can search for
> references without having to go back to the other email.

A proper mailer has at least two windows. (/duck and cover) ;-)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> On Fri, 9 Dec 2011, Ian Jackson wrote:
> > The alternative, repeating the declaration, violates the principle
> > that things should be said (and defined) in the code if that's
> > possible, rather than having the primary reference be a comment. It
> > also violates the principle of trying to avoid putting the same
> > information in two places.
>
> I would prefer having a brief explanation of what the parameters are
> before the function. At least a few times this what my eyes where
> looking for reading this patch.

The parameters are listed in the function prototype! That's what the
function prototype is! The very first thing you should be reading is
the function prototype.

If by "what they are" you mean you want to know what they mean, then
that is why we give them names rather than just using anonymous
parameters.

Or are you saying that you want something like this:

int (*fd_register)(void *user, int fd, void **for_app_registration_out,
short events, void *for_libxl);
/* @fd@ the file descriptor
...

This is pointless boilerplate. Obviously "int fd" is a file
descriptor and it doesn't help to write it out in English as well
as C.

This style also encourages formulaic descriptions like this:
* @reg@ application's registration token (out parameter)

> See for example arch/x86/include/asm/paravirt_types.h in the linux
> kernel: the long description of the MACROs is before the MACROs.

Macros are different because they don't have prototypes, just
definitions. Nontrivial macros often need a comment beforehand to
serve in place of the function prototype.

You'll see that in general my macros have the comment beforehand, for
example:

/*
* Inserts "elm_new" into the sorted list "head".
*
* "elm_search" must be a loop search variable of the same type as
* "elm_new". "new_after_search_p" must be an expression which is
* true iff the element "elm_new" sorts after the element
* "elm_search".
*
* "search_body" can be empty, or some declaration(s) and statement(s)
* needed for "new_after_search_p".
*/
#define LIBXL_TAILQ_INSERT_SORTED(head, entry, elm_new, elm_search, \
search_body, new_after_search_p) \
do { \


> > > > +typedef struct libxl_osevent_hooks {
> > > > + int (*fd_register)(void *uselibxl_event_check.r, int fd, void **for_app_registration_out,
> > > > + short events, void *for_libxl);
> > > > + int (*fd_modify)(void *user, int fd, void **for_app_registration_update,
> > > > + short events);
> > > > + void (*fd_deregister)(void *user, int fd, void *for_app_registration);
> > > > + int (*timeout_register)(void *user, void **for_app_registration_out,
> > > > + struct timeval abs, void *for_libxl);
> > > > + int (*timeout_modify)(void *user, void **for_app_registration_update,
> > > > + struct timeval abs);
> > > > + void (*timeout_deregister)(void *user, void *for_app_registration_io);
> > > > +} libxl_osevent_hooks;
> > > > +
> > > > +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> > > > + const libxl_osevent_hooks *hooks,
> > > > + void *user);
> > ...
> > > while this description is very verbose, it doesn't contain informations
> > > on:
> > >
> > > - what is void *user;
> >
> > I would have thought this was obvious. Every situation in C where a
> > function pointer is passed needs also to pass a void* (or the
> > equivalent) so that some context or dynamic information from the
> > original caller can be passed to the inner called function. So user
> > is stored by libxl and passed to the hooks.
>
> It might be obvious but it is not written anywhere. Considering the
> level of details you went through in the following paragraph, I am
> surprised that you left to guessing what this parameter is for. Better
> be pedantic than leaving things to imagination.

All of the details in that paragraph are there because they not
otherwise clear. In particular, there are many semantic details which
need to be covered. It's precisely because there are so many
important details to state, that it's not helpful to restate in
sentences in comments things which have already been stated in the
choice of function and parameter names.

But I can add a sentence about "user" if that would help:

*
* The value of user is stored by libxl and passed to the callbacks.

Would that address this objection ?

I definitely don't want to add a sentence next to "timeout_register"
saying "this is for libxl to register a timeout" and another sentence
saying that the "struct timeval abs" is the absolute time at which the
timeout should fire and another sentence saying that "int fd" is the
file descriptor and on on.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Fri, 9 Dec 2011, Ian Jackson wrote:
> All of the details in that paragraph are there because they not
> otherwise clear. In particular, there are many semantic details which
> need to be covered. It's precisely because there are so many
> important details to state, that it's not helpful to restate in
> sentences in comments things which have already been stated in the
> choice of function and parameter names.
>
> But I can add a sentence about "user" if that would help:
>
> *
> * The value of user is stored by libxl and passed to the callbacks.
>
> Would that address this objection ?

Yes, that would help.

Maybe something similar for the parameters of the functions in
libxl_osevent_hooks, in particular for_app_registration_update.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
On Fri, 2011-12-09 at 17:17 +0000, Ian Jackson wrote:
> I definitely don't want to add a sentence next to "timeout_register"
> saying "this is for libxl to register a timeout" and another sentence
> saying that the "struct timeval abs" is the absolute time at which the
> timeout should fire and another sentence saying that "int fd" is the
> file descriptor and on on.

I agree that we don't want/need this style of documentation.

WRT comment placement I don't feel especially strongly (so I'm no doubt
going to regret getting involved) but the comment-after-prototype form
does look odd to me.

Putting the comment first is the norm (at least in the projects I
follow), even those that don't fall into the poor/boilerplate style
documentation traps you describe (which we do have some of :-(). I think
people are just used to reading the prototype and then looking for the
comment above it, no matter how unnatural/inefficient/illogical that
might seem.

One specific pitfall which trips me up is that when one has multiple
commented function prototypes in a block, thus:

a_function(...)
/* describe function a...
* ...
* at length
*/
b_function(...)
/* describe function b...
* ...
* at length
*/
then figuring out which comment goes with which prototype is non-obvious
and since I naturally look above the prototype for the comment I
inevitably get the wrong one. This is compounded somewhat because we
tend to document other types of object above rather than below and
function prototypes are therefore something of a special case.
(admittedly the correct solution here is more line breaks whatever the
style of commenting used)

We seem to have a mixture of both styles in libxl headers which is
obviously much worse than either option. If an opinion is needed to tip
the scales then I lean slightly towards commenting above but not
noticeably changing the style of commentary.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 14/15] libxl: New API for providing OS events to libxl [ In reply to ]
Ian Campbell writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl"):
> One specific pitfall which trips me up is that when one has multiple
> commented function prototypes in a block, thus:
>
> a_function(...)
> /* describe function a...
> * ...
> * at length
> */
> b_function(...)
> /* describe function b...
> * ...
> * at length
> */
> then figuring out which comment goes with which prototype is non-obvious

I find these very confusing. There should be blank lines,
like this:

a_function(...)
/* describe function a...
* ...
* at length
*/

b_function(...)
/* describe function b...
* ...
* at length
*/

> and since I naturally look above the prototype for the comment I
> inevitably get the wrong one. This is compounded somewhat because we
> tend to document other types of object above rather than below and
> function prototypes are therefore something of a special case.
> (admittedly the correct solution here is more line breaks whatever the
> style of commenting used)

Yes :-).

> We seem to have a mixture of both styles in libxl headers which is
> obviously much worse than either option. If an opinion is needed to tip
> the scales then I lean slightly towards commenting above but not
> noticeably changing the style of commentary.

OK, I will move them.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel