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

[Daniel P. Berrange]

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 at 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 :|