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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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