Histrically, the first <console> element is treated as the 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 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. 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. Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; --- * 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; -- 1.7.5.4

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. > Signed-off-by: Li Zhang&lt;zhlcindy(a)linux.vnet.ibm.com&gt; > --- > * 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?

Martin -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 09/12/2012 10:32 AM, Osier Yang wrote: > On 2012年09月12日 16:06, Martin Kletzander 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. >> >>> Signed-off-by: Li Zhang&lt;zhlcindy(a)linux.vnet.ibm.com&gt; >>> --- >>> * 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? > > It's the one listed in AUTHORS. So should be fine. > That's why I asked, because the one from 'From:' is not there. OK than, pushed with fixes, thanks both of you ;)

>> >> Martin >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list >

On Wed, Sep 12, 2012 at 4:06 PM, Martin Kletzander <mkletzan(a)redhat.com&gt; 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&gt; >> --- >> * 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. -:)

> > Martin -- Best Regards -Li