libxl: convert console callback to libxl_asyncprogress_how

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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Committed-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ba0f4dee4baeb6754665b4389924a7eda956a11a..979940a14b34ea1f01aabcc3b0c892df3148a660 100644 (file)
--- 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_init(libxl_domain_config *d_config);
 void libxl_domain_config_dispose(libxl_domain_config *d_config);

diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 1534baed51f9e70c5f82cbcdf772a6f2e95f9278..8436c07e2464cb0f113baf15ed8a5bfe06e7ff78 100644 (file)
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -377,6 +377,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 63cb430fa15d74d3fd5ffa935a263ec54a7b10c9..14721eb50ad4e4a4f74cbda15f27920d303650e0 100644 (file)
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -569,10 +569,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,
@@ -590,8 +595,6 @@ static void initiate_domain_create(libxl__egc *egc,
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
     const int restore_fd = dcs->restore_fd;
-    const libxl_console_ready cb = dcs->console_cb;
-    void *const priv = dcs->console_cb_priv;
     memset(&dcs->build_state, 0, sizeof(dcs->build_state));
 
     domid = 0;
@@ -610,11 +613,6 @@ static void initiate_domain_create(libxl__egc *egc,
     dcs->guest_domid = domid;
     dcs->dmss.dm.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;
 
@@ -629,6 +627,7 @@ static void initiate_domain_create(libxl__egc *egc,
 
     if (restore_fd < 0 && bootdisk) {
         dcs->bl.callback = domcreate_bootloader_done;
+        dcs->bl.console_available = domcreate_bootloader_console_available;
         dcs->bl.info = &d_config->b_info,
         dcs->bl.disk = bootdisk;
         dcs->bl.domid = dcs->guest_domid;
@@ -644,6 +643,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)
@@ -779,8 +793,6 @@ static void domcreate_devmodel_started(libxl__egc *egc,
 
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
-    const libxl_console_ready cb = dcs->console_cb;
-    void *const priv = dcs->console_cb_priv;
 
     if (ret) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
@@ -818,12 +830,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;
@@ -863,8 +870,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;
@@ -873,9 +881,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);
@@ -897,19 +904,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 a407317d3b0eb954d321bf4a3104833864eb2695..3aa0ed7f3061fa1baeca71f9032c640e2588076b 100644 (file)
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1699,11 +1699,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;
@@ -1739,9 +1742,8 @@ 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__domain_create_cb *callback;
+    libxl_asyncprogress_how aop_console_how;
     /* private to domain_create */
     int guest_domid;
     libxl__domain_build_state build_state;

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 68599cb17ea1b9c8ed1c173fc357b6783861d90a..551e367107856a5283f87f4bee219e8dadeaaa00 100644 (file)
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -459,6 +459,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)
@@ -484,4 +485,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 621aeaf9b3f83ccb40162e418d50efae2af36b7d..b91a2d5911e9b767c62c869d54b21c9300bdf1b9 100644 (file)
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1457,16 +1457,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();
 
@@ -1533,7 +1535,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;
 
@@ -1687,24 +1689,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;