Re: [PATCH v2 1/4] eal/windows: translate Windows errors to errno-style errors

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Tue, Apr 12, 2022 at 10:26:27AM -0700, Menon, Ranjit wrote:
> Hi, Tyler
> 
> On 4/12/2022 3:43 AM, Tyler Retzlaff wrote:
> >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>
> >Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >---
> >  lib/eal/windows/rte_thread.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> >diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
> >index 667287c..c272018 100644
> >--- a/lib/eal/windows/rte_thread.c
> >+++ b/lib/eal/windows/rte_thread.c
> >@@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: BSD-3-Clause
> >   * Copyright 2021 Mellanox Technologies, Ltd
> >+ * Copyright (C) 2022 Microsoft Corporation
> >   */
> >  #include <rte_common.h>
> >@@ -11,6 +12,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;
> >+}
> >+
> 
> Shouldn't we return all these error values as negative... as in
> -EINVAL, -EFAULT etc. ?

no. since this is just a translation function it is up to the caller
what it will do with the translated value and any conversion to -errno.

since it is also used to store into rte_errno it would have been
confusing to see something like the following to reverse the sign back.

err = thread_translate_win32_error(win32err);
rte_errno = -err; // confusing

personally i don't like the -errno returns at all and this is just
another reason contributing to that opinion. you can refer to my
previous mailing list posts on the subject.

> 
> 
> ranjit m.