Re: [Xen-devel] [PATCH] x86: fix cache flushing condition in map_pages_to_xen()

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>>> Keir Fraser <keir.xen@xxxxxxxxx> 07/17/13 6:31 PM >>>
>On 17/07/2013 17:01, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>>> On 17.07.13 at 17:40, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 17/07/13 16:09, Jan Beulich wrote:
>>>> +#define flush_flags(oldf) do {                                  \
>>>> +    unsigned int o_ = (oldf);                                   \
>>>> +    if ( (o_) & _PAGE_GLOBAL )                                  \
>>> +        flush_flags |= FLUSH_TLB_GLOBAL;                        \
>>>> +    if ( (flags & _PAGE_PRESENT) &&                             \
>>>> +         (((o_) ^ flags) & PAGE_CACHE_ATTRS) )                  \
>>>> +        flush_flags |= FLUSH_CACHE;                             \
>>>> +} while (0)
>>>> +
>>> 
>>> I have to admit to being surprised that the compiler is even happy with
>>> a macro aliasing a variable, but please can it be renamed to something
>>> else (perhaps "set_flush_flags" ?) for the sanity of other humans trying
>>> to read the code.
>> 
>> Ugly. I actually picked the same name intentionally.
>
>I'm not too strongly opinionated on this one, but it did make me look twice.
>I think it would be cleaner something like:
>#define flush_flags(oldf) ({ unsigned int f_ = 0; ...; _f; })
>...
>flush_flags |= flush_flags(...);
>The 'name collision' I'm fine with, whereas going directly at a caller's
>variable within a macro is rather grubby behaviour. ;)

But that would still leave the macro access "flags" directly. And while I 
realize
that this is (slightly) odd behavior for a macro, that's precisely the reason 
why
I #undef it right after the function. Perhaps this could be made even more clear
by moving #define and #undef inside the function...

Jan


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