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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Wed, 2011-07-13 at 15:03 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re 
> event handling"):
> > > +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.

So a user would do:
        poll_fds[SOME];
        int nr = 0;
        poll_fds[nr++] = blah;
        poll_fds[nr++] = blahbla;
        libxl_osevent_beforepoll(..., &nr, &poll_fds[0], ....)
where nr is updated (+= N) and poll_fds[nr...nr+N] are initialised or is
it:
        poll_fds[SOME];
        int nr = 0, nr_libxl;
        poll_fds[nr++] = blah;
        poll_fds[nr++] = blahbla;
        nr_libxl = SOME-nr;
        libxl_osevent_beforepoll(..., &nr_libxl, &poll_fds[nr], ....)
        nr += nr_libxl;

I think you were describing the latter but the former seems marginally
easier on the user (or seems so to me).

Actually, of course it was the latter because the former requires you to
pass the limit (==SOME) somewhere which the interface doesn't include
(but perhaps it should).

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

On second reading it's clear that nfds_io is always updated but not so
much that you could deliberately call it with fds=NULL (although I know
that's a common idiom and it's obvious now you mention it).

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

OK.

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

I think it's the norm in most other projects etc too. To be honest
documenting functions afterwards just looks strange to me. Maybe it's a
Linux-ism I've become used too.

I do occasionally hope that we'll grow kerneldoc style comments in
libxl.h, I don't know if that actually relies on comments and functions
being in a certain order though.

> > > +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.

So it'll call the hooks if you've registered them etc?

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

Right.

I wonder if libxl should internally count pollbefore/after and warn if
libxl_event_check is called while they are unbalanced to catch this sort
of issue.

> > > +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.

Ah, ok that's a useful high-level thing I'd missed.

Where do the libxl_await_* functions fit in? Do they enable the
generation of the events to which they refer or do they actually stop
and wait? Await makes it sound like the latter but the comment preceding
those functions says
        * Events are only generated if they have been requested.
        * The following functions request the generation of specific
        events.
which sounds like the former.

> > > +/* Alternatively or additionally, the application may also use this: */
> > > +
> > > +void libxl_event_register_callbacks(libxl_ctx *ctx, void *user, uint64_t 
> > > mask,

Here (and in various other places) you have a "void *user" closure thing
but I noticed in libxl_await_* you have "uint64_t user" -- is this
inconsistency deliberate? (AIUI the void * is passed to event_occurs and
the uint64_t is stashed in the libxl_event structure).

> > > +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.

So it should be:
        typedef struct libxl__awaiting_disk_eject
        libxl_awaiting_disk_eject;
? and libxl__awaiting_disk_eject will be in libxl_internal.idl (which is
added by Anthony's QMP series).

> > > +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.

This:
        ("disk_eject", Struct(None, [...
        
creates an anonymous struct (due to the None) I was wondering if:
libxl_event_disk_eject = Struct("event_disk_eject", [
                                        ("vdev", string)
                                        ("disk", libxl_device_disk)])
libxl_event = Struct("event", [
    ...
           ("disk_eject", libxl_event_disk_eject)
might be convenient to users so they have a name for something they may
want to pass around. I suppose they will probably just pass around the
libxl_event in reality.

Ian.


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