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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxx>

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@xxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> ---
>  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).

 -George


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