Re: [dpdk-dev] [PATCH v7 2/8] eal: add header files to support os specifics

[Thomas Monjalon <thomas@monjalon.net>]

Hi,

29/03/2019 00:24, Anand Rawat:
> Added rte_os.h files to support os specific functionality.
> Updated rte_common.h to include rte_os.h. Updated lib/meson.build to
> inject rte_os.h in every library.
> 
> Signed-off-by: Anand Rawat <anand.rawat@intel.com>
> Signed-off-by: Pallavi Kadam <pallavi.kadam@intel.com>
> Reviewed-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
> Reviewed-by: Ranjit Menon <ranjit.menon@intel.com>
> ---
>  lib/librte_eal/common/include/rte_common.h    |  5 +++-
>  .../common/include/rte_string_fns.h           |  4 ++-
>  .../freebsd/eal/include/exec-env/rte_os.h     | 10 +++++++
>  .../linux/eal/include/exec-env/rte_os.h       |  8 +++++
>  .../windows/eal/include/exec-env/rte_os.h     | 30 +++++++++++++++++++
>  meson.build                                   |  6 ++--

An update of the legacy Makefiles is missing.
It should be something like that:

--- a/lib/librte_eal/freebsd/eal/Makefile
+++ b/lib/librte_eal/freebsd/eal/Makefile
@@ -86,7 +86,7 @@ CFLAGS_eal_thread.o += -Wno-return-type
 CFLAGS_eal_hpet.o += -Wno-return-type
 endif
 
-INC :=  # no bsd specific headers
+INC := rte_os.h
 
 SYMLINK-$(CONFIG_RTE_EXEC_ENV_FREEBSD)-include := $(addprefix include/,$(INC))
 
--- a/lib/librte_eal/linux/eal/Makefile
+++ b/lib/librte_eal/linux/eal/Makefile
@@ -93,7 +93,8 @@ ifeq ($(CONFIG_RTE_TOOLCHAIN_GCC),y)
 CFLAGS_eal_thread.o += -Wno-return-type
 endif
 
-INC := rte_kni_common.h
+INC := rte_os.h
+INC += rte_kni_common.h
 
 SYMLINK-$(CONFIG_RTE_EXEC_ENV_LINUX)-include := $(addprefix include/,$(INC))


>  create mode 100644 lib/librte_eal/freebsd/eal/include/exec-env/rte_os.h
>  create mode 100644 lib/librte_eal/linux/eal/include/exec-env/rte_os.h
>  create mode 100644 lib/librte_eal/windows/eal/include/exec-env/rte_os.h
[...]
> +/* os specific include */

Please write OS (uppercase) in this comment and others,
so we can understand it is an acronym.

> +#include <rte_os.h>

You don't prefix with exec-env/ sub-directory, unlike what is done
for rte_kni_common.h, and I think it is a good idea.
However it will require one more patch to make it work with
the make-based system which currently installs the include in exec-env/.
I have just submitted such a patch to remove the exec-env/ directory
both from installed and source hierarchy:
	https://patches.dpdk.org/patch/52031/
Please rebase on top of my patch and move rte_os.h one level upper.
Thanks


One more nit:
> --- /dev/null
> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_os.h
> +/* macro substitution for windows supported strerror_r */
> +#define strerror_r(a, b, c) strerror_s(b, c, a)
> +
> +/* macro substitution for windows supported strdup */
> +#define strdup(str)	_strdup(str)
> +
> +/* macro substitution for windows supported ssize_t type */
> +typedef SSIZE_T ssize_t;
> +
> +/* macro substitution for windows supported strtok_r */
> +#define strtok_r(str, delim, saveptr) strtok_s(str, delim, saveptr)

Please write Windows with first letter uppercase.
In this file, the comments looks superfluous and can be removed.
Or you can replace by something like "There is no strdup in Microsoft libc."