[v6,03/10] windows/eal: translate Windows errors to errno-style errors
Checks
Commit Message
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
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.
[...]
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.
>
> [...]
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.
@@ -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);
@@ -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;