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

[Anthony PERARD <anthony.perard@citrix.com>]

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