Re: [Xen-devel] [PATCH v8 12/15] xen/arm: move arch specific grant table bits into grant_table.c

[Juergen Gross <jgross@xxxxxxxx>]

On 20/09/17 16:34, Julien Grall wrote:
> Hi Juergen,
> 
> On 20/09/17 07:34, Juergen Gross wrote:
>> diff --git a/xen/include/asm-arm/grant_table.h
>> b/xen/include/asm-arm/grant_table.h
>> index 0a248a765a..0870b5b782 100644
>> --- a/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -6,6 +6,10 @@
>>     #define INITIAL_NR_GRANT_FRAMES 4
>>   +struct grant_table_arch {
>> +    gfn_t *gfn;
>> +};
>> +
>>   void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
>>   int create_grant_host_mapping(unsigned long gpaddr,
>>           unsigned long mfn, unsigned int flags, unsigned int
>> @@ -22,11 +26,19 @@ static inline int replace_grant_supported(void)
>>       return 1;
>>   }
>>   -static inline void gnttab_set_frame_gfn(struct domain *d, unsigned
>> long idx,
>> -                                        gfn_t gfn)
>> -{
>> -    d->arch.grant_table_gfn[idx] = gfn;
>> -} > +#define gnttab_init_arch(gt) 
>     \
>> +    ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) ==
>> 0     \
> 
> I am not sure to understand the 0 here. Don't you check the return of
> xzalloc_array? If so it should be NULL if it failed. And therefore the
> return error looks inverted below.

Indeed.

> 
>> +      ? 0 : -ENOMEM )
> 
> I admit I would much prefer to see static inline rather than define.
> More typesafe and usually easier to read. You cannot do that because the
> type of gt is only defined in grant-table?

Right.

> Nonetheless, I think you could clarify this code by doing:
> 
> ({
>    (gt)->arch.gfn = xzalloc_array(....);
>    ( g->arch.gfn ? 0 : -ENOMEM );
> )}

Yes, this is better.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel