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

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. > > 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(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel