Re: [Xen-devel] [PATCH 1/2] x86: separate out sanitize_e820_map return codes

[David Vrabel <david.vrabel@citrix.com>]

On 14/10/14 03:30, Martin Kelly wrote:
> Previously, sanitize_e820_map returned -1 in all cases in which it did
> nothing. However, sanitize_e820_map can do nothing either because the
> input map has size 1 (this is ok) or because the input map passed in is
> invalid (likely an issue). It is nice for the caller to be able to
> distinguish the two cases and treat them separately.

Wouldn't it be more sensible to return 0 (success) in the case of a
single entry map?  IMO, a 1 entry map is by definition sanitized.

David

> --- a/arch/x86/include/asm/e820.h
> +++ b/arch/x86/include/asm/e820.h
> @@ -12,6 +12,11 @@
>  /* see comment in arch/x86/kernel/e820.c */
>  extern struct e820map e820;
>  extern struct e820map e820_saved;
> +/* sanitize_e820_map return codes */
> +#define E820_RC_ONLY_ONE (-1)  /* return code when there's only one memory
> +				  region in the map */
> +#define E820_RC_BAD_MAP (-2)   /* return code when passed a map containing an
> +				  invalid memory region */
>  
>  extern unsigned long pci_mem_start;
>  extern int e820_any_mapped(u64 start, u64 end, unsigned type);
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 49f8864..3e1fd63 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -189,11 +189,15 @@ void __init e820_print_map(char *who)
>   * (something no more than max_nr_map.)
>   *
>   * The return value from sanitize_e820_map() is zero if it
> - * successfully 'sanitized' the map entries passed in, and is -1
> - * if it did nothing, which can happen if either of (1) it was
> - * only passed one map entry, or (2) any of the input map entries
> - * were invalid (start + size < start, meaning that the size was
> - * so big the described memory range wrapped around through zero.)
> + * successfully 'sanitized' the map entries passed in and negative if it did
> + * nothing. There are two cases in which sanitize_e820_map() does nothing:
> + * (1) it was passed only one map entry, so nothing needs to be done. In this
> + *     case, it returns E820_RC_ONLY_ONE.
> + * (2) any of the input map entries * were invalid (start + size < start)
> + *     meaning that the size was so big the described memory range wrapped
> + *     around through zero. In this case, it returns E820_RC_BAD_MAP.
> + * Since (1) is sometimes an expected case and (2) indicates an error, the
> + * distinct return codes allow callers to handle the two cases separately.
>   *
>   *	Visually we're performing the following
>   *	(1,2,3,4 = memory types)...
> @@ -269,7 +273,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
>  
>  	/* if there's only one memory region, don't bother */
>  	if (*pnr_map < 2)
> -		return -1;
> +		return E820_RC_ONLY_ONE;
>  
>  	old_nr = *pnr_map;
>  	BUG_ON(old_nr > max_nr_map);
> @@ -277,7 +281,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
>  	/* bail out if we find any unreasonable addresses in bios map */
>  	for (i = 0; i < old_nr; i++)
>  		if (biosmap[i].addr + biosmap[i].size < biosmap[i].addr)
> -			return -1;
> +			return E820_RC_BAD_MAP;
>  
>  	/* create pointers for initial change-point information (for sorting) */
>  	for (i = 0; i < 2 * old_nr; i++)
>