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

[Cole Robinson]

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 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.
> 

Yeah that would work too, I didn't try it though.

- Cole