[Xen-devel] [XEN PATCH v3] libxl: wait for console path before firing console_available

[Xen-devel] [XEN PATCH v3] libxl: wait for console path before firing console_available

[Paweł Marczewski]

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(-)

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,
-- 
2.21.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH v3] libxl: wait for console path before firing console_available

[Anthony PERARD]

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,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel