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

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(a)linux.vnet.ibm.com wrote: >>>> From: Archana Singh <archus(a)linux.vnet.ibm.com&gt; >>>> >>>> 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(a)linux.vnet.ibm.com&gt; >>>> --- >>>> 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(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/kimchi-devel >> > > _______________________________________________ > Kimchi-devel mailing list > Kimchi-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/kimchi-devel >