[v6,3/5] eal: make OS shims internal
Checks
Commit Message
DPDK code often relies on functions and macros that are not standard C,
but are found on all platforms, even if by slightly different names.
Windows <rte_os.h> provided macros or inline definitions for such symbols.
However, when placed in public header, these symbols were unnecessarily
exposed, breaking consumer POSIX compatibility code.
Move all shims to <rte_os_internal.h>, a header to be used instead of
<rte_os.h> by internal code. Include it in libraries and PMDs that
previously imported shims from <rte_os.h>.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
drivers/bus/pci/private.h | 4 +-
drivers/bus/vdev/vdev_private.h | 2 +
drivers/common/mlx5/mlx5_common.h | 1 +
drivers/net/i40e/i40e_ethdev.c | 1 +
lib/librte_cmdline/cmdline.c | 4 -
lib/librte_cmdline/cmdline_os_windows.c | 2 -
lib/librte_cmdline/cmdline_private.h | 1 +
lib/librte_cmdline/cmdline_socket.c | 4 -
lib/librte_eal/common/eal_common_config.c | 1 -
lib/librte_eal/common/eal_common_errno.c | 4 +
lib/librte_eal/common/eal_common_options.c | 2 +-
lib/librte_eal/common/eal_internal_cfg.h | 1 +
.../freebsd/include/rte_os_internal.h | 14 +++
.../linux/include/rte_os_internal.h | 14 +++
lib/librte_eal/windows/eal_hugepages.c | 1 -
lib/librte_eal/windows/eal_lcore.c | 1 -
lib/librte_eal/windows/eal_memalloc.c | 1 -
lib/librte_eal/windows/include/rte_os.h | 92 +------------------
.../windows/include/rte_os_internal.h | 28 ++++++
lib/librte_ethdev/ethdev_private.h | 2 +
lib/librte_kvargs/rte_kvargs.c | 1 +
21 files changed, 77 insertions(+), 104 deletions(-)
create mode 100644 lib/librte_eal/freebsd/include/rte_os_internal.h
create mode 100644 lib/librte_eal/linux/include/rte_os_internal.h
create mode 100644 lib/librte_eal/windows/include/rte_os_internal.h
Comments
20/03/2021 14:05, Dmitry Kozlyuk:
> DPDK code often relies on functions and macros that are not standard C,
> but are found on all platforms, even if by slightly different names.
> Windows <rte_os.h> provided macros or inline definitions for such symbols.
> However, when placed in public header, these symbols were unnecessarily
> exposed, breaking consumer POSIX compatibility code.
>
> Move all shims to <rte_os_internal.h>, a header to be used instead of
> <rte_os.h> by internal code. Include it in libraries and PMDs that
> previously imported shims from <rte_os.h>.
This shim could have been convenient for applications.
If not named "internal", we could export the header file
and allow apps including it.
Otherwise the app can recreate this file on its side,
it is not a big deal.
Opinions?
> This shim could have been convenient for applications.
> If not named "internal", we could export the header file
> and allow apps including it.
>
> Otherwise the app can recreate this file on its side,
> it is not a big deal.
> Opinions?
Based on my experience with SPDK, I believe this would get messy
very quickly.
For example, a common usage is to redefine close to _close.
If the application is going to use sockets without requiring changes,
then a fake fd is needed to represent a socket and close has to
handle it as well ... which will clash with the 'simplistic' redefinition
to _close, so we'd need some way of turning these on/off
individually.
I think there are really only two models that are viable - either an
external POSIX layer that is used by everything (in the general case,
Cygwin would be an example), or a private implementation that is
not exposed publicly.
Keeping the implementation private is not just about keeping
POSIX out of the headers. It's about avoiding conflicts with the
application; e.g. making sure that any signal implementation can
co-exist with the application.
Regards,
Nick
31/03/2021 23:05, Nick Connolly:
>
> > This shim could have been convenient for applications.
> > If not named "internal", we could export the header file
> > and allow apps including it.
> >
> > Otherwise the app can recreate this file on its side,
> > it is not a big deal.
> > Opinions?
> Based on my experience with SPDK, I believe this would get messy
> very quickly.
>
> For example, a common usage is to redefine close to _close.
> If the application is going to use sockets without requiring changes,
> then a fake fd is needed to represent a socket and close has to
> handle it as well ... which will clash with the 'simplistic' redefinition
> to _close, so we'd need some way of turning these on/off
> individually.
>
> I think there are really only two models that are viable - either an
> external POSIX layer that is used by everything (in the general case,
> Cygwin would be an example), or a private implementation that is
> not exposed publicly.
>
> Keeping the implementation private is not just about keeping
> POSIX out of the headers. It's about avoiding conflicts with the
> application; e.g. making sure that any signal implementation can
> co-exist with the application.
I don't understand your point.
I am just proposing to allow some apps to explicitly include the shim
for their convenience in case they are fully based on DPDK and
understand the risk of conflict with some other code.
>
> I don't understand your point.
> I am just proposing to allow some apps to explicitly include the shim
> for their convenience in case they are fully based on DPDK and
> understand the risk of conflict with some other code.
Agreed - there’s no harm in doing so.
My point was simply that for a non-trivial application I suspect it will
have limited value due to the sorts of conflicts that are likely to arise.
Regards,
Nick
>
31/03/2021 23:45, Nick Connolly:
> >
> > I don't understand your point.
> > I am just proposing to allow some apps to explicitly include the shim
> > for their convenience in case they are fully based on DPDK and
> > understand the risk of conflict with some other code.
>
>
> Agreed - there’s no harm in doing so.
>
> My point was simply that for a non-trivial application I suspect it will
> have limited value due to the sorts of conflicts that are likely to arise.
OK yes.
If we expose this header, we should bring it with a disclaimer.
2021-03-31 23:55 (UTC+0200), Thomas Monjalon:
> 31/03/2021 23:45, Nick Connolly:
> > >
> > > I don't understand your point.
> > > I am just proposing to allow some apps to explicitly include the shim
> > > for their convenience in case they are fully based on DPDK and
> > > understand the risk of conflict with some other code.
> >
> >
> > Agreed - there’s no harm in doing so.
> >
> > My point was simply that for a non-trivial application I suspect it will
> > have limited value due to the sorts of conflicts that are likely to arise.
>
> OK yes.
> If we expose this header, we should bring it with a disclaimer.
Let's now avoid "internal" (rte_os_shim?), but expose it later, if ever:
1. The concept of "not so public" API is new, probably a techboard subject.
2. We may want devtools checks against including it from public headers.
02/04/2021 01:10, Dmitry Kozlyuk:
> 2021-03-31 23:55 (UTC+0200), Thomas Monjalon:
> > 31/03/2021 23:45, Nick Connolly:
> > > >
> > > > I don't understand your point.
> > > > I am just proposing to allow some apps to explicitly include the shim
> > > > for their convenience in case they are fully based on DPDK and
> > > > understand the risk of conflict with some other code.
> > >
> > >
> > > Agreed - there’s no harm in doing so.
> > >
> > > My point was simply that for a non-trivial application I suspect it will
> > > have limited value due to the sorts of conflicts that are likely to arise.
> >
> > OK yes.
> > If we expose this header, we should bring it with a disclaimer.
>
> Let's now avoid "internal" (rte_os_shim?), but expose it later, if ever:
>
> 1. The concept of "not so public" API is new, probably a techboard subject.
> 2. We may want devtools checks against including it from public headers.
Yes it should be well discussed and checked with scripts.
For now, let's not expose such shim, but as you said,
adopt a more descriptive name avoiding "internal" qualifier.
@@ -7,8 +7,10 @@
#include <stdbool.h>
#include <stdio.h>
-#include <rte_pci.h>
+
#include <rte_bus_pci.h>
+#include <rte_os_internal.h>
+#include <rte_pci.h>
extern struct rte_pci_bus rte_pci_bus;
@@ -5,6 +5,8 @@
#ifndef _VDEV_PRIVATE_H_
#define _VDEV_PRIVATE_H_
+#include <rte_os_internal.h>
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -14,6 +14,7 @@
#include <rte_kvargs.h>
#include <rte_devargs.h>
#include <rte_bitops.h>
+#include <rte_os_internal.h>
#include "mlx5_prm.h"
#include "mlx5_devx_cmds.h"
@@ -27,6 +27,7 @@
#include <rte_tailq.h>
#include <rte_hash_crc.h>
#include <rte_bitmap.h>
+#include <rte_os_internal.h>
#include "i40e_logs.h"
#include "base/i40e_prototype.h"
@@ -18,10 +18,6 @@
#include "cmdline_private.h"
-#ifdef RTE_EXEC_ENV_WINDOWS
-#define write _write
-#endif
-
static void
cmdline_valid_buffer(struct rdline *rdl, const char *buf,
__rte_unused unsigned int size)
@@ -4,8 +4,6 @@
#include <io.h>
-#include <rte_os.h>
-
#include "cmdline_private.h"
/* Missing from some MinGW-w64 distributions. */
@@ -8,6 +8,7 @@
#include <stdarg.h>
#include <rte_common.h>
+#include <rte_os_internal.h>
#ifdef RTE_EXEC_ENV_WINDOWS
#include <rte_windows.h>
#endif
@@ -16,10 +16,6 @@
#include "cmdline_private.h"
#include "cmdline_socket.h"
-#ifdef RTE_EXEC_ENV_WINDOWS
-#define open _open
-#endif
-
struct cmdline *
cmdline_file_new(cmdline_parse_ctx_t *ctx, const char *prompt, const char *path)
{
@@ -3,7 +3,6 @@
*/
#include <string.h>
-#include <rte_os.h>
#include <rte_string_fns.h>
#include "eal_private.h"
@@ -15,6 +15,10 @@
#include <rte_errno.h>
#include <rte_string_fns.h>
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define strerror_r(errnum, buf, buflen) strerror_s(buf, buflen, errnum)
+#endif
+
RTE_DEFINE_PER_LCORE(int, _rte_errno);
const char *
@@ -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;
@@ -11,6 +11,7 @@
#define EAL_INTERNAL_CFG_H
#include <rte_eal.h>
+#include <rte_os_internal.h>
#include <rte_pci_dev_feature_defs.h>
#include "eal_thread.h"
new file mode 100644
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+
+#ifndef _RTE_OS_INTERNAL_H_
+#define _RTE_OS_INTERNAL_H_
+
+#include <rte_os.h>
+
+/**
+ * @file
+ * @internal
+ * Provides semi-standard OS facilities by convenient names.
+ */
+
+#endif /* _RTE_OS_INTERNAL_H_ */
new file mode 100644
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+
+#ifndef _RTE_OS_INTERNAL_H_
+#define _RTE_OS_INTERNAL_H_
+
+#include <rte_os.h>
+
+/**
+ * @file
+ * @internal
+ * Provides semi-standard OS facilities by convenient names.
+ */
+
+#endif /* _RTE_OS_INTERNAL_H_ */
@@ -6,7 +6,6 @@
#include <rte_log.h>
#include <rte_memory.h>
#include <rte_memzone.h>
-#include <rte_os.h>
#include "eal_private.h"
#include "eal_filesystem.h"
@@ -9,7 +9,6 @@
#include <rte_common.h>
#include <rte_debug.h>
#include <rte_lcore.h>
-#include <rte_os.h>
#include "eal_private.h"
#include "eal_thread.h"
@@ -3,7 +3,6 @@
*/
#include <rte_errno.h>
-#include <rte_os.h>
#include "eal_internal_cfg.h"
#include "eal_memalloc.h"
@@ -11,7 +11,6 @@
* Windows OS. It must not include Windows-specific headers.
*/
-#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -20,101 +19,18 @@
extern "C" {
#endif
-/* 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
new file mode 100644
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+
+#ifndef _RTE_OS_INTERNAL_H_
+#define _RTE_OS_INTERNAL_H_
+
+#include <rte_os.h>
+
+/**
+ * @file
+ * @internal
+ * Provides semi-standard OS facilities by convenient names.
+ */
+
+#ifndef PATH_MAX
+#define PATH_MAX _MAX_PATH
+#endif
+
+#define strdup(str) _strdup(str)
+#define strtok_r(str, delim, saveptr) strtok_s(str, delim, saveptr)
+#define strncasecmp(s1, s2, count) _strnicmp(s1, s2, count)
+
+#define open(path, flags, ...) _open(path, flags, ##__VA_ARGS__)
+#define read(fd, buf, n) _read(fd, buf, n)
+#define write(fd, buf, n) _write(fd, buf, n)
+#define close(fd) _close(fd)
+#define unlink(path) _unlink(path)
+
+#endif /* _RTE_OS_INTERNAL_H_ */
@@ -5,6 +5,8 @@
#ifndef _ETH_PRIVATE_H_
#define _ETH_PRIVATE_H_
+#include <rte_os_internal.h>
+
#include "rte_ethdev.h"
#ifdef __cplusplus
@@ -6,6 +6,7 @@
#include <string.h>
#include <stdlib.h>
+#include <rte_os_internal.h>
#include <rte_string_fns.h>
#include "rte_kvargs.h"