Re: [PATCH libvirt v2 01/11] nodedev: detect AP card device

On Fri, 13 Nov 2020 12:09:26 +0100 Shalini Chellathurai Saroja <shalini(a)linux.ibm.com&gt; 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