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

Thomas Monjalon thomas.monjalon at 6wind.com
Mon Oct 3 22:46:57 CEST 2016


2016-10-03 08:55, Jean Tourrilhes:
> On Mon, Oct 03, 2016 at 02:25:40PM +0100, Sergio Gonzalez Monroy wrote:
> > 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.
> 
> 	SOrry about that, I casually read the page on
> http://dpdk.org/dev, but obviously I need to look at it again.

No problem. This guide is more oriented towards regular contributors.
You come with a bug and its fix, we can make some effort to format
the patch :)

The title could be "mem: fix hugepage mapping error messages"

> > On 21/09/2016 22:10, Jean Tourrilhes wrote:
> > >@@ -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)
> 
> 	"close()" may change "errno" according to its man page.

Sergio, do you have more comments?
Should we wait another version or is it OK?
Maybe you'd prefer to rework it yourself?


More information about the dev mailing list