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

[Archana Singh]

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.
>>>
>>>
>>>>               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
>