Re: [PATCH] xen: domain_update_node_affinity: Correct the ASSERT

[Julien Grall <julien.grall@linaro.org>]

On 08/04/2014 03:50 PM, George Dunlap wrote:
> On 08/01/2014 04:12 PM, Jan Beulich wrote:
>>>>> On 01.08.14 at 16:52, <julien.grall@linaro.org> wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -449,7 +449,7 @@ void domain_update_node_affinity(struct domain *d)
>>>           }
>>>           /* Filter out non-online cpus */
>>>           cpumask_and(dom_cpumask, dom_cpumask, online);
>>> -        ASSERT(!cpumask_empty(dom_cpumask));
>>> +        ASSERT( !d->vcpu || !d->vcpu[0] ||
>>> !cpumask_empty(dom_cpumask));
>>>           /* And compute the intersection between hard, online and
>>> soft */
>>>           cpumask_and(dom_cpumask_soft, dom_cpumask_soft, dom_cpumask);
>>>
>>
>> Actually, with sched_move_domain() having
>>
>>      /* Do we have vcpus already? If not, no need to update
>> node-affinity */
>>      if ( d->vcpu )
>>          domain_update_node_affinity(d);
>>
>> it should really just be _that_ if() condition to get extended, and the
>> ASSERT() left alone altogether. Or, if any other path can be proven
>> to possibly reach the function with no vCPU allocated (I just went
>> through them and didn't spot any), then it should really be an early
>> bail from the function rather than a pointlessly complicated ASSERT()
>> expression. (And for the record, your expression has a coding style
>> violation anyway in that it begins with a space.)
> 
> I think changing the if() was what Julien started with; but overall I
> think that it makes more sense to update the assumption of the code in
> question than to require all the callers to be careful not to trip over it.
> 
> Doing an early bail might make sense as well.

Ok. So which one should I choose? The early bail out?

Regards,

-- 
Julien Grall