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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-01-06 at 20:35 +0000, Ian Jackson wrote:
> Replace the existing API for retrieving high-level events (events
> about domains, etc.) from libxl with a new one.
> 
> This changes the definition and semantics of the `libxl_event'
> structure, and replaces the calls for obtaining information about
> domain death and disk eject events.
> 
> This is an incompatible change, sorry.  The alternative was to try to
> provide both the previous horrid API and the new one, and would also
> involve never using the name `libxl_event' for the new interface.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

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


> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 78a1cc6..6728479 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -75,11 +75,6 @@ libxl_action_on_shutdown = 
> Enumeration("action_on_shutdown", [
>      (6, "COREDUMP_RESTART"),
>      ])
> 
> -libxl_event_type = Enumeration("event_type", [
> -    (1, "DOMAIN_DEATH"),
> -    (2, "DISK_EJECT"),
> -    ])
> -
>  libxl_button = Enumeration("button", [
>      (1, "POWER"),
>      (2, "SLEEP"),
> @@ -394,3 +389,32 @@ libxl_sched_sedf = Struct("sched_sedf", [
>      ("extratime", integer),
>      ("weight", integer),
>      ], dispose_fn=None)
> +
> +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 ;-)

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

> +    ("domid",    libxl_domid),
> +    ("domuuid",  libxl_uuid),
> +    ("for_user", libxl_ev_user),
> +    ("type",     libxl_event_type),
> +    ("u", KeyedUnion(None, libxl_event_type, "type",
> +          [("domain_shutdown", Struct(None, [
> +                                             ("shutdown_reason", uint8),
> +                                      ])),
> +           ("domain_destroy", Struct(None, [])),
> +           ("disk_eject", Struct(None, [
> +                                        ("vdev", string),
> +                                        ("disk", libxl_device_disk),
> +                                 ])),
> +           ]))])
> 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.)

Ian.


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