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@xxxxxxxx>
> ---
> 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",
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list