Re: [PATCH v2] xen/arm: gnttab: cast unused macro arguments to void

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2022 08:41, Michal Orzel wrote:
> On 03.05.2022 19:44, Julien Grall wrote:
>> On 28/04/2022 10:46, Michal Orzel wrote:
>>> @@ -89,10 +90,12 @@ int replace_grant_host_mapping(unsigned long gpaddr, 
>>> mfn_t mfn,
>>>   })
>>>     #define gnttab_shared_gfn(d, t, i)                                      
>>>  \
>>> -    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
>>> +    ((void)(d),                                                          \
>>> +     ((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
>>>   -#define gnttab_status_gfn(d, t, i)                                       
>>> \
>>> -    (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
>>> +#define gnttab_status_gfn(d, t, i)                                        \
>>> +    ((void)(d),                                                           \
>>> +     ((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
>>
>> I share Jan's opinion here. If we want to evaluate d, then we should make 
>> sure t and i should be also evaluated once. However, IIRC, they can't be 
>> turned to static inline because the type of t (struct grant_table) is not 
>> fully defined yet.
>>
> Then, we could do like this:
> 
> #define gnttab_shared_gfn(d, t, i)                                       \
>     ({                                                                   \
>         const unsigned int _i = (i);                                     \
>         const struct grant_table *_t = (t);                              \
>         (void)(d);                                                       \
>         (_i >= nr_grant_frames(_t)) ? INVALID_GFN                        \
>                                     : _t->arch.shared_gfn[_i];           \
>     })

Please avoid underscore-prefixed names here; we've started to use
underscore-suffixed names in a few macros.

Additionally please consider using typeof() instead of spelling out
types. This may help to avoid surprising behavior.

Finally, instead of merely casting d to void, please consider using it
in e.g. ASSERT((d)->grant_table == t_), which ought to also take care
of the unused variable warning. After all the explicit passing of t is
only an (attempted) optimization here.

> However, if we start modifying the macros to evaluate args only once, 
> shouldn't we also take care of the following macros in this file?:
> gnttab_set_frame_gfn
> gnttab_init_arch
> 
> I'm ok to do these changes but I'm afriad we are losing the origin of this 
> patch as we are focusing on macros not related to the issue.

Indeed - I'd leave further ones to a subsequent patch, or make
conversion of all of the macros a prereq patch to the one you're after.

Jan