On 05.05.2022 13:25, Michal Orzel wrote:
> 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.
Hmm, that's pretty odd (and Arm specific). Just like with the other remark
above, it'll be the Arm maintainers to judge, but here I think the NULLs
would better be done away with, by introducing intermediate macros, e.g.
#define gnttab_shared_gfn_(t, i) ...
#define gnttab_shared_gfn(d, t, i) ({ \
const typeof(t) t_ = (t); \
ASSERT((d)->grant_table == t_); \
gnttab_shared_gfn_(t_, i); \
})
#define gnttab_get_frame_gfn(gt, st, idx) ({ \
(st) ? gnttab_status_gfn_(gt, idx) \
: gnttab_shared_gfn_(gt, idx); \
})
Jan
