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

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

On Wed, 2011-07-13 at 17:17 +0100, Ian Jackson wrote:
> 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:
> > 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 ?

Something with mask/unmask perhaps? (e.g. libxl_event_unmask_FOO or
libxl_event_FOO_unmask?) That fits in with the idea that events start of
masked and you unmask them if you want to receive them and is consistent
with the terminology used for the equivalent parameter to
libxl_event_register_callbacks.

Or else just a simple enable/disable?

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

Well, the pointer is only useful once it completes the circuit and gets
back to the "foreign" caller but I take your point.

It might nice to declare something akin to intptr_t in the IDL and us
that?

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

I think it's consistent with the libxl naming convention (such as it is)
by distinguishing this case from a non-opaque struct getting typedef'd.
There are a bunch of existing things declared that way.

Ian.