Re: [dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.

[Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>]

Hi Jean,

There are some format issues with the patch:

You can run scripts/check-git-log.sh to check them:
Wrong headline format:
         eal: Fix misleading error messages, errno can't be trusted.
Wrong headline uppercase:
         eal: Fix misleading error messages, errno can't be trusted.
Missing 'Fixes' tag:
         eal: Fix misleading error messages, errno can't be trusted.

The script's output highlights the different issues.


On 21/09/2016 22:10, Jean Tourrilhes wrote:
>   lib/librte_eal/linuxapp/eal/eal.c        | 14 +++++++++++---
>   lib/librte_eal/linuxapp/eal/eal_memory.c | 16 ++++++++++++----
>   2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 3fb2188..5df9f6a 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -238,7 +238,8 @@ rte_eal_config_attach(void)
>   	mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
>   			PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
>   	if (mem_config == MAP_FAILED)
> -		rte_panic("Cannot mmap memory for rte_config\n");
> +		rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
> +			  errno, strerror(errno));
>   
>   	rte_config.mem_config = mem_config;
>   }
> @@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
>   	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>   			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
>   			mem_cfg_fd, 0);
> +	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> +		if (mem_config != MAP_FAILED)
> +			/* errno is stale, don't use */
> +			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n",
> +				  rte_mem_cfg_addr, mem_config);
> +		else
> +			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
> +				  errno, strerror(errno));
> +	}
>   	close(mem_cfg_fd);
> -	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
> -		rte_panic("Cannot mmap memory for rte_config\n");
>   

NIT but any reason you moved the check before closing the file 
descriptor? (not that it matters with current code as we panic anyway)

Thanks,
Sergio

>   	rte_config.mem_config = mem_config;
>   }
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 41e0a92..b036ffc 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -1615,10 +1615,18 @@ rte_eal_hugepage_attach(void)
>   				 PROT_READ, MAP_PRIVATE, fd_zero, 0);
>   		if (base_addr == MAP_FAILED ||
>   		    base_addr != mcfg->memseg[s].addr) {
> -			RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
> -				"in /dev/zero to requested address [%p]: '%s'\n",
> -				(unsigned long long)mcfg->memseg[s].len,
> -				mcfg->memseg[s].addr, strerror(errno));
> +			if (base_addr != MAP_FAILED)
> +				/* errno is stale, don't use */
> +				RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
> +					"in /dev/zero at [%p], got [%p] - "
> +					"please use '--base-virtaddr' option\n",
> +					(unsigned long long)mcfg->memseg[s].len,
> +					mcfg->memseg[s].addr, base_addr);
> +			else
> +				RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
> +					"in /dev/zero at [%p]: '%s'\n",
> +					(unsigned long long)mcfg->memseg[s].len,
> +					mcfg->memseg[s].addr, strerror(errno));
>   			if (aslr_enabled() > 0) {
>   				RTE_LOG(ERR, EAL, "It is recommended to "
>   					"disable ASLR in the kernel "