[libvirt] [PATCH 0/5] qemu: support virtio console
The following series adds virtio console XML and qemu driver support. The first 3 patches are just cleanups/improvements. Patch 4 adds XML support for more than 1 console device (not strictly required for virtio console, but it's a valid use case). Patch 5 actually adds virtio console support. Thanks, Cole Cole Robinson (5): docs: domain: Document virtio <channel> domain conf: Rename character prop targetType -> deviceType domain conf: char: Add an explicit targetType field domain conf: Support multiple <console> devices qemu: virtio console support docs/formatdomain.html.in | 31 +++- docs/schemas/domain.rng | 14 ++- src/conf/domain_conf.c | 192 ++++++++++++-------- src/conf/domain_conf.h | 26 ++- src/esx/esx_vmx.c | 4 +- src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 8 +- src/qemu/qemu_conf.c | 48 +++++- src/qemu/qemu_driver.c | 3 +- src/uml/uml_conf.c | 4 +- src/uml/uml_driver.c | 6 +- src/vbox/vbox_tmpl.c | 4 +- src/xen/xend_internal.c | 16 ++- src/xen/xm_internal.c | 19 ++- .../qemuxml2argv-console-virtio.args | 1 + .../qemuxml2argv-console-virtio.xml | 30 +++ tests/qemuxml2argvtest.c | 2 + .../xml2sexprdata/xml2sexpr-pv-multi-console.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml | 24 +++ tests/xml2sexprtest.c | 1 + 20 files changed, 324 insertions(+), 111 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-multi-console.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/formatdomain.html.in | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4cca67f..b569811 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1158,6 +1158,11 @@ qemu-kvm -net nic,model=? /dev/null <source mode='bind' path='/tmp/guestfwd'/> <target type='guestfwd' address='10.0.2.1' port='4600'/> </channel> + + <!-- KVM virtio channel --> + <channel type='pty'> + <target type='virtio' name='arbitrary.virtio.serial.port.name'/> + </channel> </devices> ...</pre> @@ -1174,6 +1179,13 @@ qemu-kvm -net nic,model=? /dev/null forwarded to the channel device on the host. The <code>target</code> element must have <code>address</code> and <code>port</code> attributes. <span class="since">Since 0.7.3</span></dd> + + <dt><code>virtio</code></dt> + <dd>Paravirtualized virtio channel. Channel is exposed in the guest under + /dev/virtio-ports/$name (for more info, please see + <a href="http://fedoraproject.org/wiki/Features/VirtioSerial">http://fedoraproject.org/wiki/Features/VirtioSerial</a>) + The <code>target</code> element must have a <code>name</code> + attribute. <span class="since">Since 0.7.7</span></dd> </dl> <h5><a name="elementsCharHostInterface">Host interface</a></h5> -- 1.6.6.1
On Wed, Jul 14, 2010 at 03:44:52PM -0400, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/formatdomain.html.in | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4cca67f..b569811 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1158,6 +1158,11 @@ qemu-kvm -net nic,model=? /dev/null <source mode='bind' path='/tmp/guestfwd'/> <target type='guestfwd' address='10.0.2.1' port='4600'/> </channel> + + <!-- KVM virtio channel --> + <channel type='pty'> + <target type='virtio' name='arbitrary.virtio.serial.port.name'/> + </channel> </devices> ...</pre>
@@ -1174,6 +1179,13 @@ qemu-kvm -net nic,model=? /dev/null forwarded to the channel device on the host. The <code>target</code> element must have <code>address</code> and <code>port</code> attributes. <span class="since">Since 0.7.3</span></dd> + + <dt><code>virtio</code></dt> + <dd>Paravirtualized virtio channel. Channel is exposed in the guest under + /dev/virtio-ports/$name (for more info, please see + <a href="http://fedoraproject.org/wiki/Features/VirtioSerial">http://fedoraproject.org/wiki/Features/VirtioSerial</a>) + The <code>target</code> element must have a <code>name</code> + attribute. <span class="since">Since 0.7.7</span></dd> </dl>
<h5><a name="elementsCharHostInterface">Host interface</a></h5>
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
There is actually a difference between the character device type (serial, parallel, channel, ...) and the target type (virtio, guestfwd). Currently they are awkwardly conflated. Start to pull them apart by renaming targetType -> deviceType. This is an entirely mechanical change. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 74 +++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 24 +++++++------- src/esx/esx_vmx.c | 4 +- src/qemu/qemu_conf.c | 10 +++--- src/qemu/qemu_driver.c | 2 +- src/vbox/vbox_tmpl.c | 4 +- src/xen/xend_internal.c | 6 ++-- src/xen/xm_internal.c | 6 ++-- 8 files changed, 65 insertions(+), 65 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 378c06e..c9140fe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -161,7 +161,7 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "internal", "direct") -VIR_ENUM_IMPL(virDomainChrTarget, VIR_DOMAIN_CHR_TARGET_TYPE_LAST, +VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, "null", "monitor", "parallel", @@ -516,12 +516,12 @@ void virDomainChrDefFree(virDomainChrDefPtr def) if (!def) return; - switch (def->targetType) { - case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: + switch (def->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: VIR_FREE(def->target.addr); break; - case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: VIR_FREE(def->target.name); break; } @@ -2379,7 +2379,7 @@ virDomainChrDefParseXML(xmlNodePtr node, char *mode = NULL; char *protocol = NULL; const char *nodeName; - const char *targetType = NULL; + const char *deviceType = NULL; const char *addrStr = NULL; const char *portStr = NULL; virDomainChrDefPtr def; @@ -2396,7 +2396,7 @@ virDomainChrDefParseXML(xmlNodePtr node, def->type = VIR_DOMAIN_CHR_TYPE_NULL; nodeName = (const char *) node->name; - if ((def->targetType = virDomainChrTargetTypeFromString(nodeName)) < 0) { + if ((def->deviceType = virDomainChrDeviceTypeFromString(nodeName)) < 0) { /* channel is handled below */ if (STRNEQ(nodeName, "channel")) { virDomainReportError(VIR_ERR_XML_ERROR, @@ -2405,7 +2405,7 @@ virDomainChrDefParseXML(xmlNodePtr node, VIR_FREE(def); return NULL; } - def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_NULL; + def->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_NULL; } cur = node->children; @@ -2455,30 +2455,30 @@ virDomainChrDefParseXML(xmlNodePtr node, protocol = virXMLPropString(cur, "type"); } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { /* If target type isn't set yet, expect it to be set here */ - if (def->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_NULL) { - targetType = virXMLPropString(cur, "type"); - if (targetType == NULL) { + if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_NULL) { + deviceType = virXMLPropString(cur, "type"); + if (deviceType == NULL) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("character device target does " "not define a type")); goto error; } - if ((def->targetType = - virDomainChrTargetTypeFromString(targetType)) < 0) + if ((def->deviceType = + virDomainChrDeviceTypeFromString(deviceType)) < 0) { virDomainReportError(VIR_ERR_XML_ERROR, _("unknown target type for " "character device: %s"), - targetType); + deviceType); goto error; } } unsigned int port; - switch (def->targetType) { - case VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL: - case VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL: - case VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE: + switch (def->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: portStr = virXMLPropString(cur, "port"); if (portStr == NULL) { /* Not required. It will be assigned automatically @@ -2494,7 +2494,7 @@ virDomainChrDefParseXML(xmlNodePtr node, } break; - case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: + case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: addrStr = virXMLPropString(cur, "address"); portStr = virXMLPropString(cur, "port"); @@ -2538,14 +2538,14 @@ virDomainChrDefParseXML(xmlNodePtr node, virSocketSetPort(def->target.addr, port); break; - case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: def->target.name = virXMLPropString(cur, "name"); break; default: virDomainReportError(VIR_ERR_XML_ERROR, _("unexpected target type type %u"), - def->targetType); + def->deviceType); } } } @@ -2678,7 +2678,7 @@ cleanup: VIR_FREE(connectHost); VIR_FREE(connectService); VIR_FREE(path); - VIR_FREE(targetType); + VIR_FREE(deviceType); VIR_FREE(addrStr); VIR_FREE(portStr); @@ -4399,7 +4399,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } def->nserials = 1; def->serials[0] = chr; - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; } } else { def->console = chr; @@ -4422,7 +4422,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->channels[def->nchannels++] = chr; - if (chr->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO && + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO && chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; @@ -4891,7 +4891,7 @@ static int virDomainDefMaybeAddVirtioSerialController(virDomainDefPtr def) for (i = 0 ; i < def->nchannels ; i++) { virDomainChrDefPtr channel = def->channels[i]; - if (channel->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO) { + if (channel->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO) { int idx = 0; if (channel->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) idx = channel->info.addr.vioserial.controller; @@ -5473,15 +5473,15 @@ virDomainChrDefFormat(virBufferPtr buf, int flags) { const char *type = virDomainChrTypeToString(def->type); - const char *targetName = virDomainChrTargetTypeToString(def->targetType); + const char *targetName = virDomainChrDeviceTypeToString(def->deviceType); const char *elementName; int ret = 0; - switch (def->targetType) { + switch (def->deviceType) { /* channel types are in a common channel element */ - case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: - case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: + case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: elementName = "channel"; break; @@ -5498,7 +5498,7 @@ virDomainChrDefFormat(virBufferPtr buf, /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); - if (def->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE && + if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) && def->data.file.path) { @@ -5573,8 +5573,8 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - switch (def->targetType) { - case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: + switch (def->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: { int port = virSocketGetPort(def->target.addr); if (port < 0) { @@ -5595,7 +5595,7 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: virBufferAddLit(buf, " <target type='virtio'"); if (def->target.name) { virBufferEscapeString(buf, " name='%s'", def->target.name); @@ -5603,16 +5603,16 @@ virDomainChrDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); break; - case VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL: - case VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL: - case VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE: + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: virBufferVSprintf(buf, " <target port='%d'/>\n", def->target.port); break; default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected character destination type %d"), - def->targetType); + def->deviceType); return -1; } @@ -6219,7 +6219,7 @@ char *virDomainDefFormat(virDomainDefPtr def, * console */ virDomainChrDef console; memcpy(&console, def->serials[0], sizeof(console)); - console.targetType = VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE; + console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; if (virDomainChrDefFormat(&buf, &console, flags) < 0) goto cleanup; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 01da17e..b4e756a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -311,16 +311,16 @@ struct _virDomainNetDef { virNWFilterHashTablePtr filterparams; }; -enum virDomainChrTargetType { - VIR_DOMAIN_CHR_TARGET_TYPE_NULL = 0, - VIR_DOMAIN_CHR_TARGET_TYPE_MONITOR, - VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL, - VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL, - VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE, - VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD, - VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO, - - VIR_DOMAIN_CHR_TARGET_TYPE_LAST +enum virDomainChrDeviceType { + VIR_DOMAIN_CHR_DEVICE_TYPE_NULL = 0, + VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR, + VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL, + VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL, + VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE, + VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD, + VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO, + + VIR_DOMAIN_CHR_DEVICE_TYPE_LAST }; enum virDomainChrType { @@ -348,7 +348,7 @@ enum virDomainChrTcpProtocol { typedef struct _virDomainChrDef virDomainChrDef; typedef virDomainChrDef *virDomainChrDefPtr; struct _virDomainChrDef { - int targetType; + int deviceType; union { int port; /* parallel, serial, console */ virSocketAddrPtr addr; /* guestfwd */ @@ -1095,7 +1095,7 @@ VIR_ENUM_DECL(virDomainController) VIR_ENUM_DECL(virDomainControllerModel) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainNet) -VIR_ENUM_DECL(virDomainChrTarget) +VIR_ENUM_DECL(virDomainChrDevice) VIR_ENUM_DECL(virDomainChr) VIR_ENUM_DECL(virDomainSoundModel) VIR_ENUM_DECL(virDomainWatchdogModel) diff --git a/src/esx/esx_vmx.c b/src/esx/esx_vmx.c index e10e745..8480b6e 100644 --- a/src/esx/esx_vmx.c +++ b/src/esx/esx_vmx.c @@ -2159,7 +2159,7 @@ esxVMX_ParseSerial(esxVI_Context *ctx, virConfPtr conf, int port, return -1; } - (*def)->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; snprintf(prefix, sizeof(prefix), "serial%d", port); @@ -2289,7 +2289,7 @@ esxVMX_ParseParallel(esxVI_Context *ctx, virConfPtr conf, int port, return -1; } - (*def)->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + (*def)->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; snprintf(prefix, sizeof(prefix), "parallel%d", port); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 1d0bd88..1c4e934 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4382,8 +4382,8 @@ int qemudBuildCommandLine(virConnectPtr conn, virDomainChrDefPtr channel = def->channels[i]; char *devstr; - switch(channel->targetType) { - case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: + switch(channel->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: if (!(qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) || !(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { qemuReportError(VIR_ERR_NO_SUPPORT, @@ -4409,7 +4409,7 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG(devstr); break; - case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { qemuReportError(VIR_ERR_NO_SUPPORT, "%s", _("virtio channel requires QEMU to support -device")); @@ -6088,7 +6088,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, virDomainChrDefFree(chr); goto no_memory; } - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; chr->target.port = def->nserials; def->serials[def->nserials++] = chr; } @@ -6102,7 +6102,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, virDomainChrDefFree(chr); goto no_memory; } - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; chr->target.port = def->nparallels; def->parallels[def->nparallels++] = chr; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 487bfa3..0510124 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3343,7 +3343,7 @@ qemuPrepareMonitorChr(struct qemud_driver *driver, virDomainChrDefPtr monConfig, const char *vm) { - monConfig->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_MONITOR; + monConfig->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR; monConfig->type = VIR_DOMAIN_CHR_TYPE_UNIX; monConfig->data.nix.listen = 1; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 0e0013b..4892f2c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2839,7 +2839,7 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { def->serials[serialPortIncCount]->type = VIR_DOMAIN_CHR_TYPE_NULL; } - def->serials[serialPortIncCount]->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + def->serials[serialPortIncCount]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; serialPort->vtbl->GetIRQ(serialPort, &IRQ); serialPort->vtbl->GetIOBase(serialPort, &IOBase); @@ -2918,7 +2918,7 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { } def->parallels[parallelPortIncCount]->type = VIR_DOMAIN_CHR_TYPE_FILE; - def->parallels[parallelPortIncCount]->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + def->parallels[parallelPortIncCount]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; parallelPort->vtbl->GetPath(parallelPort, &pathUtf16); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index a22d32b..a492231 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2346,7 +2346,7 @@ xenDaemonParseSxpr(virConnectPtr conn, virDomainChrDefFree(chr); goto no_memory; } - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; def->serials[def->nserials++] = chr; } tmp = sexpr_node(root, "domain/image/hvm/parallel"); @@ -2359,14 +2359,14 @@ xenDaemonParseSxpr(virConnectPtr conn, virDomainChrDefFree(chr); goto no_memory; } - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; def->parallels[def->nparallels++] = chr; } } else { /* Fake a paravirt console, since that's not in the sexpr */ if (!(def->console = xenDaemonParseSxprChar("pty", tty))) goto error; - def->console->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE; + def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; } VIR_FREE(tty); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index a7a09a0..4230504 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1416,7 +1416,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { virDomainChrDefFree(chr); goto no_memory; } - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; def->parallels[0] = chr; def->nparallels++; chr = NULL; @@ -1433,14 +1433,14 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { virDomainChrDefFree(chr); goto no_memory; } - chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; def->serials[0] = chr; def->nserials++; } } else { if (!(def->console = xenDaemonParseSxprChar("pty", NULL))) goto cleanup; - def->console->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE; + def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; } if (hvm) { -- 1.6.6.1
On Wed, Jul 14, 2010 at 03:44:53PM -0400, Cole Robinson wrote:
There is actually a difference between the character device type (serial, parallel, channel, ...) and the target type (virtio, guestfwd). Currently they are awkwardly conflated.
Start to pull them apart by renaming targetType -> deviceType. This is an entirely mechanical change.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 74 +++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 24 +++++++------- src/esx/esx_vmx.c | 4 +- src/qemu/qemu_conf.c | 10 +++--- src/qemu/qemu_driver.c | 2 +- src/vbox/vbox_tmpl.c | 4 +- src/xen/xend_internal.c | 6 ++-- src/xen/xm_internal.c | 6 ++-- 8 files changed, 65 insertions(+), 65 deletions(-)
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
targetType only tracks the actual <target> format we are parsing. TYPE_DEFAULT is the typical serial/parallel format, NONE is for the <monitor> device which prints nothing. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 108 ++++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 19 ++++++-- src/qemu/qemu_conf.c | 6 +- 3 files changed, 72 insertions(+), 61 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c9140fe..e4d52ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -161,14 +161,18 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "internal", "direct") +VIR_ENUM_IMPL(virDomainChrTarget, VIR_DOMAIN_CHR_TARGET_TYPE_LAST, + "none", + "default", + "guestfwd", + "virtio") + VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, - "null", "monitor", "parallel", "serial", "console", - "guestfwd", - "virtio") + "channel") VIR_ENUM_IMPL(virDomainChr, VIR_DOMAIN_CHR_TYPE_LAST, "null", @@ -516,12 +520,12 @@ void virDomainChrDefFree(virDomainChrDefPtr def) if (!def) return; - switch (def->deviceType) { - case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: + switch (def->targetType) { + case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: VIR_FREE(def->target.addr); break; - case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: VIR_FREE(def->target.name); break; } @@ -2379,7 +2383,7 @@ virDomainChrDefParseXML(xmlNodePtr node, char *mode = NULL; char *protocol = NULL; const char *nodeName; - const char *deviceType = NULL; + const char *targetType = NULL; const char *addrStr = NULL; const char *portStr = NULL; virDomainChrDefPtr def; @@ -2397,15 +2401,9 @@ virDomainChrDefParseXML(xmlNodePtr node, nodeName = (const char *) node->name; if ((def->deviceType = virDomainChrDeviceTypeFromString(nodeName)) < 0) { - /* channel is handled below */ - if (STRNEQ(nodeName, "channel")) { - virDomainReportError(VIR_ERR_XML_ERROR, - _("unknown target type for character device: %s"), - nodeName); - VIR_FREE(def); - return NULL; - } - def->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_NULL; + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown character device type: %s"), + nodeName); } cur = node->children; @@ -2454,31 +2452,36 @@ virDomainChrDefParseXML(xmlNodePtr node, if (protocol == NULL) protocol = virXMLPropString(cur, "type"); } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { - /* If target type isn't set yet, expect it to be set here */ - if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_NULL) { - deviceType = virXMLPropString(cur, "type"); - if (deviceType == NULL) { + def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT; + + if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR) { + /* Don't target <target> info for monitor device */ + def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_NONE; + + } else if (def->deviceType == + VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { + targetType = virXMLPropString(cur, "type"); + if (targetType == NULL) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("character device target does " "not define a type")); goto error; } - if ((def->deviceType = - virDomainChrDeviceTypeFromString(deviceType)) < 0) + + if ((def->targetType = + virDomainChrTargetTypeFromString(targetType)) < 0) { virDomainReportError(VIR_ERR_XML_ERROR, _("unknown target type for " "character device: %s"), - deviceType); + targetType); goto error; } } unsigned int port; - switch (def->deviceType) { - case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: - case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: - case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + switch (def->targetType) { + case VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT: portStr = virXMLPropString(cur, "port"); if (portStr == NULL) { /* Not required. It will be assigned automatically @@ -2494,7 +2497,7 @@ virDomainChrDefParseXML(xmlNodePtr node, } break; - case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: + case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: addrStr = virXMLPropString(cur, "address"); portStr = virXMLPropString(cur, "port"); @@ -2538,14 +2541,17 @@ virDomainChrDefParseXML(xmlNodePtr node, virSocketSetPort(def->target.addr, port); break; - case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: def->target.name = virXMLPropString(cur, "name"); break; + case VIR_DOMAIN_CHR_TARGET_TYPE_NONE: + break; + default: virDomainReportError(VIR_ERR_XML_ERROR, - _("unexpected target type type %u"), - def->deviceType); + _("unexpected target type %u"), + def->targetType); } } } @@ -2678,7 +2684,7 @@ cleanup: VIR_FREE(connectHost); VIR_FREE(connectService); VIR_FREE(path); - VIR_FREE(deviceType); + VIR_FREE(targetType); VIR_FREE(addrStr); VIR_FREE(portStr); @@ -4422,7 +4428,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->channels[def->nchannels++] = chr; - if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO && + if (chr->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO && chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; @@ -4891,7 +4897,7 @@ static int virDomainDefMaybeAddVirtioSerialController(virDomainDefPtr def) for (i = 0 ; i < def->nchannels ; i++) { virDomainChrDefPtr channel = def->channels[i]; - if (channel->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO) { + if (channel->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO) { int idx = 0; if (channel->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) idx = channel->info.addr.vioserial.controller; @@ -5473,28 +5479,23 @@ virDomainChrDefFormat(virBufferPtr buf, int flags) { const char *type = virDomainChrTypeToString(def->type); - const char *targetName = virDomainChrDeviceTypeToString(def->deviceType); - const char *elementName; + const char *elementName = virDomainChrDeviceTypeToString(def->deviceType); int ret = 0; - switch (def->deviceType) { - /* channel types are in a common channel element */ - case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: - case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: - elementName = "channel"; - break; - - default: - elementName = targetName; - } - if (!type) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected char type %d"), def->type); return -1; } + if (!elementName) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected char device type %d"), + def->deviceType); + return -1; + } + /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); @@ -5573,8 +5574,8 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - switch (def->deviceType) { - case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: + switch (def->targetType) { + case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: { int port = virSocketGetPort(def->target.addr); if (port < 0) { @@ -5595,7 +5596,7 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: virBufferAddLit(buf, " <target type='virtio'"); if (def->target.name) { virBufferEscapeString(buf, " name='%s'", def->target.name); @@ -5603,12 +5604,13 @@ virDomainChrDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); break; - case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: - case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: - case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + case VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT: virBufferVSprintf(buf, " <target port='%d'/>\n", def->target.port); break; + case VIR_DOMAIN_CHR_TARGET_TYPE_NONE: + break; + default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected character destination type %d"), diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b4e756a..75dc29a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -312,15 +312,22 @@ struct _virDomainNetDef { }; enum virDomainChrDeviceType { - VIR_DOMAIN_CHR_DEVICE_TYPE_NULL = 0, - VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR, + VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR = 0, VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL, VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL, VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE, - VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD, - VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO, + VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL, - VIR_DOMAIN_CHR_DEVICE_TYPE_LAST + VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, +}; + +enum virDomainChrTargetType { + VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT = 0, + VIR_DOMAIN_CHR_TARGET_TYPE_NONE, + VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD, + VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO, + + VIR_DOMAIN_CHR_TARGET_TYPE_LAST, }; enum virDomainChrType { @@ -349,6 +356,7 @@ typedef struct _virDomainChrDef virDomainChrDef; typedef virDomainChrDef *virDomainChrDefPtr; struct _virDomainChrDef { int deviceType; + int targetType; union { int port; /* parallel, serial, console */ virSocketAddrPtr addr; /* guestfwd */ @@ -1096,6 +1104,7 @@ VIR_ENUM_DECL(virDomainControllerModel) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainNet) VIR_ENUM_DECL(virDomainChrDevice) +VIR_ENUM_DECL(virDomainChrTarget) VIR_ENUM_DECL(virDomainChr) VIR_ENUM_DECL(virDomainSoundModel) VIR_ENUM_DECL(virDomainWatchdogModel) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 1c4e934..62b4fb7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -4382,8 +4382,8 @@ int qemudBuildCommandLine(virConnectPtr conn, virDomainChrDefPtr channel = def->channels[i]; char *devstr; - switch(channel->deviceType) { - case VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD: + switch(channel->targetType) { + case VIR_DOMAIN_CHR_TARGET_TYPE_GUESTFWD: if (!(qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) || !(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { qemuReportError(VIR_ERR_NO_SUPPORT, @@ -4409,7 +4409,7 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG(devstr); break; - case VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO: + case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { qemuReportError(VIR_ERR_NO_SUPPORT, "%s", _("virtio channel requires QEMU to support -device")); -- 1.6.6.1
On Wed, Jul 14, 2010 at 03:44:54PM -0400, Cole Robinson wrote:
targetType only tracks the actual <target> format we are parsing. TYPE_DEFAULT is the typical serial/parallel format, NONE is for the <monitor> device which prints nothing.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 108 ++++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 19 ++++++-- src/qemu/qemu_conf.c | 6 +- 3 files changed, 72 insertions(+), 61 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c9140fe..e4d52ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -161,14 +161,18 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "internal", "direct")
+VIR_ENUM_IMPL(virDomainChrTarget, VIR_DOMAIN_CHR_TARGET_TYPE_LAST, + "none", + "default", + "guestfwd", + "virtio") + VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, - "null", "monitor", "parallel", "serial", "console", - "guestfwd", - "virtio") + "channel")
This is breaking backwards compatability AFAICT.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b4e756a..75dc29a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -312,15 +312,22 @@ struct _virDomainNetDef { };
enum virDomainChrDeviceType { - VIR_DOMAIN_CHR_DEVICE_TYPE_NULL = 0, - VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR, + VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR = 0, VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL, VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL, VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE, - VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD, - VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO, + VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL,
- VIR_DOMAIN_CHR_DEVICE_TYPE_LAST + VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, +};
This is breaking backwards compatability with existing XML configs surely. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/15/2010 05:31 AM, Daniel P. Berrange wrote:
On Wed, Jul 14, 2010 at 03:44:54PM -0400, Cole Robinson wrote:
targetType only tracks the actual <target> format we are parsing. TYPE_DEFAULT is the typical serial/parallel format, NONE is for the <monitor> device which prints nothing.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 108 ++++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 19 ++++++-- src/qemu/qemu_conf.c | 6 +- 3 files changed, 72 insertions(+), 61 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c9140fe..e4d52ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -161,14 +161,18 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "internal", "direct")
+VIR_ENUM_IMPL(virDomainChrTarget, VIR_DOMAIN_CHR_TARGET_TYPE_LAST, + "none", + "default", + "guestfwd", + "virtio") + VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, - "null", "monitor", "parallel", "serial", "console", - "guestfwd", - "virtio") + "channel")
This is breaking backwards compatability AFAICT.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b4e756a..75dc29a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -312,15 +312,22 @@ struct _virDomainNetDef { };
enum virDomainChrDeviceType { - VIR_DOMAIN_CHR_DEVICE_TYPE_NULL = 0, - VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR, + VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR = 0, VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL, VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL, VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE, - VIR_DOMAIN_CHR_DEVICE_TYPE_GUESTFWD, - VIR_DOMAIN_CHR_DEVICE_TYPE_VIRTIO, + VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL,
- VIR_DOMAIN_CHR_DEVICE_TYPE_LAST + VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, +};
This is breaking backwards compatability with existing XML configs surely.
Hmm, not sure if it is. Let me try to clarify a bit. The "null" here being removed above is actually a string value with no meaning in the XML. There is no <null type='pty'>... character device, and there is no <serial><target type='null'></serial> device. "null" here is just a place holder to enable having a deviceType value of VIR_DOMAIN_CHR_DEVICE_TYPE_NULL, which was used internally to work around the issues of combining device type serial/parallel/console/monitor/channel and target type guestfwd/virtio Since this patch attempts to separate those differences into two fields, we can drop the "null" piece: it was never actually being read from XML. Unfortunately the targetType field in this patch suffers from the same problem: there are 2 fields "none" and "default" which are never written into the XML and only used for internal implementation. That could be unwound in another patch, but I think it would require always generating an explicit <target type=''> in the XML. Thanks, Cole
On Wed, Jul 14, 2010 at 03:44:54PM -0400, Cole Robinson wrote:
targetType only tracks the actual <target> format we are parsing. TYPE_DEFAULT is the typical serial/parallel format, NONE is for the <monitor> device which prints nothing.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 108 ++++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 19 ++++++-- src/qemu/qemu_conf.c | 6 +- 3 files changed, 72 insertions(+), 61 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c9140fe..e4d52ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -161,14 +161,18 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "internal", "direct")
+VIR_ENUM_IMPL(virDomainChrTarget, VIR_DOMAIN_CHR_TARGET_TYPE_LAST, + "none", + "default", + "guestfwd", + "virtio")
Looking at this again, I think that this list needs to vary based on device type. eg serial: uart16550a, uml (usermode linux paravirt serial) channels: guestfwd, virtio, (possibly xen in future) consoles: xen, uml, serial, virtio otherwise we're allowing for combinations that don't really make sense like console+guestfwd. I don't think we should have a 'default' type either - we should set the explicit type for all based on the virtualization type. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Change console handling to a proper device list, as done for other character devices. Even though certain drivers can actually handle multiple consoles, for now just maintain existing behavior where possible. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 72 ++++++++++++------- src/conf/domain_conf.h | 5 +- src/lxc/lxc_driver.c | 8 +- src/uml/uml_conf.c | 4 +- src/uml/uml_driver.c | 6 +- src/xen/xend_internal.c | 12 +++- src/xen/xm_internal.c | 15 ++++- .../xml2sexprdata/xml2sexpr-pv-multi-console.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml | 24 +++++++ tests/xml2sexprtest.c | 1 + 10 files changed, 106 insertions(+), 42 deletions(-) create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-multi-console.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e4d52ff..0793ac0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -703,7 +703,9 @@ void virDomainDefFree(virDomainDefPtr def) virDomainChrDefFree(def->channels[i]); VIR_FREE(def->channels); - virDomainChrDefFree(def->console); + for (i = 0 ; i < def->nconsoles ; i++) + virDomainChrDefFree(def->consoles[i]); + VIR_FREE(def->consoles); for (i = 0 ; i < def->nsounds ; i++) virDomainSoundDefFree(def->sounds[i]); @@ -995,6 +997,9 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, for (i = 0; i < def->nchannels ; i++) if (cb(def, &def->channels[i]->info, opaque) < 0) return -1; + for (i = 0; i < def->nconsoles ; i++) + if (cb(def, &def->consoles[i]->info, opaque) < 0) + return -1; for (i = 0; i < def->ninputs ; i++) if (cb(def, &def->inputs[i]->info, opaque) < 0) return -1; @@ -1004,9 +1009,6 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, if (def->watchdog) if (cb(def, &def->watchdog->info, opaque) < 0) return -1; - if (def->console) - if (cb(def, &def->console->info, opaque) < 0) - return -1; return 0; } @@ -4384,31 +4386,45 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } VIR_FREE(nodes); - if ((node = virXPathNode("./devices/console[1]", ctxt)) != NULL) { - virDomainChrDefPtr chr = virDomainChrDefParseXML(node, + if ((n = virXPathNodeSet("./devices/console", ctxt, &nodes)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot extract console devices")); + goto error; + } + if (n && VIR_ALLOC_N(def->consoles, n) < 0) + goto no_memory; + + for (i = 0; i < n ; i++) { + virDomainChrDefPtr chr = virDomainChrDefParseXML(nodes[i], flags); if (!chr) goto error; - chr->target.port = 0; - /* - * For HVM console actually created a serial device - * while for non-HVM it was a parvirt console - */ - if (STREQ(def->os.type, "hvm")) { - if (def->nserials != 0) { - virDomainChrDefFree(chr); - } else { - if (VIR_ALLOC_N(def->serials, 1) < 0) { + chr->target.port = i; + + /* Back compat handling for the first console device */ + if (i == 0) { + /* + * For HVM console actually created a serial device + * while for non-HVM it was a parvirt console + */ + if (STREQ(def->os.type, "hvm")) { + if (def->nserials != 0) { virDomainChrDefFree(chr); - goto no_memory; + } else { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + def->nserials = 1; + def->serials[0] = chr; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; } - def->nserials = 1; - def->serials[0] = chr; - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + } else { + def->consoles[def->nconsoles++] = chr; } } else { - def->console = chr; + def->consoles[def->nconsoles++] = chr; } } @@ -5496,9 +5512,10 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; } - /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); + + /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) && @@ -6213,9 +6230,10 @@ char *virDomainDefFormat(virDomainDefPtr def, goto cleanup; /* If there's a PV console that's preferred.. */ - if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) - goto cleanup; + if (def->nconsoles) { + for (n = 0 ; n < def->nconsoles ; n++) + if (virDomainChrDefFormat(&buf, def->consoles[n], flags) < 0) + goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a * console */ @@ -7260,9 +7278,9 @@ int virDomainChrDefForeach(virDomainDefPtr def, if (abortOnError && rc != 0) goto done; } - if (def->console) { + for (i = 0 ; i < def->nconsoles ; i++) { if ((iter)(def, - def->console, + def->consoles[i], opaque) < 0) rc = -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 75dc29a..1361d38 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -875,8 +875,9 @@ struct _virDomainDef { int nchannels; virDomainChrDefPtr *channels; - /* Only 1 */ - virDomainChrDefPtr console; + int nconsoles; + virDomainChrDefPtr *consoles; + virSecurityLabelDef seclabel; virDomainWatchdogDefPtr watchdog; virCPUDefPtr cpu; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 462bc9c..9b0969e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1273,10 +1273,10 @@ static int lxcVmStart(virConnectPtr conn, _("Failed to allocate tty")); goto cleanup; } - if (vm->def->console && - vm->def->console->type == VIR_DOMAIN_CHR_TYPE_PTY) { - VIR_FREE(vm->def->console->data.file.path); - vm->def->console->data.file.path = parentTtyPath; + if (vm->def->nconsoles > 0 && + vm->def->consoles[0]->type == VIR_DOMAIN_CHR_TYPE_PTY) { + VIR_FREE(vm->def->consoles[0]->data.file.path); + vm->def->consoles[0]->data.file.path = parentTtyPath; } else { VIR_FREE(parentTtyPath); } diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 785d627..6715a94 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -500,8 +500,8 @@ int umlBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < UML_MAX_CHAR_DEVICE ; i++) { char *ret; - if (i == 0 && vm->def->console) - ret = umlBuildCommandLineChr(vm->def->console, "con"); + if (i == 0 && vm->def->nconsoles > 0) + ret = umlBuildCommandLineChr(vm->def->consoles[0], "con"); else if (virAsprintf(&ret, "con%d=none", i) < 0) goto no_memory; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 110179e..aa8cbef 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -231,10 +231,10 @@ umlIdentifyChrPTY(struct uml_driver *driver, { int i; - if (dom->def->console && - dom->def->console->type == VIR_DOMAIN_CHR_TYPE_PTY) + if (dom->def->nconsoles > 0 && + dom->def->consoles[0]->type == VIR_DOMAIN_CHR_TYPE_PTY) if (umlIdentifyOneChrPTY(driver, dom, - dom->def->console, "con") < 0) + dom->def->consoles[0], "con") < 0) return -1; for (i = 0 ; i < dom->def->nserials; i++) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index a492231..02383ca 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2364,9 +2364,17 @@ xenDaemonParseSxpr(virConnectPtr conn, } } else { /* Fake a paravirt console, since that's not in the sexpr */ - if (!(def->console = xenDaemonParseSxprChar("pty", tty))) + virDomainChrDefPtr chr; + + if ((chr = xenDaemonParseSxprChar("pty", tty)) == NULL) goto error; - def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + if (VIR_REALLOC_N(def->consoles, def->nconsoles+1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + def->consoles[def->nconsoles++] = chr; } VIR_FREE(tty); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 4230504..7756662 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1438,9 +1438,20 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { def->nserials++; } } else { - if (!(def->console = xenDaemonParseSxprChar("pty", NULL))) + virDomainChrDefPtr chr; + + if ((chr = xenDaemonParseSxprChar("pty", NULL)) == NULL) goto cleanup; - def->console->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + + if (chr) { + if (VIR_ALLOC_N(def->consoles, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + def->consoles[0] = chr; + def->nconsoles++; + } } if (hvm) { diff --git a/tests/xml2sexprdata/xml2sexpr-pv-multi-console.sexpr b/tests/xml2sexprdata/xml2sexpr-pv-multi-console.sexpr new file mode 100644 index 0000000..60db610 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-pv-multi-console.sexpr @@ -0,0 +1 @@ +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (linux (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... ')))(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))) \ No newline at end of file diff --git a/tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml b/tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml new file mode 100644 index 0000000..4f9c869 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-pv-multi-console.xml @@ -0,0 +1,24 @@ +<domain type='xen' id='15'> + <name>pvtest</name> + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> + <os> + <type>linux</type> + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> + <cmdline> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_... </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <console tty='/dev/pts/4'/> + <console tty='/dev/pts/5'/> + <console tty='/dev/pts/6'/> + </devices> +</domain> diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index 0455dc4..920ee29 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -113,6 +113,7 @@ mymain(int argc, char **argv) DO_TEST("pv-vfb-new", "pv-vfb-new", "pvtest", 3); DO_TEST("pv-vfb-new-auto", "pv-vfb-new-auto", "pvtest", 3); DO_TEST("pv-bootloader", "pv-bootloader", "pvtest", 1); + DO_TEST("pv-multi-console", "pv-multi-console", "pvtest", 2); DO_TEST("disk-file", "disk-file", "pvtest", 2); DO_TEST("disk-block", "disk-block", "pvtest", 2); -- 1.6.6.1
On Wed, Jul 14, 2010 at 03:44:55PM -0400, Cole Robinson wrote:
Change console handling to a proper device list, as done for other character devices. Even though certain drivers can actually handle multiple consoles, for now just maintain existing behavior where possible.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
[snip]
- chr->target.port = 0; - /* - * For HVM console actually created a serial device - * while for non-HVM it was a parvirt console - */ - if (STREQ(def->os.type, "hvm")) { - if (def->nserials != 0) { - virDomainChrDefFree(chr); - } else { - if (VIR_ALLOC_N(def->serials, 1) < 0) { + chr->target.port = i; + + /* Back compat handling for the first console device */ + if (i == 0) { + /* + * For HVM console actually created a serial device + * while for non-HVM it was a parvirt console + */ + if (STREQ(def->os.type, "hvm")) { + if (def->nserials != 0) { virDomainChrDefFree(chr); - goto no_memory; + } else { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + def->nserials = 1; + def->serials[0] = chr; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; } - def->nserials = 1; - def->serials[0] = chr; - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + } else { + def->consoles[def->nconsoles++] = chr; } } else { - def->console = chr; + def->consoles[def->nconsoles++] = chr; } }
This is where we get into a bit of a mess with back compatability, when combined with the later DefFormat code. The original requirement is that a <console> element generates a <serial> element for HVM guests. In the original code we throw away the virDomainChrDef for the console, since we re-generate it at format time if nconsoles==0. This hack can't work anymore with multiple consoles. Since if we have 2 consoles in the XML and throw away console 0, we have nconsoles==1 during DefFormat and thus won't re-generate the original console 0. To further complicate life, we don't want todo any of this <serial> compat code this at all if console 0 is a virtio console.
@@ -5496,9 +5512,10 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; }
- /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); + + /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) &&
Since we're allowing multiple <console> now, we should restrict this hack to just the first one.
@@ -6213,9 +6230,10 @@ char *virDomainDefFormat(virDomainDefPtr def, goto cleanup;
/* If there's a PV console that's preferred.. */ - if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) - goto cleanup; + if (def->nconsoles) { + for (n = 0 ; n < def->nconsoles ; n++) + if (virDomainChrDefFormat(&buf, def->consoles[n], flags) < 0) + goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a * console */
This logic can't work anymore. What I think we need todo is to remove the hacks in both the Parse and Format methods. Then add one single hack for the parse method which simply adds a matching <serial> device if nserial==0 and the console device type is serial. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/19/2010 11:42 AM, Daniel P. Berrange wrote:
On Wed, Jul 14, 2010 at 03:44:55PM -0400, Cole Robinson wrote:
Change console handling to a proper device list, as done for other character devices. Even though certain drivers can actually handle multiple consoles, for now just maintain existing behavior where possible.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
[snip]
- chr->target.port = 0; - /* - * For HVM console actually created a serial device - * while for non-HVM it was a parvirt console - */ - if (STREQ(def->os.type, "hvm")) { - if (def->nserials != 0) { - virDomainChrDefFree(chr); - } else { - if (VIR_ALLOC_N(def->serials, 1) < 0) { + chr->target.port = i; + + /* Back compat handling for the first console device */ + if (i == 0) { + /* + * For HVM console actually created a serial device + * while for non-HVM it was a parvirt console + */ + if (STREQ(def->os.type, "hvm")) { + if (def->nserials != 0) { virDomainChrDefFree(chr); - goto no_memory; + } else { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + def->nserials = 1; + def->serials[0] = chr; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; } - def->nserials = 1; - def->serials[0] = chr; - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + } else { + def->consoles[def->nconsoles++] = chr; } } else { - def->console = chr; + def->consoles[def->nconsoles++] = chr; } }
This is where we get into a bit of a mess with back compatability, when combined with the later DefFormat code.
The original requirement is that a <console> element generates a <serial> element for HVM guests. In the original code we throw away the virDomainChrDef for the console, since we re-generate it at format time if nconsoles==0. This hack can't work anymore with multiple consoles. Since if we have 2 consoles in the XML and throw away console 0, we have nconsoles==1 during DefFormat and thus won't re-generate the original console 0. To further complicate life, we don't want todo any of this <serial> compat code this at all if console 0 is a virtio console.
@@ -5496,9 +5512,10 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; }
- /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); + + /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) &&
Since we're allowing multiple <console> now, we should restrict this hack to just the first one.
@@ -6213,9 +6230,10 @@ char *virDomainDefFormat(virDomainDefPtr def, goto cleanup;
/* If there's a PV console that's preferred.. */ - if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) - goto cleanup; + if (def->nconsoles) { + for (n = 0 ; n < def->nconsoles ; n++) + if (virDomainChrDefFormat(&buf, def->consoles[n], flags) < 0) + goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a * console */
This logic can't work anymore.
What I think we need todo is to remove the hacks in both the Parse and Format methods. Then add one single hack for the parse method which simply adds a matching <serial> device if nserial==0 and the console device type is serial.
I poked at this for a while, and it's a big pain. Adding a single hack in the XML parse step isn't enough, because drivers like xen and esx build up a domain def manually in certain cases, so wouldn't gain the benefit of a hack in the parse function. The simplest way I thought of solving this would be to format XML from the xen/esx generated domain def, and run it through the XML parser to pick up the default device handling. But since that would be yet another hack, I just decided to drop this patch, since it isn't strictly required to get virtio console hooked up. Someone can revisit later if they want, my patch implementing the single hack in the parser is here: http://fedorapeople.org/~crobinso/libvirt/libvirt-multiple-consoles.patch Thanks, Cole
On Mon, Jul 26, 2010 at 03:11:36PM -0400, Cole Robinson wrote:
On 07/19/2010 11:42 AM, Daniel P. Berrange wrote:
On Wed, Jul 14, 2010 at 03:44:55PM -0400, Cole Robinson wrote:
Change console handling to a proper device list, as done for other character devices. Even though certain drivers can actually handle multiple consoles, for now just maintain existing behavior where possible.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
[snip]
- chr->target.port = 0; - /* - * For HVM console actually created a serial device - * while for non-HVM it was a parvirt console - */ - if (STREQ(def->os.type, "hvm")) { - if (def->nserials != 0) { - virDomainChrDefFree(chr); - } else { - if (VIR_ALLOC_N(def->serials, 1) < 0) { + chr->target.port = i; + + /* Back compat handling for the first console device */ + if (i == 0) { + /* + * For HVM console actually created a serial device + * while for non-HVM it was a parvirt console + */ + if (STREQ(def->os.type, "hvm")) { + if (def->nserials != 0) { virDomainChrDefFree(chr); - goto no_memory; + } else { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + def->nserials = 1; + def->serials[0] = chr; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; } - def->nserials = 1; - def->serials[0] = chr; - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + } else { + def->consoles[def->nconsoles++] = chr; } } else { - def->console = chr; + def->consoles[def->nconsoles++] = chr; } }
This is where we get into a bit of a mess with back compatability, when combined with the later DefFormat code.
The original requirement is that a <console> element generates a <serial> element for HVM guests. In the original code we throw away the virDomainChrDef for the console, since we re-generate it at format time if nconsoles==0. This hack can't work anymore with multiple consoles. Since if we have 2 consoles in the XML and throw away console 0, we have nconsoles==1 during DefFormat and thus won't re-generate the original console 0. To further complicate life, we don't want todo any of this <serial> compat code this at all if console 0 is a virtio console.
@@ -5496,9 +5512,10 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; }
- /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); + + /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) &&
Since we're allowing multiple <console> now, we should restrict this hack to just the first one.
@@ -6213,9 +6230,10 @@ char *virDomainDefFormat(virDomainDefPtr def, goto cleanup;
/* If there's a PV console that's preferred.. */ - if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) - goto cleanup; + if (def->nconsoles) { + for (n = 0 ; n < def->nconsoles ; n++) + if (virDomainChrDefFormat(&buf, def->consoles[n], flags) < 0) + goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a * console */
This logic can't work anymore.
What I think we need todo is to remove the hacks in both the Parse and Format methods. Then add one single hack for the parse method which simply adds a matching <serial> device if nserial==0 and the console device type is serial.
I poked at this for a while, and it's a big pain. Adding a single hack in the XML parse step isn't enough, because drivers like xen and esx build up a domain def manually in certain cases, so wouldn't gain the benefit of a hack in the parse function.
Can't we just manually add an equivalent hack in those drivers where necessary ? We used to that in Xen in the past at least. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/27/2010 06:07 AM, Daniel P. Berrange wrote:
On Mon, Jul 26, 2010 at 03:11:36PM -0400, Cole Robinson wrote:
On 07/19/2010 11:42 AM, Daniel P. Berrange wrote:
On Wed, Jul 14, 2010 at 03:44:55PM -0400, Cole Robinson wrote:
Change console handling to a proper device list, as done for other character devices. Even though certain drivers can actually handle multiple consoles, for now just maintain existing behavior where possible.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
[snip]
- chr->target.port = 0; - /* - * For HVM console actually created a serial device - * while for non-HVM it was a parvirt console - */ - if (STREQ(def->os.type, "hvm")) { - if (def->nserials != 0) { - virDomainChrDefFree(chr); - } else { - if (VIR_ALLOC_N(def->serials, 1) < 0) { + chr->target.port = i; + + /* Back compat handling for the first console device */ + if (i == 0) { + /* + * For HVM console actually created a serial device + * while for non-HVM it was a parvirt console + */ + if (STREQ(def->os.type, "hvm")) { + if (def->nserials != 0) { virDomainChrDefFree(chr); - goto no_memory; + } else { + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); + goto no_memory; + } + def->nserials = 1; + def->serials[0] = chr; + chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; } - def->nserials = 1; - def->serials[0] = chr; - chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; + } else { + def->consoles[def->nconsoles++] = chr; } } else { - def->console = chr; + def->consoles[def->nconsoles++] = chr; } }
This is where we get into a bit of a mess with back compatability, when combined with the later DefFormat code.
The original requirement is that a <console> element generates a <serial> element for HVM guests. In the original code we throw away the virDomainChrDef for the console, since we re-generate it at format time if nconsoles==0. This hack can't work anymore with multiple consoles. Since if we have 2 consoles in the XML and throw away console 0, we have nconsoles==1 during DefFormat and thus won't re-generate the original console 0. To further complicate life, we don't want todo any of this <serial> compat code this at all if console 0 is a virtio console.
@@ -5496,9 +5512,10 @@ virDomainChrDefFormat(virBufferPtr buf, return -1; }
- /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", elementName, type); + + /* Compat with legacy <console tty='/dev/pts/5'/> syntax */ if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) &&
Since we're allowing multiple <console> now, we should restrict this hack to just the first one.
@@ -6213,9 +6230,10 @@ char *virDomainDefFormat(virDomainDefPtr def, goto cleanup;
/* If there's a PV console that's preferred.. */ - if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) - goto cleanup; + if (def->nconsoles) { + for (n = 0 ; n < def->nconsoles ; n++) + if (virDomainChrDefFormat(&buf, def->consoles[n], flags) < 0) + goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a * console */
This logic can't work anymore.
What I think we need todo is to remove the hacks in both the Parse and Format methods. Then add one single hack for the parse method which simply adds a matching <serial> device if nserial==0 and the console device type is serial.
I poked at this for a while, and it's a big pain. Adding a single hack in the XML parse step isn't enough, because drivers like xen and esx build up a domain def manually in certain cases, so wouldn't gain the benefit of a hack in the parse function.
Can't we just manually add an equivalent hack in those drivers where necessary ? We used to that in Xen in the past at least.
Yeah that would work too, I didn't try it though. - Cole
Enable specifying a virtio console device with: <console type='pty'> <target type='virtio' name='arbitrary.virtio.serial.port.name'/> </console> Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/formatdomain.html.in | 19 ++++++- docs/schemas/domain.rng | 14 +++++- src/conf/domain_conf.c | 52 ++++++++++++++------ src/libvirt_private.syms | 1 + src/qemu/qemu_conf.c | 44 ++++++++++++++++- src/qemu/qemu_driver.c | 1 + .../qemuxml2argv-console-virtio.args | 1 + .../qemuxml2argv-console-virtio.xml | 30 +++++++++++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 145 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b569811..49ba76d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1124,8 +1124,17 @@ qemu-kvm -net nic,model=? /dev/null <p> This represents the primary console. This can be the paravirtualized - console with Xen guests, or duplicates the primary serial port for fully - virtualized guests without a paravirtualized console. + console with Xen guests, virtio console for QEMU/KVM, or duplicates + the primary serial port for fully virtualized guests without a + paravirtualized console. + </p> + + <p> + virtio console target information is specified in the same way as + the virtio channel device. The console device is exposed in the + guest as /dev/hvc[0-7] (for more information, see + <a href="http://fedoraproject.org/wiki/Features/VirtioSerial">http://fedoraproject.org/wiki/Features/VirtioSerial</a>) + <span class="since">Since 0.8.3</span> </p> <pre> @@ -1135,6 +1144,12 @@ qemu-kvm -net nic,model=? /dev/null <source path='/dev/pts/4'/> <target port='0'/> </console> + + <!-- KVM virtio console --> + <console type='pty'> + <source path='/dev/pts/5'/> + <target type='virtio' name='arbitrary.virtio.serial.port.name'/> + </console> </devices> ...</pre> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 1d56f5b..fcb4c23 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1267,7 +1267,19 @@ </optional> <empty/> </group> - <ref name="qemucdev"/> + <choice> + <ref name="qemucdev"/> + <group> + <ref name="qemucdevSrcType"/> + <interleave> + <ref name="qemucdevSrcDef"/> + <ref name="virtioTarget"/> + <optional> + <ref name="address"/> + </optional> + </interleave> + </group> + </choice> </choice> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0793ac0..dba7255 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2454,30 +2454,35 @@ virDomainChrDefParseXML(xmlNodePtr node, if (protocol == NULL) protocol = virXMLPropString(cur, "type"); } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { - def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT; + bool typeRequired = (def->deviceType == + VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL); + bool typeAccepted = (typeRequired || + (def->deviceType == + VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE)); + def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT; if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_MONITOR) { /* Don't target <target> info for monitor device */ def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_NONE; - } else if (def->deviceType == - VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { + } else if (typeAccepted) { targetType = virXMLPropString(cur, "type"); - if (targetType == NULL) { + if (targetType == NULL && typeRequired) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("character device target does " "not define a type")); goto error; } - if ((def->targetType = - virDomainChrTargetTypeFromString(targetType)) < 0) - { - virDomainReportError(VIR_ERR_XML_ERROR, - _("unknown target type for " - "character device: %s"), - targetType); - goto error; + if (targetType) { + if ((def->targetType = + virDomainChrTargetTypeFromString(targetType)) < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown target type for " + "character device: %s"), + targetType); + goto error; + } } } @@ -4400,7 +4405,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (!chr) goto error; - chr->target.port = i; + if (chr->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT) + chr->target.port = i; /* Back compat handling for the first console device */ if (i == 0) { @@ -4408,7 +4414,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, * For HVM console actually created a serial device * while for non-HVM it was a parvirt console */ - if (STREQ(def->os.type, "hvm")) { + if (STREQ(def->os.type, "hvm") && + chr->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT) { if (def->nserials != 0) { virDomainChrDefFree(chr); } else { @@ -4907,7 +4914,7 @@ static int virDomainDefAddDiskControllersForType(virDomainDefPtr def, static int virDomainDefMaybeAddVirtioSerialController(virDomainDefPtr def) { - /* Look for any virtio serial device */ + /* Look for any virtio serial or virtio console devs */ int i; for (i = 0 ; i < def->nchannels ; i++) { @@ -4924,6 +4931,21 @@ static int virDomainDefMaybeAddVirtioSerialController(virDomainDefPtr def) } } + for (i = 0 ; i < def->nconsoles ; i++) { + virDomainChrDefPtr console = def->consoles[i]; + + if (console->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO) { + int idx = 0; + if (console->info.type == + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL) + idx = console->info.addr.vioserial.controller; + + if (virDomainDefMaybeAddController(def, + VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx) < 0) + return -1; + } + } + return 0; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 778ceb1..a7a6c45 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -225,6 +225,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotAssignDef; virDomainObjAssignDef; virDomainChrDefForeach; +virDomainChrTargetTypeToString; # domain_event.h diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 62b4fb7..ba9ee00 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -2015,6 +2015,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, unsigned long long qemuCmdFlags) if (virAsprintf(&def->channels[i]->info.alias, "channel%d", i) < 0) goto no_memory; } + for (i = 0; i < def->nconsoles ; i++) { + if (virAsprintf(&def->consoles[i]->info.alias, "console%d", i) < 0) + goto no_memory; + } if (def->watchdog) { if (virAsprintf(&def->watchdog->info.alias, "watchdog%d", 0) < 0) goto no_memory; @@ -3237,7 +3241,10 @@ char * qemuBuildVirtioSerialPortDevStr(virDomainChrDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; - virBufferAddLit(&buf, "virtserialport"); + if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE) + virBufferAddLit(&buf, "virtconsole"); + else + virBufferAddLit(&buf, "virtserialport"); if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { /* Check it's a virtio-serial address */ @@ -4429,6 +4436,41 @@ int qemudBuildCommandLine(virConnectPtr conn, } } + /* Explicit console devices */ + for (i = 0 ; i < def->nconsoles ; i++) { + virDomainChrDefPtr console = def->consoles[i]; + char *devstr; + + switch(console->targetType) { + case VIR_DOMAIN_CHR_TARGET_TYPE_VIRTIO: + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { + qemuReportError(VIR_ERR_NO_SUPPORT, "%s", + _("virtio channel requires QEMU to support -device")); + goto error; + } + + ADD_ARG_LIT("-chardev"); + if (!(devstr = qemuBuildChrChardevStr(console))) + goto error; + ADD_ARG(devstr); + + ADD_ARG_LIT("-device"); + if (!(devstr = qemuBuildVirtioSerialPortDevStr(console))) + goto error; + ADD_ARG(devstr); + break; + + case VIR_DOMAIN_CHR_TARGET_TYPE_DEFAULT: + break; + + default: + qemuReportError(VIR_ERR_NO_SUPPORT, + _("unsupported console target type %s"), + NULLSTR(virDomainChrTargetTypeToString(console->targetType))); + goto error; + } + } + ADD_ARG_LIT("-usb"); for (i = 0 ; i < def->ninputs ; i++) { virDomainInputDefPtr input = def->inputs[i]; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0510124..4780277 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2027,6 +2027,7 @@ qemudFindCharDevicePTYsMonitor(virDomainObjPtr vm, LOOKUP_PTYS(vm->def->serials, vm->def->nserials, "serial"); LOOKUP_PTYS(vm->def->parallels, vm->def->nparallels, "parallel"); LOOKUP_PTYS(vm->def->channels, vm->def->nchannels, "channel"); + LOOKUP_PTYS(vm->def->consoles, vm->def->nconsoles, "console"); #undef LOOKUP_PTYS return 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio.args b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio.args new file mode 100644 index 0000000..b048648 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -hda /dev/HostVG/QEMUGuest1 -chardev pty,id=console0 -device virtconsole,chardev=console0,name=org.linux-kvm.virtio-console1 -chardev pty,id=console1 -device virtconsole,chardev=console1,name=org.linux-kvm.virtio-console2 -usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio.xml new file mode 100644 index 0000000..1b7efb4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <console type='pty'> + <target type='virtio' name='org.linux-kvm.virtio-console1'/> + </console> + <console type='pty'> + <target type='virtio' name='org.linux-kvm.virtio-console2'/> + </console> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ad1379b..45252dd 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -348,6 +348,8 @@ mymain(int argc, char **argv) QEMUD_CMD_FLAG_NODEFCONFIG); DO_TEST("channel-virtio-auto", QEMUD_CMD_FLAG_DEVICE | QEMUD_CMD_FLAG_NODEFCONFIG); + DO_TEST("console-virtio", QEMUD_CMD_FLAG_DEVICE | + QEMUD_CMD_FLAG_NODEFCONFIG); DO_TEST("watchdog", 0); DO_TEST("watchdog-device", QEMUD_CMD_FLAG_DEVICE | -- 1.6.6.1
On Wed, Jul 14, 2010 at 03:44:56PM -0400, Cole Robinson wrote:
Enable specifying a virtio console device with:
<console type='pty'> <target type='virtio' name='arbitrary.virtio.serial.port.name'/> </console>
What purpose does the 'name' field serve here ? It is used for virtio serial devices to format the /dev/virtio-serial/arbitrary.virtio.serial.port.name device node in the guest, but AFAIK, this isn't applicable to virtio console devices. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/19/2010 11:44 AM, Daniel P. Berrange wrote:
On Wed, Jul 14, 2010 at 03:44:56PM -0400, Cole Robinson wrote:
Enable specifying a virtio console device with:
<console type='pty'> <target type='virtio' name='arbitrary.virtio.serial.port.name'/> </console>
What purpose does the 'name' field serve here ? It is used for virtio serial devices to format the /dev/virtio-serial/arbitrary.virtio.serial.port.name device node in the guest, but AFAIK, this isn't applicable to virtio console devices.
This page indicates it is a valid option: http://fedoraproject.org/wiki/Features/VirtioSerial However I don't know what the value of it is, so I've dropped it. It can always be added later if someone wants it. Thanks, Cole
participants (2)
-
Cole Robinson -
Daniel P. Berrange