[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
2016-10-04 09:03, Sergio Gonzalez Monroy: > On 03/10/2016 21:46, Thomas Monjalon wrote: > > 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" [...] > > Sergio, do you have more comments? > > Nop. > > > Should we wait another version or is it OK? > > Maybe you'd prefer to rework it yourself? > > AFAICS it really is just the commit message formatting, so maybe you can > fix that when applying it? Yes but I've just noticed that the Signed-off-by line is missing. Please Jean, could you add this required line with your explanations, rebase on fresh head and resend please?
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
On 03/10/2016 21:46, Thomas Monjalon wrote:
> 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?
Nop.
> Should we wait another version or is it OK?
> Maybe you'd prefer to rework it yourself?
AFAICS it really is just the commit message formatting, so maybe you can
fix that when applying it?
Sergio
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
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?
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jean Tourrilhes > Sent: Monday, October 3, 2016 4:56 PM > To: Gonzalez Monroy, Sergio > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, > errno can't be trusted. > > 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. The longer more detailed version is here: "Contributing Code to DPDK": http://dpdk.org/doc/guides/contributing/patches.html John
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
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 "
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
On Mon, Oct 03, 2016 at 04:15:11PM +, Mcnamara, John wrote: > > The longer more detailed version is here: "Contributing Code to DPDK": > > http://dpdk.org/doc/guides/contributing/patches.html > > John Thanks a lot. I'll try to find time to look at it. Jean
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
On Mon, Oct 03, 2016 at 02:25:40PM +0100, Sergio Gonzalez Monroy wrote: > Hi Jean, > > 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 More details, as I admit I was terse Running secondary is tricky due to the need to map the memory region at the right place, which is whatever primary has chosen. If the base address for primary happens to by already mapped in the secondary, we will hit precisely this error message (well, in a few case we might hit the other one). This is why there is already a comment about ASLR. A colleague of mine hit that message and was misled by errno claiming "permission denied", which sent him down the wrong track. It's such a common error for secondary that I feel this error message should be unambiguous and helpful. Regards, Jean
[dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.
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.
> 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.
> Thanks,
> Sergio
Thanks for the review !
Jean
