On Wed, Nov 04, 2009 at 04:21:58PM +0000, Matthew Booth wrote:

Currently a character device's target (it's interface in the guest) has only a single property: port. This patch is in preparation for adding targets which require other properties.

Target properties are moved into a union in virDomainChrDef, and a targetType field is added to identify which union member should be used. All current code which touches a virDomainChrDef is updated both to use the new union field, and to populate targetType if necessary. --- src/conf/domain_conf.c | 66 +++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 18 +++++++++++- src/esx/esx_vmx.c | 56 +++++++++++++++++++++------------------ src/qemu/qemu_conf.c | 6 +++- src/qemu/qemu_driver.c | 2 + src/uml/uml_conf.c | 12 ++++---- src/uml/uml_driver.c | 2 +- src/vbox/vbox_tmpl.c | 22 ++++++++-------- src/xen/xend_internal.c | 3 ++ src/xen/xm_internal.c | 3 ++ 10 files changed, 129 insertions(+), 61 deletions(-)

ACK, looks good.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7dd3ce7..fc70cfd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -127,6 +127,13 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "bridge", "internal")

+VIR_ENUM_IMPL(virDomainChrTarget, VIR_DOMAIN_CHR_TARGET_TYPE_LAST, + "null", + "monitor", + "parallel", + "serial", + "console") + VIR_ENUM_IMPL(virDomainChr, VIR_DOMAIN_CHR_TYPE_LAST, "null", "vc", @@ -1325,6 +1332,7 @@ virDomainChrDefParseXML(virConnectPtr conn, char *path = NULL; char *mode = NULL; char *protocol = NULL; + const char *targetType = NULL; virDomainChrDefPtr def;

if (VIR_ALLOC(def) < 0) { @@ -1338,6 +1346,21 @@ virDomainChrDefParseXML(virConnectPtr conn, else if ((def->type = virDomainChrTypeFromString(type)) < 0) def->type = VIR_DOMAIN_CHR_TYPE_NULL;

+ targetType = (const char *) node->name; + if (targetType == NULL) { + /* Shouldn't be possible */ + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + "node->name is NULL at %s:%i", + __FILE__, __LINE__); + return NULL; + } + if ((def->targetType = virDomainChrTargetTypeFromString(targetType)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown target type for character device: %s"), + targetType); + def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_NULL; + } + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -2931,7 +2954,7 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, if (!chr) goto error;

- chr->dstPort = i; + chr->target.port = i; def->parallels[def->nparallels++] = chr; } VIR_FREE(nodes); @@ -2951,7 +2974,7 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, if (!chr) goto error;

- chr->dstPort = i; + chr->target.port = i; def->serials[def->nserials++] = chr; } VIR_FREE(nodes); @@ -2963,7 +2986,7 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn, if (!chr) goto error;

- chr->dstPort = 0; + chr->target.port = 0; /* * For HVM console actually created a serial device * while for non-HVM it was a parvirt console @@ -3965,10 +3988,12 @@ static int virDomainChrDefFormat(virConnectPtr conn, virBufferPtr buf, virDomainChrDefPtr def, - const char *name, int flags) { const char *type = virDomainChrTypeToString(def->type); + const char *targetName = virDomainChrTargetTypeToString(def->targetType); + + const char *elementName = targetName; /* Currently always the same */

if (!type) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -3978,8 +4003,8 @@ virDomainChrDefFormat(virConnectPtr conn,

/* Compat with legacy <console tty='/dev/pts/5'/> syntax */ virBufferVSprintf(buf, " <%s type='%s'", - name, type); - if (STREQ(name, "console") && + elementName, type); + if (def->targetType == VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE && def->type == VIR_DOMAIN_CHR_TYPE_PTY && !(flags & VIR_DOMAIN_XML_INACTIVE) && def->data.file.path) { @@ -4054,11 +4079,23 @@ virDomainChrDefFormat(virConnectPtr conn, break; }

- virBufferVSprintf(buf, " <target port='%d'/>\n", - def->dstPort); + switch (def->targetType) { + case VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL: + case VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL: + case VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE: + virBufferVSprintf(buf, " <target port='%d'/>\n", + def->target.port); + break; + + default: + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unexpected character destination type %d"), + def->targetType); + return -1; + }

virBufferVSprintf(buf, " </%s>\n", - name); + elementName);

return 0; } @@ -4505,21 +4542,24 @@ char *virDomainDefFormat(virConnectPtr conn, goto cleanup;

for (n = 0 ; n < def->nserials ; n++) - if (virDomainChrDefFormat(conn, &buf, def->serials[n], "serial", flags) < 0) + if (virDomainChrDefFormat(conn, &buf, def->serials[n], flags) < 0) goto cleanup;

for (n = 0 ; n < def->nparallels ; n++) - if (virDomainChrDefFormat(conn, &buf, def->parallels[n], "parallel", flags) < 0) + if (virDomainChrDefFormat(conn, &buf, def->parallels[n], flags) < 0) goto cleanup;

/* If there's a PV console that's preferred.. */ if (def->console) { - if (virDomainChrDefFormat(conn, &buf, def->console, "console", flags) < 0) + if (virDomainChrDefFormat(conn, &buf, def->console, flags) < 0) goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a * console */ - if (virDomainChrDefFormat(conn, &buf, def->serials[0], "console", flags) < 0) + virDomainChrDef console; + memcpy(&console, def->serials[0], sizeof(console)); + console.targetType = VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE; + if (virDomainChrDefFormat(conn, &buf, &console, flags) < 0) goto cleanup; }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8599ee7..7bd8c63 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -211,7 +211,17 @@ virNetHasValidPciAddr(virDomainNetDefPtr def) return def->pci_addr.domain || def->pci_addr.bus || def->pci_addr.slot; }

-enum virDomainChrSrcType { +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_LAST +}; + +enum virDomainChrType { VIR_DOMAIN_CHR_TYPE_NULL, VIR_DOMAIN_CHR_TYPE_VC, VIR_DOMAIN_CHR_TYPE_PTY, @@ -236,7 +246,10 @@ enum virDomainChrTcpProtocol { typedef struct _virDomainChrDef virDomainChrDef; typedef virDomainChrDef *virDomainChrDefPtr; struct _virDomainChrDef { - int dstPort; + int targetType; + union { + int port; /* parallel, serial, console */ + } target;

int type; union { @@ -812,6 +825,7 @@ VIR_ENUM_DECL(virDomainDiskBus) VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainNet) +VIR_ENUM_DECL(virDomainChrTarget) 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 a9ff1b4..536bf2d 100644 --- a/src/esx/esx_vmx.c +++ b/src/esx/esx_vmx.c @@ -319,7 +319,7 @@ def->nets[0]... serial0.startConnected = "true" # defaults to "true"

def->serials[0]... -->dstPort = <port> +->target.port = <port>

## serials: device ############################################################# @@ -378,7 +378,7 @@ def->serials[0]... parallel0.startConnected = "true" # defaults to "true"

def->parallels[0]... -->dstPort = <port> +->target.port = <port>

## parallels: device ############################################################# @@ -1871,6 +1871,8 @@ esxVMX_ParseSerial(virConnectPtr conn, esxVI_Context *ctx, virConfPtr conf, goto failure; }

+ (*def)->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + snprintf(prefix, sizeof(prefix), "serial%d", port);

ESX_BUILD_VMX_NAME(present); @@ -1907,13 +1909,13 @@ esxVMX_ParseSerial(virConnectPtr conn, esxVI_Context *ctx, virConfPtr conf,

/* Setup virDomainChrDef */ if (STRCASEEQ(fileType, "device")) { - (*def)->dstPort = port; + (*def)->target.port = port; (*def)->type = VIR_DOMAIN_CHR_TYPE_DEV; (*def)->data.file.path = fileName;

fileName = NULL; } else if (STRCASEEQ(fileType, "file")) { - (*def)->dstPort = port; + (*def)->target.port = port; (*def)->type = VIR_DOMAIN_CHR_TYPE_FILE; (*def)->data.file.path = esxVMX_ParseFileName(conn, ctx, fileName, datastoreName, @@ -1927,7 +1929,7 @@ esxVMX_ParseSerial(virConnectPtr conn, esxVI_Context *ctx, virConfPtr conf, * FIXME: Differences between client/server and VM/application pipes * not representable in domain XML form */ - (*def)->dstPort = port; + (*def)->target.port = port; (*def)->type = VIR_DOMAIN_CHR_TYPE_PIPE; (*def)->data.file.path = fileName;

@@ -1993,6 +1995,8 @@ esxVMX_ParseParallel(virConnectPtr conn, esxVI_Context *ctx, virConfPtr conf, goto failure; }

+ (*def)->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + snprintf(prefix, sizeof(prefix), "parallel%d", port);

ESX_BUILD_VMX_NAME(present); @@ -2029,13 +2033,13 @@ esxVMX_ParseParallel(virConnectPtr conn, esxVI_Context *ctx, virConfPtr conf,

/* Setup virDomainChrDef */ if (STRCASEEQ(fileType, "device")) { - (*def)->dstPort = port; + (*def)->target.port = port; (*def)->type = VIR_DOMAIN_CHR_TYPE_DEV; (*def)->data.file.path = fileName;

fileName = NULL; } else if (STRCASEEQ(fileType, "file")) { - (*def)->dstPort = port; + (*def)->target.port = port; (*def)->type = VIR_DOMAIN_CHR_TYPE_FILE; (*def)->data.file.path = esxVMX_ParseFileName(conn, ctx, fileName, datastoreName, @@ -2706,9 +2710,9 @@ esxVMX_FormatSerial(virConnectPtr conn, esxVI_Context *ctx, { char *fileName = NULL;

- if (def->dstPort < 0 || def->dstPort > 3) { + if (def->target.port < 0 || def->target.port > 3) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, - "Serial port index %d out of [0..3] range", def->dstPort); + "Serial port index %d out of [0..3] range", def->target.port); return -1; }

@@ -2719,20 +2723,20 @@ esxVMX_FormatSerial(virConnectPtr conn, esxVI_Context *ctx, return -1; }

- virBufferVSprintf(buffer, "serial%d.present = \"true\"\n", def->dstPort); + virBufferVSprintf(buffer, "serial%d.present = \"true\"\n", def->target.port);

/* def:type -> vmx:fileType and def:data.file.path -> vmx:fileName */ switch (def->type) { case VIR_DOMAIN_CHR_TYPE_DEV: virBufferVSprintf(buffer, "serial%d.fileType = \"device\"\n", - def->dstPort); + def->target.port); virBufferVSprintf(buffer, "serial%d.fileName = \"%s\"\n", - def->dstPort, def->data.file.path); + def->target.port, def->data.file.path); break;

case VIR_DOMAIN_CHR_TYPE_FILE: virBufferVSprintf(buffer, "serial%d.fileType = \"file\"\n", - def->dstPort); + def->target.port);

fileName = esxVMX_FormatFileName(conn, ctx, def->data.file.path);

@@ -2741,22 +2745,22 @@ esxVMX_FormatSerial(virConnectPtr conn, esxVI_Context *ctx, }

virBufferVSprintf(buffer, "serial%d.fileName = \"%s\"\n", - def->dstPort, fileName); + def->target.port, fileName);

VIR_FREE(fileName); break;

case VIR_DOMAIN_CHR_TYPE_PIPE: virBufferVSprintf(buffer, "serial%d.fileType = \"pipe\"\n", - def->dstPort); + def->target.port); /* FIXME: Based on VI Client GUI default */ virBufferVSprintf(buffer, "serial%d.pipe.endPoint = \"client\"\n", - def->dstPort); + def->target.port); /* FIXME: Based on VI Client GUI default */ virBufferVSprintf(buffer, "serial%d.tryNoRxLoss = \"false\"\n", - def->dstPort); + def->target.port); virBufferVSprintf(buffer, "serial%d.fileName = \"%s\"\n", - def->dstPort, def->data.file.path); + def->target.port, def->data.file.path); break;

default: @@ -2769,7 +2773,7 @@ esxVMX_FormatSerial(virConnectPtr conn, esxVI_Context *ctx, /* vmx:yieldOnMsrRead */ /* FIXME: Based on VI Client GUI default */ virBufferVSprintf(buffer, "serial%d.yieldOnMsrRead = \"true\"\n", - def->dstPort); + def->target.port);

return 0; } @@ -2782,9 +2786,9 @@ esxVMX_FormatParallel(virConnectPtr conn, esxVI_Context *ctx, { char *fileName = NULL;

- if (def->dstPort < 0 || def->dstPort > 2) { + if (def->target.port < 0 || def->target.port > 2) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, - "Parallel port index %d out of [0..2] range", def->dstPort); + "Parallel port index %d out of [0..2] range", def->target.port); return -1; }

@@ -2795,20 +2799,20 @@ esxVMX_FormatParallel(virConnectPtr conn, esxVI_Context *ctx, return -1; }

- virBufferVSprintf(buffer, "parallel%d.present = \"true\"\n", def->dstPort); + virBufferVSprintf(buffer, "parallel%d.present = \"true\"\n", def->target.port);

/* def:type -> vmx:fileType and def:data.file.path -> vmx:fileName */ switch (def->type) { case VIR_DOMAIN_CHR_TYPE_DEV: virBufferVSprintf(buffer, "parallel%d.fileType = \"device\"\n", - def->dstPort); + def->target.port); virBufferVSprintf(buffer, "parallel%d.fileName = \"%s\"\n", - def->dstPort, def->data.file.path); + def->target.port, def->data.file.path); break;

case VIR_DOMAIN_CHR_TYPE_FILE: virBufferVSprintf(buffer, "parallel%d.fileType = \"file\"\n", - def->dstPort); + def->target.port);

fileName = esxVMX_FormatFileName(conn, ctx, def->data.file.path);

@@ -2817,7 +2821,7 @@ esxVMX_FormatParallel(virConnectPtr conn, esxVI_Context *ctx, }

virBufferVSprintf(buffer, "parallel%d.fileName = \"%s\"\n", - def->dstPort, fileName); + def->target.port, fileName);

VIR_FREE(fileName); break; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 810e7bf..19b2d36 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -3406,7 +3406,8 @@ virDomainDefPtr qemuParseCommandLine(virConnectPtr conn, virDomainChrDefFree(chr); goto no_memory; } - chr->dstPort = def->nserials; + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; + chr->target.port = def->nserials; def->serials[def->nserials++] = chr; } } else if (STREQ(arg, "-parallel")) { @@ -3419,7 +3420,8 @@ virDomainDefPtr qemuParseCommandLine(virConnectPtr conn, virDomainChrDefFree(chr); goto no_memory; } - chr->dstPort = def->nparallels; + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; + chr->target.port = def->nparallels; def->parallels[def->nparallels++] = chr; } } else if (STREQ(arg, "-usbdevice")) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0ed2136..a6b6e93 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1920,6 +1920,8 @@ qemuPrepareMonitorChr(virConnectPtr conn, virDomainChrDefPtr monitor_chr, const char *vm) { + monitor_chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_MONITOR; + monitor_chr->type = VIR_DOMAIN_CHR_TYPE_UNIX; monitor_chr->data.nix.listen = 1;

diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 1c1db61..0ace58f 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -279,21 +279,21 @@ umlBuildCommandLineChr(virConnectPtr conn,

switch (def->type) { case VIR_DOMAIN_CHR_TYPE_NULL: - if (virAsprintf(&ret, "%s%d=null", dev, def->dstPort) < 0) { + if (virAsprintf(&ret, "%s%d=null", dev, def->target.port) < 0) { virReportOOMError(conn); return NULL; } break;

case VIR_DOMAIN_CHR_TYPE_PTY: - if (virAsprintf(&ret, "%s%d=pts", dev, def->dstPort) < 0) { + if (virAsprintf(&ret, "%s%d=pts", dev, def->target.port) < 0) { virReportOOMError(conn); return NULL; } break;

case VIR_DOMAIN_CHR_TYPE_DEV: - if (virAsprintf(&ret, "%s%d=tty:%s", dev, def->dstPort, + if (virAsprintf(&ret, "%s%d=tty:%s", dev, def->target.port, def->data.file.path) < 0) { virReportOOMError(conn); return NULL; @@ -301,7 +301,7 @@ umlBuildCommandLineChr(virConnectPtr conn, break;

case VIR_DOMAIN_CHR_TYPE_STDIO: - if (virAsprintf(&ret, "%s%d=fd:0,fd:1", dev, def->dstPort) < 0) { + if (virAsprintf(&ret, "%s%d=fd:0,fd:1", dev, def->target.port) < 0) { virReportOOMError(conn); return NULL; } @@ -314,7 +314,7 @@ umlBuildCommandLineChr(virConnectPtr conn, return NULL; }

- if (virAsprintf(&ret, "%s%d=port:%s", dev, def->dstPort, + if (virAsprintf(&ret, "%s%d=port:%s", dev, def->target.port, def->data.tcp.service) < 0) { virReportOOMError(conn); return NULL; @@ -502,7 +502,7 @@ int umlBuildCommandLine(virConnectPtr conn, virDomainChrDefPtr chr = NULL; char *ret; for (j = 0 ; j < vm->def->nserials ; j++) - if (vm->def->serials[j]->dstPort == i) + if (vm->def->serials[j]->target.port == i) chr = vm->def->serials[j]; if (chr) ret = umlBuildCommandLineChr(conn, chr, "ssl"); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 212cd8f..e2525c4 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -169,7 +169,7 @@ umlIdentifyOneChrPTY(virConnectPtr conn, char *cmd; char *res = NULL; int retries = 0; - if (virAsprintf(&cmd, "config %s%d", dev, def->dstPort) < 0) { + if (virAsprintf(&cmd, "config %s%d", dev, def->target.port) < 0) { virReportOOMError(conn); return -1; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index c6305ac..d29e424 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2403,9 +2403,9 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { serialPort->vtbl->GetIRQ(serialPort, &IRQ); serialPort->vtbl->GetIOBase(serialPort, &IOBase); if ((IRQ == 4) && (IOBase == 1016)) { - def->serials[serialPortIncCount]->dstPort = 0; + def->serials[serialPortIncCount]->target.port = 0; } else if ((IRQ == 3) && (IOBase == 760)) { - def->serials[serialPortIncCount]->dstPort = 1; + def->serials[serialPortIncCount]->target.port = 1; }

serialPort->vtbl->GetPath(serialPort, &pathUtf16); @@ -2469,9 +2469,9 @@ static char *vboxDomainDumpXML(virDomainPtr dom, int flags) { parallelPort->vtbl->GetIRQ(parallelPort, &IRQ); parallelPort->vtbl->GetIOBase(parallelPort, &IOBase); if ((IRQ == 7) && (IOBase == 888)) { - def->parallels[parallelPortIncCount]->dstPort = 0; + def->parallels[parallelPortIncCount]->target.port = 0; } else if ((IRQ == 5) && (IOBase == 632)) { - def->parallels[parallelPortIncCount]->dstPort = 1; + def->parallels[parallelPortIncCount]->target.port = 1; }

def->parallels[parallelPortIncCount]->type = VIR_DOMAIN_CHR_TYPE_FILE; @@ -3493,7 +3493,7 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { ISerialPort *serialPort = NULL;

DEBUG("SerialPort(%d): Type: %d", i, def->serials[i]->type); - DEBUG("SerialPort(%d): dstPort: %d", i, def->serials[i]->dstPort); + DEBUG("SerialPort(%d): target.port: %d", i, def->serials[i]->target.port);

machine->vtbl->GetSerialPort(machine, i, &serialPort); if (serialPort) { @@ -3508,17 +3508,17 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { * TODO: make this more flexible */ /* TODO: to improve the libvirt XMl handling so - * that def->serials[i]->dstPort shows real port + * that def->serials[i]->target.port shows real port * and not always start at 0 */ if (def->serials[i]->type == VIR_DOMAIN_CHR_TYPE_DEV) { serialPort->vtbl->SetPath(serialPort, pathUtf16); - if (def->serials[i]->dstPort == 0) { + if (def->serials[i]->target.port == 0) { serialPort->vtbl->SetIRQ(serialPort, 4); serialPort->vtbl->SetIOBase(serialPort, 1016); DEBUG(" serialPort-%d irq: %d, iobase 0x%x, path: %s", i, 4, 1016, def->serials[i]->data.file.path); - } else if (def->serials[i]->dstPort == 1) { + } else if (def->serials[i]->target.port == 1) { serialPort->vtbl->SetIRQ(serialPort, 3); serialPort->vtbl->SetIOBase(serialPort, 760); DEBUG(" serialPort-%d irq: %d, iobase 0x%x, path: %s", @@ -3527,12 +3527,12 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { serialPort->vtbl->SetHostMode(serialPort, PortMode_HostDevice); } else if (def->serials[i]->type == VIR_DOMAIN_CHR_TYPE_PIPE) { serialPort->vtbl->SetPath(serialPort, pathUtf16); - if (def->serials[i]->dstPort == 0) { + if (def->serials[i]->target.port == 0) { serialPort->vtbl->SetIRQ(serialPort, 4); serialPort->vtbl->SetIOBase(serialPort, 1016); DEBUG(" serialPort-%d irq: %d, iobase 0x%x, path: %s", i, 4, 1016, def->serials[i]->data.file.path); - } else if (def->serials[i]->dstPort == 1) { + } else if (def->serials[i]->target.port == 1) { serialPort->vtbl->SetIRQ(serialPort, 3); serialPort->vtbl->SetIOBase(serialPort, 760); DEBUG(" serialPort-%d irq: %d, iobase 0x%x, path: %s", @@ -3573,7 +3573,7 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { IParallelPort *parallelPort = NULL;

DEBUG("ParallelPort(%d): Type: %d", i, def->parallels[i]->type); - DEBUG("ParallelPort(%d): dstPort: %d", i, def->parallels[i]->dstPort); + DEBUG("ParallelPort(%d): target.port: %d", i, def->parallels[i]->target.port);

machine->vtbl->GetParallelPort(machine, i, ¶llelPort); if (parallelPort) { diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 9080754..f86e022 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -2569,6 +2569,7 @@ xenDaemonParseSxpr(virConnectPtr conn, virDomainChrDefFree(chr); goto no_memory; } + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; def->serials[def->nserials++] = chr; } tmp = sexpr_node(root, "domain/image/hvm/parallel"); @@ -2581,12 +2582,14 @@ xenDaemonParseSxpr(virConnectPtr conn, virDomainChrDefFree(chr); goto no_memory; } + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; def->parallels[def->nparallels++] = chr; } } else { /* Fake a paravirt console, since that's not in the sexpr */ if (!(def->console = xenDaemonParseSxprChar(conn, "pty", tty))) goto error; + def->console->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE; } VIR_FREE(tty);

diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index b52f66e..5e8931e 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1415,6 +1415,7 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { virDomainChrDefFree(chr); goto no_memory; } + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_PARALLEL; def->parallels[0] = chr; def->nparallels++; chr = NULL; @@ -1431,12 +1432,14 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) { virDomainChrDefFree(chr); goto no_memory; } + chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL; def->serials[0] = chr; def->nserials++; } } else { if (!(def->console = xenDaemonParseSxprChar(conn, "pty", NULL))) goto cleanup; + def->console->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_CONSOLE; }

if (hvm) { -- 1.6.2.5

-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On 04/11/09 19:05, Daniel P. Berrange wrote:

On Wed, Nov 04, 2009 at 04:21:59PM +0000, Matthew Booth wrote:

...

--- src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 1 + 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 19b2d36..2981b51 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -894,6 +894,8 @@ static unsigned int qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_PCIDEVICE; if (strstr(help, "-mem-path")) flags |= QEMUD_CMD_FLAG_MEM_PATH; + if (strstr(help, "-chardev")) + flags |= QEMUD_CMD_FLAG_CHARDEV;

if (version>= 9000) flags |= QEMUD_CMD_FLAG_VNC_COLON; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 53835cf..0f82c68 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -73,6 +73,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_DRIVE_SERIAL = (1<< 19), /* -driver serial= available */ QEMUD_CMD_FLAG_XEN_DOMID = (1<< 20), /* -xen-domid (new style xen integration) */ QEMUD_CMD_FLAG_MIGRATE_QEMU_UNIX = (1<< 21), /* Does qemu support unix domain sockets for migration? */ + QEMUD_CMD_FLAG_CHARDEV = (1<< 22), /* Is the new -chardev arg available */ };

/* Main driver state */

Does this not cause a failure in the qemuhelptest test case ?

Make check passes. Why would it cause a failure? Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

On Thu, Nov 05, 2009 at 09:10:45AM +0000, Matthew Booth wrote:

On 04/11/09 19:05, Daniel P. Berrange wrote:

...

On Wed, Nov 04, 2009 at 04:21:59PM +0000, Matthew Booth wrote:

...

--- src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 1 + 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 19b2d36..2981b51 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -894,6 +894,8 @@ static unsigned int qemudComputeCmdFlags(const char *help, flags |= QEMUD_CMD_FLAG_PCIDEVICE; if (strstr(help, "-mem-path")) flags |= QEMUD_CMD_FLAG_MEM_PATH; + if (strstr(help, "-chardev")) + flags |= QEMUD_CMD_FLAG_CHARDEV;

if (version>= 9000) flags |= QEMUD_CMD_FLAG_VNC_COLON; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 53835cf..0f82c68 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -73,6 +73,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_DRIVE_SERIAL = (1<< 19), /* -driver serial= available */ QEMUD_CMD_FLAG_XEN_DOMID = (1<< 20), /* -xen-domid (new style xen integration) */ QEMUD_CMD_FLAG_MIGRATE_QEMU_UNIX = (1<< 21), /* Does qemu support unix domain sockets for migration? */ + QEMUD_CMD_FLAG_CHARDEV = (1<< 22), /* Is the new -chardev arg available */ };

/* Main driver state */

Does this not cause a failure in the qemuhelptest test case ?

I see where you're coming from now. No it doesn't, because -chardev isn't in qemu 0.11: it's only in git. The current upstream outputs a single line under Debug/Expert options. I have submitted a documentation patch (not yet accepted afaict) which substantially expands this. However, both should work.

I applied the patch, looks fine to me, no problem on reg tests. At some point we will have to extend tests/qemuhelpdata/ with an entry adding the new message, when upstream accepts it :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Nov 04, 2009 at 04:22:00PM +0000, Matthew Booth wrote:

Note that, on its own, this patch will generate a warning about an unused static function.

That says this patch should be squashed with the next one

--- src/qemu/qemu_conf.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2981b51..a9f6885 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1411,6 +1411,74 @@ qemuBuildHostNetStr(virConnectPtr conn, return 0; }

+/* This function outputs a -chardev command line option which describes only the + * host side of the character device */ +static void qemudBuildCommandLineChrDevChardevStr(virDomainChrDefPtr dev, + const char *const id, + virBufferPtr buf) +{ + bool telnet; + switch(dev->type) { + case VIR_DOMAIN_CHR_TYPE_NULL: + virBufferVSprintf(buf, "null,id=%s", id); + break; + + case VIR_DOMAIN_CHR_TYPE_VC: + virBufferVSprintf(buf, "vc,id=%s", id); + break; + + case VIR_DOMAIN_CHR_TYPE_PTY: + virBufferVSprintf(buf, "pty,id=%s", id); + break; + + case VIR_DOMAIN_CHR_TYPE_DEV: + virBufferVSprintf(buf, "tty,id=%s,path=%s", id, dev->data.file.path); + break; + + case VIR_DOMAIN_CHR_TYPE_FILE: + virBufferVSprintf(buf, "file,id=%s,path=%s", id, dev->data.file.path); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + virBufferVSprintf(buf, "pipe,id=%s,path=%s", id, dev->data.file.path); + break; + + case VIR_DOMAIN_CHR_TYPE_STDIO: + virBufferVSprintf(buf, "stdio,id=%s", id); + break; + + case VIR_DOMAIN_CHR_TYPE_UDP: + virBufferVSprintf(buf, + "udp,id=%s,host=%s,port=%s,localaddr=%s,localport=%s", + id, + dev->data.udp.connectHost, + dev->data.udp.connectService, + dev->data.udp.bindHost, + dev->data.udp.bindService); + break; + + case VIR_DOMAIN_CHR_TYPE_TCP: + telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; + virBufferVSprintf(buf, + "socket,id=%s,host=%s,port=%s%s%s", + id, + dev->data.tcp.host, + dev->data.tcp.service, + telnet ? ",telnet" : "", + dev->data.tcp.listen ? ",server,nowait" : ""); + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + virBufferVSprintf(buf, + "socket,id=%s,path=%s%s", + id, + dev->data.nix.path, + dev->data.nix.listen ? ",server,nowait" : ""); + break; + } +} + +/* This function outputs an all-in-one character device command line option */ static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) -- ACK

Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Nov 04, 2009 at 04:22:01PM +0000, Matthew Booth wrote:

This patch allows the following to be specified in a qemu domain:

<channel type='pipe'> <source path='/tmp/guestfwd'/> <target type='guestfwd' address='10.0.2.1' port='4600'/> </channel>

This will output the following on the qemu command line:

-chardev pipe,id=channel0,path=/tmp/guestfwd \ -net user,guestfwd=tcp:10.0.2.1:4600-chardev:channel0

* docs/schemas/domain.rng: Add <channel> and <guestfwd> elements * proxy/Makefile.am: add network.c as dep of domain_conf.c * src/conf/domain_conf.[ch]: Add xml parsing/formatting for channel and guestfwd * src/qemu/qemu_conf.c: Add argument output for guestfwd * tests/qemuxml2(argv|xml)test.c: Add test for guestfwd domain syntax --- docs/schemas/domain.rng | 89 ++++++---- proxy/Makefile.am | 1 + src/conf/domain_conf.c | 189 ++++++++++++++++++-- src/conf/domain_conf.h | 6 + src/qemu/qemu_conf.c | 64 +++++++ .../qemuxml2argv-channel-guestfwd.args | 1 + .../qemuxml2argv-channel-guestfwd.xml | 26 +++ tests/qemuxml2argvtest.c | 4 +- tests/qemuxml2xmltest.c | 1 + 9 files changed, 332 insertions(+), 49 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-guestfwd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-guestfwd.xml

diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 0a6ab61..b75f17e 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -930,6 +930,19 @@ definition doesn't fully specify the constraints on this node. --> <define name="qemucdev"> + <ref name="qemucdevSrcType"/> + <interleave> + <ref name="qemucdevSrcDef"/> + <optional> + <element name="target"> + <optional> + <attribute name="port"/> + </optional> + </element> + </optional> + </interleave> + </define> + <define name="qemucdevSrcType"> <attribute name="type"> <choice> <value>dev</value> @@ -944,43 +957,36 @@ <value>pty</value> </choice> </attribute> - <interleave> - <optional> - <oneOrMore> - <element name="source"> - <optional> - <attribute name="mode"/> - </optional> - <optional> - <attribute name="path"/> - </optional> - <optional> - <attribute name="host"/> - </optional> - <optional> - <attribute name="service"/> - </optional> - <optional> - <attribute name="wiremode"/> - </optional> - </element> - </oneOrMore> - </optional> - <optional> - <element name="protocol"> + </define> + <define name="qemucdevSrcDef"> + <optional> + <oneOrMore> + <element name="source"> <optional> - <attribute name="type"/> + <attribute name="mode"/> </optional> - </element> - </optional> - <optional> - <element name="target"> <optional> - <attribute name="port"/> + <attribute name="path"/> + </optional> + <optional> + <attribute name="host"/> + </optional> + <optional> + <attribute name="service"/> + </optional> + <optional> + <attribute name="wiremode"/> </optional> </element> - </optional> - </interleave> + </oneOrMore> + </optional> + <optional> + <element name="protocol"> + <optional> + <attribute name="type"/> + </optional> + </element> + </optional> </define> <!-- The description for a console @@ -1044,6 +1050,24 @@ <ref name="qemucdev"/> </element> </define> + <define name="guestfwdTarget"> + <element name="target"> + <attribute name="type"> + <value>guestfwd</value> + </attribute> + <attribute name="address"/> + <attribute name="port"/> + </element> + </define> + <define name="channel"> + <element name="channel"> + <ref name="qemucdevSrcType"/> + <interleave> + <ref name="qemucdevSrcDef"/> + <ref name="guestfwdTarget"/> + </interleave> + </element> + </define> <define name="input"> <element name="input"> <attribute name="type"> @@ -1158,6 +1182,7 @@ <ref name="console"/> <ref name="parallel"/> <ref name="serial"/> + <ref name="channel"/> </choice> </zeroOrMore> <optional>

Okay I had done a review of the XML extension before, I'm fine with <channel> :-)

@@ -1346,18 +1362,15 @@ virDomainChrDefParseXML(virConnectPtr conn, else if ((def->type = virDomainChrTypeFromString(type)) < 0) def->type = VIR_DOMAIN_CHR_TYPE_NULL;

- targetType = (const char *) node->name; - if (targetType == NULL) { - /* Shouldn't be possible */ - virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, - "node->name is NULL at %s:%i", - __FILE__, __LINE__); - return NULL; - } - if ((def->targetType = virDomainChrTargetTypeFromString(targetType)) < 0) { - virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("unknown target type for character device: %s"), - targetType); + nodeName = (const char *) node->name; + if ((def->targetType = virDomainChrTargetTypeFromString(nodeName)) < 0) { + /* channel is handled below */ + if(STRNEQ(nodeName, "channel")) { + virDomainReportError(conn, VIR_ERR_INVALID_DOMAIN, + _("unknown target type for character device: %s"), + nodeName); + return NULL; + } def->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_NULL; }

I had to manually apply this since I had modified that part in 1/5 :-) Okay, patch looks fine. the rng and conf part could probably have been isolated as a first patch and then patch 3 and the qemu side could have been merged as a second patch, but in the end this doesn't change much :-) Pushed, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Nov 04, 2009 at 04:22:02PM +0000, Matthew Booth wrote:

* src/conf/domain_conf.c: Throw an error if guestfwd address isn't IPv4 --- src/conf/domain_conf.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 94bce1e..ec2a1bc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1482,6 +1482,13 @@ virDomainChrDefParseXML(virConnectPtr conn, goto error; }

+ if(def->target.addr->stor.ss_family != AF_INET) {

^^^^ whitespace issue

+ virDomainReportError(conn, VIR_ERR_NO_SUPPORT, "%s", + _("guestfwd channel only supports " + "IPv4 addresses")); + goto error; + } + if(portStr == NULL) { virDomainReportError(conn, VIR_ERR_INVALID_DOMAIN, "%s", _("guestfwd channel does " --

NO_SUPPORT isn't really the best error code to use here - that's intended for public API calls which aren't implemented by a driver. Our reporting of XML configurations which aren't valid is pretty rubbish as we don't have a standard error code - people have been making it up as we go along. I think we should add a new error code: VIR_ERR_CONFIG_UNSUPPORTED whcih we can then standardize on for this kind of thing Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|