Re: [Xen-devel] [PATCH v8 3/8] xen/arm: return int *_dcache_va_range

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 20 Oct 2014, Jan Beulich wrote:
> >>> On 20.10.14 at 16:57, <Ian.Campbell@xxxxxxxxxx> wrote:
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
> >>  {
> >>      void *v = map_domain_page(mfn);
> >>  
> >> -    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> >> +    ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
> > 
> > Just this one assert? What about all the other callers of
> > *_dcache_va_range?

There are many many callers under xen/arch/arm, should I really add
ASSERT everywhere?
Maybe I could just add an ASSERT on the return value within
clean_dcache_va_range and clean_and_invalidate_dcache_va_range?

Or should I introduce wrappers with the ASSERT?


> I'm glad I looked at this reply (I skipped the original one because of
> being ARM-only): Uses of ASSERT() like above won't work in non-
> debug builds. You have to latch the result into a variable and assert
> on it. The fundamental rule is: Expressions passed to ASSERT() must
> never have side effects. That's different from BUG_ON(), but even
> there Keir advocates not using expressions with side effects, aiui
> both to avoid the above mistake (i.e. getting used to doing it the
> right way) and to leave open the option of adding a build option to
> eliminate the BUG()s and BUG_ON()s too.

Jan, well spotted, thanks!

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