4:27 a.m.
On Wed, Sep 12, 2012 at 5:01 PM, Li Zhang <zhlcindy(a)gmail.com> wrote:
On Wed, Sep 12, 2012 at 4:06 PM, Martin Kletzander
<mkletzan(a)redhat.com> wrote:
> On 09/11/2012 04:57 AM, Li Zhang wrote:
>> Histrically, the first <console> element is treated as the
>
> s/Histrically/Historically/
>
>> alias of a <serial> device. In the virDomainDeviceInfoIterate,
>> This situation is not considered. It still handles the first <console>
>> element as another devices, which means that for console[0] with
>> serial targetType, it calls callback function another time.
>> It will cause the problem of address conflicts when assigning
>> spapr-vio address for serial device on pSeries guest.
>>
>> For pSeries guest, the serial configuration in the xml file
>> is as the following:
>> <serial type='pty'>
>> <target port='0'/>
>> <address type='spapr-vio'/>
>> </serial>
>>
>> Console configuration is default, the dumped xml file is as the following:
>> <serial type='pty'>
>> <source path='/dev/pts/5'/>
>> <target port='0'/>
>> <alias name='serial0'/>
>> <address type='spapr-vio' reg='0x30000000'/>
>> </serial>
>> <console type='pty' tty='/dev/pts/5'>
>> <source path='/dev/pts/5'/>
>> <target type='serial' port='0'/>
>> <alias name='serial0'/>
>> <address type='spapr-vio' reg='0x30000000'/>
>> </console>
>>
>> It shows that the <console> device is the alias of serial device.
>> So its address is the same as the serial device. When dectecting
>
> s/dectecting/detecting/
>
>> the conflicts in the qemuAssignSpaprVIOAddress the first console
>> and the serial device conflicts because virDomainDeviceInfoIterate()
>> still handle these as two different devices, and in the
qemuAssignSpaprVIOAddress(),
>> it will compare these two devices' addressed. If they have same address,
>> it will report address confilict error.
>
> s/confilict/conflict/
>
>>
>> So this patch is to handle the first console which targetType is serial
>> as the alias of serial device to avoid address conflicts error reported.
>>
>
> I'm wondering if this is the place we want to fix it. If somebody wants
> to use the iterate function for something else, they must accept that
> the fact that the first serial console will be skipped. OTOH it is
> common (and the only supported way) to have the serial console.
Yes, in libvirt, only the first console can be the serial device. So
if the console type
is serial, it will be handled as the serial[0] device. -:)
In the file src/conf/domain_conf.c,
The the console handling is as the following:
if (STREQ(def->os.type, "hvm") &&
(chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)) {
if (i != 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Only the first console can be a
serial port"));
virDomainChrDefFree(chr);
goto error;
}
...
if (virDomainChrSourceDefIsEqual(&def->serials[0]->source,
&chr->source)) {
/* Alias to def->serial[0]. Skip it */
create_stub = false;
}
...
if (create_stub) {
/* And create a stub placeholder */
if (VIR_ALLOC(chr) < 0)
goto no_memory;
chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
}
On x86, serial device doesn't have address, so there is no address conflict.
If there is serial address on other platform, there will be problem.
>
>> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com>
>> ---
>> * v1 -> v2:
>> Rebase v1 on latest version of libvirt.
>>
>> src/conf/domain_conf.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 8952b69..0e71b06 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2054,6 +2054,9 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def,
>> return -1;
>> }
>> for (i = 0; i < def->nconsoles ; i++) {
>> + if ((STREQ(def->os.type, "hvm")) && i == 0
&&
>> + def->consoles[i]->targetType ==
VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
>> + continue;
>> device.data.chr = def->consoles[i];
>> if (cb(def, &device, &def->consoles[i]->info, opaque) <
0)
>> return -1;
>>
>
> I was thinkinf about fixing it in the callback function, configuration
> parser and so on, but this looks like the best solution, so ACK from me,
> is it OK if I push this with the address in Cc?
I once considered to fix this in callback function. It seems that the
almost same thing should be done as in the this function.
From above comments, I think of one condition to be added:
if (virDomainChrSourceDefIsEqual(&def->serials[0]->source,
&chr->source)) {
/* Alias to def->serial[0]. Skip it */
create_stub = false;
}
This is to assert whether the source is the same.
I didn't try to change the device source. -:)
If this condition is needed, I will send out the next version. -:)
I check it, it's not necessary to add this condition.
Thanks for pushing it. -:)
>
> Martin
--
Best Regards
-Li
--
Best Regards
-Li