[libvirt] [PATCH 08/13] conf: Introduce parser, formatter for uid and fid

[Boris Fiuczynski]

On 06/08/2018 03:40 AM, Laine Stump wrote:
> On 05/25/2018 07:05 AM, Pino Toscano wrote:
>> On Thursday, 24 May 2018 14:24:33 CEST Xiao Feng Ren wrote:
>>> From: Yi Min Zhao <zyimin at linux.ibm.com>
>>>
>>> This patch introduces new XML parser/formatter functions. Uid is
>>> 16-bit and non-zero. Fid is 32-bit. They are added as two new
>>> attributes of PCI address, and parsed/formatted along with PCI
>>> address parser/formatter.
>>>
>>> Signed-off-by: Yi Min Zhao <zyimin at linux.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>> Reviewed-by: Stefan Zimmermann <stzi at linux.ibm.com>
>>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>>> ---
>>>   docs/schemas/basictypes.rng   | 28 ++++++++++++++++
>>>   docs/schemas/domaincommon.rng |  1 +
>>>   src/conf/device_conf.c        | 74 +++++++++++++++++++++++++++++++++++++++++++
>>>   src/conf/domain_addr.c        |  3 ++
>>>   src/conf/domain_conf.c        |  4 +++
>>>   src/util/virpci.h             |  3 ++
>>>   6 files changed, 113 insertions(+)
>>>
>>> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
>>> index 1a18cd31b1..8050a3ebc4 100644
>>> --- a/docs/schemas/basictypes.rng
>>> +++ b/docs/schemas/basictypes.rng
>>> @@ -111,6 +111,34 @@
>>>         </attribute>
>>>       </optional>
>>>     </define>
>>> +  <define name="zpciaddress">
>>> +    <optional>
>>> +      <attribute name="uid">
>>> +        <choice>
>>> +          <data type="string">
>>> +            <param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param>
>>> +          </data>
>>> +          <data type="unsignedInt">
>>> +            <param name="minInclusive">1</param>
>>> +            <param name="maxInclusive">65535</param>
>>> +          </data>
>>> +        </choice>
>>> +      </attribute>
>> This seems to be the "uint16" type already.
>>
>>> +    </optional>
>>> +    <optional>
>>> +      <attribute name="fid">
>>> +        <choice>
>>> +          <data type="string">
>>> +            <param name="pattern">(0x)?[0-9a-fA-F]{1,8}</param>
>>> +          </data>
>>> +          <data type="unsignedLong">
>>> +            <param name="minInclusive">0</param>
>>> +            <param name="maxInclusive">4294967295</param>
>>> +          </data>
>>> +        </choice>
>>> +      </attribute>
>> This could be a new "uint32" type, changing the 0x prefix as
>> non-optional (otherwise the value "10" can be both valid as decimal
>> and hexadeciaml).
> 
> My personal opinion - if numbers without a leading 0x are consistently
> not interpreted as hexadecimal, then there is no ambiguity and no
> confusion. If there's a leading 0x, then it is hexadecimal. No leading
> 0x, it's decimal. Period.
Laine,
so are you saying the pattern for type string should for uid and fid 
start with 0x instead of (0x)?. That would certainly work.
The difference is than the error message if e.g. for uid a user specifies 0.
The schema error is than
error: XML document failed to validate against schema: Unable to 
validate doc against /usr/share/libvirt/schemas/domain.rng
Extra element devices in interleave
Element domain failed to validate content

and with the optional 0x the value 0 would bypass the schema and the 
checks in the code reports the user friendly error
error: XML error: Invalid PCI address uid='0x0', must be > 0x0 and <= 0xffff

I agree that the later is really tricking the schema but with usability 
in mind (you hopefully agree that the schema error message su(&s ) I 
definitely vote for the later solution.... :-(


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294