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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Feb 20, 2020 at 02:31:03PM +0100, Paweł Marczewski wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxx>

With minor fix below:
Reviewed-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
> Changed since v1:
>   * use xswait mechanism to add a timeout
> 
> As mentioned before, this is to fix a race condition that appears when
> using libxl via libvirt and not using bootloader (console_available
> fires too early).
> 
> I have tested the patch on Qubes OS 4.1 (with Xen 4.13), and it seems
> to solve the problem. I also checked the timeout: when xenconsoled is
> stopped, libxl waits for 10 seconds and then aborts domain creation.
> 
>  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..4b150d92b9 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 = 10 * 1000;

Better not use explicit value _here_, but a constant in some header. I
think LIBXL_INIT_TIMEOUT is a good fit here.

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

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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