From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 2/3] RFC: libxl: API changes re event handling Date: Wed, 13 Jul 2011 15:03:50 +0100 Message-ID: <19997.42438.667955.352924@mariner.uk.xensource.com> References: <1310495823-27077-1-git-send-email-ian.jackson@eu.citrix.com> <1310495823-27077-2-git-send-email-ian.jackson@eu.citrix.com> <1310495823-27077-3-git-send-email-ian.jackson@eu.citrix.com> <1310552507.634.377.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1310552507.634.377.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: Jim, Fehlig , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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..] > > + * and updates *timeout_upd if needed and returns ok. > > + * if space was insufficient, fds[0..] 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.