Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re event handling

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re 
event handling"):
> I think it would be good to have the model validated by Jim (CC'd) since
> I imagine that the libvirt backend will be one of the primary users of
> the registration mechanism.

Indeed.  I was thinking about libvirt (and I think I remember how the
libvirt fd and event handling works from when I read it last time)
amongst other callers.

> There's a lot of docs here, which is excellent, but I've only had two
> cups of tea so far today so I only managed a fairly high-level think
> about it all.

:-).

Thanks for all your comments.  Detailed reponsese follow:

> > + * The second approach uses libxl_osevent_register_hooks and is
> > + * suitable for programs which are already using a callback-based
> > + * event library.
> 
> An example of this style of usage would be handy too.

It's not going to be very interesting.  You basically just implement
the fd_register etc. in terms of your own event system.

> > +int libxl_osevent_beforepoll(libxl_ctx *ctx, int *nfds_io,
> > +                           struct poll *fds, int *timeout_upd);
> > +  /* app should provide beforepoll with some fds, and give number in 
> > *nfds_io.
> > +   * *nfds_io will in any case be updated according to how many we want.
> > +   * if space was sufficient, fills in fds[0..<new *nfds_io>]
> > +   *   and updates *timeout_upd if needed and returns ok.
> > +   * if space was insufficient, fds[0..<old *nfds_io>] is undefined,
> > +   *  *timeout_upd may or may not have been updated, returns 
> > ERROR_BUFERFULL.
> > +   */
> 
> It's not immediately obvious but I presume the user is entitled to
> provide an fds etc which already contains the file descriptors it is
> itself interested in and that this function will augment it.

No; the user is supposed to pass libxl a pointer to a subsection of
the array which is for libxl's use.

> Is there some way for a caller to determine how many entries in the
> struct poll * the library will use?

Yes.  Call it with fds==NULL and *nfds_io==0.  Is this not clear ?

> Should we also have an interface suitable for users of select?

Possibly.  Most people are using poll nowadays though.  I would wait
with providing it until someone wants it, at which point it's a SMOP.

> It is currently conventional in libxl.h to document a function before
> it's prototype rather than after.

I think this is unhelpful.  Can we change this convention ?

> > +void libxl_osevent_afterpoll(libxl_ctx *ctx, int nfds, const struct poll 
> > *fds);
> > +  /* nfds and fds[0..nfds] must be from _beforepoll as modified by poll
> > +   */
> 
> What does this function do in practice?

All things that libxl wants to do and which can be done according to
fds[].revents (and the time).  I should add something about this to
the comment, evidently.

If you don't call this then libxl_event_check may claim there are no
events to be had because libxl hasn't read the relevant fds.

> > +int libxl_osevent_register_hooks(libxl_ctx *ctx, void *user,
> > +  int (*fd_register)(void *user, int fd, void **for_app_registration_out,
> > +                     short events, void *for_libxl),
> > [...]
> > +  void (*time_deregister)(void *user, void *for_app_registration_io,
> > +                          void *for_libxl));
> 
> I think passing a pointer to a struct libxl_osevent_hooks would be
> better than passing loads of fn pointers.

You're right.

> > +  /* The application which calls register_fd_hooks promises to
> > +   * maintain a register of fds and timeouts that libxl is interested
> > +   * in,
> 
> It would probably be useful to provide some helper functions for
> maintaining this register and for dispatching the foo_occurred things.

Applications that want to use this interface already have one of
those.  Ie, any application with an existing callback-style event
loop.  They already have the appropriate data structures.

> [...]
> >  typedef struct {
> >      /* event type */
> > @@ -293,41 +410,136 @@ typedef struct {
> >      char *token;
> >  } libxl_event;

This struct libxl_event leaked through; it's to be abolished in favour
of the one from the idl.

> > +int libxl_event_check(libxl_ctx *ctx, libxl_event *event_r,
> > +                      unsigned long typemask,
> > +                      int (*predicate)(const libxl_event*, void *user),
> > +                      void *predicate_user);
> 
> I was a little confused for a while because this function is associated
> with the libxl_osevent_*poll methods but the register_hooks stuff is
> defined in the middle. I think it would be better to define both sets of
> functions in contiguous and distinct blocks.

No, libxl_event_check can be used with either
osevent_before/afterpoll, or osevent_register_hooks.  You can mix and
match.

libxl_osevent_* are interchangeable (and miscible) ways of getting
fd readability, timeouts, etc., into libxl.

libxl_event_8 are interchangeable (and miscible) ways of getting
higher-level occurrences about domains out of libxl and into the
application.

> > +int libxl_event_wait(libxl_ctx *ctx, libxl_event *event_r,
> > +                     unsigned long typemask,
> > +                     int (*predicate)(const libxl_event*, void *user),
> > +                     void *predicate_user);
> > +  /* Like libxl_event_check but blocks if no suitable events are
> > +   * available, until some are.  Uses
> > +   * libxl_osevent_beforepoll/_afterpoll so may be inefficient if very
> > +   * many domains are being handled by a single program.
> > +   */
> [...]
> > +int libxl_event_free(libxl_ctx, *ctx, libxl_event *event);
> 
> It looks as if the libxl_event passed to libxl_event_wait is owned by
> the caller so it could use a local struct or manage allocation itself,
> is that right?

No, there's one too few *s.  It should be
  int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_out,

> > +/* Alternatively or additionally, the application may also use this: */
> > +
> > +void libxl_event_register_callbacks(libxl_ctx *ctx, void *user, uint64_t 
> > mask,
> > +   void (*event_occurs)(void *user, const libxl_event *event),
> > +   void (*disaster)(void *user, libxl_event_type type,
> > +                    const char *msg, int errnoval));
> > +  /*
> > +   * Arranges that libxl will henceforth call event_occurs for any
> > +   * events whose type is set in mask, rather than queueing the event
> > +   * for retrieval by libxl_event_check/wait.  Events whose bit is
> > +   * clear in mask are not affected.
> 
> Would it be useful to have separate callbacks for the different event
> types?

I don't think so.

> Or perhaps just a helper function which can be registered here and takes
> a dispatch table from the app (I suppose the app could build this
> itself, but maybe it would be useful functionality).

The application might want to select by domain first, and then by
type.  I don't think it would be right to force it into a particular
structure.  It's not like a dispatch table is hard to do in the
application if you want to - it's just an array, which you can index
directly with the type number.

> > +typedef struct libxl_awaiting_disk_eject libxl_awaiting_disk_eject;
> 
> Is this struct defined in the IDL? If yes then I think you get this for
> free. If no then shouldn't it be?

No, it's an opaque thing used by the library to store its own
information about the application's interest.

> > +libxl_event = Struct("event",[
> > +    ("domid",    libxl_domid),
> > +    ("domuuid",  libxl_uuid),
> > +    ("type",     libxl_event_type),
> > +    ("for_user", uint64),
> > +    ("u", KeyedUnion(None,"type",
> > +          [("domain_shutdown", Struct(None, [
> > +                                             ("shutdown_reason", uint8)
> > +                                      ])),
> > +           ("domain_destroy", Struct()),
> > +           ("disk_eject", Struct(None, [
> > +                                        ("vdev", string)
> > +                                        ("disk", libxl_device_disk)
> > +                                 ])),
> > +           ]))])
> 
> It might be convenient to users if these structs were named so they can
> dispatch to handle_domain_shutdown(struct libxl_event_domain_shutdown
> *info) ?

I'm not sure I follow.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel