diff mbox series

eal/windows: detect insufficient privileges for hugepages

Message ID 20200707202203.8780-1-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal/windows: detect insufficient privileges for hugepages |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Dmitry Kozlyuk July 7, 2020, 8:22 p.m. UTC 
  AdjustTokenPrivileges() succeeds even if no requested privileges have
been granted; this behavior is documented. Check last error code in
addition to return value to detect such case.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/windows/eal_hugepages.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Menon, Ranjit July 7, 2020, 11:45 p.m. UTC | #1 
On 7/7/2020 1:22 PM, Dmitry Kozlyuk wrote:
> AdjustTokenPrivileges() succeeds even if no requested privileges have
> been granted; this behavior is documented. Check last error code in
> addition to return value to detect such case.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>   lib/librte_eal/windows/eal_hugepages.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
> index 5779cd325..07a5467d0 100644
> --- a/lib/librte_eal/windows/eal_hugepages.c
> +++ b/lib/librte_eal/windows/eal_hugepages.c
> @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
>   		goto exit;
>   	}
>   
> +	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
> +	if (GetLastError() != ERROR_SUCCESS)
> +		goto exit;
> +
>   	ret = 0;
>   
>   exit:

Wouldn't this be better if we could print a message here after 
explicitly checking for the ERROR_NOT_ALL_ASSIGNED return value?

Otherwise, the caller simply gets a -1 return value for a failure with 
no message.


ranjit m.
Dmitry Kozlyuk July 8, 2020, 12:20 a.m. UTC | #2 
On Tue, 7 Jul 2020 16:45:07 -0700, Ranjit Menon wrote:
> On 7/7/2020 1:22 PM, Dmitry Kozlyuk wrote:
> > AdjustTokenPrivileges() succeeds even if no requested privileges have
> > been granted; this behavior is documented. Check last error code in
> > addition to return value to detect such case.
> >
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> >   lib/librte_eal/windows/eal_hugepages.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
> > index 5779cd325..07a5467d0 100644
> > --- a/lib/librte_eal/windows/eal_hugepages.c
> > +++ b/lib/librte_eal/windows/eal_hugepages.c
> > @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
> >   		goto exit;
> >   	}
> >   
> > +	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
> > +	if (GetLastError() != ERROR_SUCCESS)
> > +		goto exit;
> > +
> >   	ret = 0;
> >   
> >   exit:  
> 
> Wouldn't this be better if we could print a message here after 
> explicitly checking for the ERROR_NOT_ALL_ASSIGNED return value?
> 
> Otherwise, the caller simply gets a -1 return value for a failure with 
> no message.

Message is printed at ERR level by the caller. There's no context to add here.
Tal Shnaiderman July 8, 2020, 6:43 a.m. UTC | #3 
> Subject: Re: [PATCH] eal/windows: detect insufficient privileges for
> hugepages
> 
> On Tue, 7 Jul 2020 16:45:07 -0700, Ranjit Menon wrote:
> > On 7/7/2020 1:22 PM, Dmitry Kozlyuk wrote:
> > > AdjustTokenPrivileges() succeeds even if no requested privileges
> > > have been granted; this behavior is documented. Check last error
> > > code in addition to return value to detect such case.
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > ---
> > >   lib/librte_eal/windows/eal_hugepages.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > >
> > > diff --git a/lib/librte_eal/windows/eal_hugepages.c
> > > b/lib/librte_eal/windows/eal_hugepages.c
> > > index 5779cd325..07a5467d0 100644
> > > --- a/lib/librte_eal/windows/eal_hugepages.c
> > > +++ b/lib/librte_eal/windows/eal_hugepages.c
> > > @@ -41,6 +41,10 @@ hugepage_claim_privilege(void)
> > >   		goto exit;
> > >   	}
> > >
> > > +	/* AdjustTokenPrivileges() may succeed with
> ERROR_NOT_ALL_ASSIGNED. */
> > > +	if (GetLastError() != ERROR_SUCCESS)
> > > +		goto exit;
> > > +
> > >   	ret = 0;
> > >
> > >   exit:
> >
> > Wouldn't this be better if we could print a message here after
> > explicitly checking for the ERROR_NOT_ALL_ASSIGNED return value?
> >
> > Otherwise, the caller simply gets a -1 return value for a failure with
> > no message.
> 
> Message is printed at ERR level by the caller. There's no context to add here.
> 

Tested successfully on Windows server 2019.
 
The output in case of missing privilege is:

EAL: Cannot claim hugepage privilege
EAL: FATAL: Cannot get hugepage information
EAL: Cannot get hugepage information

Do you think this is enough or do we want to give the user further indication that configuration action from his side might be needed? Something alike "Please verify OS Large-Page support is enabled for the current user"

> --
> Dmitry Kozlyuk
Tested-by: Tal Shnaiderman <talshn@mellanox.com>
Dmitry Kozlyuk July 8, 2020, 7:49 a.m. UTC | #4 
On Wed, 8 Jul 2020 06:43:57 +0000, Tal Shnaiderman wrote:
[snip]

Thanks for the testing.

> Do you think this is enough or do we want to give the user further indication that configuration action from his side might be needed? Something alike "Please verify OS Large-Page support is enabled for the current user"

Yes, troubleshooting suggestions would be useful.

From Microsoft "Error Message Guidelines" [1]:

	Avoid the word "please". It can be interpreted to mean that a
	required action is optional.

[1]:
https://docs.microsoft.com/en-us/windows/win32/debug/error-message-guidelines
diff mbox series

Patch

diff --git a/lib/librte_eal/windows/eal_hugepages.c b/lib/librte_eal/windows/eal_hugepages.c
index 5779cd325..07a5467d0 100644
--- a/lib/librte_eal/windows/eal_hugepages.c
+++ b/lib/librte_eal/windows/eal_hugepages.c
@@ -41,6 +41,10 @@  hugepage_claim_privilege(void)
 		goto exit;
 	}
 
+	/* AdjustTokenPrivileges() may succeed with ERROR_NOT_ALL_ASSIGNED. */
+	if (GetLastError() != ERROR_SUCCESS)
+		goto exit;
+
 	ret = 0;
 
 exit: