[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V4] add console support in libxl



Bamvor Jian Zhang wrote:
> this patch introduce the console api in libxl driver for both pv and
> hvm guest.  and import and update the libxlMakeChrdevStr function
> which was deleted in commit dfa1e1dd.
>
> Signed-off-by: Bamvor Jian Zhang <bjzhang suse com>
> ---
> Changes since V3:
> implicity forbit dev_name pass to libxl driver due to only one
> console supported by libxl.
>
> Changes since V2:
> 1), forbid parallel configure because libxl do not support it
> 2), only support one serial on libxl driver.
> 3), also remove console code in libxl driver, AFAICS serial is enough for
> connecting to libxl console.
>
> Changes since V1:
> 1), add virDomainOpenConsoleEnsureACL
> 3), remove virReportOOMErrorFull when virAsprintf fail.
> 4), change size_t for non-nagetive number in libxlDomainOpenConsole
>     size_t i;
>     size_t num = 0;
> 5), fix for make check
> (1), replace virAsprintf with VIR_STRDUP in two places
> (2), delete space.
>
>  src/libxl/libxl_conf.c   | 100 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_conf.h   |   3 ++
>  src/libxl/libxl_driver.c |  94 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 197 insertions(+)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4a0fba9..539d537 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -331,6 +331,90 @@ error:
>  }
>  
>  static int
> +libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
> +{
> +    const char *type = virDomainChrTypeToString(def->source.type);
> +    int ret;
> +
> +    if (!type) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>   

The error should be VIR_ERR_CONFIG_UNSUPPORTED.

> +                       "%s", _("unexpected chr device type"));
> +        return -1;
> +    }
> +
> +    switch (def->source.type) {
> +        case VIR_DOMAIN_CHR_TYPE_NULL:
> +        case VIR_DOMAIN_CHR_TYPE_STDIO:
> +        case VIR_DOMAIN_CHR_TYPE_VC:
> +        case VIR_DOMAIN_CHR_TYPE_PTY:
>   

Super nit: a majority of libvirt code has 'switch' and 'case' at same
indentation.  I realize there are inconsistencies even within this file,
but new code should follow the predominant style.

> +            if (VIR_STRDUP(*buf, type) < 0)
> +                return -1;
> +            break;
> +
> +        case VIR_DOMAIN_CHR_TYPE_FILE:
> +        case VIR_DOMAIN_CHR_TYPE_PIPE:
> +            if (virAsprintf(buf, "%s:%s", type,
> +                            def->source.data.file.path) < 0)
> +                return -1;
> +            break;
> +
> +        case VIR_DOMAIN_CHR_TYPE_DEV:
> +            if (VIR_STRDUP(*buf, def->source.data.file.path) < 0)
> +                return -1;
> +            break;
> +
> +        case VIR_DOMAIN_CHR_TYPE_UDP: {
> +            const char *connectHost = def->source.data.udp.connectHost;
> +            const char *bindHost = def->source.data.udp.bindHost;
> +            const char *bindService  = def->source.data.udp.bindService;
> +
> +            if (connectHost == NULL)
> +                connectHost = "";
> +            if (bindHost == NULL)
> +                bindHost = "";
> +            if (bindService == NULL)
> +                bindService = "0";
> +
> +            ret = virAsprintf(buf, "udp:%s:%s %s:%s",
> +                              connectHost,
> +                              def->source.data.udp.connectService,
> +                              bindHost,
> +                              bindService);
> +            if (ret < 0)
> +                return -1;
> +            break;
> +
> +        }
> +        case VIR_DOMAIN_CHR_TYPE_TCP:
> +            if (def->source.data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET)
>   

These long lines could be shortened by having a function-local
virDomainChrSourceDef variable.

> +                ret = virAsprintf(buf, "telnet:%s:%s%s",
> +                                  def->source.data.tcp.host,
> +                                  def->source.data.tcp.service,
> +                                  def->source.data.tcp.listen ? ",server,nowait" : "");
> +            else
> +                ret = virAsprintf(buf, "tcp:%s:%s%s",
> +                                  def->source.data.tcp.host,
> +                                  def->source.data.tcp.service,
> +                                  def->source.data.tcp.listen ? ",server,nowait" : "");
> +
> +            if (ret < 0)
>   

No need for 'ret' here.  See my attached diff, which contains a bit of
logic simplification here.

> +                return -1;
> +            break;
> +
> +        case VIR_DOMAIN_CHR_TYPE_UNIX:
> +            ret = virAsprintf(buf, "unix:%s%s",
> +                              def->source.data.nix.path,
> +                              def->source.data.nix.listen ? ",server,nowait" : "");
> +            if (ret < 0)
> +                return -1;
> +            break;
>   

There should be a default case to report error for unsupported types.

> +
> +    }
> +
> +    return 0;
> +}
> +
> +static int
>  libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>  {
>      libxl_domain_build_info *b_info = &d_config->b_info;
> @@ -403,6 +487,22 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>          if (VIR_STRDUP(b_info->u.hvm.boot, bootorder) < 0)
>              goto error;
>  
> +        if (def->nserials) {
> +            if (def->nserials > 1) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>   

VIR_ERR_CONFIG_UNSUPPORTED

> +                               "%s", _("Only one serial is supported by libxl"));
> +                goto error;
> +            }
> +            if (libxlMakeChrdevStr(def->serials[0], &b_info->u.hvm.serial) < 0)
> +                goto error;
> +        }
> +
> +        if (def->nparallels) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>   

VIR_ERR_CONFIG_UNSUPPORTED

> +                           "%s", _("Parallel is not supported"));
> +            goto error;
> +        }
> +
>          /*
>           * The following comment and calculation were taken directly from
>           * libxenlight's internal function libxl_get_required_shadow_memory():
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 2b4a281..861d689 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -34,6 +34,7 @@
>  # include "configmake.h"
>  # include "virportallocator.h"
>  # include "virobject.h"
> +# include "virchrdev.h"
>  
>  
>  # define LIBXL_VNC_PORT_MIN  5900
> @@ -94,6 +95,8 @@ struct _libxlDomainObjPrivate {
>  
>      /* per domain libxl ctx */
>      libxl_ctx *ctx;
> +    /* console */
> +    virChrdevsPtr devs;
>      libxl_evgen_domain_death *deathW;
>  
>      /* list of libxl timeout registrations */
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 358d387..9299484 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -417,6 +417,9 @@ libxlDomainObjPrivateAlloc(void)
>  
>      libxl_osevent_register_hooks(priv->ctx, &libxl_event_callbacks, priv);
>  
> +    if (!(priv->devs = virChrdevAlloc()))
> +        return NULL;
> +
>      return priv;
>  }
>  
> @@ -429,6 +432,7 @@ libxlDomainObjPrivateDispose(void *obj)
>          libxl_evdisable_domain_death(priv->ctx, priv->deathW);
>  
>      libxl_ctx_free(priv->ctx);
> +    virChrdevFree(priv->devs);
>   

IMO, freeing the libxl_ctx should be the last thing done in this function.

>  }
>  
>  static void
> @@ -4493,6 +4497,95 @@ cleanup:
>      return ret;
>  }
>  
> +
> +static int
> +libxlDomainOpenConsole(virDomainPtr dom,
> +                       const char *dev_name,
> +                       virStreamPtr st,
> +                       unsigned int flags)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +    virDomainChrDefPtr chr = NULL;
> +    libxlDomainObjPrivatePtr priv;
> +    char *console = NULL;
> +
> +    virCheckFlags(VIR_DOMAIN_CONSOLE_SAFE |
> +                  VIR_DOMAIN_CONSOLE_FORCE, -1);
>   

I verified that 'force' works, but what is 'safe' for?  I'm not quite
sure how that works in the qemu driver either.

> +
> +    if (dev_name) {
> +        /* XXX support device aliases in future */
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Named device aliases are not supported"));
> +        goto cleanup;
> +    }
> +
> +    libxlDriverLock(driver);
> +    vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
> +    libxlDriverUnlock(driver);
> +    if (!vm) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(dom->uuid, uuidstr);
> +        virReportError(VIR_ERR_NO_DOMAIN,
> +                       _("No domain with matching uuid '%s'"), uuidstr);
> +        goto cleanup;
> +    }
> +
> +    if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto cleanup;
> +    }
> +
> +    priv = vm->privateData;
> +
> +    if (vm->def->nserials)
> +        chr = vm->def->serials[0];
> +
> +    if (!chr) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("cannot find character device %s"),
> +                       NULLSTR(dev_name));
> +        goto cleanup;
> +    }
> +
> +    if (chr->source.type != VIR_DOMAIN_CHR_TYPE_PTY) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("character device %s is not using a PTY"),
> +                       NULLSTR(dev_name));
> +        goto cleanup;
> +    }
> +
> +    ret = libxl_primary_console_get_tty(priv->ctx, vm->def->id, &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,
> +                         st,
> +                         (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0);
>   

Whitespace is off a bit on these parameters.

> +
> +    if (ret == 1) {
> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                       _("Active console session exists for this domain"));
> +        ret = -1;
> +    }
> +
> +cleanup:
> +    VIR_FREE(console);
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}
> +
>  static int
>  libxlDomainSetSchedulerParameters(virDomainPtr dom, virTypedParameterPtr params,
>                                    int nparams)
> @@ -4875,6 +4968,7 @@ static virDriver libxlDriver = {
>      .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
>      .domainHasManagedSaveImage = libxlDomainHasManagedSaveImage, /* 0.9.2 */
>      .domainManagedSaveRemove = libxlDomainManagedSaveRemove, /* 0.9.2 */
> +    .domainOpenConsole = libxlDomainOpenConsole, /* 1.1.1 */
>   

We are now in 1.1.1 freeze, so this will have to wait for 1.1.2.

I've tested this patch quite a bit and haven't noticed any problems.  It
is a small patch that actually fills a big hole in the driver, so I'd
like to get it committed for 1.1.2.  Can you rebase, squash in my
changes, and post a V5?

Regards,
Jim


diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b1be91d..827dfdd 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -333,82 +333,84 @@ error:
 static int
 libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)
 {
-    const char *type = virDomainChrTypeToString(def->source.type);
-    int ret;
+    virDomainChrSourceDef srcdef = def->source;
+    const char *type = virDomainChrTypeToString(srcdef.type);
 
     if (!type) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "%s", _("unexpected chr device type"));
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       "%s", _("unknown chrdev type"));
         return -1;
     }
 
-    switch (def->source.type) {
-        case VIR_DOMAIN_CHR_TYPE_NULL:
-        case VIR_DOMAIN_CHR_TYPE_STDIO:
-        case VIR_DOMAIN_CHR_TYPE_VC:
-        case VIR_DOMAIN_CHR_TYPE_PTY:
-            if (VIR_STRDUP(*buf, type) < 0)
-                return -1;
-            break;
+    switch (srcdef.type) {
+    case VIR_DOMAIN_CHR_TYPE_NULL:
+    case VIR_DOMAIN_CHR_TYPE_STDIO:
+    case VIR_DOMAIN_CHR_TYPE_VC:
+    case VIR_DOMAIN_CHR_TYPE_PTY:
+        if (VIR_STRDUP(*buf, type) < 0)
+            return -1;
+        break;
 
-        case VIR_DOMAIN_CHR_TYPE_FILE:
-        case VIR_DOMAIN_CHR_TYPE_PIPE:
-            if (virAsprintf(buf, "%s:%s", type,
-                            def->source.data.file.path) < 0)
-                return -1;
-            break;
+    case VIR_DOMAIN_CHR_TYPE_FILE:
+    case VIR_DOMAIN_CHR_TYPE_PIPE:
+        if (virAsprintf(buf, "%s:%s", type, srcdef.data.file.path) < 0)
+            return -1;
+        break;
 
-        case VIR_DOMAIN_CHR_TYPE_DEV:
-            if (VIR_STRDUP(*buf, def->source.data.file.path) < 0)
-                return -1;
-            break;
+    case VIR_DOMAIN_CHR_TYPE_DEV:
+        if (VIR_STRDUP(*buf, srcdef.data.file.path) < 0)
+            return -1;
+        break;
+
+    case VIR_DOMAIN_CHR_TYPE_UDP: {
+        const char *connectHost = srcdef.data.udp.connectHost;
+        const char *bindHost = srcdef.data.udp.bindHost;
+        const char *bindService  = srcdef.data.udp.bindService;
+
+        if (connectHost == NULL)
+            connectHost = "";
+        if (bindHost == NULL)
+            bindHost = "";
+        if (bindService == NULL)
+            bindService = "0";
+
+        if (virAsprintf(buf, "udp:%s:%s %s:%s",
+                        connectHost,
+                        srcdef.data.udp.connectService,
+                        bindHost,
+                        bindService) < 0)
+            return -1;
+        break;
+    }
 
-        case VIR_DOMAIN_CHR_TYPE_UDP: {
-            const char *connectHost = def->source.data.udp.connectHost;
-            const char *bindHost = def->source.data.udp.bindHost;
-            const char *bindService  = def->source.data.udp.bindService;
-
-            if (connectHost == NULL)
-                connectHost = "";
-            if (bindHost == NULL)
-                bindHost = "";
-            if (bindService == NULL)
-                bindService = "0";
-
-            ret = virAsprintf(buf, "udp:%s:%s %s:%s",
-                              connectHost,
-                              def->source.data.udp.connectService,
-                              bindHost,
-                              bindService);
-            if (ret < 0)
-                return -1;
-            break;
+    case VIR_DOMAIN_CHR_TYPE_TCP: {
+        const char *prefix;
 
-        }
-        case VIR_DOMAIN_CHR_TYPE_TCP:
-            if (def->source.data.tcp.protocol ==
VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET)
-                ret = virAsprintf(buf, "telnet:%s:%s%s",
-                                  def->source.data.tcp.host,
-                                  def->source.data.tcp.service,
-                                  def->source.data.tcp.listen ?
",server,nowait" : "");
-            else
-                ret = virAsprintf(buf, "tcp:%s:%s%s",
-                                  def->source.data.tcp.host,
-                                  def->source.data.tcp.service,
-                                  def->source.data.tcp.listen ?
",server,nowait" : "");
-
-            if (ret < 0)
-                return -1;
-            break;
+        if (srcdef.data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET)
+            prefix = "telnet";
+        else
+            prefix = "tcp";
 
-        case VIR_DOMAIN_CHR_TYPE_UNIX:
-            ret = virAsprintf(buf, "unix:%s%s",
-                              def->source.data.nix.path,
-                              def->source.data.nix.listen ?
",server,nowait" : "");
-            if (ret < 0)
-                return -1;
-            break;
+        if (virAsprintf(buf, "%s:%s:%s%s",
+                        prefix,
+                        srcdef.data.tcp.host,
+                        srcdef.data.tcp.service,
+                        srcdef.data.tcp.listen ? ",server,nowait" : "")
< 0)
+            return -1;
+        break;
+    }
+
+    case VIR_DOMAIN_CHR_TYPE_UNIX:
+        if (virAsprintf(buf, "unix:%s%s",
+                        srcdef.data.nix.path,
+                        srcdef.data.nix.listen ? ",server,nowait" : "")
< 0)
+            return -1;
+        break;
 
+    default:
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unsupported chardev '%s'"), type);
+        return -1;
     }
 
     return 0;
@@ -497,8 +499,9 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm,
libxl_domain_config *d_config)
 
         if (def->nserials) {
             if (def->nserials > 1) {
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               "%s", _("Only one serial is supported by
libxl"));
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               "%s",
+                               _("Only one serial device is supported
by libxl"));
                 goto error;
             }
             if (libxlMakeChrdevStr(def->serials[0],
&b_info->u.hvm.serial) < 0)
@@ -506,8 +509,9 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm,
libxl_domain_config *d_config)
         }
 
         if (def->nparallels) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           "%s", _("Parallel is not supported"));
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           "%s",
+                           _("Parallel devices are not supported by
libxl"));
             goto error;
         }
 
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index ad1a5d1..4b603b1 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -431,8 +431,8 @@ libxlDomainObjPrivateDispose(void *obj)
     if (priv->deathW)
         libxl_evdisable_domain_death(priv->ctx, priv->deathW);
 
-    libxl_ctx_free(priv->ctx);
     virChrdevFree(priv->devs);
+    libxl_ctx_free(priv->ctx);
 }
 
 static void
@@ -4568,9 +4568,9 @@ libxlDomainOpenConsole(virDomainPtr dom,
 
     /* handle mutually exclusive access to console devices */
     ret = virChrdevOpen(priv->devs,
-                         &chr->source,
-                         st,
-                         (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0);
+                        &chr->source,
+                        st,
+                        (flags & VIR_DOMAIN_CONSOLE_FORCE) != 0);
 
     if (ret == 1) {
         virReportError(VIR_ERR_OPERATION_FAILED, "%s",


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]