[PATCH 20/20] libxl: convert console callback to libxl_asyncprogress_how

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

Remove the old console callback.  (Its reentrancy properties were
troublesome: in principle, the event loop might be reentered during
the callback, and the libxl__domain_create_state swept out from under
the feet of the domain creation.)

As a side effect of the new code arrangements, the console callback
for non-bootloader-using PV guests now occurs near the end of domain
creation, in the same place as for HVM guests, rather than near the
start.

The new arrangements should in principle mean that by the time the
console is described as ready by the callback, the xenstore key is
indeed ready.  So in the future the timeout might be removed from
the console client.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.h            |   17 ++++++-----
 tools/libxl/libxl_bootloader.c |    3 ++
 tools/libxl/libxl_create.c     |   59 +++++++++++++++++++++++-----------------
 tools/libxl/libxl_internal.h   |    6 +++-
 tools/libxl/libxl_types.idl    |    2 +
 tools/libxl/xl_cmdimpl.c       |   25 ++++++++++-------
 6 files changed, 67 insertions(+), 45 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1bfe684..de8d1872 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -509,18 +509,19 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
 int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
 
 /* domain related functions */
-typedef int (*libxl_console_ready)(libxl_ctx *ctx, uint32_t domid, void *priv);
-  /* fixme-ao   Need to review this API.  If we keep it, the reentrancy
-   * properties need to be documented but they may turn out to be too
-   * awkward */
 
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
-                            libxl_console_ready cb, void *priv, uint32_t *domid,
-                            const libxl_asyncop_how *ao_how);
+                            uint32_t *domid,
+                            const libxl_asyncop_how *ao_how,
+                            const libxl_asyncprogress_how *aop_console_how);
 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,
-                                const libxl_asyncop_how *ao_how);
+                                const libxl_asyncop_how *ao_how,
+                                const libxl_asyncprogress_how *aop_console_how);
+  /* A progress report will be made via ao_console_how, of type
+   * domain_create_console_available, when the domain's primary
+   * console is available and can be connected to.
+   */
 
 void libxl_domain_config_dispose(libxl_domain_config *d_config);
 int libxl_domain_suspend(libxl_ctx *ctx, libxl_domain_suspend_info *info,
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index eaec46a..3f6f129 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -375,6 +375,9 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
         goto out;
     }
 
+    if (bl->console_available)
+        bl->console_available(egc, bl);
+
     int bootloader_master = libxl__carefd_fd(bl->ptys[0].master);
     int xenconsole_master = libxl__carefd_fd(bl->ptys[1].master);
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 156c5c7..a457e09 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -550,10 +550,15 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc);
+static void domcreate_bootloader_console_available(libxl__egc *egc,
+                                                   libxl__bootloader_state *bl);
 static void domcreate_bootloader_done(libxl__egc *egc,
                                       libxl__bootloader_state *bl,
                                       int rc);
 
+static void domcreate_console_available(libxl__egc *egc,
+                                        libxl__domain_create_state *dcs);
+
 /* Our own function to clean up and call the user's callback.
  * The final call in the sequence. */
 static void domcreate_complete(libxl__egc *egc,
@@ -571,8 +576,6 @@ static void initiate_domain_create(libxl__egc *egc,
     /* convenience aliases */
     libxl_domain_config *d_config = dcs->guest_config;
     int restore_fd = dcs->restore_fd;
-    libxl_console_ready cb = dcs->console_cb;
-    void *priv = dcs->console_cb_priv;
     memset(&dcs->build_state, 0, sizeof(dcs->build_state));
 
     domid = 0;
@@ -590,11 +593,6 @@ static void initiate_domain_create(libxl__egc *egc,
     dcs->guest_domid = domid;
     dcs->dmss.guest_domid = 0; /* means we haven't spawned */
 
-    if ( d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV && cb ) {
-        ret = (*cb)(ctx, domid, priv);
-        if (ret) goto error_out;
-    }
-
     ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
     if (ret) goto error_out;
 
@@ -610,6 +608,7 @@ static void initiate_domain_create(libxl__egc *egc,
         dcs->bl.ao = ao;
         dcs->bl.disk = bootdisk;
         dcs->bl.callback = domcreate_bootloader_done;
+        dcs->bl.console_available = domcreate_bootloader_console_available;
         dcs->bl.info = &d_config->b_info,
             
         libxl__bootloader_run(egc, &dcs->bl);
@@ -623,6 +622,21 @@ error_out:
     domcreate_complete(egc, dcs, ret);
 }
 
+static void domcreate_bootloader_console_available(libxl__egc *egc,
+                                                   libxl__bootloader_state *bl)
+{
+    libxl__domain_create_state *dcs = CONTAINER_OF(bl, *dcs, bl);
+    STATE_AO_GC(bl->ao);
+    domcreate_console_available(egc, dcs);
+}
+
+static void domcreate_console_available(libxl__egc *egc,
+                                        libxl__domain_create_state *dcs) {
+    libxl__ao_progress_report(egc, dcs->ao, &dcs->aop_console_how,
+                              NEW_EVENT(egc, DOMAIN_CREATE_CONSOLE_AVAILABLE,
+                                        dcs->guest_domid));
+}
+
 static void domcreate_bootloader_done(libxl__egc *egc,
                                       libxl__bootloader_state *bl,
                                       int ret)
@@ -759,8 +773,6 @@ static void domcreate_devmodel_started(libxl__egc *egc,
 
     /* convenience aliases */
     libxl_domain_config *d_config = dcs->guest_config;
-    libxl_console_ready cb = dcs->console_cb;
-    void *priv = dcs->console_cb_priv;
 
     if (ret) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
@@ -798,12 +810,7 @@ static void domcreate_devmodel_started(libxl__egc *egc,
             goto error_out;
         }
     }
-    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 ))) {
-        ret = (*cb)(ctx, domid, priv);
-        if (ret) goto error_out;
-    }
+    domcreate_console_available(egc, dcs);
 
     domcreate_complete(egc, dcs, 0);
     return;
@@ -842,8 +849,9 @@ static void domain_create_cb(libxl__egc *egc,
                              int rc, uint32_t domid);
 
 static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
-                            libxl_console_ready cb, void *priv, uint32_t *domid,
-                            int restore_fd, const libxl_asyncop_how *ao_how)
+                            uint32_t *domid,
+                            int restore_fd, const libxl_asyncop_how *ao_how,
+                            const libxl_asyncprogress_how *aop_console_how)
 {
     AO_CREATE(ctx, 0, ao_how);
     libxl__app_domain_create_state *cdcs;
@@ -852,9 +860,8 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
     cdcs->dcs.ao = ao;
     cdcs->dcs.guest_config = d_config;
     cdcs->dcs.restore_fd = restore_fd;
-    cdcs->dcs.console_cb = cb;
-    cdcs->dcs.console_cb_priv = priv;
     cdcs->dcs.callback = domain_create_cb;
+    libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how);
     cdcs->domid_out = domid;
 
     initiate_domain_create(egc, &cdcs->dcs);
@@ -876,19 +883,21 @@ static void domain_create_cb(libxl__egc *egc,
 }
     
 int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
-                            libxl_console_ready cb, void *priv,
                             uint32_t *domid,
-                            const libxl_asyncop_how *ao_how)
+                            const libxl_asyncop_how *ao_how,
+                            const libxl_asyncprogress_how *aop_console_how)
 {
-    return do_domain_create(ctx, d_config, cb, priv, domid, -1, ao_how);
+    return do_domain_create(ctx, d_config, domid, -1,
+                            ao_how, aop_console_how);
 }
 
 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,
-                                const libxl_asyncop_how *ao_how)
+                                const libxl_asyncop_how *ao_how,
+                            const libxl_asyncprogress_how *aop_console_how)
 {
-    return do_domain_create(ctx, d_config, cb, priv, domid, restore_fd, ao_how);
+    return do_domain_create(ctx, d_config, domid, restore_fd,
+                            ao_how, aop_console_how);
 }
 
 /*
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 977db2a..b2eefd6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1651,11 +1651,14 @@ int libxl__openptys(libxl__openpty_state *op,
 typedef struct libxl__bootloader_state libxl__bootloader_state;
 typedef void libxl__run_bootloader_callback(libxl__egc*,
                                 libxl__bootloader_state*, int rc);
+typedef void libxl__bootloader_console_callback(libxl__egc*,
+                                libxl__bootloader_state*);
 
 struct libxl__bootloader_state {
     /* caller must fill these in, and they must all remain valid */
     libxl__ao *ao;
     libxl__run_bootloader_callback *callback;
+    libxl__bootloader_console_callback *console_available;
     libxl_domain_build_info *info; /* u.pv.{kernel,ramdisk,cmdline} updated */
     libxl_device_disk *disk;
     uint32_t domid;
@@ -1691,8 +1694,7 @@ struct libxl__domain_create_state {
     libxl__ao *ao;
     libxl_domain_config *guest_config;
     int restore_fd;
-    libxl_console_ready console_cb;
-    void *console_cb_priv;
+    libxl_asyncprogress_how aop_console_how;
     libxl__domain_create_cb *callback;
     /* private to domain_create */
     int guest_domid;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 5cf9708..f28d785 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -444,6 +444,7 @@ libxl_event_type = Enumeration("event_type", [
     (2, "DOMAIN_DEATH"),
     (3, "DISK_EJECT"),
     (4, "OPERATION_COMPLETE"),
+    (5, "DOMAIN_CREATE_CONSOLE_AVAILABLE"),
     ])
 
 libxl_ev_user = UInt(64)
@@ -469,4 +470,5 @@ libxl_event = Struct("event",[
            ("operation_complete", Struct(None, [
                                         ("rc", integer),
                                  ])),
+           ("domain_create_console_available", Struct(None, [])),
            ]))])
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9e66536..ce52599 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1430,16 +1430,18 @@ static int freemem(libxl_domain_build_info *b_info)
     return ERROR_NOMEM;
 }
 
-static int autoconnect_console(libxl_ctx *ctx, uint32_t domid, void *priv)
+static void autoconnect_console(libxl_ctx *ctx, libxl_event *ev, void *priv)
 {
     pid_t *pid = priv;
 
+    libxl_event_free(ctx, ev);
+
     *pid = fork();
     if (*pid < 0) {
         perror("unable to fork xenconsole");
-        return ERROR_FAIL;
+        return;
     } else if (*pid > 0)
-        return 0;
+        return;
 
     postfork();
 
@@ -1506,7 +1508,7 @@ static int create_domain(struct domain_create *dom_info)
     int config_len = 0;
     int restore_fd = -1;
     int status = 0;
-    libxl_console_ready cb;
+    const libxl_asyncprogress_how *autoconnect_console_how;
     pid_t child_console_pid = -1;
     struct save_file_header hdr;
 
@@ -1660,24 +1662,27 @@ start:
         goto error_out;
     }
 
+    libxl_asyncprogress_how autoconnect_console_how_buf;
     if ( dom_info->console_autoconnect ) {
-        cb = autoconnect_console;
+        autoconnect_console_how_buf.callback = autoconnect_console;
+        autoconnect_console_how_buf.for_callback = &child_console_pid;
+        autoconnect_console_how = &autoconnect_console_how_buf;
     }else{
-        cb = NULL;
+        autoconnect_console_how = 0;
     }
 
     if ( restore_file ) {
         ret = libxl_domain_create_restore(ctx, &d_config,
-                                          cb, &child_console_pid,
-                                          &domid, restore_fd, 0);
+                                          &domid, restore_fd,
+                                          0, autoconnect_console_how);
         /*
          * On subsequent reboot etc we should create the domain, not
          * restore/migrate-receive it again.
          */
         restore_file = NULL;
     }else{
-        ret = libxl_domain_create_new(ctx, &d_config,
-                                      cb, &child_console_pid, &domid, 0);
+        ret = libxl_domain_create_new(ctx, &d_config, &domid,
+                                      0, autoconnect_console_how);
     }
     if ( ret )
         goto error_out;
-- 
1.7.2.5