diff mbox series

[v2,2/6] eal: add header files to support windows

Message ID 20190306041634.12976-3-anand.rawat@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series HelloWorld example for windows |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Anand Rawat March 6, 2019, 4:16 a.m. UTC 
  Added header files to support windows on x86 platforms.
Updated rte_config to include rte_windows.h for windows
build.

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>
---
 config/rte_config.h                           |  7 +++++-
 .../eal/include/exec-env/rte_windows.h        | 23 +++++++++++++++++++
 lib/librte_eal/windows/eal/meson.build        |  2 ++
 3 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_eal/windows/eal/include/exec-env/rte_windows.h

Comments

Thomas Monjalon March 6, 2019, 11:31 a.m. UTC | #1 
06/03/2019 05:16, Anand Rawat:
> Added header files to support windows on x86 platforms.
> Updated rte_config to include rte_windows.h for windows
> build.
[...]
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> +/* windows specific*/
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#include <rte_windows.h>
> +#endif

Include in the config file looks wrong.

> --- /dev/null
> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h

I think we could remove the sub-directory exec-env.
Could we include this file from rte_common.h?

> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 Intel Corporation
> + */
> +
> +#ifndef _RTE_WINDOWS_H_
> +#define _RTE_WINDOWS_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#define __extension__
> +#define __thread __declspec(thread)
> +
> +#define strerror_r(a, b, c) strerror_s(b, c, a)
> +
> +typedef void *ssize_t;
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_WINDOWS_H_ */

It is missing comments to explain the need of each line.
Anand Rawat March 7, 2019, 3:27 a.m. UTC | #2 
On 3/6/2019 3:31 AM, Thomas Monjalon wrote:
> 06/03/2019 05:16, Anand Rawat:
>> Added header files to support windows on x86 platforms.
>> Updated rte_config to include rte_windows.h for windows
>> build.
> [...]
>> --- a/config/rte_config.h
>> +++ b/config/rte_config.h
>> +/* windows specific*/
>> +#ifdef RTE_EXEC_ENV_WINDOWS
>> +#include <rte_windows.h>
>> +#endif
> 
> Include in the config file looks wrong. >
>> --- /dev/null
>> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
> 
> I think we could remove the sub-directory exec-env.
> Could we include this file from rte_common.h?
rte_windows.h defines types and substitution macros which are
needed to support common code on windows. So it should be
included as a global include for every library on windows.
rte_common.h is not included in all the source code and headers
we currently build for windows.

> 
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2019 Intel Corporation
>> + */
>> +
>> +#ifndef _RTE_WINDOWS_H_
>> +#define _RTE_WINDOWS_H_
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#define __extension__
>> +#define __thread __declspec(thread)
>> +
>> +#define strerror_r(a, b, c) strerror_s(b, c, a)
>> +
>> +typedef void *ssize_t;
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
>> +#endif /* _RTE_WINDOWS_H_ */
> 
> It is missing comments to explain the need of each line.
> 
> 

Will be done in v3
Thomas Monjalon March 7, 2019, 8:45 a.m. UTC | #3 
07/03/2019 04:27, Anand Rawat:
> On 3/6/2019 3:31 AM, Thomas Monjalon wrote:
> > 06/03/2019 05:16, Anand Rawat:
> >> Added header files to support windows on x86 platforms.
> >> Updated rte_config to include rte_windows.h for windows
> >> build.
> > [...]
> >> --- a/config/rte_config.h
> >> +++ b/config/rte_config.h
> >> +/* windows specific*/
> >> +#ifdef RTE_EXEC_ENV_WINDOWS
> >> +#include <rte_windows.h>
> >> +#endif
> > 
> > Include in the config file looks wrong. >
> >> --- /dev/null
> >> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
> > 
> > I think we could remove the sub-directory exec-env.
> > Could we include this file from rte_common.h?
> rte_windows.h defines types and substitution macros which are
> needed to support common code on windows. So it should be
> included as a global include for every library on windows.
> rte_common.h is not included in all the source code and headers
> we currently build for windows.

I think it is not an issue adding some rte_common.h include
here and there.
Bruce Richardson March 7, 2019, 10:24 a.m. UTC | #4 
On Thu, Mar 07, 2019 at 09:45:57AM +0100, Thomas Monjalon wrote:
> 07/03/2019 04:27, Anand Rawat:
> > On 3/6/2019 3:31 AM, Thomas Monjalon wrote:
> > > 06/03/2019 05:16, Anand Rawat:
> > >> Added header files to support windows on x86 platforms.
> > >> Updated rte_config to include rte_windows.h for windows
> > >> build.
> > > [...]
> > >> --- a/config/rte_config.h
> > >> +++ b/config/rte_config.h
> > >> +/* windows specific*/
> > >> +#ifdef RTE_EXEC_ENV_WINDOWS
> > >> +#include <rte_windows.h>
> > >> +#endif
> > > 
> > > Include in the config file looks wrong. >
> > >> --- /dev/null
> > >> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
> > > 
> > > I think we could remove the sub-directory exec-env.
> > > Could we include this file from rte_common.h?
> > rte_windows.h defines types and substitution macros which are
> > needed to support common code on windows. So it should be
> > included as a global include for every library on windows.
> > rte_common.h is not included in all the source code and headers
> > we currently build for windows.
> 
> I think it is not an issue adding some rte_common.h include
> here and there.
> 

I'd be hesitant about putting it in rte_common.h without knowing the
scope of the changes - "here and there" could end up being "everywhere".

Another alternative is to see if most/all the definitions could actually be
put in dpdk_conf and thereby go into the standard config.h file generated
at build time. Anything that couldn't be done via macros or defines in the
config could then look to be put maybe in rte_common.h or other locations,
perhaps.

/Bruce
Thomas Monjalon March 7, 2019, 11:33 a.m. UTC | #5 
07/03/2019 11:24, Bruce Richardson:
> On Thu, Mar 07, 2019 at 09:45:57AM +0100, Thomas Monjalon wrote:
> > 07/03/2019 04:27, Anand Rawat:
> > > On 3/6/2019 3:31 AM, Thomas Monjalon wrote:
> > > > 06/03/2019 05:16, Anand Rawat:
> > > >> Added header files to support windows on x86 platforms.
> > > >> Updated rte_config to include rte_windows.h for windows
> > > >> build.
> > > > [...]
> > > >> --- a/config/rte_config.h
> > > >> +++ b/config/rte_config.h
> > > >> +/* windows specific*/
> > > >> +#ifdef RTE_EXEC_ENV_WINDOWS
> > > >> +#include <rte_windows.h>
> > > >> +#endif
> > > > 
> > > > Include in the config file looks wrong. >
> > > >> --- /dev/null
> > > >> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
> > > > 
> > > > I think we could remove the sub-directory exec-env.
> > > > Could we include this file from rte_common.h?
> > > rte_windows.h defines types and substitution macros which are
> > > needed to support common code on windows. So it should be
> > > included as a global include for every library on windows.
> > > rte_common.h is not included in all the source code and headers
> > > we currently build for windows.
> > 
> > I think it is not an issue adding some rte_common.h include
> > here and there.
> > 
> 
> I'd be hesitant about putting it in rte_common.h without knowing the
> scope of the changes - "here and there" could end up being "everywhere".

It's already almost everywhere, and I think it's normal,
it is the meaning of "common".

% git grep rte_common.h | wc -l
459

% git grep rte_common.h lib | fgrep .h: | wc -l
79

If some .c files doesn't have rte_common.h included directly or
indirectly from other includes, it may be fixed.

> Another alternative is to see if most/all the definitions could actually be
> put in dpdk_conf and thereby go into the standard config.h file generated
> at build time. Anything that couldn't be done via macros or defines in the
> config could then look to be put maybe in rte_common.h or other locations,
> perhaps.

I think we should keep the semantic of what a config file is.
I don't see the need of adding more than strict config in it.
Bruce Richardson March 7, 2019, 11:53 a.m. UTC | #6 
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, March 7, 2019 11:33 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Rawat, Anand <anand.rawat@intel.com>; dev@dpdk.org; Kadam, Pallavi
> <pallavi.kadam@intel.com>; Menon, Ranjit <ranjit.menon@intel.com>; Shaw,
> Jeffrey B <jeffrey.b.shaw@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 2/6] eal: add header files to support
> windows
> 
> 07/03/2019 11:24, Bruce Richardson:
> > On Thu, Mar 07, 2019 at 09:45:57AM +0100, Thomas Monjalon wrote:
> > > 07/03/2019 04:27, Anand Rawat:
> > > > On 3/6/2019 3:31 AM, Thomas Monjalon wrote:
> > > > > 06/03/2019 05:16, Anand Rawat:
> > > > >> Added header files to support windows on x86 platforms.
> > > > >> Updated rte_config to include rte_windows.h for windows build.
> > > > > [...]
> > > > >> --- a/config/rte_config.h
> > > > >> +++ b/config/rte_config.h
> > > > >> +/* windows specific*/
> > > > >> +#ifdef RTE_EXEC_ENV_WINDOWS
> > > > >> +#include <rte_windows.h>
> > > > >> +#endif
> > > > >
> > > > > Include in the config file looks wrong. >
> > > > >> --- /dev/null
> > > > >> +++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
> > > > >
> > > > > I think we could remove the sub-directory exec-env.
> > > > > Could we include this file from rte_common.h?
> > > > rte_windows.h defines types and substitution macros which are
> > > > needed to support common code on windows. So it should be included
> > > > as a global include for every library on windows.
> > > > rte_common.h is not included in all the source code and headers we
> > > > currently build for windows.
> > >
> > > I think it is not an issue adding some rte_common.h include here and
> > > there.
> > >
> >
> > I'd be hesitant about putting it in rte_common.h without knowing the
> > scope of the changes - "here and there" could end up being "everywhere".
> 
> It's already almost everywhere, and I think it's normal, it is the meaning
> of "common".
> 
> % git grep rte_common.h | wc -l
> 459
> 
> % git grep rte_common.h lib | fgrep .h: | wc -l
> 79
> 
> If some .c files doesn't have rte_common.h included directly or indirectly
> from other includes, it may be fixed.
> 
> > Another alternative is to see if most/all the definitions could
> > actually be put in dpdk_conf and thereby go into the standard config.h
> > file generated at build time. Anything that couldn't be done via
> > macros or defines in the config could then look to be put maybe in
> > rte_common.h or other locations, perhaps.
> 
> I think we should keep the semantic of what a config file is.
> I don't see the need of adding more than strict config in it.
> 

Ok.
diff mbox series

Patch

diff --git a/config/rte_config.h b/config/rte_config.h
index 7606f5d7b..1ad7e1d30 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -1,5 +1,5 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2017 Intel Corporation
+ * Copyright(c) 2017-2019 Intel Corporation
  */
 
 /**
@@ -118,4 +118,9 @@ 
 /* QEDE PMD defines */
 #define RTE_LIBRTE_QEDE_FW ""
 
+/* windows specific*/
+#ifdef RTE_EXEC_ENV_WINDOWS
+#include <rte_windows.h>
+#endif
+
 #endif /* _RTE_CONFIG_H_ */
diff --git a/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
new file mode 100644
index 000000000..8e4dc72bb
--- /dev/null
+++ b/lib/librte_eal/windows/eal/include/exec-env/rte_windows.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2019 Intel Corporation
+ */
+
+#ifndef _RTE_WINDOWS_H_
+#define _RTE_WINDOWS_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define __extension__
+#define __thread __declspec(thread)
+
+#define strerror_r(a, b, c) strerror_s(b, c, a)
+
+typedef void *ssize_t;
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_WINDOWS_H_ */
diff --git a/lib/librte_eal/windows/eal/meson.build b/lib/librte_eal/windows/eal/meson.build
index 8b1735623..d6c540f82 100644
--- a/lib/librte_eal/windows/eal/meson.build
+++ b/lib/librte_eal/windows/eal/meson.build
@@ -1,6 +1,8 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2019 Intel Corporation
 
+eal_inc += include_directories('include/exec-env')
+
 env_objs = []
 env_headers = []
 env_sources = files('eal.c',