Re: [Xen-devel] [PATCH V3] libxl: Set path to console on domain startup.

[Jim Fehlig <jfehlig@xxxxxxxx>]

Anthony PERARD wrote:
> The path to the pty of a Xen PV console is set only in
> virDomainOpenConsole. But this is done too late. A call to
> virDomainGetXMLDesc done before OpenConsole will not have the path to
> the pty, but a call after OpenConsole will.
>
> e.g. of the current issue.
> Starting a domain with '<console type="pty"/>'
> Then:
> virDomainGetXMLDesc():
>   <devices>
>     <console type='pty'>
>       <target type='xen' port='0'/>
>     </console>
>   </devices>
> virDomainOpenConsole()
> virDomainGetXMLDesc():
>   <devices>
>     <console type='pty' tty='/dev/pts/30'>
>       <source path='/dev/pts/30'/>
>       <target type='xen' port='0'/>
>     </console>
>   </devices>
>
> The patch intend to have the TTY path on the first call of GetXMLDesc.
> This is done by setting up the path at domain start up instead of in
> OpenConsole.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1170743
>
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>
> ---
> Change in V3:
>   Using aop_console_how from libxl_domain_create_new()
>   Ignore empty string that can return libxl_console_get_tty()
>
> Change in V2:
>   Adding bug report link.
>   Reword the last part of the patch description.
>   Cleanup the code.
>   Use VIR_FREE before VIR_STRDUP.
>   Remove the code from OpenConsole as it is now a duplicate.
>
> CC: Jim Fehlig <jfehlig@xxxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> ---
>  src/libxl/libxl_domain.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  src/libxl/libxl_driver.c | 15 ---------------
>  2 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 9185117..804f9b9 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1149,6 +1149,42 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, 
> libxl_domain_config *d_config)
>      return ret;
>  }
>  
> +static void
> +libxlConsoleCallback(libxl_ctx *ctx, libxl_event* ev, void *for_callback)
> +{
> +    virDomainObjPtr vm = for_callback;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
> +    int i;
>   

Fails 'make syntax-check'

src/libxl/libxl_domain.c:1157:    int i;
maint.mk: use size_t, not int/unsigned int for loop vars i, j, k
make: *** [sc_prohibit_int_ijk] Error 1

> +
> +    virObjectLock(vm);
> +    for (i = 0; i < vm->def->nconsoles; i++) {
> +        virDomainChrDefPtr chr = vm->def->consoles[i];
> +        if (chr && chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
> +            libxl_console_type console_type;
> +            char *console = NULL;
> +            int ret;
> +
> +            console_type =
> +                (chr->targetType == 
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ?
> +                 LIBXL_CONSOLE_TYPE_SERIAL : LIBXL_CONSOLE_TYPE_PV);
> +            ret = libxl_console_get_tty(priv->ctx, ev->domid,
> +                                        chr->target.port, console_type,
> +                                        &console);
> +            if (!ret) {
> +                VIR_FREE(chr->source.data.file.path);
> +                if (console && console[0] != '\0') {
> +                    ignore_value(VIR_STRDUP(chr->source.data.file.path,
> +                                            console));
> +                }
> +            }
> +            VIR_FREE(console);
> +        }
> +    }
> +    virObjectUnlock(vm);
> +    libxl_event_free(ctx, ev);
> +}
> +
> +
>  /*
>   * Start a domain through libxenlight.
>   *
> @@ -1173,6 +1209,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
> virDomainObjPtr vm,
>      libxl_domain_restore_params params;
>  #endif
>      virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +    libxl_asyncprogress_how aop_console_how;
>  
>      libxl_domain_config_init(&d_config);
>  
> @@ -1242,17 +1279,21 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
> virDomainObjPtr vm,
>  
>      /* Unlock virDomainObj while creating the domain */
>      virObjectUnlock(vm);
> +
> +    aop_console_how.for_callback = vm;
> +    aop_console_how.callback = libxlConsoleCallback;
>      if (restore_fd < 0) {
>          ret = libxl_domain_create_new(priv->ctx, &d_config,
> -                                      &domid, NULL, NULL);
> +                                      &domid, NULL, &aop_console_how);
>      } else {
>  #ifdef LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS
>          params.checkpointed_stream = 0;
>          ret = libxl_domain_create_restore(priv->ctx, &d_config, &domid,
> -                                          restore_fd, &params, NULL, NULL);
> +                                          restore_fd, &params, NULL,
> +                                          &aop_console_how);
>  #else
>          ret = libxl_domain_create_restore(priv->ctx, &d_config, &domid,
> -                                          restore_fd, NULL, NULL);
> +                                          restore_fd, NULL, 
> &aop_console_how);
>  #endif
>      }
>      virObjectLock(vm);
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index cad5101..fc0949d 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3982,10 +3982,8 @@ libxlDomainOpenConsole(virDomainPtr dom,
>  {
>      virDomainObjPtr vm = NULL;
>      int ret = -1;
> -    libxl_console_type console_type;
>      virDomainChrDefPtr chr = NULL;
>      libxlDomainObjPrivatePtr priv;
> -    char *console = NULL;
>  
>      virCheckFlags(VIR_DOMAIN_CONSOLE_FORCE, -1);
>  
> @@ -4027,18 +4025,6 @@ libxlDomainOpenConsole(virDomainPtr dom,
>          goto cleanup;
>      }
>  
> -    console_type =
> -        (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ?
> -                            LIBXL_CONSOLE_TYPE_SERIAL : 
> LIBXL_CONSOLE_TYPE_PV);
> -
> -    ret = libxl_console_get_tty(priv->ctx, vm->def->id, chr->target.port,
> -                                console_type, &console);
> -    if (ret)
> -        goto cleanup;
> -
> -    if (VIR_STRDUP(chr->source.data.file.path, console) < 0)
> -        goto cleanup;
> -
>      /* handle mutually exclusive access to console devices */
>      ret = virChrdevOpen(priv->devs,
>                          &chr->source,
> @@ -4052,7 +4038,6 @@ libxlDomainOpenConsole(virDomainPtr dom,
>      }
>  
>   cleanup:
> -    VIR_FREE(console);
>      if (vm)
>          virObjectUnlock(vm);
>      return ret;
>   

Otherwise, looks good.  I fixed the 'make syntax-check' failure and
pushed.  Thanks!

Regards,
Jim

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel