Re: [PATCH 2/2] x86/mm: Annotate gfn_get_* helpers as requiring non-NULL parameters

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/07/16 16:58, George Dunlap wrote:
> On 27/07/16 19:08, Andrew Cooper wrote:
>> Introduce and use the nonnull attribute to help the compiler catch NULL
>> parameters being passed to function which require their parameters not to be
>> NULL.  Experimentally, GCC 4.9 on Debian Jessie only warns of non-NULL-ness
>> from immediate callers, so propagate the attributes out to all helpers.
>>
>> A sample error looks like:
>>
>> mem_sharing.c: In function ‘mem_sharing_nominate_page’:
>> mem_sharing.c:884:13: error: null argument where non-null required (argument 3) [-Werror=nonnull]
>>              amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>>              ^
>>
>> As part of this, replace the get_gfn_type_access() macro with an equivalent
>> static inline function for extra type safety, and the ability to be annotated.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> At a high level this looks like it's probably an improvement; I'd like
> to hear opinions of people who tend to have stronger opinions here first.
>
> One technical comment...
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> ---
>>  xen/include/asm-x86/p2m.h  | 19 +++++++++++--------
>>  xen/include/xen/compiler.h |  2 ++
>>  2 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 194020e..e35d59c 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -380,9 +380,9 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m);
>>   * After calling any of the variants below, caller needs to use
>>   * put_gfn. ****/
>>  
>> -mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn,
>> -                    p2m_type_t *t, p2m_access_t *a, p2m_query_t q,
>> -                    unsigned int *page_order, bool_t locked);
>> +mfn_t __nonnull(1, 3, 4) __get_gfn_type_access(
> __get_gfn_type_access() explicitly tolerates p2m being NULL, so '1'
> should be removed from the list (both here and below).

So it does.  I wonder why that is?  I presume PV guests don't have a p2m.

Looking though this code, it seems to be an unnecessarily complicated
tangle :s

~Andrew

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