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

[Jonathon Jongsma]

On Fri, 13 Nov 2020 12:09:26 +0100
Shalini Chellathurai Saroja <shalini at 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