Re: [Xen-devel] [PATCH 08/10] libxl: New event generation API

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 08/10] libxl: New event generation 
API"):
> I think I've seen most of this before too so again I only skimmed it.
> Nothing much jumped out but I must admit I'm suffering from Friday
> afternoon review fatigue...

Heh.

> On Fri, 2012-01-06 at 20:35 +0000, Ian Jackson wrote:
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 78a1cc6..6728479 100644
...
> > +libxl_event_type = Enumeration("event_type", [
> > +    (1, "DOMAIN_SHUTDOWN"),
> > +    (2, "DOMAIN_DESTROY"),
> > +    (3, "DISK_EJECT"),
> > +    ])
> > +
> > +libxl_ev_user = Number("libxl_ev_user")
> 
> This doesn't turn into libxl_libxl_ev_user in the code, does it?
> 
> No, I checked, It's right, Good ;-)

In fact to make the ocaml binding work it is necessary to write this
as
   libxl_ev_user = UInt(64);
but we can keep the typedef.  This is because the ocaml idl generator
wants to know the size of the integer.  I think this is not an
unreasonable requirement so I will keep that change even though I'm
about to disable the generation of an ocaml binding for libxl_event
for other reasons (mainly, the lack of KeyedUnion).

> > +libxl_ev_link = Builtin("ev_link", passby=PASS_BY_REFERENCE, private=True)
> > +
> > +libxl_event = Struct("event",[
> > +    ("link",     libxl_ev_link,0,
> > +     "for use by libxl; caller may use this once the event has been"
> > +     " returned by libxl_event_{check,wait}"),
> 
> I've got a pending patch which does away with comments in the IDL syntax
> in favour of source level comments (e.g. "# for use by...."). I don't
> think anyone reads the comments in generated code in preference to the
> comments in the IDL source...
> 
> Either we race here or you can just go straight for the source level
> comments.

I'll do the latter.

> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 8270f34..a18c6b2 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> [...]
> > +        case LIBXL_EVENT_TYPE_DISK_EJECT:
> > +            /* XXX what is this for? */
> 
> This is signalled by qemu when the guest opens the CD-ROM drive.
> 
> (not sure if this is your comment or an existing one which got
> reindented.)

It's an existing comment, which I didn't feel confident to remove.
I guess if we're happy with all the code here I could delete it.

Ian.

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