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
|
Home