On Fri, 13 Nov 2020 12:09:26 +0100 Shalini Chellathurai Saroja <shalini@linux.ibm.com> wrote:
Hi Jonathon,
Thank you for the quick review:-)
On 11/12/20 5:27 PM, Jonathon Jongsma wrote:
+ <define name='address'> + <element name='address'> + <attribute name='domain'><ref name='hexuint'/></attribute> + <attribute name='bus'><ref name='hexuint'/></attribute> + <attribute name='slot'><ref name='hexuint'/></attribute> + <attribute name='function'><ref name='hexuint'/></attribute> It seems that you're unnecessarily changing double-quotes to single-quotes here, which adds spurious changes to the diff. The rest of the file uses double-quotes. Let's stick with that. Sure.
</element> </define>
@@ -716,4 +726,16 @@ </element> </define>
+ <define name="apAdapterRange"> + <choice> + <data type="string"> + <param name="pattern">(0x)?[0-9a-fA-F]{1,2}</param> + </data> + <data type="int"> + <param name="minInclusive">0</param> + <param name="maxInclusive">255</param> + </data> + </choice> + </define>
As far as I can tell, this is identical to the definition of the 'uint8' type in basictypes.rng. Is there a reason for defining a different type?
Yes, In definition apAdapterRange, the prefix '0x' is optional.
Oh, I guess I missed that part. Now that I look at basictypes.rng, I see that the '0x' is required for uint8 and uint24, but it is optional for uint16 and uint32. Does anybody know the justification for these differences? That said, I don't believe that your parsing code actually supports an optional '0x' prefix. In virNodeDevCapAPCardParseXML(), you call virStrToLong_uip(adapter, NULL, 0, &ap_card->ap_adapter) But I'm quite sure that passing a value of e.g. 'ff' for adapter will result in a parsing failure. Try changing the ap-adapter value in tests/nodedevschemadata/ap_card07.xml to some different values and see what happens. Jonathon