[v6,03/10] windows/eal: translate Windows errors to errno-style errors

Message ID 1617413948-10504-4-git-send-email-navasile@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series eal: Add new API for threading |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Narcisa Ana Maria Vasile April 3, 2021, 1:39 a.m. UTC
From: Narcisa Vasile <navasile@microsoft.com>

Add function to translate Windows error codes to
errno-style error codes.

Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
---
 lib/librte_eal/include/rte_thread.h |  5 +-
 lib/librte_eal/windows/rte_thread.c | 75 ++++++++++++++++++++++-------
 2 files changed, 60 insertions(+), 20 deletions(-)
  

Comments

Dmitry Kozlyuk April 29, 2021, 12:50 a.m. UTC | #1
2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> Add function to translate Windows error codes to
> errno-style error codes.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>

Commit topic should be "eal/windows", not "windows/eal".

> ---
>  lib/librte_eal/include/rte_thread.h |  5 +-
>  lib/librte_eal/windows/rte_thread.c | 75 ++++++++++++++++++++++-------
>  2 files changed, 60 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/librte_eal/include/rte_thread.h b/lib/librte_eal/include/rte_thread.h
> index bfdd8e1b1..2d7b3bc05 100644
> --- a/lib/librte_eal/include/rte_thread.h
> +++ b/lib/librte_eal/include/rte_thread.h
> @@ -221,9 +221,8 @@ int rte_thread_value_set(rte_thread_key key, const void *value);
>   *
>   * @return
>   *   On success, value data pointer (can also be NULL).
> - *   On failure, NULL and an error number is set in rte_errno.
> - *   rte_errno can be: EINVAL  - Invalid parameter passed.
> - *                     ENOEXEC - Specific OS error.
> + *   On failure, NULL and a positive error number is set in rte_errno.
> + *
>   */
>  __rte_experimental
>  void *rte_thread_value_get(rte_thread_key key);
> diff --git a/lib/librte_eal/windows/rte_thread.c b/lib/librte_eal/windows/rte_thread.c
> index b29336cbd..ecd2f810f 100644
> --- a/lib/librte_eal/windows/rte_thread.c
> +++ b/lib/librte_eal/windows/rte_thread.c
> @@ -12,6 +12,51 @@ struct eal_tls_key {
>  	DWORD thread_index;
>  };
>  
> +/* Translates the most common error codes related to threads */
> +static int rte_thread_translate_win32_error(void)

Return type should be on a separate line.

> +{
> +	DWORD error = 0;
> +
> +	error = GetLastError();

You could declare "error" and assign it in one statement.

Don't initialize variables if there's no good initial value:
it prevents compiler to issue a warning if the variable is not assigned a
meaningful value afterwards.

> +
> +	switch (error) {
> +	case ERROR_SUCCESS:
> +		return 0;
> +
> +	case ERROR_INVALID_PARAMETER:
> +		return EINVAL;
> +
> +	case ERROR_INVALID_HANDLE:
> +		return EFAULT;
> +
> +	case ERROR_NOT_ENOUGH_MEMORY:
> +	/* FALLTHROUGH */
> +	case ERROR_NO_SYSTEM_RESOURCES:
> +		return ENOMEM;
> +
> +	case ERROR_PRIVILEGE_NOT_HELD:
> +	/* FALLTHROUGH */
> +	case ERROR_ACCESS_DENIED:
> +		return EACCES;
> +
> +	case ERROR_ALREADY_EXISTS:
> +		return EEXIST;
> +
> +	case ERROR_POSSIBLE_DEADLOCK:
> +		return EDEADLK;
> +
> +	case ERROR_INVALID_FUNCTION:
> +	/* FALLTHROUGH */
> +	case ERROR_CALL_NOT_IMPLEMENTED:
> +		return ENOSYS;
> +
> +	default:
> +		return EINVAL;
> +	}

Default branch is redundant at the end of function.

> +
> +	return EINVAL;
> +}
> +
>  rte_thread_t
>  rte_thread_self(void)
>  {
> @@ -87,15 +132,13 @@ rte_thread_key_create(rte_thread_key *key,
>  	*key = malloc(sizeof(**key));
>  	if ((*key) == NULL) {
>  		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
> -		rte_errno = ENOMEM;
> -		return -1;
> +		return ENOMEM;
>  	}
>  	(*key)->thread_index = TlsAlloc();
>  	if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
>  		RTE_LOG_WIN32_ERR("TlsAlloc()");
>  		free(*key);
> -		rte_errno = ENOEXEC;
> -		return -1;
> +		return rte_thread_translate_win32_error();

Logging above can overwrite GetLastError() value.
I suggest splitting rte_thread_translate_win32_error() into translation part
for cases when you have error number already, and a wrapper that calls
GetLastError() to shorten calling code.

Same applies below in this file.

[...]
  
Narcisa Ana Maria Vasile April 30, 2021, 2:39 a.m. UTC | #2
On Thu, Apr 29, 2021 at 03:50:38AM +0300, Dmitry Kozlyuk wrote:
> 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> > From: Narcisa Vasile <navasile@microsoft.com>
> > 
> > Add function to translate Windows error codes to
> > errno-style error codes.
> > 
> > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> 
> Commit topic should be "eal/windows", not "windows/eal".
> 
> > ---
> >  lib/librte_eal/include/rte_thread.h |  5 +-
> >  lib/librte_eal/windows/rte_thread.c | 75 ++++++++++++++++++++++-------
> >  2 files changed, 60 insertions(+), 20 deletions(-)
> > 
> > diff --git a/lib/librte_eal/include/rte_thread.h b/lib/librte_eal/include/rte_thread.h
> > index bfdd8e1b1..2d7b3bc05 100644
> 
> > @@ -87,15 +132,13 @@ rte_thread_key_create(rte_thread_key *key,
> >  	*key = malloc(sizeof(**key));
> >  	if ((*key) == NULL) {
> >  		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
> > -		rte_errno = ENOMEM;
> > -		return -1;
> > +		return ENOMEM;
> >  	}
> >  	(*key)->thread_index = TlsAlloc();
> >  	if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
> >  		RTE_LOG_WIN32_ERR("TlsAlloc()");
> >  		free(*key);
> > -		rte_errno = ENOEXEC;
> > -		return -1;
> > +		return rte_thread_translate_win32_error();
> 
> Logging above can overwrite GetLastError() value.
> I suggest splitting rte_thread_translate_win32_error() into translation part
> for cases when you have error number already, and a wrapper that calls
> GetLastError() to shorten calling code.
> 

  Thanks Dmitry! I can split the translation function into something like:
  static int rte_thread_translate_win32_error(DWORD error) {
	switch(error) {
	    [...]
	}
  } 

  static int get_error_and_translate(void) {
     return rte_thread_translate_win32_error(GetLastError());
  }

  Is the above what you meant?

  Here, however, I don't think the wrapper over GetLastError() will be of much help, as 
  we still need to do something like:
  if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
       ret = GetLastError();
       RTE_LOG(DEBUG, EAL, "TlsAlloc() failed, GetLastError()=%lu: ", ret);
       free(*key);
       return rte_thread_translate_win32_error(ret);
  }

> Same applies below in this file.
> 
> [...]
  
Dmitry Kozlyuk April 30, 2021, 11:05 p.m. UTC | #3
2021-04-29 19:39 (UTC-0700), Narcisa Ana Maria Vasile:
> On Thu, Apr 29, 2021 at 03:50:38AM +0300, Dmitry Kozlyuk wrote:
> > 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:  
> > > From: Narcisa Vasile <navasile@microsoft.com>
[...]
> > >  	(*key)->thread_index = TlsAlloc();
> > >  	if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
> > >  		RTE_LOG_WIN32_ERR("TlsAlloc()");
> > >  		free(*key);
> > > -		rte_errno = ENOEXEC;
> > > -		return -1;
> > > +		return rte_thread_translate_win32_error();  
> > 
> > Logging above can overwrite GetLastError() value.
> > I suggest splitting rte_thread_translate_win32_error() into translation part
> > for cases when you have error number already, and a wrapper that calls
> > GetLastError() to shorten calling code.
> >   
> 
>   Thanks Dmitry! I can split the translation function into something like:
>   static int rte_thread_translate_win32_error(DWORD error) {
> 	switch(error) {
> 	    [...]
> 	}
>   } 
> 
>   static int get_error_and_translate(void) {
>      return rte_thread_translate_win32_error(GetLastError());
>   }
> 
>   Is the above what you meant?
> 
>   Here, however, I don't think the wrapper over GetLastError() will be of much help, as 
>   we still need to do something like:
>   if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
>        ret = GetLastError();
>        RTE_LOG(DEBUG, EAL, "TlsAlloc() failed, GetLastError()=%lu: ", ret);
>        free(*key);
>        return rte_thread_translate_win32_error(ret);
>   }

I'm afraid we don't have a choice, the logs with exact Win32 API call and its
result is very handy for debugging. Patches 4/10 and 5/10 shorten it a bit.
  

Patch

diff --git a/lib/librte_eal/include/rte_thread.h b/lib/librte_eal/include/rte_thread.h
index bfdd8e1b1..2d7b3bc05 100644
--- a/lib/librte_eal/include/rte_thread.h
+++ b/lib/librte_eal/include/rte_thread.h
@@ -221,9 +221,8 @@  int rte_thread_value_set(rte_thread_key key, const void *value);
  *
  * @return
  *   On success, value data pointer (can also be NULL).
- *   On failure, NULL and an error number is set in rte_errno.
- *   rte_errno can be: EINVAL  - Invalid parameter passed.
- *                     ENOEXEC - Specific OS error.
+ *   On failure, NULL and a positive error number is set in rte_errno.
+ *
  */
 __rte_experimental
 void *rte_thread_value_get(rte_thread_key key);
diff --git a/lib/librte_eal/windows/rte_thread.c b/lib/librte_eal/windows/rte_thread.c
index b29336cbd..ecd2f810f 100644
--- a/lib/librte_eal/windows/rte_thread.c
+++ b/lib/librte_eal/windows/rte_thread.c
@@ -12,6 +12,51 @@  struct eal_tls_key {
 	DWORD thread_index;
 };
 
+/* Translates the most common error codes related to threads */
+static int rte_thread_translate_win32_error(void)
+{
+	DWORD error = 0;
+
+	error = GetLastError();
+
+	switch (error) {
+	case ERROR_SUCCESS:
+		return 0;
+
+	case ERROR_INVALID_PARAMETER:
+		return EINVAL;
+
+	case ERROR_INVALID_HANDLE:
+		return EFAULT;
+
+	case ERROR_NOT_ENOUGH_MEMORY:
+	/* FALLTHROUGH */
+	case ERROR_NO_SYSTEM_RESOURCES:
+		return ENOMEM;
+
+	case ERROR_PRIVILEGE_NOT_HELD:
+	/* FALLTHROUGH */
+	case ERROR_ACCESS_DENIED:
+		return EACCES;
+
+	case ERROR_ALREADY_EXISTS:
+		return EEXIST;
+
+	case ERROR_POSSIBLE_DEADLOCK:
+		return EDEADLK;
+
+	case ERROR_INVALID_FUNCTION:
+	/* FALLTHROUGH */
+	case ERROR_CALL_NOT_IMPLEMENTED:
+		return ENOSYS;
+
+	default:
+		return EINVAL;
+	}
+
+	return EINVAL;
+}
+
 rte_thread_t
 rte_thread_self(void)
 {
@@ -87,15 +132,13 @@  rte_thread_key_create(rte_thread_key *key,
 	*key = malloc(sizeof(**key));
 	if ((*key) == NULL) {
 		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
-		rte_errno = ENOMEM;
-		return -1;
+		return ENOMEM;
 	}
 	(*key)->thread_index = TlsAlloc();
 	if ((*key)->thread_index == TLS_OUT_OF_INDEXES) {
 		RTE_LOG_WIN32_ERR("TlsAlloc()");
 		free(*key);
-		rte_errno = ENOEXEC;
-		return -1;
+		return rte_thread_translate_win32_error();
 	}
 	return 0;
 }
@@ -103,16 +146,14 @@  rte_thread_key_create(rte_thread_key *key,
 int
 rte_thread_key_delete(rte_thread_key key)
 {
-	if (!key) {
+	if (key == NULL) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
-		rte_errno = EINVAL;
-		return -1;
+		return EINVAL;
 	}
 	if (!TlsFree(key->thread_index)) {
 		RTE_LOG_WIN32_ERR("TlsFree()");
 		free(key);
-		rte_errno = ENOEXEC;
-		return -1;
+		return rte_thread_translate_win32_error();
 	}
 	free(key);
 	return 0;
@@ -123,17 +164,15 @@  rte_thread_value_set(rte_thread_key key, const void *value)
 {
 	char *p;
 
-	if (!key) {
+	if (key == NULL) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
-		rte_errno = EINVAL;
-		return -1;
+		return EINVAL;
 	}
 	/* discard const qualifier */
 	p = (char *) (uintptr_t) value;
 	if (!TlsSetValue(key->thread_index, p)) {
 		RTE_LOG_WIN32_ERR("TlsSetValue()");
-		rte_errno = ENOEXEC;
-		return -1;
+		return rte_thread_translate_win32_error();
 	}
 	return 0;
 }
@@ -142,16 +181,18 @@  void *
 rte_thread_value_get(rte_thread_key key)
 {
 	void *output;
+	int ret = 0;
 
-	if (!key) {
+	if (key == NULL) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
 		rte_errno = EINVAL;
 		return NULL;
 	}
 	output = TlsGetValue(key->thread_index);
-	if (GetLastError() != ERROR_SUCCESS) {
+	ret = rte_thread_translate_win32_error();
+	if (ret != 0) {
 		RTE_LOG_WIN32_ERR("TlsGetValue()");
-		rte_errno = ENOEXEC;
+		rte_errno = ret;
 		return NULL;
 	}
 	return output;