Re: [libvirt] [PATCHv2 3/3] xen_common: convert to typesafe virConf accessors
On Mon, May 28, 2018 at 9:16 AM, Ján Tomko <jtomko(a)redhat.com> wrote:
On Sun, May 27, 2018 at 02:08:58PM +0200, Fabiano Fidêncio wrote:
>
> On Sun, May 27, 2018 at 1:17 PM, Ján Tomko <jtomko(a)redhat.com> wrote:
>>
>> On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
>>>
>>>
>>> From: Fabiano Fidêncio <fidencio(a)redhat.com>
>>>
>>> There are still some places using virConfGetValue() and then checking
>>> the specific type of the pointers and so on.
>>>
>>> Those place are not going to be changed as:
>>> - Directly using virConfGetValue*() would trigger virReportError() on
>>> their current code
>>
>>
>>
>> Is that a problem in xenParseCPUFeatures?
>
>
> It would, at least, generate one more log, which would be misleading
> whoever ends up debugging some issue on that codepath later on.
>
I don't see it.
xenConfigGetULong already reports an error when the "maxvcpus" value is
malformed.
What xenConfigGetULong does is:
val = virConfGetValue()
if (val->type == _ULLONG) {
...
} else if (val->type == _STRING) {
virStrToLong_ul(...) ...
and in case of failure, it reports an error;
} else {
an error is reported
}
If we simply try to do something similar with virConfGetValue ... we'd
end up with:
if (virConfGetValueULLong() < 0) {
/* here, in case virConfGetValueULLong fails, an error is already
reported ...
and we don't want it to happen, as the config may come as a string */
if (virConfGetString() ... {
}
}
So, summing up, the main difference is that checking by ULLONG in the
current approach doesn't generate any error/failure. While checking
for ULLONG wih virConfGetValueULLong() would trigger the error from
inside the function.
One thing that could be done ... expand virConfGetValueULLong() to
also support receiving its value as VIR_CONF_STRING. I've talked to
Michal about that and he explicitly mentioned that this may not be the
way to go and then I've decided to leave the code as it is.
Is it clear? Did I miss something?
Best Regards,
--
Fabiano Fidêncio