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