Re: [libvirt] [PATCH v5 03/10] conf: introduce launch-security element in domain
[...]
>
> 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.
>
[...]