Re: [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Jan,

On 05.05.2022 13:20, Jan Beulich wrote:
> On 05.05.2022 12:36, Michal Orzel wrote:
>> Modify macros to evaluate all the arguments and make sure the arguments
>> are evaluated only once. While doing so, use typeof for basic types
>> and use const qualifier when applicable.
> 
> Why only for basic types? To take an example, passing void * into
> gnttab_need_iommu_mapping() would imo also better not work.
> 
Just by looking at the majority of macros in Xen, typeof is used mostly for 
basic data types.
Also I think it is better to explictly use a struct type for better readability.
Otherwise one need to search in other files, to what type does typeof evaluates.

>> @@ -98,13 +104,36 @@ 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])
>> +    ({                                                                   \
>> +        const struct domain *d_ = (d);                                   \
>> +        const struct grant_table *t_ = (t);                              \
>> +        const typeof(i) i_ = (i);                                        \
>> +                                                                         \
>> +        if ( d_ != NULL )                                                \
>> +            ASSERT(d_->grant_table == t_);                               \
> 
> I'm puzzled by this NULL check (and the similar instance further down):
> Are you suggesting NULL can legitimately come into here? If not, maybe
> better ASSERT(d_ && d_->grant_table == t_)?
> 
Example:
NULL is coming explictly from macro gnttab_get_frame_gfn right above 
gnttab_shared_gfn.

> Jan
> 
Cheers,
Michal