diff mbox series

[v16,3/9] eal/windows: translate Windows errors to errno-style errors

Message ID 1633765318-28356-4-git-send-email-navasile@linux.microsoft.com (mailing list archive)
State Changes Requested, archived
Delegated to: David Marchand
Headers
Series eal: Add EAL API for threading |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Narcisa Ana Maria Vasile Oct. 9, 2021, 7:41 a.m. UTC 
  From: Narcisa Vasile <navasile@microsoft.com>

Add function to translate Windows error codes to
errno-style error codes. The possible return values are chosen
so that we have as much semantical compatibility between platforms as
possible.

Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
---
 lib/eal/common/rte_thread.c  |  6 +--
 lib/eal/include/rte_thread.h |  5 +-
 lib/eal/windows/rte_thread.c | 95 +++++++++++++++++++++++++++---------
 3 files changed, 76 insertions(+), 30 deletions(-)

Comments

Thomas Monjalon Oct. 12, 2021, 4:16 p.m. UTC | #1 
09/10/2021 09:41, Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> Add function to translate Windows error codes to
> errno-style error codes. The possible return values are chosen
> so that we have as much semantical compatibility between platforms as
> possible.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  lib/eal/common/rte_thread.c  |  6 +--
>  lib/eal/include/rte_thread.h |  5 +-
>  lib/eal/windows/rte_thread.c | 95 +++++++++++++++++++++++++++---------
>  3 files changed, 76 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> index e1a4d7eae4..27ad1c7eb0 100644
> --- a/lib/eal/common/rte_thread.c
> +++ b/lib/eal/common/rte_thread.c
> @@ -47,7 +47,7 @@ rte_thread_attr_init(rte_thread_attr_t *attr)
>  
>  int
>  rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
> -			     rte_cpuset_t *cpuset)
> +		rte_cpuset_t *cpuset)
>  {
>  	RTE_VERIFY(thread_attr != NULL);
>  	RTE_VERIFY(cpuset != NULL);
> @@ -59,7 +59,7 @@ rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
>  
>  int
>  rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
> -			     rte_cpuset_t *cpuset)
> +		rte_cpuset_t *cpuset)
>  {
>  	RTE_VERIFY(thread_attr != NULL);
>  	RTE_VERIFY(cpuset != NULL);
> @@ -71,7 +71,7 @@ rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
>  
>  int
>  rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
> -			     enum rte_thread_priority priority)
> +		enum rte_thread_priority priority)

Above are unrelated changes.

> --- a/lib/eal/windows/rte_thread.c
> +++ b/lib/eal/windows/rte_thread.c
> @@ -13,6 +13,54 @@ struct eal_tls_key {
>  	DWORD thread_index;
>  };
>  
> +/* Translates the most common error codes related to threads */
> +static int
> +thread_translate_win32_error(DWORD error)

So you decide to adopt POSIX error codes for the DPDK API. OK

> +{
> +	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;
> +	}
> +
> +	return EINVAL;
> +}
[...]
>  rte_thread_key_create(rte_thread_key *key,
>  		__rte_unused void (*destructor)(void *))
>  {
> +	int ret;
> +
>  	*key = malloc(sizeof(**key));
>  	if ((*key) == NULL) {
>  		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
> -		rte_errno = ENOMEM;
> -		return -1;
> +		return ENOMEM;
>  	}

Why this change? rte_errno and negative error code are good.
Narcisa Ana Maria Vasile Nov. 9, 2021, 2:02 a.m. UTC | #2 
On Tue, Oct 12, 2021 at 06:16:19PM +0200, Thomas Monjalon wrote:
> 09/10/2021 09:41, Narcisa Ana Maria Vasile:
> > From: Narcisa Vasile <navasile@microsoft.com>
> > 
> > Add function to translate Windows error codes to
> > errno-style error codes. The possible return values are chosen
> > so that we have as much semantical compatibility between platforms as
> > possible.
> > 
> > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> > ---
> >  lib/eal/common/rte_thread.c  |  6 +--
> >  lib/eal/include/rte_thread.h |  5 +-
> >  lib/eal/windows/rte_thread.c | 95 +++++++++++++++++++++++++++---------
> >  3 files changed, 76 insertions(+), 30 deletions(-)
> > 
> > diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> > index e1a4d7eae4..27ad1c7eb0 100644
> > --- a/lib/eal/common/rte_thread.c
> > +++ b/lib/eal/common/rte_thread.c
> > @@ -47,7 +47,7 @@ rte_thread_attr_init(rte_thread_attr_t *attr)
> >  
> >  int
> >  rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
> > -			     rte_cpuset_t *cpuset)
> > +		rte_cpuset_t *cpuset)
> >  {
> >  	RTE_VERIFY(thread_attr != NULL);
> >  	RTE_VERIFY(cpuset != NULL);
> > @@ -59,7 +59,7 @@ rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
> >  
> >  int
> >  rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
> > -			     rte_cpuset_t *cpuset)
> > +		rte_cpuset_t *cpuset)
> >  {
> >  	RTE_VERIFY(thread_attr != NULL);
> >  	RTE_VERIFY(cpuset != NULL);
> > @@ -71,7 +71,7 @@ rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
> >  
> >  int
> >  rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
> > -			     enum rte_thread_priority priority)
> > +		enum rte_thread_priority priority)
> 
> Above are unrelated changes.
> 

  Thanks, will fix!

> > --- a/lib/eal/windows/rte_thread.c
> > +++ b/lib/eal/windows/rte_thread.c
> > @@ -13,6 +13,54 @@ struct eal_tls_key {
> >  	DWORD thread_index;
> >  };
> >  
> > +/* Translates the most common error codes related to threads */
> > +static int
> > +thread_translate_win32_error(DWORD error)
> 
> So you decide to adopt POSIX error codes for the DPDK API. OK
> 
> > +{
> > +	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;
> > +	}
> > +
> > +	return EINVAL;
> > +}
> [...]
> >  rte_thread_key_create(rte_thread_key *key,
> >  		__rte_unused void (*destructor)(void *))
> >  {
> > +	int ret;
> > +
> >  	*key = malloc(sizeof(**key));
> >  	if ((*key) == NULL) {
> >  		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
> > -		rte_errno = ENOMEM;
> > -		return -1;
> > +		return ENOMEM;
> >  	}
> 
> Why this change? rte_errno and negative error code are good.
> 

  This error could have been handled using rte_errno and negative return,
  but for consistency, a positive error number is returned. As different platforms
  have different error codes, the approach here is to translate the Windows error
  to POSIX-style ones to have uniformity over the values returned. All functions
  in this thread module return the possible error through the return value.

>
Thomas Monjalon Nov. 9, 2021, 8:21 a.m. UTC | #3 
09/11/2021 03:02, Narcisa Ana Maria Vasile:
> On Tue, Oct 12, 2021 at 06:16:19PM +0200, Thomas Monjalon wrote:
> > 09/10/2021 09:41, Narcisa Ana Maria Vasile:
> > > From: Narcisa Vasile <navasile@microsoft.com>
> > >  rte_thread_key_create(rte_thread_key *key,
> > >  		__rte_unused void (*destructor)(void *))
> > >  {
> > > +	int ret;
> > > +
> > >  	*key = malloc(sizeof(**key));
> > >  	if ((*key) == NULL) {
> > >  		RTE_LOG(DEBUG, EAL, "Cannot allocate TLS key.\n");
> > > -		rte_errno = ENOMEM;
> > > -		return -1;
> > > +		return ENOMEM;
> > >  	}
> > 
> > Why this change? rte_errno and negative error code are good.
> > 
> 
>   This error could have been handled using rte_errno and negative return,
>   but for consistency, a positive error number is returned. As different platforms
>   have different error codes, the approach here is to translate the Windows error
>   to POSIX-style ones to have uniformity over the values returned. All functions
>   in this thread module return the possible error through the return value.

We can have the same consistency with rte_errno.
What others think? Should we use rte_errno?
diff mbox series

Patch

diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
index e1a4d7eae4..27ad1c7eb0 100644
--- a/lib/eal/common/rte_thread.c
+++ b/lib/eal/common/rte_thread.c
@@ -47,7 +47,7 @@  rte_thread_attr_init(rte_thread_attr_t *attr)
 
 int
 rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
-			     rte_cpuset_t *cpuset)
+		rte_cpuset_t *cpuset)
 {
 	RTE_VERIFY(thread_attr != NULL);
 	RTE_VERIFY(cpuset != NULL);
@@ -59,7 +59,7 @@  rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
 
 int
 rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
-			     rte_cpuset_t *cpuset)
+		rte_cpuset_t *cpuset)
 {
 	RTE_VERIFY(thread_attr != NULL);
 	RTE_VERIFY(cpuset != NULL);
@@ -71,7 +71,7 @@  rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
 
 int
 rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
-			     enum rte_thread_priority priority)
+		enum rte_thread_priority priority)
 {
 	RTE_VERIFY(thread_attr != NULL);
 
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index 4ac36957ce..8a20215a94 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -233,9 +233,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/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
index 01966e7745..c1ecfbd6ae 100644
--- a/lib/eal/windows/rte_thread.c
+++ b/lib/eal/windows/rte_thread.c
@@ -13,6 +13,54 @@  struct eal_tls_key {
 	DWORD thread_index;
 };
 
+/* Translates the most common error codes related to threads */
+static int
+thread_translate_win32_error(DWORD error)
+{
+	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;
+	}
+
+	return EINVAL;
+}
+
+static int
+thread_log_last_error(const char *message)
+{
+	DWORD error = GetLastError();
+	RTE_LOG(DEBUG, EAL, "GetLastError()=%lu: %s\n", error, message);
+
+	return thread_translate_win32_error(error);
+}
+
 rte_thread_t
 rte_thread_self(void)
 {
@@ -42,7 +90,7 @@  rte_thread_attr_init(rte_thread_attr_t *attr)
 
 int
 rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
-			     rte_cpuset_t *cpuset)
+		rte_cpuset_t *cpuset)
 {
 	RTE_VERIFY(thread_attr != NULL);
 	thread_attr->cpuset = *cpuset;
@@ -52,7 +100,7 @@  rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
 
 int
 rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
-			     rte_cpuset_t *cpuset)
+		rte_cpuset_t *cpuset)
 {
 	RTE_VERIFY(thread_attr != NULL);
 
@@ -63,7 +111,7 @@  rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
 
 int
 rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
-			     enum rte_thread_priority priority)
+		enum rte_thread_priority priority)
 {
 	RTE_VERIFY(thread_attr != NULL);
 
@@ -76,18 +124,18 @@  int
 rte_thread_key_create(rte_thread_key *key,
 		__rte_unused void (*destructor)(void *))
 {
+	int ret;
+
 	*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()");
+		ret = thread_log_last_error("TlsAlloc()");
 		free(*key);
-		rte_errno = ENOEXEC;
-		return -1;
+		return ret;
 	}
 	return 0;
 }
@@ -95,16 +143,16 @@  rte_thread_key_create(rte_thread_key *key,
 int
 rte_thread_key_delete(rte_thread_key key)
 {
-	if (!key) {
+	int ret;
+
+	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()");
+		ret = thread_log_last_error("TlsFree()");
 		free(key);
-		rte_errno = ENOEXEC;
-		return -1;
+		return ret;
 	}
 	free(key);
 	return 0;
@@ -115,17 +163,14 @@  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 thread_log_last_error("TlsSetValue()");
 	}
 	return 0;
 }
@@ -134,16 +179,18 @@  void *
 rte_thread_value_get(rte_thread_key key)
 {
 	void *output;
+	DWORD 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) {
-		RTE_LOG_WIN32_ERR("TlsGetValue()");
-		rte_errno = ENOEXEC;
+	ret = GetLastError();
+	if (ret != 0) {
+		RTE_LOG(DEBUG, EAL, "GetLastError()=%lu: TlsGetValue()\n", ret);
+		rte_errno = thread_translate_win32_error(ret);
 		return NULL;
 	}
 	return output;