Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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