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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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