[RFC 0/3] libxl event handling

[RFC 0/3] libxl event handling

[Ian Jackson]

libxl's arrangements for dealing with events are broken.

Following some informal discussions, I have come up with this set of
proposals.  At the moment this is just a set of IDL and header file
changes, describing the new interfaces.

Unfortunately I think it's inevitable that this will be an
incompatible API change.  The existing API is really bad.

[PATCH 1/3] RFC: libxl: API changes re domain type (and keyed union semantics)

[Ian Jackson]

Give the HVM domain type the name "HVM" as is generally used in the
Xen universe, rather than "FV" which is not used elsewhere.

Keyed unions should be keyed off the individual union value, not off
an arbitrary expression.

Change the "hvm" field in domain_build_info to be called "type" and
have an appropriate enum type.

These are are incompatible changes to the API/ABI.

THIS PATCH IS AN RFC AND SHOULD NOT BE APPLIED.  It contains only the
suggested changes to libxl.idl, and not any of the necessary
implementation in the idl compiler nor consequential changes to libxl
and xl.
---
 tools/libxl/libxl.idl |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
index 6e39734..8fb883f 100644
--- a/tools/libxl/libxl.idl
+++ b/tools/libxl/libxl.idl
@@ -21,7 +21,7 @@ libxl_hwcap = Builtin("hwcap")
 #
 
 libxl_domain_type = Enumeration("domain_type", [
-    (1, "FV"),
+    (1, "HVM"),
     (2, "PV"),
     ])
 
@@ -158,9 +158,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("shadow_memkb",    uint32),
     ("disable_migrate", bool),
     ("cpuid",           libxl_cpuid_policy_list),
-    ("hvm",             bool),
-    ("u", KeyedUnion(None, "hvm",
-                [("hvm", "%s", Struct(None,
+    ("type",            libxl_domain_type),
+    ("u", KeyedUnion(None, "type",
+                [("hvm", Struct(None,
                                        [("firmware", string),
                                         ("pae", bool),
                                         ("apic", bool),
@@ -173,7 +173,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                         ("timer_mode", integer),
                                         ("nested_hvm", bool),
                                         ])),
-                 ("pv", "!%s", Struct(None,
+                 ("pv", Struct(None,
                                        [("kernel", libxl_file_reference),
                                         ("slack_memkb", uint32),
                                         ("bootloader", string),
-- 
1.5.6.5

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

[Ian Jackson]

Replace all the existing libxl_event calls and types with a new,
rational scheme.

These are are incompatible changes to the API/ABI.

THIS PATCH IS AN RFC AND SHOULD NOT BE APPLIED.  It contains only the
suggested changes to libxl.h, and not any of the necessary
implementation nor consequential changes.
---
 tools/libxl/libxl.h   |  280 +++++++++++++++++++++++++++++++++++++++++++------
 tools/libxl/libxl.idl |   28 ++++-
 2 files changed, 269 insertions(+), 39 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index da878e4..ec20b2a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -218,6 +218,8 @@ enum {
     ERROR_INVAL = -6,
     ERROR_BADFAIL = -7,
     ERROR_GUEST_TIMEDOUT = -8,
+    ERROR_NOT_READY = -9,
+    ERROR_OSEVENT_REG_FAIL = -10,
 };
 
 #define LIBXL_VERSION 0
@@ -283,7 +285,122 @@ int libxl_run_bootloader(libxl_ctx *ctx,
 
   /* 0 means ERROR_ENOMEM, which we have logged */
 
-/* events handling */
+
+
+/*
+ * OS event handling - passing low-level OS events to libxl
+ *
+ * Event-driven programs must use these facilities to allow libxl
+ * to become aware of readability/writeability of file descriptors
+ * and the occurrence of timeouts.
+ *
+ * There are two approaches available.  The first is appropriate for
+ * simple programs handling reasonably small numbers of domains:
+ *
+ *   for (;;) {
+ *      libxl_osevent_beforepoll(...)
+ *      poll();
+ *      libxl_osevent_afterpoll(...);
+ *      for (;;) {
+ *        r=libxl_event_check(...);
+ *        if (r==LIBXL_NOT_READY) break;
+ *        if (r) handle failure;
+ *        do something with the event;
+ *      }
+ *   }
+ *
+ * The second approach uses libxl_osevent_register_hooks and is
+ * suitable for programs which are already using a callback-based
+ * event library.
+ *
+ * An application may freely mix the two styles of interaction.
+ */
+
+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.
+   */
+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
+   */
+
+
+int libxl_osevent_register_hooks(libxl_ctx *ctx, void *user,
+  int (*fd_register)(void *user, int fd, void **for_app_registration_out,
+                     short events, void *for_libxl),
+  int (*fd_modify)(void *user, int fd, void **for_app_registration_update,
+                   short events, void *for_libxl),
+  void (*fd_deregister)(void *user, int fd, void *for_app_registration),
+  void (*timeout_register)(void *user, void **for_app_registration_out,
+                           int milliseconds, void *for_libxl),
+  void (*timeout_modify)(void *user, void **for_app_registration_update,
+                         int milliseconds, void *for_libxl),
+  void (*time_deregister)(void *user, void *for_app_registration_io,
+                          void *for_libxl));
+  /* The application which calls register_fd_hooks promises to
+   * maintain a register of fds and timeouts that libxl is interested
+   * in, and make calls into libxl (libxl_osevent_occurred_*)
+   * when those fd events and timeouts occur.  This is more efficient
+   * than _beforepoll/_afterpoll if there are many fds (which can
+   * happen if the same libxl application is managing many domains).
+   *
+   * For an fd event, events is as for poll().  register or modify may
+   * be called with events==0, in which case it must still work
+   * normally, just not generate any events.
+   *
+   * For a timeout event, milliseconds is as for poll().
+   * Specifically, negative values of milliseconds mean NO TIMEOUT.
+   * This is used by libxl to temporarily disable a timeout.
+   *
+   * If the register or modify function succeeds it may update
+   * *for_app_registration_out/_update and must then return 0.
+   * On entry to register, *for_app_registration_out is always NULL.
+   *
+   * If it fails it must leave the registration state of the fd or
+   * timeout unchanged.  It may then either return
+   * ERROR_OSEVENT_REG_FAIL or any positive int.  The value returned
+   * will be passed up through libxl and eventually returned back to
+   * the application.  When register fails, any value stored into
+   * *for_registration_out is ignored; when modify fails, any changed
+   * value stored into *for_registration_update is honoured and will
+   * be passed to future modify or deregister calls.
+   *
+   * libxl will only attempt to register one callback for any one fd.
+   * libxl will remember the value stored in *for_app_registration_io
+   * by a successful call to register or modify and pass it into
+   * subsequent calls to modify or deregister.
+   *
+   * register_fd_hooks may be called only once for each libxl_ctx.
+   * libxl may make register/modify/deregister from within any libxl
+   * function (indeed, it will usually call register from
+   * register_event_hooks).  Conversely, the application is NOT
+   * PERMITTED to make the event occurrence calls
+   * (libxl_osevent_occurred_*) into libxl reentrantly from
+   * within libxl (for example, from within the register/modify
+   * functions.
+   */
+
+void libxl_osevent_occurred_fd(libxl_ctx *ctx, void *for_libxl,
+                               int fd, short events, short revents);
+void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
+  /* It is NOT legal to call these functions reentrantly within any libxl
+   * function.  Specifically it is NOT legal to call it from within
+   * a register callback.  Conversely, libxl MAY call register/deregister
+   * from within libxl_event_registerd_call_*.
+   */
+
+
+/*
+ * Domain event handling - getting Xen events from libxl
+ */
+
+
+#define LIBXL_EVENTMASK_ALL (~(unsigned long)0)
 
 typedef struct {
     /* event type */
@@ -293,41 +410,136 @@ typedef struct {
     char *token;
 } libxl_event;
 
-typedef struct {
-    char *path;
-    char *token;
-} libxl_waiter;
+int libxl_event_check(libxl_ctx *ctx, libxl_event *event_r,
+                      unsigned long typemask,
+                      int (*predicate)(const libxl_event*, void *user),
+                      void *predicate_user);
+  /* Searches for an event, already-happened, which matches typemask
+   * and predicate.  predicate==0 matches any event.  Returns the
+   * event, which must then later be freed by the caller using
+   * libxl_event_free.
+   *
+   * Returns ERROR_NOT_READY if no such event has happened.
+   */
 
+int libxl_event_wait(libxl_ctx *ctx, libxl_event *event_r,
+                     unsigned long typemask,
+                     int (*predicate)(const libxl_event*, void *user),
+                     void *predicate_user);
+  /* Like libxl_event_check but blocks if no suitable events are
+   * available, until some are.  Uses
+   * libxl_osevent_beforepoll/_afterpoll so may be inefficient if very
+   * many domains are being handled by a single program.
+   */
 
-int libxl_get_wait_fd(libxl_ctx *ctx, int *fd);
-/* waiter is allocated by the caller */
-int libxl_wait_for_domain_death(libxl_ctx *ctx, uint32_t domid, libxl_waiter *waiter);
-/* waiter is a preallocated array of num_disks libxl_waiter elements */
-int libxl_wait_for_disk_ejects(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disks, int num_disks, libxl_waiter *waiter);
-int libxl_get_event(libxl_ctx *ctx, libxl_event *event);
-int libxl_stop_waiting(libxl_ctx *ctx, libxl_waiter *waiter);
-int libxl_free_event(libxl_event *event);
-int libxl_free_waiter(libxl_waiter *waiter);
+int libxl_event_free(libxl_ctx, *ctx, libxl_event *event);
+
+
+/* Alternatively or additionally, the application may also use this: */
+
+void libxl_event_register_callbacks(libxl_ctx *ctx, void *user, uint64_t mask,
+   void (*event_occurs)(void *user, const libxl_event *event),
+   void (*disaster)(void *user, libxl_event_type type,
+                    const char *msg, int errnoval));
+  /*
+   * Arranges that libxl will henceforth call event_occurs for any
+   * events whose type is set in mask, rather than queueing the event
+   * for retrieval by libxl_event_check/wait.  Events whose bit is
+   * clear in mask are not affected.
+   *
+   * event becomes owned by the application and must be freed, either
+   * by event_occurs or later.
+   *
+   * event_occurs may be NULL if mask is 0.
+   *
+   * libxl_event_register_callback also provides a way for libxl to
+   * report to the application that there was a problem reporting
+   * events; this can occur due to lack of host memory during event
+   * handling, or other wholly unrecoverable errors from system calls
+   * made by libxl.  This will not happen for frivolous reasons - only
+   * if the system, or the Xen components of it, are badly broken.
+   *
+   * msg and errnoval will describe the action that libxl was trying
+   * to do, and type specifies the type of libxl events which may be
+   * missing.  type may be 0 in which case events of all types may be
+   * missing.
+   *
+   * disaster may be NULL.  If it is, or if
+   * libxl_event_register_callbacks has not been called, errors of
+   * this kind are fatal to the entire application: libxl will print a
+   * message to stderr and call exit(-1).
+   *
+   * If disaster returns, it may be the case that some or all future
+   * libxl calls will return errors; likewise it may be the case that
+   * no more events (of the specified type, if applicable) can be
+   * produced.  An application which supplies a disaster function
+   * should normally react either by exiting, or by (when it has
+   * returned to its main event loop) shutting down libxl with
+   * libxl_ctx_free and perhaps trying to restart it with
+   * libxl_ctx_init.
+   *
+   * Reentrancy: it IS permitted to call libxl from within
+   * event_occurs.  It is NOT permitted to call libxl from within
+   * disaster.
+   *
+   * libxl_event_register_callbacks may be called as many times, with
+   * different parameters, as the application likes; the most recent
+   * call determines the libxl behaviour.  There is only one mask.
+   */
 
-/*
- * Returns:
- *  - 0 if the domain is dead but there is no cleanup to be done. e.g
- *    because someone else has already done it.
- *  - 1 if the domain is dead and there is cleanup to be done.
- *
- * Can return error if the domain exists and is still running.
- *
- * *info will contain valid domain state iff 1 is returned. In
- * particular if 1 is returned then info->shutdown_reason is
- * guaranteed to be valid since by definition the domain is
- * (shutdown||dying))
- */
-int libxl_event_get_domain_death_info(libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_dominfo *info);
 
-/*
- * Returns true and fills *disk if the caller should eject the disk
+/* Events are only generated if they have been requested.
+ * The following functions request the generation of specific events.
  */
-int libxl_event_get_disk_eject_info(libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk);
+
+typedef struct libxl_awaiting_domain_death libxl_awaiting_domain_death;
+int libxl_await_domain_death(libxl_ctx *ctx, uint32_t domid, uint64_t user,
+                             libxl_awaiting_domain_death **waiter_out);
+void libxl_noawait_domain_death(libxl_ctx *ctx, uint32_t domid,
+                             libxl_awaiting_domain_death *waiter);
+  /* Generates DOMAIN_SHUTDOWN and DOMAIN_DESTROY events.
+   * A domain which is destroyed before it shuts down generates
+   * only a DESTROY event. */
+
+typedef struct libxl_awaiting_disk_eject libxl_awaiting_disk_eject;
+int libxl_await_disk_eject(libxl_ctx *ctx, uint32_t domid, uint64_t user,
+                           const char *vdev,
+                           libxl_awaiting_disk_eject **waiter_out);
+void int libxl_noawait_disk_eject(libxl_ctx *ctx, uint32_t domid,
+                                  const char *vdev,
+                                  libxl_awaiting_disk_eject *waiter);
+  /* Generates DISK_EJECT events.  vdev will be copied, and will be
+   * returned exactly as the vdev member of event.u. */
+
+
+ /* General rules for event request functions:
+  *
+  * The caller may wait for identical events more than once.  If they
+  * do so, each actual occurrence will generate several events to be
+  * returned by libxl_event_check.  Aside from this, each occurrence
+  * of each event is returned by libxl_event_check exactly once.
+  *
+  * The user value is returned in the generated events and may be
+  * used by the caller for whatever it likes.  Storing a pointer value
+  * in it is permissible.
+  *
+  * If _noawait is called after the event has occurred, the event may
+  * still be returned by libxl_event_check.
+  *
+  * The value "waiter" must be stored by the caller if the caller is
+  * going to call noawait, but may otherwise be ignored.
+  *
+  * If libxl_ctx_free is called, all event generation is cancelled and
+  * it is no longer permissible to call nowait; all "waiter" values
+  * are invalidated by libxl_ctx_free.
+  *
+  * Applications should ensure that they eventually retrieve every
+  * event using libxl_event_check or libxl_event_wait, since events
+  * which occur but are not retreived by the application will be
+  * queued inside libxl indefinitely.  libxl_event_check/_wait
+  * may be O(n) where n is the number of such queued events.
+  */
+
 
 int libxl_domain_rename(libxl_ctx *ctx, uint32_t domid,
                         const char *old_name, const char *new_name);
diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
index 8fb883f..8b285a4 100644
--- a/tools/libxl/libxl.idl
+++ b/tools/libxl/libxl.idl
@@ -73,11 +73,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"),
@@ -371,3 +366,26 @@ libxl_sched_credit = Struct("sched_credit", [
     ("weight", integer),
     ("cap", integer),
     ], destructor_fn=None)
+
+libxl_event_type = Enumeration("event_type", [
+    (1, "DOMAIN_SHUTDOWN"),
+    (2, "DOMAIN_DESTROY"),
+    (3, "DISK_EJECT"),
+    ])
+
+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)
+                                 ])),
+           ]))])
+
-- 
1.5.6.5

[PATCH 3/3] RFC: libxl: internal API for event handling

[Ian Jackson]

This is a proposed internal API for other parts of libxl to use when
dealing with events (both osevents and API events ie libxl_event).

There will probably also be some helper functions for
libxl_awaiting_*.

THIS PATCH IS AN RFC AND SHOULD NOT BE APPLIED.  It contains only the
suggested changes to libxl_internal.h, and not any of the necessary
implementation nor consequential changes.
---
 tools/libxl/libxl_internal.h |   52 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 579188e..8eaf85a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -165,6 +165,58 @@ _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
                                    char *path, unsigned int *nb);
    /* On error: returns NULL, sets errno (no logging) */
 
+
+/* Event handling functions provided by the libxl event core to the
+ * rest of libxl.  Implemented in terms of _beforepoll/_afterpoll
+ * and/or the fd registration machinery, as provided by the
+ * application.
+ *
+ * Semantics are similar to those of the fd and timeout registration
+ * functions provided to libxl_osevent_register_hooks.
+ *
+ * Non-0 returns from libxl__ev_{modify,deregister} have already been
+ * logged by the core and should be returned unmodified to libxl's
+ * caller; NB that they may be valid libxl error codes but they may
+ * also be positive numbers supplied by the caller.
+ */
+
+typedef struct libxl__ev_fd libxl__ev_fd;
+typedef void libxl__ev_fd_callback(libxl__gc *gc, libxl__ev_fd *ev,
+                                   int fd, short events, short revents,
+                                   void *for_callback);
+
+int libxl__ev_fd_register(libxl__gc*, libxl__ev_fd *ev_out,
+                          int fd, short events,
+                          libxl__ev_fd_callback*, void *for_callback);
+int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
+                        short events);
+void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
+
+typedef struct libxl__ev_time libxl__ev_time;
+typedef void libxl__ev_time_callback(libxl__gc *gc, libxl__ev_fd *ev,
+                                     void *for_callback);
+
+int libxl__ev_time_register(libxl__gc*, libxl__ev_fd *ev_out,
+                            int milliseconds,
+                            libxl__ev_time_callback*, void *for_callback);
+int libxl__ev_time_modify(libxl__gc*, libxl__ev_fd *ev,
+                          int milliseconds);
+void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
+
+
+void libxl__event_occured(libxl__gc*, libxl_event *event);
+  /* Arranges to notify the application that the event has occurred.
+   * event should be suitable for passing to libxl_event_free. */
+
+void libxl__event_disaster(libxl__gc*, const char *msg, int errnoval,
+                           libxl_event_type type /* may be 0 */);
+  /* See the "disaster" argument to libxl_event_register_callbacks.
+   * NB that this function may return and the caller isn't supposed to
+   * then crash, although it may fail (and henceforth leave things in
+   * a state where many or all calls fail).
+   */
+
+
 /* from xl_dom */
 _hidden int libxl__domain_is_hvm(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
-- 
1.5.6.5

Re: [PATCH 3/3] RFC: libxl: internal API for event handling

[Ian Campbell]

On Tue, 2011-07-12 at 19:37 +0100, Ian Jackson wrote:
> This is a proposed internal API for other parts of libxl to use when
> dealing with events (both osevents and API events ie libxl_event).
> 
> There will probably also be some helper functions for
> libxl_awaiting_*.
> 
> THIS PATCH IS AN RFC AND SHOULD NOT BE APPLIED.  It contains only the
> suggested changes to libxl_internal.h, and not any of the necessary
> implementation nor consequential changes.

Seems sane in the context of patch 2/3.

Ian.

> ---
>  tools/libxl/libxl_internal.h |   52 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 579188e..8eaf85a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -165,6 +165,58 @@ _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
>                                     char *path, unsigned int *nb);
>     /* On error: returns NULL, sets errno (no logging) */
>  
> +
> +/* Event handling functions provided by the libxl event core to the
> + * rest of libxl.  Implemented in terms of _beforepoll/_afterpoll
> + * and/or the fd registration machinery, as provided by the
> + * application.
> + *
> + * Semantics are similar to those of the fd and timeout registration
> + * functions provided to libxl_osevent_register_hooks.
> + *
> + * Non-0 returns from libxl__ev_{modify,deregister} have already been
> + * logged by the core and should be returned unmodified to libxl's
> + * caller; NB that they may be valid libxl error codes but they may
> + * also be positive numbers supplied by the caller.
> + */
> +
> +typedef struct libxl__ev_fd libxl__ev_fd;
> +typedef void libxl__ev_fd_callback(libxl__gc *gc, libxl__ev_fd *ev,
> +                                   int fd, short events, short revents,
> +                                   void *for_callback);
> +
> +int libxl__ev_fd_register(libxl__gc*, libxl__ev_fd *ev_out,
> +                          int fd, short events,
> +                          libxl__ev_fd_callback*, void *for_callback);
> +int libxl__ev_fd_modify(libxl__gc*, libxl__ev_fd *ev,
> +                        short events);
> +void libxl__ev_fd_deregister(libxl__gc*, libxl__ev_fd *ev);
> +
> +typedef struct libxl__ev_time libxl__ev_time;
> +typedef void libxl__ev_time_callback(libxl__gc *gc, libxl__ev_fd *ev,
> +                                     void *for_callback);
> +
> +int libxl__ev_time_register(libxl__gc*, libxl__ev_fd *ev_out,
> +                            int milliseconds,
> +                            libxl__ev_time_callback*, void *for_callback);
> +int libxl__ev_time_modify(libxl__gc*, libxl__ev_fd *ev,
> +                          int milliseconds);
> +void libxl__ev_time_deregister(libxl__gc*, libxl__ev_time *ev);
> +
> +
> +void libxl__event_occured(libxl__gc*, libxl_event *event);
> +  /* Arranges to notify the application that the event has occurred.
> +   * event should be suitable for passing to libxl_event_free. */
> +
> +void libxl__event_disaster(libxl__gc*, const char *msg, int errnoval,
> +                           libxl_event_type type /* may be 0 */);
> +  /* See the "disaster" argument to libxl_event_register_callbacks.
> +   * NB that this function may return and the caller isn't supposed to
> +   * then crash, although it may fail (and henceforth leave things in
> +   * a state where many or all calls fail).
> +   */
> +
> +
>  /* from xl_dom */
>  _hidden int libxl__domain_is_hvm(libxl__gc *gc, uint32_t domid);
>  _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);

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

[Ian Campbell]

I think it would be good to have the model validated by Jim (CC'd) since
I imagine that the libvirt backend will be one of the primary users of
the registration mechanism.

There's a lot of docs here, which is excellent, but I've only had two
cups of tea so far today so I only managed a fairly high-level think
about it all.

On Tue, 2011-07-12 at 19:37 +0100, Ian Jackson wrote:
> -/* events handling */
> +
> +
> +/*
> + * OS event handling - passing low-level OS events to libxl
> + *
> + * Event-driven programs must use these facilities to allow libxl
> + * to become aware of readability/writeability of file descriptors
> + * and the occurrence of timeouts.
> + *
> + * There are two approaches available.  The first is appropriate for
> + * simple programs handling reasonably small numbers of domains:
> + *
> + *   for (;;) {
> + *      libxl_osevent_beforepoll(...)
> + *      poll();
> + *      libxl_osevent_afterpoll(...);
> + *      for (;;) {
> + *        r=libxl_event_check(...);
> + *        if (r==LIBXL_NOT_READY) break;
> + *        if (r) handle failure;
> + *        do something with the event;
> + *      }
> + *   }
> + *
> + * The second approach uses libxl_osevent_register_hooks and is
> + * suitable for programs which are already using a callback-based
> + * event library.

An example of this style of usage would be handy too.

> + *
> + * An application may freely mix the two styles of interaction.
> + */
> +
> +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.

Is there some way for a caller to determine how many entries in the
struct poll * the library will use?

Should we also have an interface suitable for users of select?

It is currently conventional in libxl.h to document a function before
it's prototype rather than after.

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

> +int libxl_osevent_register_hooks(libxl_ctx *ctx, void *user,
> +  int (*fd_register)(void *user, int fd, void **for_app_registration_out,
> +                     short events, void *for_libxl),
> [...]
> +  void (*time_deregister)(void *user, void *for_app_registration_io,
> +                          void *for_libxl));

I think passing a pointer to a struct libxl_osevent_hooks would be
better than passing loads of fn pointers.

> +  /* The application which calls register_fd_hooks promises to
> +   * maintain a register of fds and timeouts that libxl is interested
> +   * in,

It would probably be useful to provide some helper functions for
maintaining this register and for dispatching the foo_occurred things.

[...]
>  typedef struct {
>      /* event type */
> @@ -293,41 +410,136 @@ typedef struct {
>      char *token;
>  } libxl_event;
> 
> -typedef struct {
> -    char *path;
> -    char *token;
> -} libxl_waiter;
> +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.

> +  /* Searches for an event, already-happened, which matches typemask
> +   * and predicate.  predicate==0 matches any event.  Returns the
> +   * event, which must then later be freed by the caller using
> +   * libxl_event_free.
> +   *
> +   * Returns ERROR_NOT_READY if no such event has happened.
> +   */
> 
> +int libxl_event_wait(libxl_ctx *ctx, libxl_event *event_r,
> +                     unsigned long typemask,
> +                     int (*predicate)(const libxl_event*, void *user),
> +                     void *predicate_user);
> +  /* Like libxl_event_check but blocks if no suitable events are
> +   * available, until some are.  Uses
> +   * libxl_osevent_beforepoll/_afterpoll so may be inefficient if very
> +   * many domains are being handled by a single program.
> +   */
[...]
> +int libxl_event_free(libxl_ctx, *ctx, libxl_event *event);

It looks as if the libxl_event passed to libxl_event_wait is owned by
the caller so it could use a local struct or manage allocation itself,
is that right?

In which case the autogenerated libxl_event_destroy is sufficient
instead of free?


> +
> +
> +/* Alternatively or additionally, the application may also use this: */
> +
> +void libxl_event_register_callbacks(libxl_ctx *ctx, void *user, uint64_t mask,
> +   void (*event_occurs)(void *user, const libxl_event *event),
> +   void (*disaster)(void *user, libxl_event_type type,
> +                    const char *msg, int errnoval));
> +  /*
> +   * Arranges that libxl will henceforth call event_occurs for any
> +   * events whose type is set in mask, rather than queueing the event
> +   * for retrieval by libxl_event_check/wait.  Events whose bit is
> +   * clear in mask are not affected.

Would it be useful to have separate callbacks for the different event
types?

Or perhaps just a helper function which can be registered here and takes
a dispatch table from the app (I suppose the app could build this
itself, but maybe it would be useful functionality).

> +   *
> +   * event becomes owned by the application and must be freed, either
> +   * by event_occurs or later.
> +   *
> +   * event_occurs may be NULL if mask is 0.
> +   *
> +   * libxl_event_register_callback also provides a way for libxl to
> +   * report to the application that there was a problem reporting
> +   * events; this can occur due to lack of host memory during event
> +   * handling, or other wholly unrecoverable errors from system calls
> +   * made by libxl.  This will not happen for frivolous reasons - only
> +   * if the system, or the Xen components of it, are badly broken.
> +   *
> +   * msg and errnoval will describe the action that libxl was trying
> +   * to do, and type specifies the type of libxl events which may be
> +   * missing.  type may be 0 in which case events of all types may be
> +   * missing.
> +   *
> +   * disaster may be NULL.  If it is, or if
> +   * libxl_event_register_callbacks has not been called, errors of
> +   * this kind are fatal to the entire application: libxl will print a
> +   * message to stderr and call exit(-1).
> +   *
> +   * If disaster returns, it may be the case that some or all future
> +   * libxl calls will return errors; likewise it may be the case that
> +   * no more events (of the specified type, if applicable) can be
> +   * produced.  An application which supplies a disaster function
> +   * should normally react either by exiting, or by (when it has
> +   * returned to its main event loop) shutting down libxl with
> +   * libxl_ctx_free and perhaps trying to restart it with
> +   * libxl_ctx_init.
> +   *
> +   * Reentrancy: it IS permitted to call libxl from within
> +   * event_occurs.  It is NOT permitted to call libxl from within
> +   * disaster.
> +   *
> +   * libxl_event_register_callbacks may be called as many times, with
> +   * different parameters, as the application likes; the most recent
> +   * call determines the libxl behaviour.  There is only one mask.
> +   */
> 
> -/*
> - * Returns:
> - *  - 0 if the domain is dead but there is no cleanup to be done. e.g
> - *    because someone else has already done it.
> - *  - 1 if the domain is dead and there is cleanup to be done.
> - *
> - * Can return error if the domain exists and is still running.
> - *
> - * *info will contain valid domain state iff 1 is returned. In
> - * particular if 1 is returned then info->shutdown_reason is
> - * guaranteed to be valid since by definition the domain is
> - * (shutdown||dying))
> - */
> -int libxl_event_get_domain_death_info(libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_dominfo *info);
> 
> -/*
> - * Returns true and fills *disk if the caller should eject the disk
> +/* Events are only generated if they have been requested.
> + * The following functions request the generation of specific events.
>   */
> -int libxl_event_get_disk_eject_info(libxl_ctx *ctx, uint32_t domid, libxl_event *event, libxl_device_disk *disk);
> +
> +typedef struct libxl_awaiting_domain_death libxl_awaiting_domain_death;
> +int libxl_await_domain_death(libxl_ctx *ctx, uint32_t domid, uint64_t user,
> +                             libxl_awaiting_domain_death **waiter_out);
> +void libxl_noawait_domain_death(libxl_ctx *ctx, uint32_t domid,
> +                             libxl_awaiting_domain_death *waiter);
> +  /* Generates DOMAIN_SHUTDOWN and DOMAIN_DESTROY events.
> +   * A domain which is destroyed before it shuts down generates
> +   * only a DESTROY event. */
> +
> +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?

> diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
> index 8fb883f..8b285a4 100644
> --- a/tools/libxl/libxl.idl
> +++ b/tools/libxl/libxl.idl
> @@ -73,11 +73,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"),
> @@ -371,3 +366,26 @@ libxl_sched_credit = Struct("sched_credit", [
>      ("weight", integer),
>      ("cap", integer),
>      ], destructor_fn=None)
> +
> +libxl_event_type = Enumeration("event_type", [
> +    (1, "DOMAIN_SHUTDOWN"),
> +    (2, "DOMAIN_DESTROY"),
> +    (3, "DISK_EJECT"),
> +    ])
> +
> +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) ?

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re event handling"):
> I think it would be good to have the model validated by Jim (CC'd) since
> I imagine that the libvirt backend will be one of the primary users of
> the registration mechanism.

Indeed.  I was thinking about libvirt (and I think I remember how the
libvirt fd and event handling works from when I read it last time)
amongst other callers.

> There's a lot of docs here, which is excellent, but I've only had two
> cups of tea so far today so I only managed a fairly high-level think
> about it all.

:-).

Thanks for all your comments.  Detailed reponsese follow:

> > + * The second approach uses libxl_osevent_register_hooks and is
> > + * suitable for programs which are already using a callback-based
> > + * event library.
> 
> An example of this style of usage would be handy too.

It's not going to be very interesting.  You basically just implement
the fd_register etc. in terms of your own event system.

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

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

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

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

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

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.

> > +int libxl_osevent_register_hooks(libxl_ctx *ctx, void *user,
> > +  int (*fd_register)(void *user, int fd, void **for_app_registration_out,
> > +                     short events, void *for_libxl),
> > [...]
> > +  void (*time_deregister)(void *user, void *for_app_registration_io,
> > +                          void *for_libxl));
> 
> I think passing a pointer to a struct libxl_osevent_hooks would be
> better than passing loads of fn pointers.

You're right.

> > +  /* The application which calls register_fd_hooks promises to
> > +   * maintain a register of fds and timeouts that libxl is interested
> > +   * in,
> 
> It would probably be useful to provide some helper functions for
> maintaining this register and for dispatching the foo_occurred things.

Applications that want to use this interface already have one of
those.  Ie, any application with an existing callback-style event
loop.  They already have the appropriate data structures.

> [...]
> >  typedef struct {
> >      /* event type */
> > @@ -293,41 +410,136 @@ typedef struct {
> >      char *token;
> >  } libxl_event;

This struct libxl_event leaked through; it's to be abolished in favour
of the one from the idl.

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

> > +int libxl_event_wait(libxl_ctx *ctx, libxl_event *event_r,
> > +                     unsigned long typemask,
> > +                     int (*predicate)(const libxl_event*, void *user),
> > +                     void *predicate_user);
> > +  /* Like libxl_event_check but blocks if no suitable events are
> > +   * available, until some are.  Uses
> > +   * libxl_osevent_beforepoll/_afterpoll so may be inefficient if very
> > +   * many domains are being handled by a single program.
> > +   */
> [...]
> > +int libxl_event_free(libxl_ctx, *ctx, libxl_event *event);
> 
> It looks as if the libxl_event passed to libxl_event_wait is owned by
> the caller so it could use a local struct or manage allocation itself,
> is that right?

No, there's one too few *s.  It should be
  int libxl_event_wait(libxl_ctx *ctx, libxl_event **event_out,

> > +/* Alternatively or additionally, the application may also use this: */
> > +
> > +void libxl_event_register_callbacks(libxl_ctx *ctx, void *user, uint64_t mask,
> > +   void (*event_occurs)(void *user, const libxl_event *event),
> > +   void (*disaster)(void *user, libxl_event_type type,
> > +                    const char *msg, int errnoval));
> > +  /*
> > +   * Arranges that libxl will henceforth call event_occurs for any
> > +   * events whose type is set in mask, rather than queueing the event
> > +   * for retrieval by libxl_event_check/wait.  Events whose bit is
> > +   * clear in mask are not affected.
> 
> Would it be useful to have separate callbacks for the different event
> types?

I don't think so.

> Or perhaps just a helper function which can be registered here and takes
> a dispatch table from the app (I suppose the app could build this
> itself, but maybe it would be useful functionality).

The application might want to select by domain first, and then by
type.  I don't think it would be right to force it into a particular
structure.  It's not like a dispatch table is hard to do in the
application if you want to - it's just an array, which you can index
directly with the type number.

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

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

Ian.

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

[Ian Campbell]

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.

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

[Ian Jackson]

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.

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

[Ian Campbell]

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.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/3] RFC: libxl: API changes re event handling"):
> 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?

That's probably better.  "mask" gives the impression that they're
being generated anyway, just filtered, whereas in fact the library
might have to go to some effort to provide them.

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

Programs which communicate via IPC shouldn't rely on pointers provided
by their peers.  IMO.  OK perhaps there are exceptions (eg, the kernel
and xenstore) but I think it's a good general rule.   And a void*
can't be used to store integers, whereas the reverse is true.

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

That would be fine IMO.  Although it ought to be a type which is (a)
big enough for pointers (b) big enough for uint64.  So not intptr_t.
The bigger of uint64_t or uintptr_t would do.

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

Are there ?  That just goes to show what you assume if you don't read
carefully.  OK then.

Ian.

Re: [PATCH 1/3] RFC: libxl: API changes re domain type (and keyed union semantics)

[Ian Campbell]

On Tue, 2011-07-12 at 19:37 +0100, Ian Jackson wrote:
> Give the HVM domain type the name "HVM" as is generally used in the
> Xen universe, rather than "FV" which is not used elsewhere.
> 
> Keyed unions should be keyed off the individual union value, not off
> an arbitrary expression.
> 
> Change the "hvm" field in domain_build_info to be called "type" and
> have an appropriate enum type.

I think this is a good idea.

There is a similar "bool hvm" in libxl_domain_create_info, which I think
should also be changed (it's copied from create_info to build_info in
libxl_init_build_info).

The one in libxl_device_model_info is already correctly typed.

Not directly related to this patch but I've been wondering if the split
between create_info, build_info etc makes sense now that the builder
stuff has been pushed down into libxl itself.

> 
> These are are incompatible changes to the API/ABI.
> 
> THIS PATCH IS AN RFC AND SHOULD NOT BE APPLIED

In case you suffer from a split brain and apply it ;-)

Ian.

> ---
>  tools/libxl/libxl.idl |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
> index 6e39734..8fb883f 100644
> --- a/tools/libxl/libxl.idl
> +++ b/tools/libxl/libxl.idl
> @@ -21,7 +21,7 @@ libxl_hwcap = Builtin("hwcap")
>  #
>  
>  libxl_domain_type = Enumeration("domain_type", [
> -    (1, "FV"),
> +    (1, "HVM"),
>      (2, "PV"),
>      ])
>  
> @@ -158,9 +158,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("shadow_memkb",    uint32),
>      ("disable_migrate", bool),
>      ("cpuid",           libxl_cpuid_policy_list),
> -    ("hvm",             bool),
> -    ("u", KeyedUnion(None, "hvm",
> -                [("hvm", "%s", Struct(None,
> +    ("type",            libxl_domain_type),
> +    ("u", KeyedUnion(None, "type",
> +                [("hvm", Struct(None,
>                                         [("firmware", string),
>                                          ("pae", bool),
>                                          ("apic", bool),
> @@ -173,7 +173,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                          ("timer_mode", integer),
>                                          ("nested_hvm", bool),
>                                          ])),
> -                 ("pv", "!%s", Struct(None,
> +                 ("pv", Struct(None,
>                                         [("kernel", libxl_file_reference),
>                                          ("slack_memkb", uint32),
>                                          ("bootloader", string),

Re: [PATCH 1/3] RFC: libxl: API changes re domain type (and keyed union semantics)

[Ian Campbell]

On Tue, 2011-07-12 at 19:37 +0100, Ian Jackson wrote:
> 
> @@ -158,9 +158,9 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>      ("shadow_memkb",    uint32),
>      ("disable_migrate", bool),
>      ("cpuid",           libxl_cpuid_policy_list),
> -    ("hvm",             bool),
> -    ("u", KeyedUnion(None, "hvm",
> -                [("hvm", "%s", Struct(None,
> +    ("type",            libxl_domain_type),
> +    ("u", KeyedUnion(None, "type",
> +                [("hvm", Struct(None,
>                                         [("firmware", string),
>                                          ("pae", bool),
>                                          ("apic", bool),
> @@ -173,7 +173,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>                                          ("timer_mode", integer),
>                                          ("nested_hvm", bool),
>                                          ])),
> -                 ("pv", "!%s", Struct(None,
> +                 ("pv", Struct(None,
>                                         [("kernel",
> libxl_file_reference),
>                                          ("slack_memkb", uint32),
>                                          ("bootloader", string), 

I've just found a partially complete patch in my series which does
pretty much this. Is it worth my resurrecting it or are you also
partially there?

Ian.

Re: [PATCH 1/3] RFC: libxl: API changes re domain type (and keyed union semantics)

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1/3] RFC: libxl: API changes re domain type (and keyed union semantics)"):
> On Tue, 2011-07-12 at 19:37 +0100, Ian Jackson wrote:
...
> > -                 ("pv", "!%s", Struct(None,
> > +                 ("pv", Struct(None,
...
> I've just found a partially complete patch in my series which does
> pretty much this. Is it worth my resurrecting it or are you also
> partially there?

Oh, good, please resurrect it.

Ian.

[PATCH 0 of 6] libxl: IDL improvements

[Ian Campbell]

Shave various yacks for Ian J ;-)

* LIBXL_DOMAIN_TYPE_FV -> LIBXL_DOMAIN_TYPE_HVM
* Make more use of libxl_domain_type enum:
  * replace libxl__domain_is_hvm with libxl__domain_type
  * use in IDL in preference to 'bool hvm'
* KeyedUnion keyvar must now be a Enumeration.

[PATCH 1 of 6] libxl: Give the HVM domain type the name "HVM"

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310978325 -3600
# Node ID 3237f9de6c5053543fa13a7303a8c48733c58afd
# Parent  466379d7fef09b05f8f4a36f9f190e893668ea28
libxl: Give the HVM domain type the name "HVM"

This is generally used in the Xen universe, rather than "FV" which is
not used elsewhere.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 466379d7fef0 -r 3237f9de6c50 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 09:38:30 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 09:38:45 2011 +0100
@@ -22,7 +22,7 @@ libxl_hwcap = Builtin("hwcap")
 #
 
 libxl_domain_type = Enumeration("domain_type", [
-    (1, "FV"),
+    (1, "HVM"),
     (2, "PV"),
     ])
 
diff -r 466379d7fef0 -r 3237f9de6c50 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Mon Jul 18 09:38:30 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Mon Jul 18 09:38:45 2011 +0100
@@ -144,7 +144,7 @@ static char ** libxl__build_device_model
     if (info->serial) {
         flexarray_vappend(dm_args, "-serial", info->serial, NULL);
     }
-    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         int ioemu_vifs = 0;
 
         if (info->videoram) {
@@ -211,7 +211,7 @@ static char ** libxl__build_device_model
     case LIBXL_DOMAIN_TYPE_PV:
         flexarray_append(dm_args, "xenpv");
         break;
-    case LIBXL_DOMAIN_TYPE_FV:
+    case LIBXL_DOMAIN_TYPE_HVM:
         flexarray_append(dm_args, "xenfv");
         break;
     }
@@ -336,7 +336,7 @@ static char ** libxl__build_device_model
     if (info->serial) {
         flexarray_vappend(dm_args, "-serial", info->serial, NULL);
     }
-    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         int ioemu_vifs = 0;
 
         if (info->stdvga) {
@@ -408,7 +408,7 @@ static char ** libxl__build_device_model
     case LIBXL_DOMAIN_TYPE_PV:
         flexarray_append(dm_args, "xenpv");
         break;
-    case LIBXL_DOMAIN_TYPE_FV:
+    case LIBXL_DOMAIN_TYPE_HVM:
         flexarray_append(dm_args, "xenfv");
         break;
     }
@@ -417,7 +417,7 @@ static char ** libxl__build_device_model
     flexarray_append(dm_args, "-m");
     flexarray_append(dm_args, libxl__sprintf(gc, "%d", info->target_ram));
 
-    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         for (i = 0; i < num_disks; i++) {
             int disk, part;
             int dev_number =
diff -r 466379d7fef0 -r 3237f9de6c50 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 09:38:30 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 09:38:45 2011 +0100
@@ -1083,7 +1083,7 @@ skip_vfb:
     }
 
     dm_info->type = c_info->hvm ?
-        LIBXL_DOMAIN_TYPE_FV :
+        LIBXL_DOMAIN_TYPE_HVM :
         LIBXL_DOMAIN_TYPE_PV;
 
     xlu_cfg_destroy(config);

[PATCH 2 of 6] libxl: replace libxl__domain_is_hvm with libxl__domain_type

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310978325 -3600
# Node ID 8c7190363635ebae729f5cf7f4ca254a0972326e
# Parent  3237f9de6c5053543fa13a7303a8c48733c58afd
libxl: replace libxl__domain_is_hvm with libxl__domain_type

New function returns a libxl_domain_type enum.

Add LIBXL__DOMAIN_IS_TYPE helper macro.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 3237f9de6c50 -r 8c7190363635 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 18 09:38:45 2011 +0100
@@ -240,7 +240,7 @@ int libxl_domain_resume(libxl_ctx *ctx, 
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     int rc = 0;
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM)) {
         LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Called domain_resume on "
                 "non-cooperative hvm domain %u", domid);
         rc = ERROR_NI;
@@ -474,7 +474,7 @@ int libxl_domain_suspend(libxl_ctx *ctx,
                          uint32_t domid, int fd)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    int hvm = libxl__domain_is_hvm(&gc, domid);
+    int hvm = LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM);
     int live = info != NULL && info->flags & XL_SUSPEND_LIVE;
     int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG;
     int rc = 0;
@@ -517,7 +517,7 @@ int libxl_domain_unpause(libxl_ctx *ctx,
     char *state;
     int ret, rc = 0;
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM)) {
         path = libxl__sprintf(&gc, "/local/domain/0/device-model/%d/state", domid);
         state = libxl__xs_read(&gc, XBT_NULL, path);
         if (state != NULL && !strcmp(state, "paused")) {
@@ -560,7 +560,7 @@ int libxl_domain_shutdown(libxl_ctx *ctx
         return ERROR_FAIL;
     }
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM)) {
         unsigned long pvdriver = 0;
         int ret;
         ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ, &pvdriver);
@@ -722,6 +722,7 @@ int libxl_domain_destroy(libxl_ctx *ctx,
     libxl_dominfo dominfo;
     char *dom_path;
     char *vm_path;
+    char *pid;
     int rc, dm_present;
 
     rc = libxl_domain_info(ctx, &dominfo, domid);
@@ -734,12 +735,16 @@ int libxl_domain_destroy(libxl_ctx *ctx,
         return rc;
     }
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    switch (libxl__domain_type(&gc, domid)) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         dm_present = 1;
-    } else {
-        char *pid;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         pid = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "/local/domain/%d/image/device-model-pid", domid));
         dm_present = (pid != NULL);
+        break;
+    default:
+        abort();
     }
 
     dom_path = libxl__xs_get_dompath(&gc, domid);
@@ -818,10 +823,16 @@ int libxl_primary_console_exec(libxl_ctx
         rc = libxl_console_exec(ctx, stubdomid,
                                 STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV);
     else {
-        if (libxl__domain_is_hvm(&gc, domid_vm))
+        switch (libxl__domain_type(&gc, domid_vm)) {
+        case LIBXL_DOMAIN_TYPE_HVM:
             rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_SERIAL);
-        else
+            break;
+        case LIBXL_DOMAIN_TYPE_PV:
             rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
+            break;
+        default:
+            abort();
+        }
     }
     libxl__free_all(&gc);
     return rc;
diff -r 3237f9de6c50 -r 8c7190363635 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 18 09:38:45 2011 +0100
@@ -35,7 +35,7 @@
 #include "libxl.h"
 #include "libxl_internal.h"
 
-int libxl__domain_is_hvm(libxl__gc *gc, uint32_t domid)
+libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xc_domaininfo_t info;
@@ -46,7 +46,10 @@ int libxl__domain_is_hvm(libxl__gc *gc, 
         return -1;
     if (info.domain != domid)
         return -1;
-    return !!(info.flags & XEN_DOMINF_hvm_guest);
+    if (info.flags & XEN_DOMINF_hvm_guest)
+        return LIBXL_DOMAIN_TYPE_HVM;
+    else
+        return LIBXL_DOMAIN_TYPE_PV;
 }
 
 int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid)
diff -r 3237f9de6c50 -r 8c7190363635 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Mon Jul 18 09:38:45 2011 +0100
@@ -167,9 +167,10 @@ _hidden char **libxl__xs_directory(libxl
    /* On error: returns NULL, sets errno (no logging) */
 
 /* from xl_dom */
-_hidden int libxl__domain_is_hvm(libxl__gc *gc, uint32_t domid);
+_hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
-
+#define LIBXL__DOMAIN_IS_TYPE(gc, domid, type) \
+    libxl__domain_type((gc), (domid)) == LIBXL_DOMAIN_TYPE_##type
 typedef struct {
     uint32_t store_port;
     unsigned long store_mfn;
diff -r 3237f9de6c50 -r 8c7190363635 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_pci.c	Mon Jul 18 09:38:45 2011 +0100
@@ -286,7 +286,7 @@ static int libxl__device_pci_add_xenstor
     if (!num_devs)
         return libxl__create_pci_backend(gc, domid, pcidev, 1);
 
-    if (!starting && !libxl__domain_is_hvm(gc, domid)) {
+    if (!starting && LIBXL__DOMAIN_IS_TYPE(gc, domid, PV)) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0)
             return ERROR_FAIL;
     }
@@ -329,7 +329,7 @@ static int libxl__device_pci_remove_xens
         return ERROR_INVAL;
     num = atoi(num_devs);
 
-    if (!libxl__domain_is_hvm(gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(gc, domid, PV)) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0) {
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "pci backend at %s is not ready", be_path);
             return ERROR_FAIL;
@@ -357,7 +357,7 @@ retry_transaction:
         if (errno == EAGAIN)
             goto retry_transaction;
 
-    if (!libxl__domain_is_hvm(gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(gc, domid, PV)) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0) {
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "pci backend at %s is not ready", be_path);
             return ERROR_FAIL;
@@ -604,10 +604,11 @@ static int do_pci_add(libxl__gc *gc, uin
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *path;
     char *state, *vdevfn;
-    int rc, hvm;
+    int rc, hvm = 0;
 
-    hvm = libxl__domain_is_hvm(gc, domid);
-    if (hvm) {
+    switch (libxl__domain_type(gc, domid)) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
         if (libxl__wait_for_device_model(gc, domid, "running",
                                          NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
@@ -635,7 +636,9 @@ static int do_pci_add(libxl__gc *gc, uin
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
         if ( rc )
             return ERROR_FAIL;
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                          pcidev->bus, pcidev->dev, pcidev->func);
         FILE *f = fopen(sysfs_path, "r");
@@ -694,6 +697,9 @@ static int do_pci_add(libxl__gc *gc, uin
         }
         fclose(f);
     }
+    default:
+        abort();
+    }
 out:
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
         rc = xc_assign_device(ctx->xch, domid, pcidev_encode_bdf(pcidev));
@@ -831,7 +837,7 @@ static int do_pci_remove(libxl__gc *gc, 
     libxl_device_pci *assigned;
     char *path;
     char *state;
-    int hvm, rc, num;
+    int hvm = 0, rc, num;
     int stubdomid = 0;
 
     if ( !libxl_device_pci_list_assigned(ctx, &assigned, domid, &num) ) {
@@ -842,8 +848,9 @@ static int do_pci_remove(libxl__gc *gc, 
         }
     }
 
-    hvm = libxl__domain_is_hvm(gc, domid);
-    if (hvm) {
+    switch (libxl__domain_type(gc, domid)) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
         if (libxl__wait_for_device_model(gc, domid, "running",
                                          NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
@@ -871,7 +878,9 @@ static int do_pci_remove(libxl__gc *gc, 
         }
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                          pcidev->bus, pcidev->dev, pcidev->func);
         FILE *f = fopen(sysfs_path, "r");
@@ -921,6 +930,9 @@ skip1:
         }
         fclose(f);
     }
+    default:
+        abort();
+    }
 out:
     /* don't do multiple resets while some functions are still passed through */
     if ( (pcidev->vdevfn & 0x7) == 0 ) {

[PATCH 3 of 6] libxl: specify HVM vs PV in build_info using libxl_domain_type enum

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310978325 -3600
# Node ID bd0ccd8f7e1370e78025084ea9ca3e4f04d7c6e0
# Parent  8c7190363635ebae729f5cf7f4ca254a0972326e
libxl: specify HVM vs PV in build_info using libxl_domain_type enum

Also caught one place (in libxl__domain_restore_common) which used
info->u.hvm.pae even if !hvm. (fortunately the value was unused in
xc_domain_restore).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 18 09:38:45 2011 +0100
@@ -2036,17 +2036,27 @@ int libxl_domain_need_memory(libxl_ctx *
         libxl_device_model_info *dm_info, uint32_t *need_memkb)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int rc = ERROR_INVAL;
     *need_memkb = b_info->target_memkb;
-    if (b_info->hvm) {
+    switch (b_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
         if (dm_info->device_model_stubdomain)
             *need_memkb += 32 * 1024;
-    } else
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
+        break;
+    default:
+        goto out;
+    }
     if (*need_memkb % (2 * 1024))
         *need_memkb += (2 * 1024) - (*need_memkb % (2 * 1024));
+    rc = 0;
+out:
     libxl__free_all(&gc);
-    return 0;
+    return rc;
+
 }
 
 int libxl_get_free_memory(libxl_ctx *ctx, uint32_t *memkb)
diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 09:38:45 2011 +0100
@@ -159,9 +159,9 @@ libxl_domain_build_info = Struct("domain
     ("shadow_memkb",    uint32),
     ("disable_migrate", bool),
     ("cpuid",           libxl_cpuid_policy_list),
-    ("hvm",             bool),
-    ("u", KeyedUnion(None, "hvm",
-                [("hvm", "%s", Struct(None,
+    ("type",            libxl_domain_type),
+    ("u", KeyedUnion(None, "type",
+                [("hvm", "%s == LIBXL_DOMAIN_TYPE_HVM", Struct(None,
                                        [("firmware", string),
                                         ("pae", bool),
                                         ("apic", bool),
@@ -174,7 +174,7 @@ libxl_domain_build_info = Struct("domain
                                         ("timer_mode", integer),
                                         ("nested_hvm", bool),
                                         ])),
-                 ("pv", "!%s", Struct(None,
+                 ("pv", "%s == LIBXL_DOMAIN_TYPE_PV", Struct(None,
                                        [("kernel", libxl_file_reference),
                                         ("slack_memkb", uint32),
                                         ("bootloader", string),
diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_bootloader.c	Mon Jul 18 09:38:45 2011 +0100
@@ -323,7 +323,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
 
     struct stat st_buf;
 
-    if (info->hvm || !info->u.pv.bootloader)
+    if (info->type != LIBXL_DOMAIN_TYPE_PV || !info->u.pv.bootloader)
         goto out;
 
     rc = ERROR_INVAL;
diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_create.c	Mon Jul 18 09:38:45 2011 +0100
@@ -84,7 +84,7 @@ void libxl_init_build_info(libxl_domain_
     b_info->shadow_memkb = 0;
     if (c_info->hvm) {
         b_info->video_memkb = 8 * 1024;
-        b_info->hvm = 1;
+        b_info->type = LIBXL_DOMAIN_TYPE_HVM;
         b_info->u.hvm.firmware = NULL;
         b_info->u.hvm.pae = 1;
         b_info->u.hvm.apic = 1;
@@ -96,6 +96,7 @@ void libxl_init_build_info(libxl_domain_
         b_info->u.hvm.timer_mode = 1;
         b_info->u.hvm.nested_hvm = 0;
     } else {
+        b_info->type = LIBXL_DOMAIN_TYPE_PV;
         b_info->u.pv.slack_memkb = 8 * 1024;
     }
 }
@@ -160,7 +161,8 @@ int libxl__domain_build(libxl__gc *gc,
 
     gettimeofday(&start_time, NULL);
 
-    if (info->hvm) {
+    switch (info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         ret = libxl__build_hvm(gc, domid, info, dm_info, state);
         if (ret)
             goto out;
@@ -172,7 +174,8 @@ int libxl__domain_build(libxl__gc *gc,
         vments[3] = "hvm";
         vments[4] = "start_time";
         vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         ret = libxl__build_pv(gc, domid, info, state);
         if (ret)
             goto out;
@@ -193,6 +196,10 @@ int libxl__domain_build(libxl__gc *gc,
             vments[i++] = "image/cmdline";
             vments[i++] = (char*) info->u.pv.cmdline;
         }
+        break;
+    default:
+        ret = ERROR_INVAL;
+        goto out;
     }
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
@@ -219,7 +226,8 @@ static int domain_restore(libxl__gc *gc,
 
     gettimeofday(&start_time, NULL);
 
-    if (info->hvm) {
+    switch (info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         vments = libxl__calloc(gc, 7, sizeof(char *));
         vments[0] = "rtc/timeoffset";
         vments[1] = (info->u.hvm.timeoffset) ? info->u.hvm.timeoffset : "";
@@ -227,7 +235,8 @@ static int domain_restore(libxl__gc *gc,
         vments[3] = "hvm";
         vments[4] = "start_time";
         vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         vments = libxl__calloc(gc, 11, sizeof(char *));
         i = 0;
         vments[i++] = "image/ostype";
@@ -244,20 +253,24 @@ static int domain_restore(libxl__gc *gc,
             vments[i++] = "image/cmdline";
             vments[i++] = (char*) info->u.pv.cmdline;
         }
+        break;
+    default:
+        ret = ERROR_INVAL;
+        goto out;
     }
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
     if (ret)
         goto out;
 
     dm_info->saved_state = NULL;
-    if (info->hvm) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         ret = asprintf(&dm_info->saved_state,
                        XC_DEVICE_MODEL_RESTORE_FILE".%d", domid);
         ret = (ret < 0) ? ERROR_FAIL : 0;
     }
 
 out:
-    if (!info->hvm) {
+    if (info->type == LIBXL_DOMAIN_TYPE_PV) {
         libxl__file_reference_unmap(&info->u.pv.kernel);
         libxl__file_reference_unmap(&info->u.pv.ramdisk);
     }
diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Mon Jul 18 09:38:45 2011 +0100
@@ -642,12 +642,13 @@ static int libxl__create_stubdom(libxl__
     b_info.max_vcpus = 1;
     b_info.max_memkb = 32 * 1024;
     b_info.target_memkb = b_info.max_memkb;
+
+    b_info.type = LIBXL_DOMAIN_TYPE_PV;
     b_info.u.pv.kernel.path = libxl__abs_path(gc, "ioemu-stubdom.gz",
                                               libxl_xenfirmwaredir_path());
     b_info.u.pv.cmdline = libxl__sprintf(gc, " -d %d", info->domid);
     b_info.u.pv.ramdisk.path = "";
     b_info.u.pv.features = "";
-    b_info.hvm = 0;
 
     /* fixme: this function can leak the stubdom if it fails */
 
diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 18 09:38:45 2011 +0100
@@ -75,14 +75,14 @@ int libxl__build_pre(libxl__gc *gc, uint
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
     xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + LIBXL_MAXMEM_CONSTANT);
-    if (!info->hvm)
+    if (info->type == LIBXL_DOMAIN_TYPE_PV)
         xc_domain_set_memmap_limit(ctx->xch, domid,
                 (info->max_memkb + info->u.pv.slack_memkb));
     xc_domain_set_tsc_info(ctx->xch, domid, info->tsc_mode, 0, 0, 0);
     if ( info->disable_migrate )
         xc_domain_disable_migrate(ctx->xch, domid);
 
-    if (info->hvm) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         unsigned long shadow;
         shadow = (info->shadow_memkb + 1023) / 1024;
         xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, NULL, 0, &shadow, 0, NULL);
@@ -340,10 +340,25 @@ int libxl__domain_restore_common(libxl__
     libxl_ctx *ctx = libxl__gc_owner(gc);
     /* read signature */
     int rc;
+    int hvm, pae, superpages;
+    switch (info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
+        superpages = 1;
+        pae = info->u.hvm.pae;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+        hvm = 0;
+        superpages = 0;
+        pae = 1;
+        break;
+    default:
+        return ERROR_INVAL;
+    }
     rc = xc_domain_restore(ctx->xch, fd, domid,
-                             state->store_port, &state->store_mfn,
-                             state->console_port, &state->console_mfn,
-                             info->hvm, info->u.hvm.pae, !!info->hvm);
+                           state->store_port, &state->store_mfn,
+                           state->console_port, &state->console_mfn,
+                           hvm, pae, superpages);
     if ( rc ) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "restoring domain");
         return ERROR_FAIL;
diff -r 8c7190363635 -r bd0ccd8f7e13 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 09:38:45 2011 +0100
@@ -381,7 +381,7 @@ static void printf_info(int domid,
         printf("\t\t\t(spiceagent_mouse %d)\n", dm_info->spiceagent_mouse);
         printf("\t\t)\n");
     } else {
-        printf("\t\t(linux %d)\n", b_info->hvm);
+        printf("\t\t(linux %d)\n", 0);
         printf("\t\t\t(kernel %s)\n", b_info->u.pv.kernel.path);
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk.path);

[PATCH 4 of 6] libxl: specify HVM vs PV in create_info using libxl_domain_type enum

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310979573 -3600
# Node ID 66815389db5e368e1f5980f833e01965d45cf7bb
# Parent  bd0ccd8f7e1370e78025084ea9ca3e4f04d7c6e0
libxl: specify HVM vs PV in create_info using libxl_domain_type enum

Since libxl_init_build_info now needs an error return and a ctx (to
log to) switch all libxl_init_*_info to have an int return and a ctx.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r bd0ccd8f7e13 -r 66815389db5e tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl.h	Mon Jul 18 09:59:33 2011 +0100
@@ -250,9 +250,14 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 i
 int libxl_ctx_postfork(libxl_ctx *ctx);
 
 /* domain related functions */
-void libxl_init_create_info(libxl_domain_create_info *c_info);
-void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info);
-void libxl_init_dm_info(libxl_device_model_info *dm_info, libxl_domain_create_info *c_info, libxl_domain_build_info *b_info);
+int libxl_init_create_info(libxl_ctx *ctx, libxl_domain_create_info *c_info);
+int libxl_init_build_info(libxl_ctx *ctx,
+                          libxl_domain_build_info *b_info,
+                          libxl_domain_create_info *c_info);
+int libxl_init_dm_info(libxl_ctx *ctx,
+                       libxl_device_model_info *dm_info,
+                       libxl_domain_create_info *c_info,
+                       libxl_domain_build_info *b_info);
 typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
diff -r bd0ccd8f7e13 -r 66815389db5e tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 09:59:33 2011 +0100
@@ -137,7 +137,7 @@ libxl_version_info = Struct("version_inf
     ])
                                              
 libxl_domain_create_info = Struct("domain_create_info",[
-    ("hvm",          bool),
+    ("type",         libxl_domain_type),
     ("hap",          bool),
     ("oos",          bool),
     ("ssidref",      uint32),
diff -r bd0ccd8f7e13 -r 66815389db5e tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_create.c	Mon Jul 18 09:59:33 2011 +0100
@@ -60,19 +60,22 @@ void libxl_domain_config_destroy(libxl_d
     libxl_device_model_info_destroy(&d_config->dm_info);
 }
 
-void libxl_init_create_info(libxl_domain_create_info *c_info)
+int libxl_init_create_info(libxl_ctx *ctx, libxl_domain_create_info *c_info)
 {
     memset(c_info, '\0', sizeof(*c_info));
     c_info->xsdata = NULL;
     c_info->platformdata = NULL;
     c_info->hap = 1;
-    c_info->hvm = 1;
+    c_info->type = LIBXL_DOMAIN_TYPE_HVM;
     c_info->oos = 1;
     c_info->ssidref = 0;
     c_info->poolid = 0;
+    return 0;
 }
 
-void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info)
+int libxl_init_build_info(libxl_ctx *ctx,
+                          libxl_domain_build_info *b_info,
+                          libxl_domain_create_info *c_info)
 {
     memset(b_info, '\0', sizeof(*b_info));
     b_info->max_vcpus = 1;
@@ -82,9 +85,10 @@ void libxl_init_build_info(libxl_domain_
     b_info->disable_migrate = 0;
     b_info->cpuid = NULL;
     b_info->shadow_memkb = 0;
-    if (c_info->hvm) {
+    b_info->type = c_info->type;
+    switch (b_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         b_info->video_memkb = 8 * 1024;
-        b_info->type = LIBXL_DOMAIN_TYPE_HVM;
         b_info->u.hvm.firmware = NULL;
         b_info->u.hvm.pae = 1;
         b_info->u.hvm.apic = 1;
@@ -95,14 +99,23 @@ void libxl_init_build_info(libxl_domain_
         b_info->u.hvm.vpt_align = 1;
         b_info->u.hvm.timer_mode = 1;
         b_info->u.hvm.nested_hvm = 0;
-    } else {
-        b_info->type = LIBXL_DOMAIN_TYPE_PV;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         b_info->u.pv.slack_memkb = 8 * 1024;
+        break;
+    default:
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "invalid domain type %s in create info",
+                   libxl_domain_type_to_string(b_info->type));
+        return ERROR_INVAL;
     }
+    return 0;
 }
 
-void libxl_init_dm_info(libxl_device_model_info *dm_info,
-        libxl_domain_create_info *c_info, libxl_domain_build_info *b_info)
+int libxl_init_dm_info(libxl_ctx *ctx,
+                       libxl_device_model_info *dm_info,
+                       libxl_domain_create_info *c_info,
+                       libxl_domain_build_info *b_info)
 {
     memset(dm_info, '\0', sizeof(*dm_info));
 
@@ -132,6 +145,7 @@ void libxl_init_dm_info(libxl_device_mod
     dm_info->usb = 0;
     dm_info->usbdevice = NULL;
     dm_info->xen_platform_pci = 1;
+    return 0;
 }
 
 static int init_console_info(libxl_device_console *console, int dev_num)
@@ -316,9 +330,12 @@ int libxl__domain_make(libxl__gc *gc, li
         goto out;
     }
 
-    flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
-    flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
-    flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
+    flags = 0;
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        flags |= XEN_DOMCTL_CDF_hvm_guest;
+        flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
+        flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
+    }
     *domid = -1;
 
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
@@ -432,7 +449,7 @@ static int do_domain_create(libxl__gc *g
         goto error_out;
     }
 
-    if ( !d_config->c_info.hvm && cb ) {
+    if ( d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && cb ) {
         if ( (*cb)(ctx, domid, priv) )
             goto error_out;
     }
@@ -486,7 +503,9 @@ static int do_domain_create(libxl__gc *g
             goto error_out;
         }
     }
-    if (d_config->c_info.hvm) {
+    switch (d_config->c_info.type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+    {
         libxl_device_console console;
 
         ret = init_console_info(&console, 0);
@@ -505,7 +524,10 @@ static int do_domain_create(libxl__gc *g
                        "failed to create device model: %d", ret);
             goto error_out;
         }
-    } else {
+        break;
+    }
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         int need_qemu = 0;
         libxl_device_console console;
 
@@ -530,6 +552,11 @@ static int do_domain_create(libxl__gc *g
 
         if (need_qemu)
             libxl__create_xenpv_qemu(gc, domid, d_config->vfbs, &dm_starting);
+        break;
+    }
+    default:
+        ret = ERROR_INVAL;
+        goto error_out;
     }
 
     if (dm_starting) {
@@ -552,7 +579,8 @@ static int do_domain_create(libxl__gc *g
         goto error_out;
     }
 
-    if (!d_config->c_info.hvm && d_config->b_info.u.pv.e820_host) {
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
+        d_config->b_info.u.pv.e820_host) {
         int rc;
         rc = libxl__e820_alloc(ctx, domid, d_config);
         if (rc)
@@ -560,7 +588,9 @@ static int do_domain_create(libxl__gc *g
                       "Failed while collecting E820 with: %d (errno:%d)\n",
                       rc, errno);
     }
-    if ( cb && (d_config->c_info.hvm || d_config->b_info.u.pv.bootloader )) {
+    if ( cb && (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM ||
+                (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
+                 d_config->b_info.u.pv.bootloader ))) {
         if ( (*cb)(ctx, domid, priv) )
             goto error_out;
     }
diff -r bd0ccd8f7e13 -r 66815389db5e tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Mon Jul 18 09:59:33 2011 +0100
@@ -633,7 +633,7 @@ static int libxl__create_stubdom(libxl__
     }
 
     memset(&c_info, 0x00, sizeof(libxl_domain_create_info));
-    c_info.hvm = 0;
+    c_info.type = LIBXL_DOMAIN_TYPE_PV;
     c_info.name = libxl__sprintf(gc, "%s-dm", libxl__domid_to_name(gc, info->domid));
 
     libxl_uuid_copy(&c_info.uuid, &info->uuid);
diff -r bd0ccd8f7e13 -r 66815389db5e tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/libxl_pci.c	Mon Jul 18 09:59:33 2011 +0100
@@ -1275,7 +1275,7 @@ int libxl__e820_alloc(libxl_ctx *ctx, ui
     struct e820entry map[E820MAX];
     libxl_domain_build_info *b_info;
 
-    if (d_config == NULL || d_config->c_info.hvm)
+    if (d_config == NULL || d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
         return ERROR_INVAL;
 
     b_info = &d_config->b_info;
diff -r bd0ccd8f7e13 -r 66815389db5e tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 09:38:45 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 09:59:33 2011 +0100
@@ -300,7 +300,7 @@ static void printf_info(int domid,
 
     printf("(domain\n\t(domid %d)\n", domid);
     printf("\t(create_info)\n");
-    printf("\t(hvm %d)\n", c_info->hvm);
+    printf("\t(hvm %d)\n", c_info->type == LIBXL_DOMAIN_TYPE_HVM);
     printf("\t(hap %d)\n", c_info->hap);
     printf("\t(oos %d)\n", c_info->oos);
     printf("\t(ssidref %d)\n", c_info->ssidref);
@@ -333,14 +333,15 @@ static void printf_info(int domid,
     printf("\t(target_memkb %d)\n", b_info->target_memkb);
     printf("\t(nomigrate %d)\n", b_info->disable_migrate);
 
-    if (!c_info->hvm && b_info->u.pv.bootloader) {
+    if (c_info->type == LIBXL_DOMAIN_TYPE_PV && b_info->u.pv.bootloader) {
         printf("\t(bootloader %s)\n", b_info->u.pv.bootloader);
         if (b_info->u.pv.bootloader_args)
             printf("\t(bootloader_args %s)\n", b_info->u.pv.bootloader_args);
     }
 
     printf("\t(image\n");
-    if (c_info->hvm) {
+    switch (c_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         printf("\t\t(hvm\n");
         printf("\t\t\t(firmware %s)\n", b_info->u.hvm.firmware);
         printf("\t\t\t(video_memkb %d)\n", b_info->video_memkb);
@@ -380,13 +381,18 @@ static void printf_info(int domid,
                     dm_info->spicedisable_ticketing);
         printf("\t\t\t(spiceagent_mouse %d)\n", dm_info->spiceagent_mouse);
         printf("\t\t)\n");
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         printf("\t\t(linux %d)\n", 0);
         printf("\t\t\t(kernel %s)\n", b_info->u.pv.kernel.path);
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk.path);
         printf("\t\t\t(e820_host %d)\n", b_info->u.pv.e820_host);
         printf("\t\t)\n");
+        break;
+    default:
+        fprintf(stderr, "Unknown domain type %d\n", c_info->type);
+        exit(1);
     }
     printf("\t)\n");
 
@@ -453,7 +459,7 @@ static void printf_info(int domid,
         printf("\t\t)\n");
         printf("\t)\n");
     }
-       printf(")\n");
+    printf(")\n");
 }
 
 static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a)
@@ -538,7 +544,8 @@ static void parse_config_data(const char
         exit(1);
     }
 
-    libxl_init_create_info(c_info);
+    if (libxl_init_create_info(ctx, c_info))
+        exit(1);
 
     if (!xlu_cfg_get_string (config, "seclabel", &buf)) {
         e = libxl_flask_context_to_sid(ctx, (char *)buf, strlen(buf),
@@ -553,10 +560,10 @@ static void parse_config_data(const char
         }
     }
 
-    c_info->hvm = 0;
+    c_info->type = LIBXL_DOMAIN_TYPE_PV;
     if (!xlu_cfg_get_string (config, "builder", &buf) &&
         !strncmp(buf, "hvm", strlen(buf)))
-        c_info->hvm = 1;
+        c_info->type = LIBXL_DOMAIN_TYPE_HVM;
 
     if (!xlu_cfg_get_long (config, "hap", &l))
         c_info->hap = l;
@@ -586,7 +593,8 @@ static void parse_config_data(const char
         exit(1);
     }
 
-    libxl_init_build_info(b_info, c_info);
+    if (libxl_init_build_info(ctx, b_info, c_info))
+        exit(1);
 
     /* the following is the actual config parsing with overriding values in the structures */
     if (!xlu_cfg_get_long (config, "vcpus", &l)) {
@@ -654,7 +662,8 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "gfx_passthru", &l))
         dm_info->gfx_passthru = l;
 
-    if (c_info->hvm == 1) {
+    switch(c_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         if (!xlu_cfg_get_string (config, "kernel", &buf))
             fprintf(stderr, "WARNING: ignoring \"kernel\" directive for HVM guest. "
                     "Use \"firmware_override\" instead if you really want a non-default firmware\n");
@@ -679,7 +688,9 @@ static void parse_config_data(const char
             b_info->u.hvm.timer_mode = l;
         if (!xlu_cfg_get_long (config, "nestedhvm", &l))
             b_info->u.hvm.nested_hvm = l;
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         char *cmdline = NULL;
         const char *root = NULL, *extra = "";
 
@@ -712,6 +723,10 @@ static void parse_config_data(const char
 
         b_info->u.pv.cmdline = cmdline;
         xlu_cfg_replace_string (config, "ramdisk", &b_info->u.pv.ramdisk.path);
+        break;
+    }
+    default:
+        abort();
     }
 
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
@@ -888,11 +903,16 @@ skip_vfb:
     /* To be reworked (automatically enabled) once the auto ballooning
      * after guest starts is done (with PCI devices passed in). */
     if (!xlu_cfg_get_long (config, "e820_host", &l)) {
-        if (c_info->hvm)
-          fprintf(stderr, "Can't do e820_host in HVM mode!");
-        else {
-          if (l)
-            b_info->u.pv.e820_host = true;
+        switch (c_info->type) {
+        case LIBXL_DOMAIN_TYPE_HVM:
+            fprintf(stderr, "Can't do e820_host in HVM mode!");
+            break;
+        case LIBXL_DOMAIN_TYPE_PV:
+            if (l)
+                b_info->u.pv.e820_host = true;
+            break;
+        default:
+            abort();
         }
     }
     if (!xlu_cfg_get_list (config, "pci", &pcis, 0, 0)) {
@@ -911,7 +931,7 @@ skip_vfb:
             if (!libxl_device_pci_parse_bdf(ctx, pcidev, buf))
                 d_config->num_pcidevs++;
         }
-        if (d_config->num_pcidevs && !c_info->hvm)
+        if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
             b_info->u.pv.e820_host = true;
     }
 
@@ -994,12 +1014,13 @@ skip_vfb:
         break;
     }
 
-    if (c_info->hvm == 1) {
+    if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         XLU_ConfigList *dmargs;
         int nr_dmargs = 0;
 
         /* init dm from c and b */
-        libxl_init_dm_info(dm_info, c_info, b_info);
+        if (libxl_init_dm_info(ctx, dm_info, c_info, b_info))
+            exit(1);
 
         /* then process config related to dm */
         if (!xlu_cfg_get_string (config, "device_model", &buf)) {
@@ -1082,9 +1103,7 @@ skip_vfb:
         }
     }
 
-    dm_info->type = c_info->hvm ?
-        LIBXL_DOMAIN_TYPE_HVM :
-        LIBXL_DOMAIN_TYPE_PV;
+    dm_info->type = c_info->type;
 
     xlu_cfg_destroy(config);
 }

[PATCH 5 of 6] libxl: use libxl_domain_type enum with libxl__domain_suspend_common

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310979573 -3600
# Node ID 6bd6decb4f8760cc5f2fb47fef27de522fbc8c8a
# Parent  66815389db5e368e1f5980f833e01965d45cf7bb
libxl: use libxl_domain_type enum with libxl__domain_suspend_common

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 66815389db5e -r 6bd6decb4f87 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 18 09:59:33 2011 +0100
@@ -474,13 +474,13 @@ int libxl_domain_suspend(libxl_ctx *ctx,
                          uint32_t domid, int fd)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    int hvm = LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM);
+    libxl_domain_type type = libxl__domain_type(&gc, domid);
     int live = info != NULL && info->flags & XL_SUSPEND_LIVE;
     int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG;
     int rc = 0;
 
-    rc = libxl__domain_suspend_common(&gc, domid, fd, hvm, live, debug);
-    if (!rc && hvm)
+    rc = libxl__domain_suspend_common(&gc, domid, fd, type, live, debug);
+    if (!rc && type == LIBXL_DOMAIN_TYPE_HVM)
         rc = libxl__domain_save_device_model(&gc, domid, fd);
     libxl__free_all(&gc);
     return rc;
diff -r 66815389db5e -r 6bd6decb4f87 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 18 09:59:33 2011 +0100
@@ -513,14 +513,26 @@ static int libxl__domain_suspend_common_
 }
 
 int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
-		int hvm, int live, int debug)
+                                 libxl_domain_type type,
+                                 int live, int debug)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int flags;
     int port;
     struct save_callbacks callbacks;
     struct suspendinfo si;
-    int rc = ERROR_FAIL;
+    int hvm, rc = ERROR_FAIL;
+
+    switch (type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+        hvm = 0;
+        break;
+    default:
+        return ERROR_INVAL;
+    }
 
     flags = (live) ? XCFLAGS_LIVE : 0
           | (debug) ? XCFLAGS_DEBUG : 0
diff -r 66815389db5e -r 6bd6decb4f87 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Mon Jul 18 09:59:33 2011 +0100
@@ -200,7 +200,9 @@ _hidden int libxl__domain_restore_common
                                          libxl_domain_build_info *info,
                                          libxl__domain_build_state *state,
                                          int fd);
-_hidden int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd, int hvm, int live, int debug);
+_hidden int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
+                                         libxl_domain_type type,
+                                         int live, int debug);
 _hidden int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd);
 _hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);

[PATCH 6 of 6] libxl: Keyed unions key off an enum instead of an arbitrary expression

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310979574 -3600
# Node ID cf2f4b79c21f5a1b72b59f5da2f644b6d704b51e
# Parent  6bd6decb4f8760cc5f2fb47fef27de522fbc8c8a
libxl: Keyed unions key off an enum instead of an arbitrary expression

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 6bd6decb4f87 -r cf2f4b79c21f tools/libxl/gentest.py
--- a/tools/libxl/gentest.py	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/gentest.py	Mon Jul 18 09:59:34 2011 +0100
@@ -30,12 +30,13 @@ def gen_rand_init(ty, v, indent = "    "
     elif isinstance(ty, libxltypes.KeyedUnion):
         if parent is None:
             raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar_name)        
         for f in ty.fields:
             (nparent,fexpr) = ty.member(v, f, parent is None)
-            keyvar_expr = f.keyvar_expr % (parent + ty.keyvar_name)
-            s += "if (" + keyvar_expr + ") {\n"
+            s += "case %s:\n" % f.enumname
             s += gen_rand_init(f.type, fexpr, indent + "    ", nparent)
-            s += "}\n"
+            s += "    break;\n"
+        s += "}\n"
     elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn is None):
         for f in [f for f in ty.fields if not f.const]:
             (nparent,fexpr) = ty.member(v, f, parent is None)
diff -r 6bd6decb4f87 -r cf2f4b79c21f tools/libxl/gentypes.py
--- a/tools/libxl/gentypes.py	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/gentypes.py	Mon Jul 18 09:59:34 2011 +0100
@@ -79,12 +79,13 @@ def libxl_C_type_destroy(ty, v, indent =
     if isinstance(ty, libxltypes.KeyedUnion):
         if parent is None:
             raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar_name)        
         for f in ty.fields:
             (nparent,fexpr) = ty.member(v, f, parent is None)
-            keyvar_expr = f.keyvar_expr % (parent + ty.keyvar_name)
-            s += "if (" + keyvar_expr + ") {\n"
+            s += "case %s:\n" % f.enumname
             s += libxl_C_type_destroy(f.type, fexpr, indent + "    ", nparent)
-            s += "}\n"
+            s += "    break;\n"
+        s += "}\n"
     elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.destructor_fn is None):
         for f in [f for f in ty.fields if not f.const]:
             (nparent,fexpr) = ty.member(v, f, parent is None)
@@ -114,12 +115,13 @@ def libxl_C_type_gen_json(ty, v, indent 
     elif isinstance(ty, libxltypes.KeyedUnion):
         if parent is None:
             raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar_name)        
         for f in ty.fields:
             (nparent,fexpr) = ty.member(v, f, parent is None)
-            keyvar_expr = f.keyvar_expr % (parent + ty.keyvar_name)
-            s += "if (" + keyvar_expr + ") {\n"
+            s += "case %s:\n" % f.enumname
             s += libxl_C_type_gen_json(f.type, fexpr, indent + "    ", nparent)
-            s += "}\n"
+            s += "    break;\n"
+        s += "}\n"
     elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn is None):
         s += "s = yajl_gen_map_open(hand);\n"
         s += "if (s != yajl_gen_status_ok)\n"
diff -r 6bd6decb4f87 -r cf2f4b79c21f tools/libxl/idl.txt
--- a/tools/libxl/idl.txt	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/idl.txt	Mon Jul 18 09:59:34 2011 +0100
@@ -136,13 +136,9 @@ libxltype.KeyedUnion
  upon another member in the containing type.
 
  The KeyedUnion.keyvar_name must contain the name of the member of the
- containing type which determines the valid member of the union
-
- The fields in a KeyedUnion have an extra Field.keyvar_expr
- property. This must be a string containing a single "%s" format
- specifier such that when "%s" is substited by an instance of
- KeyedUnion.keyvar_name it becomes a C expression which evaluates to
- True IFF this field currently contains valid data.
+ containing type which determines the valid member of the union. The
+ member referenced by KeyedUnion.keyvar_name has type
+ KeyedUnion.keyvar_type which must be an instance of the Enumeration type.
 
 Standard Types
 --------------
diff -r 6bd6decb4f87 -r cf2f4b79c21f tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 09:59:34 2011 +0100
@@ -160,30 +160,28 @@ libxl_domain_build_info = Struct("domain
     ("disable_migrate", bool),
     ("cpuid",           libxl_cpuid_policy_list),
     ("type",            libxl_domain_type),
-    ("u", KeyedUnion(None, "type",
-                [("hvm", "%s == LIBXL_DOMAIN_TYPE_HVM", Struct(None,
-                                       [("firmware", string),
-                                        ("pae", bool),
-                                        ("apic", bool),
-                                        ("acpi", bool),
-                                        ("nx", bool),
-                                        ("viridian", bool),
-                                        ("timeoffset", string),
-                                        ("hpet", bool),
-                                        ("vpt_align", bool),
-                                        ("timer_mode", integer),
-                                        ("nested_hvm", bool),
-                                        ])),
-                 ("pv", "%s == LIBXL_DOMAIN_TYPE_PV", Struct(None,
-                                       [("kernel", libxl_file_reference),
-                                        ("slack_memkb", uint32),
-                                        ("bootloader", string),
-                                        ("bootloader_args", string),
-                                        ("cmdline", string),
-                                        ("ramdisk", libxl_file_reference),
-                                        ("features", string, True),
-                                        ("e820_host", bool, False, "Use host's E820 for PCI passthrough."),
-                                        ])),
+    ("u", KeyedUnion(None, libxl_domain_type, "type",
+                [("hvm", Struct(None, [("firmware", string),
+                                       ("pae", bool),
+                                       ("apic", bool),
+                                       ("acpi", bool),
+                                       ("nx", bool),
+                                       ("viridian", bool),
+                                       ("timeoffset", string),
+                                       ("hpet", bool),
+                                       ("vpt_align", bool),
+                                       ("timer_mode", integer),
+                                       ("nested_hvm", bool),
+                                       ])),
+                 ("pv", Struct(None, [("kernel", libxl_file_reference),
+                                      ("slack_memkb", uint32),
+                                      ("bootloader", string),
+                                      ("bootloader_args", string),
+                                      ("cmdline", string),
+                                      ("ramdisk", libxl_file_reference),
+                                      ("features", string, True),
+                                      ("e820_host", bool, False, "Use host's E820 for PCI passthrough."),
+                                      ])),
                  ])),
     ],
     comment =
diff -r 6bd6decb4f87 -r cf2f4b79c21f tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Mon Jul 18 09:59:33 2011 +0100
+++ b/tools/libxl/libxltypes.py	Mon Jul 18 09:59:34 2011 +0100
@@ -125,6 +125,11 @@ class Enumeration(Type):
             self.values.append(EnumerationValue(self, num, name,
                                                 comment=comment,
                                                 typename=self.rawname))
+    def lookup(self, name):
+        for v in self.values:
+            if v.valuename == str.upper(name):
+                return v
+        return ValueError
         
 class Field(object):
     """An element of an Aggregate type"""
@@ -133,7 +138,7 @@ class Field(object):
         self.name = name
         self.const = kwargs.setdefault('const', False)
         self.comment = kwargs.setdefault('comment', None)
-        self.keyvar_expr = kwargs.setdefault('keyvar_expr', None)
+        self.enumname = kwargs.setdefault('enumname', None)
 
 class Aggregate(Type):
     """A type containing a collection of other types"""
@@ -192,18 +197,21 @@ class Union(Aggregate):
 
 class KeyedUnion(Aggregate):
     """A union which is keyed of another variable in the parent structure"""
-    def __init__(self, name, keyvar_name, fields, **kwargs):
+    def __init__(self, name, keyvar_type, keyvar_name, fields, **kwargs):
         Aggregate.__init__(self, "union", name, [], **kwargs)
 
+        if not isinstance(keyvar_type, Enumeration):
+            raise ValueError
+        
         self.keyvar_name = keyvar_name
+        self.keyvar_type = keyvar_type
 
         for f in fields:
-            # (name, keyvar_expr, type)
-
-            # keyvar_expr must contain exactly one %s which will be replaced with the keyvar_name
-
-            n, kve, ty = f
-            self.fields.append(Field(ty, n, keyvar_expr=kve))
+            # (name, enum, type)
+            e, ty = f
+            ev = keyvar_type.lookup(e)
+            en = ev.name
+            self.fields.append(Field(ty, e, enumname=en))
 
 #
 # Standard Types

[PATCH 0 of 6 V2] libxl: IDL improvements

[Ian Campbell]

Shave various yacks for Ian J ;-)

* LIBXL_DOMAIN_TYPE_FV -> LIBXL_DOMAIN_TYPE_HVM
* Make more use of libxl_domain_type enum:
  * replace libxl__domain_is_hvm with libxl__domain_type
  * use in IDL in preference to 'bool hvm'
* KeyedUnion keyvar must now be a Enumeration.

V2:
  post a version which applies without needing my WIP JSON series.

[PATCH 1 of 6 V2] libxl: Give the HVM domain type the name "HVM"

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310997149 -3600
# Node ID 90e2ae994ebbc37ba6279b9071ab2e76c2ebeec2
# Parent  b18728227bf9e45058a4cc22425d1d35317e2c8d
libxl: Give the HVM domain type the name "HVM"

This is generally used in the Xen universe, rather than "FV" which is
not used elsewhere.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r b18728227bf9 -r 90e2ae994ebb tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 14:52:29 2011 +0100
@@ -21,7 +21,7 @@ libxl_hwcap = Builtin("hwcap")
 #
 
 libxl_domain_type = Enumeration("domain_type", [
-    (1, "FV"),
+    (1, "HVM"),
     (2, "PV"),
     ])
 
diff -r b18728227bf9 -r 90e2ae994ebb tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:29 2011 +0100
@@ -144,7 +144,7 @@ static char ** libxl__build_device_model
     if (info->serial) {
         flexarray_vappend(dm_args, "-serial", info->serial, NULL);
     }
-    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         int ioemu_vifs = 0;
 
         if (info->videoram) {
@@ -211,7 +211,7 @@ static char ** libxl__build_device_model
     case LIBXL_DOMAIN_TYPE_PV:
         flexarray_append(dm_args, "xenpv");
         break;
-    case LIBXL_DOMAIN_TYPE_FV:
+    case LIBXL_DOMAIN_TYPE_HVM:
         flexarray_append(dm_args, "xenfv");
         break;
     }
@@ -336,7 +336,7 @@ static char ** libxl__build_device_model
     if (info->serial) {
         flexarray_vappend(dm_args, "-serial", info->serial, NULL);
     }
-    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         int ioemu_vifs = 0;
 
         if (info->stdvga) {
@@ -408,7 +408,7 @@ static char ** libxl__build_device_model
     case LIBXL_DOMAIN_TYPE_PV:
         flexarray_append(dm_args, "xenpv");
         break;
-    case LIBXL_DOMAIN_TYPE_FV:
+    case LIBXL_DOMAIN_TYPE_HVM:
         flexarray_append(dm_args, "xenfv");
         break;
     }
@@ -417,7 +417,7 @@ static char ** libxl__build_device_model
     flexarray_append(dm_args, "-m");
     flexarray_append(dm_args, libxl__sprintf(gc, "%d", info->target_ram));
 
-    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         for (i = 0; i < num_disks; i++) {
             int disk, part;
             int dev_number =
diff -r b18728227bf9 -r 90e2ae994ebb tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:29 2011 +0100
@@ -1083,7 +1083,7 @@ skip_vfb:
     }
 
     dm_info->type = c_info->hvm ?
-        LIBXL_DOMAIN_TYPE_FV :
+        LIBXL_DOMAIN_TYPE_HVM :
         LIBXL_DOMAIN_TYPE_PV;
 
     xlu_cfg_destroy(config);

Re: [PATCH 1 of 6 V2] libxl: Give the HVM domain type the name "HVM"

[Wei LIU]

On Mon, 2011-07-18 at 14:57 +0100, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1310997149 -3600
> # Node ID 90e2ae994ebbc37ba6279b9071ab2e76c2ebeec2
> # Parent  b18728227bf9e45058a4cc22425d1d35317e2c8d
> libxl: Give the HVM domain type the name "HVM"
> 
> This is generally used in the Xen universe, rather than "FV" which is
> not used elsewhere.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> diff -r b18728227bf9 -r 90e2ae994ebb tools/libxl/libxl.idl
> --- a/tools/libxl/libxl.idl	Mon Jul 18 14:52:29 2011 +0100
> +++ b/tools/libxl/libxl.idl	Mon Jul 18 14:52:29 2011 +0100
> @@ -21,7 +21,7 @@ libxl_hwcap = Builtin("hwcap")
>  #
>  
>  libxl_domain_type = Enumeration("domain_type", [
> -    (1, "FV"),
> +    (1, "HVM"),
>      (2, "PV"),
>      ])
>  
> diff -r b18728227bf9 -r 90e2ae994ebb tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:29 2011 +0100
> +++ b/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:29 2011 +0100
> @@ -144,7 +144,7 @@ static char ** libxl__build_device_model
>      if (info->serial) {
>          flexarray_vappend(dm_args, "-serial", info->serial, NULL);
>      }
> -    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          int ioemu_vifs = 0;
>  
>          if (info->videoram) {
> @@ -211,7 +211,7 @@ static char ** libxl__build_device_model
>      case LIBXL_DOMAIN_TYPE_PV:
>          flexarray_append(dm_args, "xenpv");
>          break;
> -    case LIBXL_DOMAIN_TYPE_FV:
> +    case LIBXL_DOMAIN_TYPE_HVM:
>          flexarray_append(dm_args, "xenfv");
>          break;
>      }
> @@ -336,7 +336,7 @@ static char ** libxl__build_device_model
>      if (info->serial) {
>          flexarray_vappend(dm_args, "-serial", info->serial, NULL);
>      }
> -    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          int ioemu_vifs = 0;
>  
>          if (info->stdvga) {
> @@ -408,7 +408,7 @@ static char ** libxl__build_device_model
>      case LIBXL_DOMAIN_TYPE_PV:
>          flexarray_append(dm_args, "xenpv");
>          break;
> -    case LIBXL_DOMAIN_TYPE_FV:
> +    case LIBXL_DOMAIN_TYPE_HVM:
>          flexarray_append(dm_args, "xenfv");

QEMU uses the term "xenfv", this is a terminology mismatch between QEMU
and Xen...

Wei.

>          break;
>      }
> @@ -417,7 +417,7 @@ static char ** libxl__build_device_model
>      flexarray_append(dm_args, "-m");
>      flexarray_append(dm_args, libxl__sprintf(gc, "%d", info->target_ram));
>  
> -    if (info->type == LIBXL_DOMAIN_TYPE_FV) {
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          for (i = 0; i < num_disks; i++) {
>              int disk, part;
>              int dev_number =
> diff -r b18728227bf9 -r 90e2ae994ebb tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:29 2011 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:29 2011 +0100
> @@ -1083,7 +1083,7 @@ skip_vfb:
>      }
>  
>      dm_info->type = c_info->hvm ?
> -        LIBXL_DOMAIN_TYPE_FV :
> +        LIBXL_DOMAIN_TYPE_HVM :
>          LIBXL_DOMAIN_TYPE_PV;
>  
>      xlu_cfg_destroy(config);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 1 of 6 V2] libxl: Give the HVM domain type the name "HVM"

[Ian Jackson]

Wei LIU writes ("Re: [Xen-devel] [PATCH 1 of 6 V2] libxl: Give the HVM domain type the name "HVM""):
> On Mon, 2011-07-18 at 14:57 +0100, Ian Campbell wrote:
> > +    case LIBXL_DOMAIN_TYPE_HVM:
> >          flexarray_append(dm_args, "xenfv");
> 
> QEMU uses the term "xenfv", this is a terminology mismatch between QEMU
> and Xen...

Yes.  It might be nice to fix that I suppose but I think our effort
for that kind of change, in the upstream qemu tree, is limited ...

Ian.

[PATCH 2 of 6 V2] libxl: replace libxl__domain_is_hvm with libxl__domain_type

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310997150 -3600
# Node ID e821a5d1144ba8ff44701eaeee4671f9b0ac1330
# Parent  90e2ae994ebbc37ba6279b9071ab2e76c2ebeec2
libxl: replace libxl__domain_is_hvm with libxl__domain_type

New function returns a libxl_domain_type enum.

Add LIBXL__DOMAIN_IS_TYPE helper macro.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 90e2ae994ebb -r e821a5d1144b tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 18 14:52:30 2011 +0100
@@ -240,7 +240,7 @@ int libxl_domain_resume(libxl_ctx *ctx, 
     libxl__gc gc = LIBXL_INIT_GC(ctx);
     int rc = 0;
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM)) {
         LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Called domain_resume on "
                 "non-cooperative hvm domain %u", domid);
         rc = ERROR_NI;
@@ -474,7 +474,7 @@ int libxl_domain_suspend(libxl_ctx *ctx,
                          uint32_t domid, int fd)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    int hvm = libxl__domain_is_hvm(&gc, domid);
+    int hvm = LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM);
     int live = info != NULL && info->flags & XL_SUSPEND_LIVE;
     int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG;
     int rc = 0;
@@ -517,7 +517,7 @@ int libxl_domain_unpause(libxl_ctx *ctx,
     char *state;
     int ret, rc = 0;
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM)) {
         path = libxl__sprintf(&gc, "/local/domain/0/device-model/%d/state", domid);
         state = libxl__xs_read(&gc, XBT_NULL, path);
         if (state != NULL && !strcmp(state, "paused")) {
@@ -560,7 +560,7 @@ int libxl_domain_shutdown(libxl_ctx *ctx
         return ERROR_FAIL;
     }
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM)) {
         unsigned long pvdriver = 0;
         int ret;
         ret = xc_get_hvm_param(ctx->xch, domid, HVM_PARAM_CALLBACK_IRQ, &pvdriver);
@@ -722,6 +722,7 @@ int libxl_domain_destroy(libxl_ctx *ctx,
     libxl_dominfo dominfo;
     char *dom_path;
     char *vm_path;
+    char *pid;
     int rc, dm_present;
 
     rc = libxl_domain_info(ctx, &dominfo, domid);
@@ -734,12 +735,16 @@ int libxl_domain_destroy(libxl_ctx *ctx,
         return rc;
     }
 
-    if (libxl__domain_is_hvm(&gc, domid)) {
+    switch (libxl__domain_type(&gc, domid)) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         dm_present = 1;
-    } else {
-        char *pid;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         pid = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "/local/domain/%d/image/device-model-pid", domid));
         dm_present = (pid != NULL);
+        break;
+    default:
+        abort();
     }
 
     dom_path = libxl__xs_get_dompath(&gc, domid);
@@ -818,10 +823,16 @@ int libxl_primary_console_exec(libxl_ctx
         rc = libxl_console_exec(ctx, stubdomid,
                                 STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV);
     else {
-        if (libxl__domain_is_hvm(&gc, domid_vm))
+        switch (libxl__domain_type(&gc, domid_vm)) {
+        case LIBXL_DOMAIN_TYPE_HVM:
             rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_SERIAL);
-        else
+            break;
+        case LIBXL_DOMAIN_TYPE_PV:
             rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
+            break;
+        default:
+            abort();
+        }
     }
     libxl__free_all(&gc);
     return rc;
diff -r 90e2ae994ebb -r e821a5d1144b tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 18 14:52:30 2011 +0100
@@ -35,7 +35,7 @@
 #include "libxl.h"
 #include "libxl_internal.h"
 
-int libxl__domain_is_hvm(libxl__gc *gc, uint32_t domid)
+libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xc_domaininfo_t info;
@@ -46,7 +46,10 @@ int libxl__domain_is_hvm(libxl__gc *gc, 
         return -1;
     if (info.domain != domid)
         return -1;
-    return !!(info.flags & XEN_DOMINF_hvm_guest);
+    if (info.flags & XEN_DOMINF_hvm_guest)
+        return LIBXL_DOMAIN_TYPE_HVM;
+    else
+        return LIBXL_DOMAIN_TYPE_PV;
 }
 
 int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid)
diff -r 90e2ae994ebb -r e821a5d1144b tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Mon Jul 18 14:52:30 2011 +0100
@@ -166,9 +166,10 @@ _hidden char **libxl__xs_directory(libxl
    /* On error: returns NULL, sets errno (no logging) */
 
 /* from xl_dom */
-_hidden int libxl__domain_is_hvm(libxl__gc *gc, uint32_t domid);
+_hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
 _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
-
+#define LIBXL__DOMAIN_IS_TYPE(gc, domid, type) \
+    libxl__domain_type((gc), (domid)) == LIBXL_DOMAIN_TYPE_##type
 typedef struct {
     uint32_t store_port;
     unsigned long store_mfn;
diff -r 90e2ae994ebb -r e821a5d1144b tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon Jul 18 14:52:29 2011 +0100
+++ b/tools/libxl/libxl_pci.c	Mon Jul 18 14:52:30 2011 +0100
@@ -286,7 +286,7 @@ static int libxl__device_pci_add_xenstor
     if (!num_devs)
         return libxl__create_pci_backend(gc, domid, pcidev, 1);
 
-    if (!starting && !libxl__domain_is_hvm(gc, domid)) {
+    if (!starting && LIBXL__DOMAIN_IS_TYPE(gc, domid, PV)) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0)
             return ERROR_FAIL;
     }
@@ -329,7 +329,7 @@ static int libxl__device_pci_remove_xens
         return ERROR_INVAL;
     num = atoi(num_devs);
 
-    if (!libxl__domain_is_hvm(gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(gc, domid, PV)) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0) {
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "pci backend at %s is not ready", be_path);
             return ERROR_FAIL;
@@ -357,7 +357,7 @@ retry_transaction:
         if (errno == EAGAIN)
             goto retry_transaction;
 
-    if (!libxl__domain_is_hvm(gc, domid)) {
+    if (LIBXL__DOMAIN_IS_TYPE(gc, domid, PV)) {
         if (libxl__wait_for_backend(gc, be_path, "4") < 0) {
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "pci backend at %s is not ready", be_path);
             return ERROR_FAIL;
@@ -604,10 +604,11 @@ static int do_pci_add(libxl__gc *gc, uin
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *path;
     char *state, *vdevfn;
-    int rc, hvm;
+    int rc, hvm = 0;
 
-    hvm = libxl__domain_is_hvm(gc, domid);
-    if (hvm) {
+    switch (libxl__domain_type(gc, domid)) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
         if (libxl__wait_for_device_model(gc, domid, "running",
                                          NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
@@ -635,7 +636,9 @@ static int do_pci_add(libxl__gc *gc, uin
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
         if ( rc )
             return ERROR_FAIL;
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                          pcidev->bus, pcidev->dev, pcidev->func);
         FILE *f = fopen(sysfs_path, "r");
@@ -694,6 +697,9 @@ static int do_pci_add(libxl__gc *gc, uin
         }
         fclose(f);
     }
+    default:
+        abort();
+    }
 out:
     if (!libxl_is_stubdom(ctx, domid, NULL)) {
         rc = xc_assign_device(ctx->xch, domid, pcidev_encode_bdf(pcidev));
@@ -831,7 +837,7 @@ static int do_pci_remove(libxl__gc *gc, 
     libxl_device_pci *assigned;
     char *path;
     char *state;
-    int hvm, rc, num;
+    int hvm = 0, rc, num;
     int stubdomid = 0;
 
     if ( !libxl_device_pci_list_assigned(ctx, &assigned, domid, &num) ) {
@@ -842,8 +848,9 @@ static int do_pci_remove(libxl__gc *gc, 
         }
     }
 
-    hvm = libxl__domain_is_hvm(gc, domid);
-    if (hvm) {
+    switch (libxl__domain_type(gc, domid)) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
         if (libxl__wait_for_device_model(gc, domid, "running",
                                          NULL, NULL, NULL) < 0) {
             return ERROR_FAIL;
@@ -871,7 +878,9 @@ static int do_pci_remove(libxl__gc *gc, 
         }
         path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
         xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         char *sysfs_path = libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
                                          pcidev->bus, pcidev->dev, pcidev->func);
         FILE *f = fopen(sysfs_path, "r");
@@ -921,6 +930,9 @@ skip1:
         }
         fclose(f);
     }
+    default:
+        abort();
+    }
 out:
     /* don't do multiple resets while some functions are still passed through */
     if ( (pcidev->vdevfn & 0x7) == 0 ) {

[PATCH 3 of 6 V2] libxl: specify HVM vs PV in build_info using libxl_domain_type enum

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310997150 -3600
# Node ID 9f059f087152cd98b509dc915be384ea97948d6f
# Parent  e821a5d1144ba8ff44701eaeee4671f9b0ac1330
libxl: specify HVM vs PV in build_info using libxl_domain_type enum

Also caught one place (in libxl__domain_restore_common) which used
info->u.hvm.pae even if !hvm. (fortunately the value was unused in
xc_domain_restore).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r e821a5d1144b -r 9f059f087152 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 18 14:52:30 2011 +0100
@@ -2036,17 +2036,27 @@ int libxl_domain_need_memory(libxl_ctx *
         libxl_device_model_info *dm_info, uint32_t *need_memkb)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
+    int rc = ERROR_INVAL;
     *need_memkb = b_info->target_memkb;
-    if (b_info->hvm) {
+    switch (b_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
         if (dm_info->device_model_stubdomain)
             *need_memkb += 32 * 1024;
-    } else
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
+        break;
+    default:
+        goto out;
+    }
     if (*need_memkb % (2 * 1024))
         *need_memkb += (2 * 1024) - (*need_memkb % (2 * 1024));
+    rc = 0;
+out:
     libxl__free_all(&gc);
-    return 0;
+    return rc;
+
 }
 
 int libxl_get_free_memory(libxl_ctx *ctx, uint32_t *memkb)
diff -r e821a5d1144b -r 9f059f087152 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 14:52:30 2011 +0100
@@ -158,9 +158,9 @@ libxl_domain_build_info = Struct("domain
     ("shadow_memkb",    uint32),
     ("disable_migrate", bool),
     ("cpuid",           libxl_cpuid_policy_list),
-    ("hvm",             bool),
-    ("u", KeyedUnion(None, "hvm",
-                [("hvm", "%s", Struct(None,
+    ("type",            libxl_domain_type),
+    ("u", KeyedUnion(None, "type",
+                [("hvm", "%s == LIBXL_DOMAIN_TYPE_HVM", Struct(None,
                                        [("firmware", string),
                                         ("pae", bool),
                                         ("apic", bool),
@@ -173,7 +173,7 @@ libxl_domain_build_info = Struct("domain
                                         ("timer_mode", integer),
                                         ("nested_hvm", bool),
                                         ])),
-                 ("pv", "!%s", Struct(None,
+                 ("pv", "%s == LIBXL_DOMAIN_TYPE_PV", Struct(None,
                                        [("kernel", libxl_file_reference),
                                         ("slack_memkb", uint32),
                                         ("bootloader", string),
diff -r e821a5d1144b -r 9f059f087152 tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_bootloader.c	Mon Jul 18 14:52:30 2011 +0100
@@ -323,7 +323,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
 
     struct stat st_buf;
 
-    if (info->hvm || !info->u.pv.bootloader)
+    if (info->type != LIBXL_DOMAIN_TYPE_PV || !info->u.pv.bootloader)
         goto out;
 
     rc = ERROR_INVAL;
diff -r e821a5d1144b -r 9f059f087152 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_create.c	Mon Jul 18 14:52:30 2011 +0100
@@ -84,7 +84,7 @@ void libxl_init_build_info(libxl_domain_
     b_info->shadow_memkb = 0;
     if (c_info->hvm) {
         b_info->video_memkb = 8 * 1024;
-        b_info->hvm = 1;
+        b_info->type = LIBXL_DOMAIN_TYPE_HVM;
         b_info->u.hvm.firmware = NULL;
         b_info->u.hvm.pae = 1;
         b_info->u.hvm.apic = 1;
@@ -96,6 +96,7 @@ void libxl_init_build_info(libxl_domain_
         b_info->u.hvm.timer_mode = 1;
         b_info->u.hvm.nested_hvm = 0;
     } else {
+        b_info->type = LIBXL_DOMAIN_TYPE_PV;
         b_info->u.pv.slack_memkb = 8 * 1024;
     }
 }
@@ -160,7 +161,8 @@ int libxl__domain_build(libxl__gc *gc,
 
     gettimeofday(&start_time, NULL);
 
-    if (info->hvm) {
+    switch (info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         ret = libxl__build_hvm(gc, domid, info, dm_info, state);
         if (ret)
             goto out;
@@ -172,7 +174,8 @@ int libxl__domain_build(libxl__gc *gc,
         vments[3] = "hvm";
         vments[4] = "start_time";
         vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         ret = libxl__build_pv(gc, domid, info, state);
         if (ret)
             goto out;
@@ -193,6 +196,10 @@ int libxl__domain_build(libxl__gc *gc,
             vments[i++] = "image/cmdline";
             vments[i++] = (char*) info->u.pv.cmdline;
         }
+        break;
+    default:
+        ret = ERROR_INVAL;
+        goto out;
     }
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
@@ -219,7 +226,8 @@ static int domain_restore(libxl__gc *gc,
 
     gettimeofday(&start_time, NULL);
 
-    if (info->hvm) {
+    switch (info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         vments = libxl__calloc(gc, 7, sizeof(char *));
         vments[0] = "rtc/timeoffset";
         vments[1] = (info->u.hvm.timeoffset) ? info->u.hvm.timeoffset : "";
@@ -227,7 +235,8 @@ static int domain_restore(libxl__gc *gc,
         vments[3] = "hvm";
         vments[4] = "start_time";
         vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         vments = libxl__calloc(gc, 11, sizeof(char *));
         i = 0;
         vments[i++] = "image/ostype";
@@ -244,20 +253,24 @@ static int domain_restore(libxl__gc *gc,
             vments[i++] = "image/cmdline";
             vments[i++] = (char*) info->u.pv.cmdline;
         }
+        break;
+    default:
+        ret = ERROR_INVAL;
+        goto out;
     }
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
     if (ret)
         goto out;
 
     dm_info->saved_state = NULL;
-    if (info->hvm) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         ret = asprintf(&dm_info->saved_state,
                        XC_DEVICE_MODEL_RESTORE_FILE".%d", domid);
         ret = (ret < 0) ? ERROR_FAIL : 0;
     }
 
 out:
-    if (!info->hvm) {
+    if (info->type == LIBXL_DOMAIN_TYPE_PV) {
         libxl__file_reference_unmap(&info->u.pv.kernel);
         libxl__file_reference_unmap(&info->u.pv.ramdisk);
     }
diff -r e821a5d1144b -r 9f059f087152 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:30 2011 +0100
@@ -642,12 +642,13 @@ static int libxl__create_stubdom(libxl__
     b_info.max_vcpus = 1;
     b_info.max_memkb = 32 * 1024;
     b_info.target_memkb = b_info.max_memkb;
+
+    b_info.type = LIBXL_DOMAIN_TYPE_PV;
     b_info.u.pv.kernel.path = libxl__abs_path(gc, "ioemu-stubdom.gz",
                                               libxl_xenfirmwaredir_path());
     b_info.u.pv.cmdline = libxl__sprintf(gc, " -d %d", info->domid);
     b_info.u.pv.ramdisk.path = "";
     b_info.u.pv.features = "";
-    b_info.hvm = 0;
 
     /* fixme: this function can leak the stubdom if it fails */
 
diff -r e821a5d1144b -r 9f059f087152 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 18 14:52:30 2011 +0100
@@ -75,14 +75,14 @@ int libxl__build_pre(libxl__gc *gc, uint
     libxl_ctx *ctx = libxl__gc_owner(gc);
     xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
     xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + LIBXL_MAXMEM_CONSTANT);
-    if (!info->hvm)
+    if (info->type == LIBXL_DOMAIN_TYPE_PV)
         xc_domain_set_memmap_limit(ctx->xch, domid,
                 (info->max_memkb + info->u.pv.slack_memkb));
     xc_domain_set_tsc_info(ctx->xch, domid, info->tsc_mode, 0, 0, 0);
     if ( info->disable_migrate )
         xc_domain_disable_migrate(ctx->xch, domid);
 
-    if (info->hvm) {
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         unsigned long shadow;
         shadow = (info->shadow_memkb + 1023) / 1024;
         xc_shadow_control(ctx->xch, domid, XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION, NULL, 0, &shadow, 0, NULL);
@@ -340,10 +340,25 @@ int libxl__domain_restore_common(libxl__
     libxl_ctx *ctx = libxl__gc_owner(gc);
     /* read signature */
     int rc;
+    int hvm, pae, superpages;
+    switch (info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
+        superpages = 1;
+        pae = info->u.hvm.pae;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+        hvm = 0;
+        superpages = 0;
+        pae = 1;
+        break;
+    default:
+        return ERROR_INVAL;
+    }
     rc = xc_domain_restore(ctx->xch, fd, domid,
-                             state->store_port, &state->store_mfn,
-                             state->console_port, &state->console_mfn,
-                             info->hvm, info->u.hvm.pae, !!info->hvm);
+                           state->store_port, &state->store_mfn,
+                           state->console_port, &state->console_mfn,
+                           hvm, pae, superpages);
     if ( rc ) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "restoring domain");
         return ERROR_FAIL;
diff -r e821a5d1144b -r 9f059f087152 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:30 2011 +0100
@@ -381,7 +381,7 @@ static void printf_info(int domid,
         printf("\t\t\t(spiceagent_mouse %d)\n", dm_info->spiceagent_mouse);
         printf("\t\t)\n");
     } else {
-        printf("\t\t(linux %d)\n", b_info->hvm);
+        printf("\t\t(linux %d)\n", 0);
         printf("\t\t\t(kernel %s)\n", b_info->u.pv.kernel.path);
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk.path);

[PATCH 4 of 6 V2] libxl: specify HVM vs PV in create_info using libxl_domain_type enum

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310997150 -3600
# Node ID 967b5e7bba040a8d7341cfe81faa57556b8cb5c0
# Parent  9f059f087152cd98b509dc915be384ea97948d6f
libxl: specify HVM vs PV in create_info using libxl_domain_type enum

Since libxl_init_build_info now needs an error return and a ctx (to
log to) switch all libxl_init_*_info to have an int return and a ctx.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 9f059f087152 -r 967b5e7bba04 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl.h	Mon Jul 18 14:52:30 2011 +0100
@@ -250,9 +250,14 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 i
 int libxl_ctx_postfork(libxl_ctx *ctx);
 
 /* domain related functions */
-void libxl_init_create_info(libxl_domain_create_info *c_info);
-void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info);
-void libxl_init_dm_info(libxl_device_model_info *dm_info, libxl_domain_create_info *c_info, libxl_domain_build_info *b_info);
+int libxl_init_create_info(libxl_ctx *ctx, libxl_domain_create_info *c_info);
+int libxl_init_build_info(libxl_ctx *ctx,
+                          libxl_domain_build_info *b_info,
+                          libxl_domain_create_info *c_info);
+int libxl_init_dm_info(libxl_ctx *ctx,
+                       libxl_device_model_info *dm_info,
+                       libxl_domain_create_info *c_info,
+                       libxl_domain_build_info *b_info);
 typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid);
 int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_console_ready cb, void *priv, uint32_t *domid, int restore_fd);
diff -r 9f059f087152 -r 967b5e7bba04 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 14:52:30 2011 +0100
@@ -136,7 +136,7 @@ libxl_version_info = Struct("version_inf
     ])
                                              
 libxl_domain_create_info = Struct("domain_create_info",[
-    ("hvm",          bool),
+    ("type",         libxl_domain_type),
     ("hap",          bool),
     ("oos",          bool),
     ("ssidref",      uint32),
diff -r 9f059f087152 -r 967b5e7bba04 tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_create.c	Mon Jul 18 14:52:30 2011 +0100
@@ -60,19 +60,22 @@ void libxl_domain_config_destroy(libxl_d
     libxl_device_model_info_destroy(&d_config->dm_info);
 }
 
-void libxl_init_create_info(libxl_domain_create_info *c_info)
+int libxl_init_create_info(libxl_ctx *ctx, libxl_domain_create_info *c_info)
 {
     memset(c_info, '\0', sizeof(*c_info));
     c_info->xsdata = NULL;
     c_info->platformdata = NULL;
     c_info->hap = 1;
-    c_info->hvm = 1;
+    c_info->type = LIBXL_DOMAIN_TYPE_HVM;
     c_info->oos = 1;
     c_info->ssidref = 0;
     c_info->poolid = 0;
+    return 0;
 }
 
-void libxl_init_build_info(libxl_domain_build_info *b_info, libxl_domain_create_info *c_info)
+int libxl_init_build_info(libxl_ctx *ctx,
+                          libxl_domain_build_info *b_info,
+                          libxl_domain_create_info *c_info)
 {
     memset(b_info, '\0', sizeof(*b_info));
     b_info->max_vcpus = 1;
@@ -82,9 +85,10 @@ void libxl_init_build_info(libxl_domain_
     b_info->disable_migrate = 0;
     b_info->cpuid = NULL;
     b_info->shadow_memkb = 0;
-    if (c_info->hvm) {
+    b_info->type = c_info->type;
+    switch (b_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         b_info->video_memkb = 8 * 1024;
-        b_info->type = LIBXL_DOMAIN_TYPE_HVM;
         b_info->u.hvm.firmware = NULL;
         b_info->u.hvm.pae = 1;
         b_info->u.hvm.apic = 1;
@@ -95,14 +99,23 @@ void libxl_init_build_info(libxl_domain_
         b_info->u.hvm.vpt_align = 1;
         b_info->u.hvm.timer_mode = 1;
         b_info->u.hvm.nested_hvm = 0;
-    } else {
-        b_info->type = LIBXL_DOMAIN_TYPE_PV;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         b_info->u.pv.slack_memkb = 8 * 1024;
+        break;
+    default:
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "invalid domain type %s in create info",
+                   libxl_domain_type_to_string(b_info->type));
+        return ERROR_INVAL;
     }
+    return 0;
 }
 
-void libxl_init_dm_info(libxl_device_model_info *dm_info,
-        libxl_domain_create_info *c_info, libxl_domain_build_info *b_info)
+int libxl_init_dm_info(libxl_ctx *ctx,
+                       libxl_device_model_info *dm_info,
+                       libxl_domain_create_info *c_info,
+                       libxl_domain_build_info *b_info)
 {
     memset(dm_info, '\0', sizeof(*dm_info));
 
@@ -132,6 +145,7 @@ void libxl_init_dm_info(libxl_device_mod
     dm_info->usb = 0;
     dm_info->usbdevice = NULL;
     dm_info->xen_platform_pci = 1;
+    return 0;
 }
 
 static int init_console_info(libxl_device_console *console, int dev_num)
@@ -316,9 +330,12 @@ int libxl__domain_make(libxl__gc *gc, li
         goto out;
     }
 
-    flags = info->hvm ? XEN_DOMCTL_CDF_hvm_guest : 0;
-    flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
-    flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
+    flags = 0;
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        flags |= XEN_DOMCTL_CDF_hvm_guest;
+        flags |= info->hap ? XEN_DOMCTL_CDF_hap : 0;
+        flags |= info->oos ? 0 : XEN_DOMCTL_CDF_oos_off;
+    }
     *domid = -1;
 
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
@@ -432,7 +449,7 @@ static int do_domain_create(libxl__gc *g
         goto error_out;
     }
 
-    if ( !d_config->c_info.hvm && cb ) {
+    if ( d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && cb ) {
         if ( (*cb)(ctx, domid, priv) )
             goto error_out;
     }
@@ -486,7 +503,9 @@ static int do_domain_create(libxl__gc *g
             goto error_out;
         }
     }
-    if (d_config->c_info.hvm) {
+    switch (d_config->c_info.type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+    {
         libxl_device_console console;
 
         ret = init_console_info(&console, 0);
@@ -505,7 +524,10 @@ static int do_domain_create(libxl__gc *g
                        "failed to create device model: %d", ret);
             goto error_out;
         }
-    } else {
+        break;
+    }
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         int need_qemu = 0;
         libxl_device_console console;
 
@@ -530,6 +552,11 @@ static int do_domain_create(libxl__gc *g
 
         if (need_qemu)
             libxl__create_xenpv_qemu(gc, domid, d_config->vfbs, &dm_starting);
+        break;
+    }
+    default:
+        ret = ERROR_INVAL;
+        goto error_out;
     }
 
     if (dm_starting) {
@@ -552,7 +579,8 @@ static int do_domain_create(libxl__gc *g
         goto error_out;
     }
 
-    if (!d_config->c_info.hvm && d_config->b_info.u.pv.e820_host) {
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
+        d_config->b_info.u.pv.e820_host) {
         int rc;
         rc = libxl__e820_alloc(ctx, domid, d_config);
         if (rc)
@@ -560,7 +588,9 @@ static int do_domain_create(libxl__gc *g
                       "Failed while collecting E820 with: %d (errno:%d)\n",
                       rc, errno);
     }
-    if ( cb && (d_config->c_info.hvm || d_config->b_info.u.pv.bootloader )) {
+    if ( cb && (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM ||
+                (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
+                 d_config->b_info.u.pv.bootloader ))) {
         if ( (*cb)(ctx, domid, priv) )
             goto error_out;
     }
diff -r 9f059f087152 -r 967b5e7bba04 tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_dm.c	Mon Jul 18 14:52:30 2011 +0100
@@ -633,7 +633,7 @@ static int libxl__create_stubdom(libxl__
     }
 
     memset(&c_info, 0x00, sizeof(libxl_domain_create_info));
-    c_info.hvm = 0;
+    c_info.type = LIBXL_DOMAIN_TYPE_PV;
     c_info.name = libxl__sprintf(gc, "%s-dm", libxl__domid_to_name(gc, info->domid));
 
     libxl_uuid_copy(&c_info.uuid, &info->uuid);
diff -r 9f059f087152 -r 967b5e7bba04 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_pci.c	Mon Jul 18 14:52:30 2011 +0100
@@ -1275,7 +1275,7 @@ int libxl__e820_alloc(libxl_ctx *ctx, ui
     struct e820entry map[E820MAX];
     libxl_domain_build_info *b_info;
 
-    if (d_config == NULL || d_config->c_info.hvm)
+    if (d_config == NULL || d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
         return ERROR_INVAL;
 
     b_info = &d_config->b_info;
diff -r 9f059f087152 -r 967b5e7bba04 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Jul 18 14:52:30 2011 +0100
@@ -300,7 +300,7 @@ static void printf_info(int domid,
 
     printf("(domain\n\t(domid %d)\n", domid);
     printf("\t(create_info)\n");
-    printf("\t(hvm %d)\n", c_info->hvm);
+    printf("\t(hvm %d)\n", c_info->type == LIBXL_DOMAIN_TYPE_HVM);
     printf("\t(hap %d)\n", c_info->hap);
     printf("\t(oos %d)\n", c_info->oos);
     printf("\t(ssidref %d)\n", c_info->ssidref);
@@ -333,14 +333,15 @@ static void printf_info(int domid,
     printf("\t(target_memkb %d)\n", b_info->target_memkb);
     printf("\t(nomigrate %d)\n", b_info->disable_migrate);
 
-    if (!c_info->hvm && b_info->u.pv.bootloader) {
+    if (c_info->type == LIBXL_DOMAIN_TYPE_PV && b_info->u.pv.bootloader) {
         printf("\t(bootloader %s)\n", b_info->u.pv.bootloader);
         if (b_info->u.pv.bootloader_args)
             printf("\t(bootloader_args %s)\n", b_info->u.pv.bootloader_args);
     }
 
     printf("\t(image\n");
-    if (c_info->hvm) {
+    switch (c_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         printf("\t\t(hvm\n");
         printf("\t\t\t(firmware %s)\n", b_info->u.hvm.firmware);
         printf("\t\t\t(video_memkb %d)\n", b_info->video_memkb);
@@ -380,13 +381,18 @@ static void printf_info(int domid,
                     dm_info->spicedisable_ticketing);
         printf("\t\t\t(spiceagent_mouse %d)\n", dm_info->spiceagent_mouse);
         printf("\t\t)\n");
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
         printf("\t\t(linux %d)\n", 0);
         printf("\t\t\t(kernel %s)\n", b_info->u.pv.kernel.path);
         printf("\t\t\t(cmdline %s)\n", b_info->u.pv.cmdline);
         printf("\t\t\t(ramdisk %s)\n", b_info->u.pv.ramdisk.path);
         printf("\t\t\t(e820_host %d)\n", b_info->u.pv.e820_host);
         printf("\t\t)\n");
+        break;
+    default:
+        fprintf(stderr, "Unknown domain type %d\n", c_info->type);
+        exit(1);
     }
     printf("\t)\n");
 
@@ -453,7 +459,7 @@ static void printf_info(int domid,
         printf("\t\t)\n");
         printf("\t)\n");
     }
-       printf(")\n");
+    printf(")\n");
 }
 
 static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a)
@@ -538,7 +544,8 @@ static void parse_config_data(const char
         exit(1);
     }
 
-    libxl_init_create_info(c_info);
+    if (libxl_init_create_info(ctx, c_info))
+        exit(1);
 
     if (!xlu_cfg_get_string (config, "seclabel", &buf)) {
         e = libxl_flask_context_to_sid(ctx, (char *)buf, strlen(buf),
@@ -553,10 +560,10 @@ static void parse_config_data(const char
         }
     }
 
-    c_info->hvm = 0;
+    c_info->type = LIBXL_DOMAIN_TYPE_PV;
     if (!xlu_cfg_get_string (config, "builder", &buf) &&
         !strncmp(buf, "hvm", strlen(buf)))
-        c_info->hvm = 1;
+        c_info->type = LIBXL_DOMAIN_TYPE_HVM;
 
     if (!xlu_cfg_get_long (config, "hap", &l))
         c_info->hap = l;
@@ -586,7 +593,8 @@ static void parse_config_data(const char
         exit(1);
     }
 
-    libxl_init_build_info(b_info, c_info);
+    if (libxl_init_build_info(ctx, b_info, c_info))
+        exit(1);
 
     /* the following is the actual config parsing with overriding values in the structures */
     if (!xlu_cfg_get_long (config, "vcpus", &l)) {
@@ -654,7 +662,8 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "gfx_passthru", &l))
         dm_info->gfx_passthru = l;
 
-    if (c_info->hvm == 1) {
+    switch(c_info->type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
         if (!xlu_cfg_get_string (config, "kernel", &buf))
             fprintf(stderr, "WARNING: ignoring \"kernel\" directive for HVM guest. "
                     "Use \"firmware_override\" instead if you really want a non-default firmware\n");
@@ -679,7 +688,9 @@ static void parse_config_data(const char
             b_info->u.hvm.timer_mode = l;
         if (!xlu_cfg_get_long (config, "nestedhvm", &l))
             b_info->u.hvm.nested_hvm = l;
-    } else {
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+    {
         char *cmdline = NULL;
         const char *root = NULL, *extra = "";
 
@@ -712,6 +723,10 @@ static void parse_config_data(const char
 
         b_info->u.pv.cmdline = cmdline;
         xlu_cfg_replace_string (config, "ramdisk", &b_info->u.pv.ramdisk.path);
+        break;
+    }
+    default:
+        abort();
     }
 
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
@@ -888,11 +903,16 @@ skip_vfb:
     /* To be reworked (automatically enabled) once the auto ballooning
      * after guest starts is done (with PCI devices passed in). */
     if (!xlu_cfg_get_long (config, "e820_host", &l)) {
-        if (c_info->hvm)
-          fprintf(stderr, "Can't do e820_host in HVM mode!");
-        else {
-          if (l)
-            b_info->u.pv.e820_host = true;
+        switch (c_info->type) {
+        case LIBXL_DOMAIN_TYPE_HVM:
+            fprintf(stderr, "Can't do e820_host in HVM mode!");
+            break;
+        case LIBXL_DOMAIN_TYPE_PV:
+            if (l)
+                b_info->u.pv.e820_host = true;
+            break;
+        default:
+            abort();
         }
     }
     if (!xlu_cfg_get_list (config, "pci", &pcis, 0, 0)) {
@@ -911,7 +931,7 @@ skip_vfb:
             if (!libxl_device_pci_parse_bdf(ctx, pcidev, buf))
                 d_config->num_pcidevs++;
         }
-        if (d_config->num_pcidevs && !c_info->hvm)
+        if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
             b_info->u.pv.e820_host = true;
     }
 
@@ -994,12 +1014,13 @@ skip_vfb:
         break;
     }
 
-    if (c_info->hvm == 1) {
+    if (c_info->type == LIBXL_DOMAIN_TYPE_HVM) {
         XLU_ConfigList *dmargs;
         int nr_dmargs = 0;
 
         /* init dm from c and b */
-        libxl_init_dm_info(dm_info, c_info, b_info);
+        if (libxl_init_dm_info(ctx, dm_info, c_info, b_info))
+            exit(1);
 
         /* then process config related to dm */
         if (!xlu_cfg_get_string (config, "device_model", &buf)) {
@@ -1082,9 +1103,7 @@ skip_vfb:
         }
     }
 
-    dm_info->type = c_info->hvm ?
-        LIBXL_DOMAIN_TYPE_HVM :
-        LIBXL_DOMAIN_TYPE_PV;
+    dm_info->type = c_info->type;
 
     xlu_cfg_destroy(config);
 }

[PATCH 5 of 6 V2] libxl: use libxl_domain_type enum with libxl__domain_suspend_common

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310997150 -3600
# Node ID 686d683a3a57f7a1b315c49e10d5e60ec4d11133
# Parent  967b5e7bba040a8d7341cfe81faa57556b8cb5c0
libxl: use libxl_domain_type enum with libxl__domain_suspend_common

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 967b5e7bba04 -r 686d683a3a57 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl.c	Mon Jul 18 14:52:30 2011 +0100
@@ -474,13 +474,13 @@ int libxl_domain_suspend(libxl_ctx *ctx,
                          uint32_t domid, int fd)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    int hvm = LIBXL__DOMAIN_IS_TYPE(&gc,  domid, HVM);
+    libxl_domain_type type = libxl__domain_type(&gc, domid);
     int live = info != NULL && info->flags & XL_SUSPEND_LIVE;
     int debug = info != NULL && info->flags & XL_SUSPEND_DEBUG;
     int rc = 0;
 
-    rc = libxl__domain_suspend_common(&gc, domid, fd, hvm, live, debug);
-    if (!rc && hvm)
+    rc = libxl__domain_suspend_common(&gc, domid, fd, type, live, debug);
+    if (!rc && type == LIBXL_DOMAIN_TYPE_HVM)
         rc = libxl__domain_save_device_model(&gc, domid, fd);
     libxl__free_all(&gc);
     return rc;
diff -r 967b5e7bba04 -r 686d683a3a57 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_dom.c	Mon Jul 18 14:52:30 2011 +0100
@@ -513,14 +513,26 @@ static int libxl__domain_suspend_common_
 }
 
 int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
-		int hvm, int live, int debug)
+                                 libxl_domain_type type,
+                                 int live, int debug)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int flags;
     int port;
     struct save_callbacks callbacks;
     struct suspendinfo si;
-    int rc = ERROR_FAIL;
+    int hvm, rc = ERROR_FAIL;
+
+    switch (type) {
+    case LIBXL_DOMAIN_TYPE_HVM:
+        hvm = 1;
+        break;
+    case LIBXL_DOMAIN_TYPE_PV:
+        hvm = 0;
+        break;
+    default:
+        return ERROR_INVAL;
+    }
 
     flags = (live) ? XCFLAGS_LIVE : 0
           | (debug) ? XCFLAGS_DEBUG : 0
diff -r 967b5e7bba04 -r 686d683a3a57 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl_internal.h	Mon Jul 18 14:52:30 2011 +0100
@@ -199,7 +199,9 @@ _hidden int libxl__domain_restore_common
                                          libxl_domain_build_info *info,
                                          libxl__domain_build_state *state,
                                          int fd);
-_hidden int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd, int hvm, int live, int debug);
+_hidden int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
+                                         libxl_domain_type type,
+                                         int live, int debug);
 _hidden int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd);
 _hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);

[PATCH 6 of 6 V2] libxl: Keyed unions key off an enum instead of an arbitrary expression

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1310997150 -3600
# Node ID 3e9a884f93b77ba3b885dcd99518aac267d97ff8
# Parent  686d683a3a57f7a1b315c49e10d5e60ec4d11133
libxl: Keyed unions key off an enum instead of an arbitrary expression

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 686d683a3a57 -r 3e9a884f93b7 tools/libxl/gentypes.py
--- a/tools/libxl/gentypes.py	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/gentypes.py	Mon Jul 18 14:52:30 2011 +0100
@@ -81,12 +81,13 @@ def libxl_C_type_destroy(ty, v, indent =
     if isinstance(ty, libxltypes.KeyedUnion):
         if parent is None:
             raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar_name)        
         for f in ty.fields:
             (nparent,fexpr) = ty.member(v, f, parent is None)
-            keyvar_expr = f.keyvar_expr % (parent + ty.keyvar_name)
-            s += "if (" + keyvar_expr + ") {\n"
+            s += "case %s:\n" % f.enumname
             s += libxl_C_type_destroy(f.type, fexpr, indent + "    ", nparent)
-            s += "}\n"
+            s += "    break;\n"
+        s += "}\n"
     elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.destructor_fn is None):
         for f in [f for f in ty.fields if not f.const]:
             (nparent,fexpr) = ty.member(v, f, parent is None)
diff -r 686d683a3a57 -r 3e9a884f93b7 tools/libxl/idl.txt
--- a/tools/libxl/idl.txt	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/idl.txt	Mon Jul 18 14:52:30 2011 +0100
@@ -127,13 +127,9 @@ libxltype.KeyedUnion
  upon another member in the containing type.
 
  The KeyedUnion.keyvar_name must contain the name of the member of the
- containing type which determines the valid member of the union
-
- The fields in a KeyedUnion have an extra Field.keyvar_expr
- property. This must be a string containing a single "%s" format
- specifier such that when "%s" is substited by an instance of
- KeyedUnion.keyvar_name it becomes a C expression which evaluates to
- True IFF this field currently contains valid data.
+ containing type which determines the valid member of the union. The
+ member referenced by KeyedUnion.keyvar_name has type
+ KeyedUnion.keyvar_type which must be an instance of the Enumeration type.
 
 Standard Types
 --------------
diff -r 686d683a3a57 -r 3e9a884f93b7 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxl.idl	Mon Jul 18 14:52:30 2011 +0100
@@ -159,30 +159,28 @@ libxl_domain_build_info = Struct("domain
     ("disable_migrate", bool),
     ("cpuid",           libxl_cpuid_policy_list),
     ("type",            libxl_domain_type),
-    ("u", KeyedUnion(None, "type",
-                [("hvm", "%s == LIBXL_DOMAIN_TYPE_HVM", Struct(None,
-                                       [("firmware", string),
-                                        ("pae", bool),
-                                        ("apic", bool),
-                                        ("acpi", bool),
-                                        ("nx", bool),
-                                        ("viridian", bool),
-                                        ("timeoffset", string),
-                                        ("hpet", bool),
-                                        ("vpt_align", bool),
-                                        ("timer_mode", integer),
-                                        ("nested_hvm", bool),
-                                        ])),
-                 ("pv", "%s == LIBXL_DOMAIN_TYPE_PV", Struct(None,
-                                       [("kernel", libxl_file_reference),
-                                        ("slack_memkb", uint32),
-                                        ("bootloader", string),
-                                        ("bootloader_args", string),
-                                        ("cmdline", string),
-                                        ("ramdisk", libxl_file_reference),
-                                        ("features", string, True),
-                                        ("e820_host", bool, False, "Use host's E820 for PCI passthrough."),
-                                        ])),
+    ("u", KeyedUnion(None, libxl_domain_type, "type",
+                [("hvm", Struct(None, [("firmware", string),
+                                       ("pae", bool),
+                                       ("apic", bool),
+                                       ("acpi", bool),
+                                       ("nx", bool),
+                                       ("viridian", bool),
+                                       ("timeoffset", string),
+                                       ("hpet", bool),
+                                       ("vpt_align", bool),
+                                       ("timer_mode", integer),
+                                       ("nested_hvm", bool),
+                                       ])),
+                 ("pv", Struct(None, [("kernel", libxl_file_reference),
+                                      ("slack_memkb", uint32),
+                                      ("bootloader", string),
+                                      ("bootloader_args", string),
+                                      ("cmdline", string),
+                                      ("ramdisk", libxl_file_reference),
+                                      ("features", string, True),
+                                      ("e820_host", bool, False, "Use host's E820 for PCI passthrough."),
+                                      ])),
                  ])),
     ],
     comment =
diff -r 686d683a3a57 -r 3e9a884f93b7 tools/libxl/libxltypes.py
--- a/tools/libxl/libxltypes.py	Mon Jul 18 14:52:30 2011 +0100
+++ b/tools/libxl/libxltypes.py	Mon Jul 18 14:52:30 2011 +0100
@@ -116,6 +116,11 @@ class Enumeration(Type):
             self.values.append(EnumerationValue(self, num, name,
                                                 comment=comment,
                                                 typename=self.rawname))
+    def lookup(self, name):
+        for v in self.values:
+            if v.valuename == str.upper(name):
+                return v
+        return ValueError
         
 class Field(object):
     """An element of an Aggregate type"""
@@ -124,7 +129,7 @@ class Field(object):
         self.name = name
         self.const = kwargs.setdefault('const', False)
         self.comment = kwargs.setdefault('comment', None)
-        self.keyvar_expr = kwargs.setdefault('keyvar_expr', None)
+        self.enumname = kwargs.setdefault('enumname', None)
 
 class Aggregate(Type):
     """A type containing a collection of other types"""
@@ -181,18 +186,21 @@ class Union(Aggregate):
 
 class KeyedUnion(Aggregate):
     """A union which is keyed of another variable in the parent structure"""
-    def __init__(self, name, keyvar_name, fields, **kwargs):
+    def __init__(self, name, keyvar_type, keyvar_name, fields, **kwargs):
         Aggregate.__init__(self, "union", name, [], **kwargs)
 
+        if not isinstance(keyvar_type, Enumeration):
+            raise ValueError
+        
         self.keyvar_name = keyvar_name
+        self.keyvar_type = keyvar_type
 
         for f in fields:
-            # (name, keyvar_expr, type)
-
-            # keyvar_expr must contain exactly one %s which will be replaced with the keyvar_name
-
-            n, kve, ty = f
-            self.fields.append(Field(ty, n, keyvar_expr=kve))
+            # (name, enum, type)
+            e, ty = f
+            ev = keyvar_type.lookup(e)
+            en = ev.name
+            self.fields.append(Field(ty, e, enumname=en))
 
 #
 # Standard Types

Re: [PATCH 0 of 6 V2] libxl: IDL improvements

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 6 V2] libxl: IDL improvements"):
> Shave various yacks for Ian J ;-)
> 
> * LIBXL_DOMAIN_TYPE_FV -> LIBXL_DOMAIN_TYPE_HVM
> * Make more use of libxl_domain_type enum:
>   * replace libxl__domain_is_hvm with libxl__domain_type
>   * use in IDL in preference to 'bool hvm'
> * KeyedUnion keyvar must now be a Enumeration.
> 
> V2:
>   post a version which applies without needing my WIP JSON series.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks!

Ian.