diff mbox series

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

Message ID 1617057640-24301-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 March 29, 2021, 10:40 p.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/windows/rte_thread.c | 65 ++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 15 deletions(-)

Comments

Tal Shnaiderman March 31, 2021, 1:56 p.m. UTC | #1 
> Subject: [PATCH v5 03/10] windows/eal: translate Windows errors to errno-
> style errors
> 
> External email: Use caution opening links or attachments
> 
> 
> 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/windows/rte_thread.c | 65 ++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/librte_eal/windows/rte_thread.c
> b/lib/librte_eal/windows/rte_thread.c
> index b29336cbd..e9181b47f 100644
> --- a/lib/librte_eal/windows/rte_thread.c
> +++ b/lib/librte_eal/windows/rte_thread.c
> @@ -12,6 +12,47 @@ struct eal_tls_key {
>         DWORD thread_index;
>  };
> 
> +/* Translates the most common error codes related to threads */ static
> +int rte_thread_translate_win32_error(DWORD error) {

This DWORD error will always the output of GetLastError()? If so can we move it inside the function?

Also, I don't think this is a thread specific function, other implementations can use it in the future, maybe move it to rte_windows.h? 

> +       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 +128,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(GetLastError());
>         }
>         return 0;
>  }
> @@ -103,16 +142,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(GetLastError());
>         }
>         free(key);
>         return 0;
> @@ -123,17 +160,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(GetLastError());
>         }
>         return 0;
>  }
> @@ -143,7 +178,7 @@ rte_thread_value_get(rte_thread_key key)  {
>         void *output;
> 

This function is missing the change to rte_thread_translate_win32_error.
Aldo need to change function docu.

> -       if (!key) {
> +       if (key == NULL) {
>                 RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
>                 rte_errno = EINVAL;
>                 return NULL;
> --
> 2.30.0.vfs.0.2
Nick Connolly March 31, 2021, 9:19 p.m. UTC | #2 
> Also, I don't think this is a thread specific function, other implementations can use it in the future, maybe move it to rte_windows.h?

I'd suggest that it's probably better suited to a .c file rather
than a header.  As an example of what it might end up like
see https://github.com/wpdk/wpdk/blob/master/src/error.c

Regards,
Nick
Tal Shnaiderman April 1, 2021, 12:29 p.m. UTC | #3 
> Subject: Re: [dpdk-dev] [PATCH v5 03/10] windows/eal: translate Windows
> errors to errno-style errors
> 
> External email: Use caution opening links or attachments
> 
> 
> > Also, I don't think this is a thread specific function, other implementations
> can use it in the future, maybe move it to rte_windows.h?
> 
> I'd suggest that it's probably better suited to a .c file rather than a header.  As
> an example of what it might end up like see
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Fwpdk%2Fwpdk%2Fblob%2Fmaster%2Fsrc%2Ferror.c&amp;data=
> 04%7C01%7Ctalshn%40nvidia.com%7C4ab2e17b824842b9f83208d8f48ab4ec
> %7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637528223860057761
> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=AnBHMtSobODNzb
> Uhun3GVnsTiePl%2BQzfPSASlur81Ks%3D&amp;reserved=0
> 
> Regards,
> Nick
> 

Agreed, thanks Nick.
Narcisa Ana Maria Vasile April 1, 2021, 7:07 p.m. UTC | #4 
On Wed, Mar 31, 2021 at 01:56:09PM +0000, Tal Shnaiderman wrote:
> > Subject: [PATCH v5 03/10] windows/eal: translate Windows errors to errno-
> > style errors
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 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/windows/rte_thread.c | 65 ++++++++++++++++++++++-------
> >  1 file changed, 50 insertions(+), 15 deletions(-)
> > 
> > diff --git a/lib/librte_eal/windows/rte_thread.c
> > b/lib/librte_eal/windows/rte_thread.c
> > index b29336cbd..e9181b47f 100644
> > --- a/lib/librte_eal/windows/rte_thread.c
> > +++ b/lib/librte_eal/windows/rte_thread.c
> > @@ -12,6 +12,47 @@ struct eal_tls_key {
> >         DWORD thread_index;
> >  };
> > 
> > +/* Translates the most common error codes related to threads */ static
> > +int rte_thread_translate_win32_error(DWORD error) {
> 
> This DWORD error will always the output of GetLastError()? If so can we move it inside the function?
 
  Yes, I think we can move the call to GetLastError() inside the function, thanks Tal.
> 
> Also, I don't think this is a thread specific function, other implementations can use it in the future, maybe move it to rte_windows.h? 
  
  I think it's better to keep these error-translation functions inside a specific module (threads, memory, etc.).
  The reason for that is that the same error code may mean different things in different modules. When I implemented
  this function I've went through all the Windows threads functions and noted down the type of errors that GetLastError()
  returns for them and what they mean. For a different module, a different set of errors might be more suitable,
  so to keep the translations as semantically compatible as possible to Linux, it's probably better to keep it
  at the module level (threading in this case).
> 
> > +       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;
> > +}
> > +
> > @@ -143,7 +178,7 @@ rte_thread_value_get(rte_thread_key key)  {
> >         void *output;
> > 
> 
> This function is missing the change to rte_thread_translate_win32_error.
> Aldo need to change function docu.
  
  Thanks, will fix and send v6.
> 
> > -       if (!key) {
> > +       if (key == NULL) {
> >                 RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
> >                 rte_errno = EINVAL;
> >                 return NULL;
> > --
> > 2.30.0.vfs.0.2
diff mbox series

Patch

diff --git a/lib/librte_eal/windows/rte_thread.c b/lib/librte_eal/windows/rte_thread.c
index b29336cbd..e9181b47f 100644
--- a/lib/librte_eal/windows/rte_thread.c
+++ b/lib/librte_eal/windows/rte_thread.c
@@ -12,6 +12,47 @@  struct eal_tls_key {
 	DWORD thread_index;
 };
 
+/* Translates the most common error codes related to threads */
+static int rte_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;
+
+	default:
+		return EINVAL;
+	}
+
+	return EINVAL;
+}
+
 rte_thread_t
 rte_thread_self(void)
 {
@@ -87,15 +128,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(GetLastError());
 	}
 	return 0;
 }
@@ -103,16 +142,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(GetLastError());
 	}
 	free(key);
 	return 0;
@@ -123,17 +160,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(GetLastError());
 	}
 	return 0;
 }
@@ -143,7 +178,7 @@  rte_thread_value_get(rte_thread_key key)
 {
 	void *output;
 
-	if (!key) {
+	if (key == NULL) {
 		RTE_LOG(DEBUG, EAL, "Invalid TLS key.\n");
 		rte_errno = EINVAL;
 		return NULL;