diff mbox series

[XEN,v3] libxl: wait for console path before firing console_available

Message ID f9aa5afab28d3a8c9e581845030a0c971fa537a0.1583156916.git.pawel@invisiblethingslab.com (mailing list archive)
State Superseded
Headers show
Series [XEN,v3] libxl: wait for console path before firing console_available | expand

Commit Message

Paweł Marczewski March 2, 2020, 1:54 p.m. UTC 
If we skip the bootloader, the TTY path will be set for xenconsoled.
However, there is no guarantee that this will happen by the time we
want to call the console_available callback, so we have to wait.

Signed-off-by: Paweł Marczewski <pawel@invisiblethingslab.com>
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changed since v2:
  * replace hardcoded value with LIBXL_INIT_TIMEOUT
Changed since v1:
  * use xswait mechanism to add a timeout

 tools/libxl/libxl_create.c   | 43 +++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_internal.h |  1 +
 2 files changed, 41 insertions(+), 3 deletions(-)

Comments

Anthony PERARD March 2, 2020, 4:28 p.m. UTC | #1 
On Mon, Mar 02, 2020 at 02:54:08PM +0100, Paweł Marczewski wrote:
> @@ -1190,6 +1190,33 @@ static void domcreate_console_available(libxl__egc *egc,
>                                          dcs->aop_console_how.for_event));
>  }
>  
> +static void console_xswait_callback(libxl__egc *egc, libxl__xswait_state *xswa,
> +                                    int rc, const char *p)

That function needs to go after domcreate_attach_devices() (and before
domcreate_complete) in the source file. The argument for that is in the
CODING_STYLE, in "ASYNCHRONOUS/LONG-RUNNING OPERATIONS" section.

> +{
> +    EGC_GC;
> +    libxl__domain_create_state *dcs = CONTAINER_OF(xswa, *dcs, console_xswait);
> +    char *dompath = libxl__xs_get_dompath(gc, dcs->guest_domid);

You probably should check that dompath isn't NULL.
libxl__xs_get_dompath() might return it.

> +    char *tty_path = GCSPRINTF("%s/console/tty", dompath);
> +    char *tty;
> +
> +    if (rc) {
> +        if (rc == ERROR_TIMEDOUT)
> +            LOG(ERROR, "%s: timed out", xswa->what);
> +        libxl__xswait_stop(gc, xswa);
> +        domcreate_complete(egc, dcs, rc);
> +        return;
> +    }
> +
> +    tty = libxl__xs_read(gc, XBT_NULL, tty_path);

`tty_path' seems to be the same value as `console_xswait.path'
(xswa->path here) set in domcreate_attach_devices(). If that's the case,
there's no need to read it again, `p' should have the value.

> +
> +    if (tty && tty[0] != '\0') {
> +        libxl__xswait_stop(gc, xswa);
> +
> +        domcreate_console_available(egc, dcs);
> +        domcreate_complete(egc, dcs, 0);
> +    }

Could we have a single exit path out of this function?
I think that would be:
out:
    libxl__xswait_stop()
    domcreate_complete(egc, dcs, rc);

rc might be 0 on success.

> @@ -1728,9 +1755,18 @@ static void domcreate_attach_devices(libxl__egc *egc,
>          return;
>      }
>  
> -    domcreate_console_available(egc, dcs);
> -
> -    domcreate_complete(egc, dcs, 0);
> +    dcs->console_xswait.ao = ao;
> +    dcs->console_xswait.what = GCSPRINTF("domain %d console tty", domid);
> +    dcs->console_xswait.path = GCSPRINTF("%s/console/tty",
> +                                         libxl__xs_get_dompath(gc, domid));

Shouldn't you check that libxl__xs_get_dompath() actually returns
something? The function might return NULL.

Or even better, it seems that there's a function that generate that path
for you, could you have a look at libxl__console_tty_path() ? It's
probably what we need.

> @@ -1861,6 +1897,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
>  
>      libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how);
>      cdcs->domid_out = domid;
> +    libxl__xswait_init(&cdcs->dcs.console_xswait);

I think this initialisation needs to go in initiate_domain_create(),
because console_xswait is private to domain_create and that seems to be
the first function that uses the private parts.

Also, could you call libxl__xswait_stop() in domcreate_complete()?

Also, maybe the commit message should mention that if the path doesn't
become available, domain creation fail?

Thanks,
diff mbox series

Patch

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 3a7364e2ac..25ab6f6ff4 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1190,6 +1190,33 @@  static void domcreate_console_available(libxl__egc *egc,
                                         dcs->aop_console_how.for_event));
 }
 
+static void console_xswait_callback(libxl__egc *egc, libxl__xswait_state *xswa,
+                                    int rc, const char *p)
+{
+    EGC_GC;
+    libxl__domain_create_state *dcs = CONTAINER_OF(xswa, *dcs, console_xswait);
+    char *dompath = libxl__xs_get_dompath(gc, dcs->guest_domid);
+    char *tty_path = GCSPRINTF("%s/console/tty", dompath);
+    char *tty;
+
+    if (rc) {
+        if (rc == ERROR_TIMEDOUT)
+            LOG(ERROR, "%s: timed out", xswa->what);
+        libxl__xswait_stop(gc, xswa);
+        domcreate_complete(egc, dcs, rc);
+        return;
+    }
+
+    tty = libxl__xs_read(gc, XBT_NULL, tty_path);
+
+    if (tty && tty[0] != '\0') {
+        libxl__xswait_stop(gc, xswa);
+
+        domcreate_console_available(egc, dcs);
+        domcreate_complete(egc, dcs, 0);
+    }
+}
+
 static void domcreate_bootloader_done(libxl__egc *egc,
                                       libxl__bootloader_state *bl,
                                       int rc)
@@ -1728,9 +1755,18 @@  static void domcreate_attach_devices(libxl__egc *egc,
         return;
     }
 
-    domcreate_console_available(egc, dcs);
-
-    domcreate_complete(egc, dcs, 0);
+    dcs->console_xswait.ao = ao;
+    dcs->console_xswait.what = GCSPRINTF("domain %d console tty", domid);
+    dcs->console_xswait.path = GCSPRINTF("%s/console/tty",
+                                         libxl__xs_get_dompath(gc, domid));
+    dcs->console_xswait.timeout_ms = LIBXL_INIT_TIMEOUT * 1000;
+    dcs->console_xswait.callback = console_xswait_callback;
+    ret = libxl__xswait_start(gc, &dcs->console_xswait);
+    if (ret) {
+        LOG(ERROR, "unable to set up watch for domain %d console path",
+            domid);
+        goto error_out;
+    }
 
     return;
 
@@ -1861,6 +1897,7 @@  static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
 
     libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how);
     cdcs->domid_out = domid;
+    libxl__xswait_init(&cdcs->dcs.console_xswait);
 
     initiate_domain_create(egc, &cdcs->dcs);
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4936446069..d8129417dc 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4180,6 +4180,7 @@  struct libxl__domain_create_state {
     /* necessary if the domain creation failed and we have to destroy it */
     libxl__domain_destroy_state dds;
     libxl__multidev multidev;
+    libxl__xswait_state console_xswait;
 };
 
 _hidden int libxl__device_nic_set_devids(libxl__gc *gc,