Re: [libvirt] [PATCH] schema: Allow multiple machines for sparc VMs

On Monday 13 April 2015 07:50 PM, Daniel P. Berrange wrote: > On Mon, Apr 13, 2015 at 04:14:53PM +0200, Martin Kletzander wrote: >> Use the same pattern as there is for x86 machines. >> >> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >> --- >> docs/schemas/domaincommon.rng | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index 03fd541..80b30df 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -384,7 +384,9 @@ >> </optional> >> <optional> >> <attribute name="machine"> >> - <value>sun4m</value> >> + <data type="string"> >> + <param name="pattern">[a-zA-Z0-9_\.\-]+</param> >> + </data> >> </attribute> >> </optional> >> </group> > I think you could probably simplify this all much more. All these > architecture specific blocks of machine type names should just be > deleted and so this: > > <define name="ostypehvm"> > <element name="type"> > <optional> > <choice> > <ref name="hvmx86"/> > <ref name="hvmmips"/> > <ref name="hvmsparc"/> > <ref name="hvmppc"/> > <ref name="hvmppc64"/> > <ref name="hvms390"/> > <ref name="hvmarm"/> > <ref name="hvmaarch64"/> > </choice> > </optional> > <value>hvm</value> > </element> > </define> > > Would simplify to just > > <define name="ostypehvm"> > <element name="type"> > <optional> > <attribute name="arch"> > <choice> > <value>i686</value> > ....others... > </choice> > </attribute> > </optional> > <optional> > <attribute name="machine"> > <data type="string"> > <param name="pattern">[a-zA-Z0-9_\.\-]+</param> > </data> > </attribute> > </optional> > </element> > </define> Hi Martin, Daniel, I have not been able to try this patch, it fails with this error : error: internal error: Unable to parse RNG /test-libvirt/share/libvirt/schemas/domain.rng: Reference osexe has no matching definition Internal found no define for ref osexe However, had some concerns purely by looking at this patch. This change is very x86-centric, it does not respect other architectures. I think the rationale for simplifying domaincommon.rng would have been to group all types that obey this pattern string: <param name="pattern">[a-zA-Z0-9_\.\-]+</param> However, this regex does not conform to machine types for _all_ architectures. As an example, see this : <define name="hvms390"> <group> <optional> <attribute name="arch"> <choice> <value>s390</value> <value>s390x</value> </choice> </attribute> </optional> <optional> <attribute name="machine"> <choice> <value>s390</value> <value>s390-virtio</value> <value>s390-ccw</value> <value>s390-ccw-virtio</value> </choice> </attribute> </optional> </group> </define> The s390 arch only allows four machine names : "s390", "s390-virtio", "s390-ccw", "s390-ccw-virtio". With the patch you suggest, even a string such as "abcdefg" will become a legitimate machine type for s390x, which seems like an odd thing. Likewise, ppc64[le] architecture allows only strings such as pseries, pseries-2.1, pseries-2.2 .. This patch will allow any random machine name, which seems somewhat odd to me.