[libvirt] [PATCH v2 1/1] Backcompt for console devices in virDomainDeviceInfoIterate

[Martin Kletzander]

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 <zhlcindy at 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?

Martin