7:45 a.m.
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(a)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