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@xxxxxxxxxx>
>
> [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.
The simplest way I thought of solving this would be to format XML from
the xen/esx generated domain def, and run it through the XML parser to
pick up the default device handling. But since that would be yet another
hack, I just decided to drop this patch, since it isn't strictly
required to get virtio console hooked up. Someone can revisit later if
they want, my patch implementing the single hack in the parser is here:
http://fedorapeople.org/~crobinso/libvirt/libvirt-multiple-consoles.patch
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list