[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



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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.