[v2,7/7] eal/windows: do not expose POSIX symbols

Message ID 20210221012831.14643-8-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal/windows: do not expose POSIX symbols |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot fail travis build: failed
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot success github build: passed
ci/iol-testing fail Testing issues

Commit Message

Dmitry Kozlyuk Feb. 21, 2021, 1:28 a.m. UTC
  Exposing POSIX symbols could break consumer POSIX compatibility code.

* Make renaming of close() and unlink() private to EAL.

* Remove renaming of strncasecmp(), strtok_r(), and sleep()
  in favor of using EAL wrappers. Similarly remove PATH_MAX macro.

* Replace index(3p), which is not available on Windows, with strchr(3),
  as recommended by POSIX.1-2008. strerror_r() is only used inside EAL,
  rename it only where it's needed. Same for asprintf(), it has an
  internal EAL wrapper and is removed from public API.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 doc/guides/rel_notes/release_21_05.rst     |  9 ++
 lib/librte_eal/common/eal_common_errno.c   |  4 +
 lib/librte_eal/common/eal_common_options.c |  2 +-
 lib/librte_eal/common/eal_private.h        |  5 ++
 lib/librte_eal/freebsd/include/rte_os.h    |  4 +-
 lib/librte_eal/linux/include/rte_os.h      |  4 +-
 lib/librte_eal/windows/include/rte_os.h    | 99 ++--------------------
 7 files changed, 29 insertions(+), 98 deletions(-)
  

Comments

Tal Shnaiderman Feb. 21, 2021, 8:59 a.m. UTC | #1
> Subject: [dpdk-dev] [PATCH v2 7/7] eal/windows: do not expose POSIX
> symbols
> 
> External email: Use caution opening links or attachments
> 
> 
> Exposing POSIX symbols could break consumer POSIX compatibility code.
> 
> * Make renaming of close() and unlink() private to EAL.
> 
> * Remove renaming of strncasecmp(), strtok_r(), and sleep()
>   in favor of using EAL wrappers. Similarly remove PATH_MAX macro.
> 
> * Replace index(3p), which is not available on Windows, with strchr(3),
>   as recommended by POSIX.1-2008. strerror_r() is only used inside EAL,
>   rename it only where it's needed. Same for asprintf(), it has an
>   internal EAL wrapper and is removed from public API.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---

[snip]

> diff --git a/lib/librte_eal/windows/include/rte_os.h
> b/lib/librte_eal/windows/include/rte_os.h
> index edca11bd2..9c9c31214 100644
> --- a/lib/librte_eal/windows/include/rte_os.h
> +++ b/lib/librte_eal/windows/include/rte_os.h
> @@ -6,15 +6,11 @@
>  #define _RTE_OS_H_
> 
>  /**
> - * This is header should contain any function/macro definition
> - * which are not supported natively or named differently in the
> - * Windows OS. It must not include Windows-specific headers.
> + * This header should contain any function/macro definition
> + * which are not supported natively or named differently in Windows OS.
>   */
> 
> -#include <stdarg.h>
> -#include <stdio.h>
>  #include <stdlib.h>
> -#include <string.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -22,101 +18,18 @@ extern "C" {
> 
>  #define RTE_PATH_MAX _MAX_PATH
> 
> -/* limits.h replacement, value as in <windows.h> */ -#ifndef PATH_MAX -
> #define PATH_MAX _MAX_PATH -#endif
> -
> -#ifndef sleep
> -#define sleep(x) Sleep(1000 * (x))
> -#endif
> -
> -#ifndef strerror_r
> -#define strerror_r(a, b, c) strerror_s(b, c, a) -#endif
> -
> -#ifndef strdup
> -/* strdup is deprecated in Microsoft libc and _strdup is preferred */ -
> #define strdup(str) _strdup(str) -#endif
> -
> -#ifndef strtok_r
> -#define strtok_r(str, delim, saveptr) strtok_s(str, delim, saveptr) -#endif
> -
> -#ifndef index
> -#define index(a, b)     strchr(a, b)
> -#endif
> -
> -#ifndef rindex
> -#define rindex(a, b)    strrchr(a, b)
> -#endif
> -
> -#ifndef strncasecmp
> -#define strncasecmp(s1, s2, count)        _strnicmp(s1, s2, count)
> -#endif
> -
> -#ifndef close
> -#define close _close
> -#endif

mlx5 uses close() in mlx5.c and is broken after this change above, BTW why not add an rte_close instead of local definition?

> -
> -#ifndef unlink
> -#define unlink _unlink
> -#endif
> -

[snip]
  
Dmitry Kozlyuk Feb. 21, 2021, 10:24 a.m. UTC | #2
On Sun, 21 Feb 2021 08:59:50 +0000, Tal Shnaiderman wrote:
[...]
> > -#ifndef close
> > -#define close _close
> > -#endif  
> 
> mlx5 uses close() in mlx5.c and is broken after this change above, BTW why not add an rte_close instead of local definition?

I'm reluctant to add file manipulation API to EAL. It would be replication of
POSIX with "rte_" prefix, while standard C has everything needed to deal with
files (and IOCTLs are platform-specific code anyway). I think libraries and
PMDs striving to be cross-platform should move to using FILE* some day. For
now, local definitions keep them running without any risk of breakage.

Thanks for all your comments, it's weird I didn't hit the failures locally.
Will fix in v3.
  
Tal Shnaiderman Feb. 21, 2021, 11:58 a.m. UTC | #3
> Subject: Re: [dpdk-dev] [PATCH v2 7/7] eal/windows: do not expose POSIX
> symbols
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sun, 21 Feb 2021 08:59:50 +0000, Tal Shnaiderman wrote:
> [...]
> > > -#ifndef close
> > > -#define close _close
> > > -#endif
> >
> > mlx5 uses close() in mlx5.c and is broken after this change above, BTW why
> not add an rte_close instead of local definition?
> 
> I'm reluctant to add file manipulation API to EAL. It would be replication of
> POSIX with "rte_" prefix, while standard C has everything needed to deal
> with files (and IOCTLs are platform-specific code anyway). I think libraries and
> PMDs striving to be cross-platform should move to using FILE* some day. For
> now, local definitions keep them running without any risk of breakage.
> 
> Thanks for all your comments, it's weird I didn't hit the failures locally.
> Will fix in v3.

You might be missing the DevX SDK installation, without it meson will skip the mlx5 build.

You can get it either by installation WIOF2 2.60 [1] or get only the SDK from [2] (I attached it to the Bugzilla ticket when asked the UNH team to add mlx5 compilation to CI.

[1] - https://www.mellanox.com/products/adapter-software/ethernet/windows/winof-2
[2] - https://bugs.dpdk.org/show_bug.cgi?id=620
  
Dmitry Kozlyuk Feb. 21, 2021, 2:33 p.m. UTC | #4
On Sun, 21 Feb 2021 11:58:39 +0000, Tal Shnaiderman wrote:
[...] 
> > Thanks for all your comments, it's weird I didn't hit the failures locally.
> > Will fix in v3.  
> 
> You might be missing the DevX SDK installation, without it meson will skip the mlx5 build.
> 
> You can get it either by installation WIOF2 2.60 [1] or get only the SDK from [2] (I attached it to the Bugzilla ticket when asked the UNH team to add mlx5 compilation to CI.
> 
> [1] - https://www.mellanox.com/products/adapter-software/ethernet/windows/winof-2
> [2] - https://bugs.dpdk.org/show_bug.cgi?id=620

Indeed it was the case, thank you again.
  

Patch

diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 5aa9ed7db..6380dfa53 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -84,6 +84,15 @@  API Changes
    Also, make sure to start the actual text at the margin.
    =======================================================
 
+* eal/windows: Removed POSIX symbols from EAL headers. Exposing POSIX symbols
+  has been incorrect and could conflict with consumer POSIX implementations.
+  Wrappers are provided for
+  ``strtok_r(3p)`` (``rte_strtok``),
+  ``strncasecmp(3p)`` (``rte_strncasecmp``),
+  ``sleep(3p)`` (``rte_thread_sleep``),
+  ``PATH_MAX`` (``RTE_PATH_MAX``).
+  Removed are ``strerror_r(3p)`` and ``asprintf(3p)``.
+
 
 ABI Changes
 -----------
diff --git a/lib/librte_eal/common/eal_common_errno.c b/lib/librte_eal/common/eal_common_errno.c
index 2a10fb823..536eea0c3 100644
--- a/lib/librte_eal/common/eal_common_errno.c
+++ b/lib/librte_eal/common/eal_common_errno.c
@@ -15,6 +15,10 @@ 
 #include <rte_errno.h>
 #include <rte_string_fns.h>
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define strerror_r(a, b, c) strerror_s(b, c, a)
+#endif
+
 RTE_DEFINE_PER_LCORE(int, _rte_errno);
 
 const char *
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 275f879d7..fd3b22e8a 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -1936,7 +1936,7 @@  eal_check_common_options(struct internal_config *internal_cfg)
 		RTE_LOG(ERR, EAL, "Invalid length of --" OPT_MBUF_POOL_OPS_NAME" option\n");
 		return -1;
 	}
-	if (index(eal_get_hugefile_prefix(), '%') != NULL) {
+	if (strchr(eal_get_hugefile_prefix(), '%') != NULL) {
 		RTE_LOG(ERR, EAL, "Invalid char, '%%', in --"OPT_FILE_PREFIX" "
 			"option\n");
 		return -1;
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index a5d9c5123..860551661 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -756,4 +756,9 @@  int eal_asprintf(char **buffer, const char *format, ...);
 #define eal_asprintf asprintf
 #endif
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define close _close
+#define unlink _unlink
+#endif
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/freebsd/include/rte_os.h b/lib/librte_eal/freebsd/include/rte_os.h
index b37d59b5e..a5bf10021 100644
--- a/lib/librte_eal/freebsd/include/rte_os.h
+++ b/lib/librte_eal/freebsd/include/rte_os.h
@@ -6,9 +6,9 @@ 
 #define _RTE_OS_H_
 
 /**
- * This is header should contain any function/macro definition
+ * This header should contain any function/macro definition
  * which are not supported natively or named differently in the
- * freebsd OS. Functions will be added in future releases.
+ * freebsd OS. It must not define symbols without "rte_" prefix.
  */
 
 #include <pthread_np.h>
diff --git a/lib/librte_eal/linux/include/rte_os.h b/lib/librte_eal/linux/include/rte_os.h
index af7d052d9..04f510eec 100644
--- a/lib/librte_eal/linux/include/rte_os.h
+++ b/lib/librte_eal/linux/include/rte_os.h
@@ -6,9 +6,9 @@ 
 #define _RTE_OS_H_
 
 /**
- * This is header should contain any function/macro definition
+ * This header should contain any function/macro definition
  * which are not supported natively or named differently in the
- * linux OS. Functions will be added in future releases.
+ * linux OS. It must not define symbols without "rte_" prefix.
  */
 
 #include <sched.h>
diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
index edca11bd2..9c9c31214 100644
--- a/lib/librte_eal/windows/include/rte_os.h
+++ b/lib/librte_eal/windows/include/rte_os.h
@@ -6,15 +6,11 @@ 
 #define _RTE_OS_H_
 
 /**
- * This is header should contain any function/macro definition
- * which are not supported natively or named differently in the
- * Windows OS. It must not include Windows-specific headers.
+ * This header should contain any function/macro definition
+ * which are not supported natively or named differently in Windows OS.
  */
 
-#include <stdarg.h>
-#include <stdio.h>
 #include <stdlib.h>
-#include <string.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -22,101 +18,18 @@  extern "C" {
 
 #define RTE_PATH_MAX _MAX_PATH
 
-/* limits.h replacement, value as in <windows.h> */
-#ifndef PATH_MAX
-#define PATH_MAX _MAX_PATH
-#endif
-
-#ifndef sleep
-#define sleep(x) Sleep(1000 * (x))
-#endif
-
-#ifndef strerror_r
-#define strerror_r(a, b, c) strerror_s(b, c, a)
-#endif
-
-#ifndef strdup
-/* strdup is deprecated in Microsoft libc and _strdup is preferred */
-#define strdup(str) _strdup(str)
-#endif
-
-#ifndef strtok_r
-#define strtok_r(str, delim, saveptr) strtok_s(str, delim, saveptr)
-#endif
-
-#ifndef index
-#define index(a, b)     strchr(a, b)
-#endif
-
-#ifndef rindex
-#define rindex(a, b)    strrchr(a, b)
-#endif
-
-#ifndef strncasecmp
-#define strncasecmp(s1, s2, count)        _strnicmp(s1, s2, count)
-#endif
-
-#ifndef close
-#define close _close
-#endif
-
-#ifndef unlink
-#define unlink _unlink
-#endif
-
 /* cpu_set macros implementation */
 #define RTE_CPU_AND(dst, src1, src2) CPU_AND(dst, src1, src2)
 #define RTE_CPU_OR(dst, src1, src2) CPU_OR(dst, src1, src2)
 #define RTE_CPU_FILL(set) CPU_FILL(set)
 #define RTE_CPU_NOT(dst, src) CPU_NOT(dst, src)
 
-/* as in <windows.h> */
+/* This is an exception without "rte_" prefix, because Windows does have
+ * ssize_t, but it's defined in <windows.h> which we avoid to expose.
+ * If ssize_t is defined in user code, it necessarily has the same type.
+ */
 typedef long long ssize_t;
 
-#ifndef RTE_TOOLCHAIN_GCC
-
-static inline int
-asprintf(char **buffer, const char *format, ...)
-{
-	int size, ret;
-	va_list arg;
-
-	va_start(arg, format);
-	size = vsnprintf(NULL, 0, format, arg);
-	va_end(arg);
-	if (size < 0)
-		return -1;
-	size++;
-
-	*buffer = (char *)malloc(size);
-	if (*buffer == NULL)
-		return -1;
-
-	va_start(arg, format);
-	ret = vsnprintf(*buffer, size, format, arg);
-	va_end(arg);
-	if (ret != size - 1) {
-		free(*buffer);
-		return -1;
-	}
-	return ret;
-}
-
-static inline const char *
-eal_strerror(int code)
-{
-	static char buffer[128];
-
-	strerror_s(buffer, sizeof(buffer), code);
-	return buffer;
-}
-
-#ifndef strerror
-#define strerror eal_strerror
-#endif
-
-#endif /* RTE_TOOLCHAIN_GCC */
-
 #ifdef __cplusplus
 }
 #endif