On 11/22/2017 04:45 AM, Michal Privoznik wrote:
> On 11/22/2017 12:38 AM, John Ferlan wrote:
>>
>>
>> On 11/14/2017 09:47 AM, Michal Privoznik wrote:
>>> The function returns true/false depending on distance
>>> configuration being present in the domain XML.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>>> ---
>>> src/conf/numa_conf.c | 13 +++++++++++++
>>> src/conf/numa_conf.h | 4 ++++
>>> src/libvirt_private.syms | 1 +
>>> 3 files changed, 18 insertions(+)
>>>
>>> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
>>> index 5f0b3f9ed..6a42777e2 100644
>>> --- a/src/conf/numa_conf.c
>>> +++ b/src/conf/numa_conf.c
>>> @@ -1137,6 +1137,19 @@ virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
>>> return numa->nmem_nodes;
>>> }
>>>
>>
>> Two blank lines here too.
>>
>>> +bool
>>> +virDomainNumaNodeDistanceSpecified(virDomainNumaPtr numa,
>>> + size_t node,
>>> + size_t sibling)
>>> +{
>>> + return node < numa->nmem_nodes &&
>>> + sibling < numa->nmem_nodes &&
>>> + numa->mem_nodes[node].distances &&
>>> + numa->mem_nodes[node].distances[sibling].value != LOCAL_DISTANCE &&
>>> + numa->mem_nodes[node].distances[sibling].value != REMOTE_DISTANCE;
>>> +}
>>> +
>>> +
>>
>> According to how I read commit message '74119a03' it *is* possible to
>> set the @value to the same value as LOCAL_DISTANCE(10) or
>> REMOTE_DISTANCE(20) - so using that as a comparison for whether it was
>> specified would seem to be wrong.
>
> Sure it is possible. But see my reply to 4/5 why I need this function.
>
>>
>> Still if a distance *is* provided, then it seems that 'id' and 'value'
>> are also required to be provided or defaulted. That means what seems to
>> matter regarding whether something was provided is if the *.value and/or
>> "*.cellid are zero
>>
>> At least that's how I'm reading virDomainNumaDefNodeDistanceParseXML
>
> Okay, so the name is confusing. Would you be okay if I rename this to
> virDomainNumaNodeDistanceSpecifiedAndNotDefault()? Ugrh, long name. Any
> suggestion is welcome.
Sometimes comments to the function that state what it's returning help -
especially the long single boolean condition.
Just remember, naming is hard! How about
virDomainNumaNodeDistanceIsUsingDefaults? The prefix
virDomainNumaNodeDistance makes names longer too...
Not sure I was as hung up over the name as I was the functionality that
at this point in time didn't make as much sense. Although paired with
your comments in patch 4, perhaps make more sense.
I'm also not a fan of the large single condition for the bool function -
especially where some conditions are positive and some are negative, but
that probably just me... If I try to decompose it...
/* Out of range, not created, or not supplied */
if (node >= numa->nmem_nodes || sibling >= numa->nmem_nodes ||
!numa->mem_nodes[node].distances ||
numa->mem_nodes[node].distances[sibling].value == 0)
return false;
/* Supplied using default distances */
if (numa->mem_nodes[node].distances[sibling].value == LOCAL_DISTANCE ||
numa->mem_nodes[node].distances[sibling].value == REMOTE_DISTANCE)
return true;
return false;
>
> Alternatively, I can move LOCAL/REMOTE_DISTANCE macros to numa_conf.h,
> give them proper prefix and call virDomainNumaGetNodeDistance() from
> qemu and check if returned value is LOCAL/REMOTE.
Hiding comparisons behind some other function that requires me to jump
elsewhere which can be painful too.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
