[...]
>>
>> Hopefully hexuint will suffice over time... On the other hand, this
>> patch uses virXPathULongHex in order to parse.
>>
>
> IIRC, I was not able to find anything other than hexuint in
> basictypes.rng and also was not able to a function like
> virXPathUIntHex(..). If you can point me to the function which can be
> used to get the hexuint then I am good with it. Also, I am open to
> adding such function if it does not exist and anyone else sees the need
> for it.
>
>
Right - if they need to be created, then you can do so.
I think in the long run since field is defined as a 32 bit unsigned
quantity, then we should be OK with sticking with that. If you need to
invent a 'hexulong' which is a slight change from 'hexuint' that's also
a possibility.
OTOH: if 32 bits are fine, it's "OK" to use the virXPathULongHex as long
as you also test that the result isn't longer than 32 bits. I wouldn't
bother with a virXPathUIntHex API since in the long run all it "should"
do is call the Long one and do the max uint comparison.
>
>>> + <optional>
>>> + <element name="handle">
>>> + <ref name='unsignedInt'/>
>>> + </element>
>>> + </optional>
>>> + <optional>
>>> + <element name="dh-cert">
>>> + <data type="string"/>
>>> + </element>
>>> + </optional>
>>> + <optional>
>>> + <element name="session">
>>> + <data type="string"/>
>>> + </element>
>>> + </optional>
>>> + </interleave>
>>> + </element>
>>> + </define>
>>> +
[...]
>>> +
>>> + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0)>
>>> + policy = 0x1;
>>
>> Hmmm... This one is optional which makes things a bit interesting. How
>> do you know it was provided? Today the default could be 0x1, but
>> sometime in the future if you decide upon a different default, then
>> things get really complicated. Or for some other chip and/or hypervisor
>> the default won't be 1.
>>
>
> Firmware does not have default policy, a caller must provide the policy
> value. In our cases, we are saying if caller does not provide a policy
> in xml then we default to 0x1.
>
As noted in my 6/10 response - you could change to making policy
required and then not worry about a default. It is a gray area, but it
will be in the long run some sort of hypervisor and even chip dependent
type field.
>> Also virXPathULongHex returns -1 or -2 w/ different meanings - if it's
>> not there, You get a -1 when not provided and -2 when you have invalid
>> value in field, which should be an error.
>>
>
> ah thats good information, I was not aware of -2 thing.
>
>
>> Finally, ULongHex returns 64 bits and your field is 32 bits leading to
>> possible overflow
>>
>
> Right, this is where I was struggling because there is no such function
> as virXPathUIntHex(...) which ca be used to get uint32_t. Please let me
> know your thoughts on how do you want me to handle this situation.
>
See my note above - I think the code below covers the UINT_MAX case
while using the ULong API.
Obviously if you make policy required, then the code changes a bit to
get/return rc and check rc vs. -1 for not found and vs. -2 for found,
but invalid (common occurrence elsewhere).
John
>
>
>
>> So, either you have to have a "policy_provided" boolean of some sort or
>> I think preferably leave it as 0 (zero) indicating not provided and then
>> when generating a command line check against 0 and provide the
>> hypervisor dependent value on the command line *OR* just don't provided
>> it an let the hypervisor choose it's default value because the value
>> wasn't provided (that's a later patch though)
>>
>> Also see [1] below...
>>
>> So my suggestion is:
>>
>> if (virXPathULongHex("string(./policy)", ctxt, &policy) == -2 ||
>> policy > UINT_MAX) {
>> virReportError(VIR_ERR_XML_ERROR, "%s",
>> _("invalid launch-security policy value"));
>> goto error;
>> }
>> def->policy = policy;
>>
>> If -1 is returned, the def->policy = 0; otherwise, it's set to something.
>>
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
