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

[Aline Manera]

On 09/02/2016 12:51 PM, Archana Singh wrote:
> Missed to replay for second validation, added comment inline.
>
> Thanks,
>
> Archana Singh
>
>
> On 09/02/2016 09:48 AM, Archana Singh wrote:
>> 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.
> Yes, it has to be validated for s390x as well.
>>>>
>>>> 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
> So summary is first one has to be ignored on s390x but second one has 
> to be validated on s390x also.

OK. Got it.

I thought the both cases should be ignored in case of s390x.

>>>>
>>>>
>>>>>               return False
>>>>>
>>>>>           expr = "/domain/devices/console/@type"
>>>>
>>>
>>> _______________________________________________
>>> Kimchi-devel mailing list
>>> Kimchi-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>>
>>
>> _______________________________________________
>> Kimchi-devel mailing list
>> Kimchi-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>