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

[Li Zhang]

On Wed, Sep 12, 2012 at 5:01 PM, Li Zhang <zhlcindy at gmail.com> wrote:
> On Wed, Sep 12, 2012 at 4:06 PM, Martin Kletzander <mkletzan at 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 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?
>
> 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