Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2022 09:16, Michal Orzel wrote:
> Hi Andrew, Jan
> 
> On 27.04.2022 14:33, Andrew Cooper wrote:
>> On 27/04/2022 12:06, Michal Orzel wrote:
>>> Hi Jan,
>>>
>>> On 27.04.2022 11:59, Jan Beulich wrote:
>>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>>> this macro does not make use of any arguments causing a compiler to
>>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>>> to gnttab_host_mapping_get_page_type.
>>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>>> rather the Arm macro which wants adjusting to not leave this argument
>>>> unused.
>>>>
>>> I would agree provided that the ld variable was used in more than one place.
>>> As it is not, it does not seem very beneficial to keep a variable that is 
>>> used
>>> just in one place and stores the macro value.
>>>
>>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is 
>>> defined as (0)
>>> so modifying it seems to be a quite big overhead.
>>
>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>> b/xen/arch/arm/include/asm/grant_table.h
>> index d31a4d6805d6..9f68c2a37eb6 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>> *d, mfn_t mfn)
>>  
>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>                                unsigned int flags, unsigned int
>> cache_flags);
>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>                                 unsigned long new_gpaddr, unsigned int
>> flags);
>> -#define gnttab_release_host_mappings(domain) 1
>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>  
>>  /*
>>   * The region used by Xen on the memory will never be mapped in DOM0
>>
>> It's about parameter evaluation, not about adding extra code when compiled.
>>
> 
> Unfortunately, solution presented by Andrew does not work.
> We will get the following error due to -Werror=unused-value:
> "left-hand operand of comma expression has no effect"

Perhaps

#define gnttab_host_mapping_get_page_type(ro, ld, rd) \
    ((void)(ro), (void)(ld), (void)(rd), 0)

?

Jan