|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl
> [...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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |