[libvirt] [PATCH] bhyve: add console support through nmdm device
Some additional info on nmdm devices that probably doesn't fit commit message. Man page: http://www.freebsd.org/cgi/man.cgi?query=nmdm&sektion=4 To use this device, we pass device name like '/dev/nmdmNA' to bhyve. Device is created automatically when bhyve accesses it. The pair device name is always '/dev/nmdmNB' which is used to provide console support. Even though this patch doesn't directly depend on virFDStreamOpenPTY() addition patch: https://www.redhat.com/archives/libvir-list/2014-March/msg00582.html it's not much useful without it because of mangled console output. Roman Bogorodskiy (1): bhyve: add console support through nmdm device src/bhyve/bhyve_command.c | 34 ++++++++++++++++++++++ src/bhyve/bhyve_driver.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_audit.c | 1 + src/conf/domain_conf.c | 12 +++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_monitor_json.c | 1 + 6 files changed, 117 insertions(+), 1 deletion(-) -- 1.8.4.2
nmdm is a FreeBSD driver which allows to create a pair of tty devices one of which is passed to the guest and second is used by the client. This patch adds new 'nmdm' character device type and implements domainOpenConsole() for bhyve driver based on that. --- src/bhyve/bhyve_command.c | 34 ++++++++++++++++++++++ src/bhyve/bhyve_driver.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_audit.c | 1 + src/conf/domain_conf.c | 12 +++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_monitor_json.c | 1 + 6 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index a19daac..b9df6d0 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -181,6 +181,38 @@ bhyveBuildNetArgStr(const virDomainDef *def, virCommandPtr cmd) } static int +bhyveBuildConsoleArgStr(const virDomainDef *def, virCommandPtr cmd) +{ + + virDomainChrDefPtr chr = NULL; + + if (!def->nserials) + return 0; + + chr = def->serials[0]; + + if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only nmdm console types are supported")); + return -1; + } + + /* bhyve supports only two ports: com1 and com2 */ + if (chr->target.port > 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only two serial ports are supported")); + return -1; + } + + virCommandAddArgList(cmd, "-s", "31,lpc", NULL); + virCommandAddArg(cmd, "-l"); + virCommandAddArgFormat(cmd, "com%d,%s", + chr->target.port + 1, chr->source.data.file.path); + + return 0; +} + +static int bhyveBuildDiskArgStr(const virDomainDef *def, virCommandPtr cmd) { virDomainDiskDefPtr disk; @@ -265,6 +297,8 @@ virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED, goto error; if (bhyveBuildDiskArgStr(vm->def, cmd) < 0) goto error; + if (bhyveBuildConsoleArgStr(vm->def, cmd) < 0) + goto error; virCommandAddArg(cmd, vm->def->name); return cmd; diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 9dbb299..4a564d1 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -22,6 +22,7 @@ #include <config.h> +#include <fcntl.h> #include <sys/utsname.h> #include "virerror.h" @@ -575,6 +576,73 @@ cleanup: } static int +bhyveDomainOpenConsole(virDomainPtr dom, + const char *dev_name ATTRIBUTE_UNUSED, + virStreamPtr st, + unsigned int flags) +{ + virDomainObjPtr vm = NULL; + virDomainChrDefPtr chr = NULL; + char *console_path; + char c; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = bhyveDomObjFromDomain(dom))) + 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; + } + + if (!vm->def->nserials) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("no console devices available")); + goto cleanup; + } + + chr = vm->def->serials[0]; + + if (VIR_STRDUP(console_path, chr->source.data.file.path) < 0) + goto cleanup; + + /* nmdm(4) uses the device pair with naming schema like that: + * + * nmdm0A, nmdm0B + * + * So if domain uses nmdm0A, we have to open nmdm0B and vice versa. + */ + c = chr->source.data.file.path[strlen(chr->source.data.file.path) - 1]; + if (c == 'A') + console_path[strlen(console_path) - 1] = 'B'; + else if (c == 'B') + console_path[strlen(console_path) - 1] = 'A'; + else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid name for nmdm device: %s"), + chr->source.data.file.path); + goto cleanup; + } + + if (virFDStreamOpenPTY(st, console_path, + 0, 0, O_RDWR) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnlock(vm); + VIR_FREE(console_path); + return ret; +} + +static int bhyveNodeGetCPUStats(virConnectPtr conn, int cpuNum, virNodeCPUStatsPtr params, @@ -701,6 +769,7 @@ static virDriver bhyveDriver = { .domainGetXMLDesc = bhyveDomainGetXMLDesc, /* 1.2.2 */ .domainIsActive = bhyveDomainIsActive, /* 1.2.2 */ .domainIsPersistent = bhyveDomainIsPersistent, /* 1.2.2 */ + .domainOpenConsole = bhyveDomainOpenConsole, /* 1.2.3 */ .nodeGetCPUStats = bhyveNodeGetCPUStats, /* 1.2.2 */ .nodeGetMemoryStats = bhyveNodeGetMemoryStats, /* 1.2.2 */ }; diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index b6564c2..461af11 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -70,6 +70,7 @@ virDomainAuditChardevPath(virDomainChrSourceDefPtr chr) case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_NMDM: return chr->data.file.path; case VIR_DOMAIN_CHR_TYPE_UNIX: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 92b1324..2397a7d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -440,7 +440,8 @@ VIR_ENUM_IMPL(virDomainChr, VIR_DOMAIN_CHR_TYPE_LAST, "tcp", "unix", "spicevmc", - "spiceport") + "spiceport", + "nmdm") VIR_ENUM_IMPL(virDomainChrTcpProtocol, VIR_DOMAIN_CHR_TCP_PROTOCOL_LAST, "raw", @@ -1514,6 +1515,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_NMDM: if (VIR_STRDUP(dest->data.file.path, src->data.file.path) < 0) return -1; break; @@ -7198,6 +7200,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_UNIX: + case VIR_DOMAIN_CHR_TYPE_NMDM: /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || @@ -7278,6 +7281,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_NMDM: if (!path && def->type != VIR_DOMAIN_CHR_TYPE_PTY) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -7458,6 +7462,11 @@ virDomainChrDefNew(void) { * <target port="1"/> * </serial> * + * <serial type="nmdm"> + * <source path="/dev/nmdm0A"/> + * <target port="1"> + * </serial> + * */ static virDomainChrDefPtr virDomainChrDefParseXML(xmlXPathContextPtr ctxt, @@ -15734,6 +15743,7 @@ virDomainChrSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_NMDM: if (def->type != VIR_DOMAIN_CHR_TYPE_PTY || (def->data.file.path && !(flags & VIR_DOMAIN_XML_INACTIVE))) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 37191a8..8b6081e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1107,6 +1107,7 @@ enum virDomainChrType { VIR_DOMAIN_CHR_TYPE_UNIX, VIR_DOMAIN_CHR_TYPE_SPICEVMC, VIR_DOMAIN_CHR_TYPE_SPICEPORT, + VIR_DOMAIN_CHR_TYPE_NMDM, VIR_DOMAIN_CHR_TYPE_LAST }; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index dbc04f6..bae8ff5 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5325,6 +5325,7 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, case VIR_DOMAIN_CHR_TYPE_SPICEPORT: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_NMDM: case VIR_DOMAIN_CHR_TYPE_LAST: virReportError(VIR_ERR_OPERATION_FAILED, _("Unsupported char device type '%d'"), -- 1.8.4.2
On Sat, Mar 15, 2014 at 04:30:01PM +0400, Roman Bogorodskiy wrote:
nmdm is a FreeBSD driver which allows to create a pair of tty devices one of which is passed to the guest and second is used by the client.
This patch adds new 'nmdm' character device type and implements domainOpenConsole() for bhyve driver based on that.
static int +bhyveBuildConsoleArgStr(const virDomainDef *def, virCommandPtr cmd) +{ + + virDomainChrDefPtr chr = NULL; + + if (!def->nserials) + return 0; + + chr = def->serials[0]; + + if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only nmdm console types are supported")); + return -1; + } + + /* bhyve supports only two ports: com1 and com2 */ + if (chr->target.port > 2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only two serial ports are supported")); + return -1; + } + + virCommandAddArgList(cmd, "-s", "31,lpc", NULL); + virCommandAddArg(cmd, "-l"); + virCommandAddArgFormat(cmd, "com%d,%s", + chr->target.port + 1, chr->source.data.file.path);
So building the argv, the path in the XML is given to the hypervisor
static int +bhyveDomainOpenConsole(virDomainPtr dom, + const char *dev_name ATTRIBUTE_UNUSED, + virStreamPtr st, + unsigned int flags) +{
[snip]
+ + /* nmdm(4) uses the device pair with naming schema like that: + * + * nmdm0A, nmdm0B + * + * So if domain uses nmdm0A, we have to open nmdm0B and vice versa. + */ + c = chr->source.data.file.path[strlen(chr->source.data.file.path) - 1]; + if (c == 'A') + console_path[strlen(console_path) - 1] = 'B'; + else if (c == 'B') + console_path[strlen(console_path) - 1] = 'A'; + else {
But client apps have to munge the path before opening it. I must say I'm still finding this a little wierd as an XML design. Looking at the type=pty case, the XML has no path, but is filled in with the path that the client can open - QEMU is just opening /dev/ptmx. If giving QEMU a real TTY, then the XML contains the path that QEMU itself should open and there's nothing a client can open since the client side is a real physical serial cable or linux virtual tty. So no real consistent approach there. Looking at this nmdm driver
+ * <serial type="nmdm"> + * <source path="/dev/nmdm0A"/> + * <target port="1"> + * </serial>
We could just go with the device prefix eg <source path='/dev/nmdm0'/> and let clients append 'B' before opening the device and append 'A' on bhyve command line. That feels like it is exposing the FreBSD private impl in the XML though. ie if some other OS implemented a similar nmdm concept, we can't guarantee they'd pick A & B as device names. So I wonder if we should just include both paths ? eg <source deva='/dev/nmdm0A' devb='/dev/nmdm0B'/> and declare that whatever is in 'deva' side is the hypervisor's dev and 'devb' is the client's path. The FreeBSD driver would sanity check to make sure both devices match up though. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
But client apps have to munge the path before opening it. I must say I'm still finding this a little wierd as an XML design.
Looking at the type=pty case, the XML has no path, but is filled in with the path that the client can open - QEMU is just opening /dev/ptmx. If giving QEMU a real TTY, then the XML contains the path that QEMU itself should open and there's nothing a client can open since the client side is a real physical serial cable or linux virtual tty. So no real consistent approach there.
I like the idea of openpty() on client and passing an fd like LXC does. I've submitted a patch to add support for that to bhyve: http://docs.freebsd.org/cgi/getmsg.cgi?fetch=384757+0+current/freebsd-virtua... But I don't know how long that will take to get landed. And then it'll also make some time to be MFC to -STABLE, so I guess it will not be available in a near future.
Looking at this nmdm driver
+ * <serial type="nmdm"> + * <source path="/dev/nmdm0A"/> + * <target port="1"> + * </serial>
We could just go with the device prefix eg
<source path='/dev/nmdm0'/>
and let clients append 'B' before opening the device and append 'A' on bhyve command line. That feels like it is exposing the FreBSD private impl in the XML though. ie if some other OS implemented a similar nmdm concept, we can't guarantee they'd pick A & B as device names.
Yeah. And it not only exposes internals, but looks strange from user's point of view: I guess it could be not obvious for a user that he needs to specify some device name that will never exist.
So I wonder if we should just include both paths ? eg
<source deva='/dev/nmdm0A' devb='/dev/nmdm0B'/>
and declare that whatever is in 'deva' side is the hypervisor's dev and 'devb' is the client's path.
The FreeBSD driver would sanity check to make sure both devices match up though.
That's the only way not to expose internals, I think. And actually this way of expressing devices doesn't require it to be nmdm at all, it could be pty devices created with 'socat -d -d pty,raw,echo=0 pty,raw,echo=0' (or some similar tool), but I'm not sure how to add a general validation that these devices match. Roman Bogorodskiy
participants (2)
-
Daniel P. Berrange -
Roman Bogorodskiy