[Kimchi-devel] [PATCH V1] [Kimchi] For s390x architecture, serial type is not validated in vm xml as as not supported on s390x.

[Archana Singh]

HI Aline,

Please find my comment below. Please let me know if it make sense to you.

Thanks,

Archana Singh

On 08/30/2016 11:56 PM, Archana Singh wrote:
>
>
> On 08/30/2016 10:14 PM, Aline Manera wrote:
>>
>>
>> On 08/30/2016 09:43 AM, archus at linux.vnet.ibm.com wrote:
>>> From: Archana Singh <archus at linux.vnet.ibm.com>
>>>
>>> This changes ensure that in vm xml serial type is not validated for 
>>> s390x
>>> as not supported in s390x architecture and only console type is 
>>> validated
>>> in _vm_check_serial.
>>>
>>> Signed-off-by: Archana Singh <archus at linux.vnet.ibm.com>
>>> ---
>>>   model/vms.py | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/model/vms.py b/model/vms.py
>>> index 7f607f5..d3d2cb1 100644
>>> --- a/model/vms.py
>>> +++ b/model/vms.py
>>> @@ -1486,7 +1486,8 @@ class VMModel(object):
>>>           xml = dom.XMLDesc(libvirt.VIR_DOMAIN_XML_SECURE)
>>>
>>>           expr = "/domain/devices/serial/@type"
>>
>>> -        if not xpath_get_text(xml, expr):
>>> +        # on s390x serial is not supported
>>> +        if platform.machine() != 's390x' and not 
>>> xpath_get_text(xml, expr):
>>
>> Seems this validation is not correct.
>>
>> The full code is:
>>
>>         expr = "/domain/devices/serial/@type"
>>         if not xpath_get_text(xml, expr):
>>             return False
>>
>>         expr = "/domain/devices/console/@type"
>>         if not xpath_get_text(xml, expr):
>>             return False
>>
>> With this patch, it turned to:
>>
>>         expr = "/domain/devices/serial/@type"
>>         if platform.machine() != 's390x' and not xpath_get_text(xml, 
>> expr):
>>             return False
>>
>>         expr = "/domain/devices/console/@type"
>>         if not xpath_get_text(xml, expr):
>>             return False
>>
>> If the system is x86 and the guest does have 
>> "/domain/devices/serial/@type" it will not enter the first 'if' 
>> statement.
> So even without this patch if system is x86 and has 
> "/domain/devices/serial/@type" it will not enter the first 'if' 
> statement. If I understood correctly, purpose of this check is to 
> ensure that guest has "/domain/devices/serial/@type" otherwise return 
> false.
>
> And with this patch first check(platform.machine() != 's390x' ) in 
> first 'if' statement will always be true and hence 'if' statement is 
> similar to what it was before this patch.
>
> This patch ensure that on s390x system even if guest does not have 
> "/domain/devices/serial/@type" it should not enter in first if 
> statement. And for all other system, it should enter in first if 
> statement in case guest does not have 
> "/domain/devices/serial/@type"(similar to what it was before)
>
> Am I miss understood some thing?
>
>> And the second if will be validated for all the platforms, including 
>> s390x. Which means, the function _vm_check_serial can still return 
>> True for a s390x system.
>>
>> Does that make sense for you?
>>
>> Said that, I suggest to add a exclusive if statement to cover the 
>> s390x system:
>>
>> if platform.machine() == 's390x':
>>     return False
>>
>>
>>>               return False
>>>
>>>           expr = "/domain/devices/console/@type"
>>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>