[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Michal Orzel <michal.orzel@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 4 May 2022 10:13:56 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=4eT6ziFmR+oEmt5PfVan7mIVFt45G6awZJDDsAjaG+Y=; b=kqYsNVXAlBworOWZhlUlpvR7s+655dbsp9BlORQ9JLsaCC0ARAsi/0tUE5sX+01M/tGjLJX2PY+D6ipJrX+/n4zWXH+68fiygNZ1xHg656huWdW2Z+EgrOtECEsYDQv73l0xipNT/+EFXeb+B8FHFc8HMmWXaLUENFpCYMcUw0ZmhUMH/cUTflbhim4OOuwRgiX2osvg9tn3oFPvpnkH3hRbCkDThklqztPuQUTx6Ud7C1A5jStcwkwHZVobXkaPoN35FUovCcMTqGLbXKhSjU02Cv28CEhmFX5O5efJnmZtFblRk8ZhpzRDRnBHa0k0rfnH8WE4A1n8lxMEEh85Xw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jMN1efV8sFPLIqOWJ2dPbOqa+feueJZMWzt4kVbYWnTXpn29z1Adnl2ByZ3d6fH7v3GB5+WzmcbwEyl2pMhtdEwFjFzvhojw21G7ofPRi1ADq2LLorATvfY7RIa0MUkIf84nwx3Z6rZ9q50EkEI9VWicRo3yBsTyM2iTj2T2ea3iQupmBHkFMmhsAuLr9b1sOJZEgbSrRxqBbHAElVMnxyCnXXrd9iY/SyPo2wxh7DLlSi3TRuz/g/zaTnfbF0oz0E3b9MxlRg4BQVH4+1RYk2uXNiga3LWyk9wUL3p+tG6mn3U6sTAAAakajLFU2y8UzM3KBz35dRoknD5wT69F8Q==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 04 May 2022 08:14:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.