Re: [Xen-devel] [PATCH] xen: domain_update_node_affinity: Correct the ASSERT

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On ven, 2014-07-25 at 16:59 +0100, Jan Beulich wrote:
> >>> On 25.07.14 at 17:44, <julien.grall@xxxxxxxxxx> wrote:
> > Hi Jan,
> > 
> > On 07/25/2014 04:42 PM, Jan Beulich wrote:
> >>>>> On 25.07.14 at 17:30, <julien.grall@xxxxxxxxxx> 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));
> >> 
> >> Wouldn't it be better for the function to bail early in that case?
> > 
> > I've no idea. I mostly followed the advice on this thread:
> > 
> > http://www.gossamer-threads.com/lists/xen/devel/340233 
> 
> Yeah, I recall that discussion. But looking at the function, nothing
> useful will be done when the domain has no vCPU yet. Dario?
> 
For sure nothing useful happens if the for_each_vcpu() loop never
executes, because of lack of vcpus.

Functionally wise, bailing or going ahead, but avoiding the ASSERT to
trigger (as this patch does) is exactly the same.

I'm not sure which approach I personally prefer... If calling the
function without any allocated vcpus is a "legitimate" and frequent
enough use case, I'd say let's bail explicitly. If it's a corner case,
then I think this patch is ok.

It looks to me like we're in the latter situation (corner case), so I'm
actually fine with this patch. That's why I'm acking it, but really,
it's a matter of taste.. FWIW, I'd ack a version that bails too.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel