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

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re event handling"):
> On Wed, 2011-07-13 at 15:03 +0100, Ian Jackson wrote:
> > No; the user is supposed to pass libxl a pointer to a subsection of
> > the array which is for libxl's use.
...
> 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;

Yes.

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

The former doesn't pass SOME to libxl and has to.  And what if they
don't fit ?  You have to realloc the array, probably.

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

That would be possible but it seems like leaking the caller's stuff
into the library to me.

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

I'll add a comment about that.

> > 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 think the point is that in a C header file the function declaration
is the primary piece of information and the doc comment is subtleties
to do with the parameters.  Having a comment describing a bunch of
subtleties in something you haven't read yet is just weird.  And
comments restating the declaration is obviously EBW.

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

Yes.

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

That might be worthwhile although it's not clear to me which calling
patterns are wrong.  Calling beforepoll and then not calling poll is
fine, for example.  You might change your mind and call beforepoll
again.  What's wrong is calling poll and then not calling afterpoll
and then calling check but libxl can't see you call poll.

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

They don't wait; they enable the generation of events.  Would you care
to suggest a different name ?

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

We don't want to put a void* in an idl type.  A "foreign" caller (eg,
one in a different process communicating by IPC) can't easily invent
pointers.

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

Except that I think the extra _ is confusing rather than helpful, yes.

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

Oh, I see, yes, perhaps making it non-anonymous would be better.

Ian.