Re: [libvirt] [PATCH 4/5] domain conf: Support multiple <console> devices

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(a)redhat.com&gt; > > [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.