WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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

<Prev in Thread] Current Thread [Next in Thread>