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

Wednesday, 24 July 2013

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(a)suse.com&gt;
 ---
 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",


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

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