[dpdk-dev] [PATCH v1 0/6] Address various issues with exported headers

[dpdk-dev] [PATCH v1 0/6] Address various issues with exported headers

[Adrien Mazarguil]

Exported header files are installed system-wide and must be as clean and
self-sufficient as possible. In particular they must not conflict with other
system headers and cannot assume specific command-line options will be
provided to the compiler by users, unless their absence is detected and
triggers appropriate #error messages.

This series addresses the following issues:

- The lack of rte_config.h inclusion in most of them.
- The inability to combine rte_ether.h with system's net/ethernet.h,
  preventing applications from using the ether_ntoa() family of functions.
- A couple of GNUisms (e.g. named variadic arguments in macro definitions).

Adrien Mazarguil (6):
  devtools: update check-includes exceptions
  net/i40e: fix issue in exported header
  flow_classify: fix issue in exported header
  member: fix issue in exported header
  fix missing includes in exported headers
  net: fix rte_ether conflicts with libc

 devtools/check-includes.sh                      |  1 +
 drivers/net/avp/rte_avp_common.h                |  1 +
 drivers/net/i40e/base/i40e_osdep.h              |  1 +
 drivers/net/i40e/rte_pmd_i40e.h                 |  2 +-
 drivers/net/ixgbe/base/ixgbe_osdep.h            |  1 +
 drivers/net/qede/base/bcm_osal.h                |  1 -
 lib/librte_cmdline/cmdline_cirbuf.h             |  2 ++
 lib/librte_cryptodev/rte_cryptodev.h            |  1 +
 lib/librte_cryptodev/rte_cryptodev_pmd.h        |  1 +
 .../common/include/arch/x86/rte_atomic.h        |  1 +
 .../common/include/arch/x86/rte_byteorder.h     |  1 +
 .../common/include/arch/x86/rte_cycles.h        |  1 +
 .../common/include/arch/x86/rte_memcpy.h        |  1 +
 .../common/include/arch/x86/rte_vect.h          |  1 +
 .../common/include/generic/rte_byteorder.h      |  1 +
 lib/librte_eal/common/include/rte_bitmap.h      |  1 +
 lib/librte_eal/common/include/rte_common.h      |  2 ++
 lib/librte_eal/common/include/rte_dev.h         |  1 +
 lib/librte_eal/common/include/rte_eal.h         |  1 +
 .../common/include/rte_eal_memconfig.h          |  1 +
 lib/librte_eal/common/include/rte_keepalive.h   |  1 +
 lib/librte_eal/common/include/rte_lcore.h       |  1 +
 lib/librte_eal/common/include/rte_log.h         |  1 +
 lib/librte_eal/common/include/rte_memory.h      |  1 +
 lib/librte_eal/common/include/rte_service.h     |  1 +
 .../eal/include/exec-env/rte_kni_common.h       |  1 +
 lib/librte_ether/rte_ethdev.h                   |  1 +
 lib/librte_ether/rte_ethdev_pci.h               |  1 +
 lib/librte_ether/rte_ethdev_vdev.h              |  1 +
 lib/librte_eventdev/rte_eventdev.h              |  1 +
 lib/librte_eventdev/rte_eventdev_pmd.h          |  1 +
 lib/librte_eventdev/rte_eventdev_pmd_pci.h      |  1 +
 lib/librte_eventdev/rte_eventdev_pmd_vdev.h     |  1 +
 lib/librte_flow_classify/rte_flow_classify.h    | 10 +++++--
 lib/librte_hash/rte_fbk_hash.h                  |  1 +
 lib/librte_hash/rte_hash_crc.h                  |  1 +
 lib/librte_hash/rte_jhash.h                     |  1 +
 lib/librte_hash/rte_thash.h                     |  1 +
 lib/librte_ip_frag/rte_ip_frag.h                |  1 +
 lib/librte_lpm/rte_lpm.h                        |  1 +
 lib/librte_mbuf/rte_mbuf.h                      |  1 +
 lib/librte_member/rte_member.h                  | 12 ++++++--
 lib/librte_mempool/rte_mempool.h                |  1 +
 lib/librte_net/rte_ether.h                      | 30 +++++++++++++-------
 lib/librte_ring/rte_ring.h                      |  1 +
 lib/librte_table/rte_lru.h                      |  1 +
 lib/librte_table/rte_lru_x86.h                  |  2 ++
 lib/librte_timer/rte_timer.h                    |  1 +
 48 files changed, 83 insertions(+), 18 deletions(-)

-- 
2.11.0

[dpdk-dev] [PATCH v1 1/6] devtools: update check-includes exceptions

[Adrien Mazarguil]

rte_eal_interrupts.h is an internal file not supposed to be included
directly by applications.

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>
---
 devtools/check-includes.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/devtools/check-includes.sh b/devtools/check-includes.sh
index c4ec73f15..685a3e772 100755
--- a/devtools/check-includes.sh
+++ b/devtools/check-includes.sh
@@ -111,6 +111,7 @@ include_dir=${1:-build/include}
 	'exec-env/*' \
 	'rte_vhost.h' \
 	'rte_eth_vhost.h' \
+	'rte_eal_interrupts.h' \
 }
 : ${IGNORE_CXX= \
 	'rte_vhost.h' \
-- 
2.11.0

[dpdk-dev] [PATCH v1 2/6] net/i40e: fix issue in exported header

[Adrien Mazarguil]

Reported by check-includes.sh:

 [...]/rte_pmd_i40e.h:97:30: error: ISO C restricts enumerator values to
     range of `int' [-Werror=pedantic]
   RTE_PMD_I40E_PKG_INFO_MAX = 0xFFFFFFFF
                               ^

Fixes: edeab742edac ("net/i40e: get information about DDP profile")
Cc: Andrey Chilikin <andrey.chilikin@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/i40e/rte_pmd_i40e.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h
index 580ca4ae9..49f4427a5 100644
--- a/drivers/net/i40e/rte_pmd_i40e.h
+++ b/drivers/net/i40e/rte_pmd_i40e.h
@@ -94,7 +94,7 @@ enum rte_pmd_i40e_package_info {
 	RTE_PMD_I40E_PKG_INFO_PCTYPE_LIST,
 	RTE_PMD_I40E_PKG_INFO_PTYPE_NUM,
 	RTE_PMD_I40E_PKG_INFO_PTYPE_LIST,
-	RTE_PMD_I40E_PKG_INFO_MAX = 0xFFFFFFFF
+	RTE_PMD_I40E_PKG_INFO_MAX = (int)0xFFFFFFFF
 };
 
 /**
-- 
2.11.0

[dpdk-dev] [PATCH v1 3/6] flow_classify: fix issue in exported header

[Adrien Mazarguil]

Reported by check-includes.sh:

 [...]/rte_flow_classify.h:85:47: error: ISO C does not permit named
     variadic macros [-Werror=variadic-macros]
  #define RTE_FLOW_CLASSIFY_LOG(level, fmt, args...) \
                                                ^

Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
Cc: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: Bernard Iremonger <bernard.iremonger@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_flow_classify/rte_flow_classify.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/librte_flow_classify/rte_flow_classify.h b/lib/librte_flow_classify/rte_flow_classify.h
index 1211873a1..e14bc787e 100644
--- a/lib/librte_flow_classify/rte_flow_classify.h
+++ b/lib/librte_flow_classify/rte_flow_classify.h
@@ -70,6 +70,7 @@
  *    with rte_flow_classifier_free()
  */
 
+#include <rte_common.h>
 #include <rte_ethdev.h>
 #include <rte_ether.h>
 #include <rte_flow.h>
@@ -82,9 +83,12 @@ extern "C" {
 
 extern int librte_flow_classify_logtype;
 
-#define RTE_FLOW_CLASSIFY_LOG(level, fmt, args...) \
-rte_log(RTE_LOG_ ## level, librte_flow_classify_logtype, "%s(): " fmt, \
-	__func__, ## args)
+#define RTE_FLOW_CLASSIFY_LOG(level, ...) \
+	rte_log(RTE_LOG_ ## level, \
+		librte_flow_classify_logtype, \
+		RTE_FMT("%s(): " RTE_FMT_HEAD(__VA_ARGS__,), \
+			__func__, \
+			RTE_FMT_TAIL(__VA_ARGS__,)))
 
 /** Opaque data type for flow classifier */
 struct rte_flow_classifier;
-- 
2.11.0

[dpdk-dev] [PATCH v1 4/6] member: fix issue in exported header

[Adrien Mazarguil]

Reported by check-includes.sh:

 [...]/rte_member.h:107:40: error: ISO C does not permit named variadic
     macros [-Werror=variadic-macros]
  #define RTE_MEMBER_LOG(level, fmt, args...) \
                                         ^

Fixes: 857ed6c68cf2 ("member: implement main API")
Cc: Yipeng Wang <yipeng1.wang@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_member/rte_member.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/librte_member/rte_member.h b/lib/librte_member/rte_member.h
index 9b0c8f99c..fa0871122 100644
--- a/lib/librte_member/rte_member.h
+++ b/lib/librte_member/rte_member.h
@@ -80,6 +80,8 @@ extern "C" {
 
 #include <stdint.h>
 
+#include <rte_common.h>
+
 /** The set ID type that stored internally in hash table based set summary. */
 typedef uint16_t member_set_t;
 /** Invalid set ID used to mean no match found. */
@@ -104,9 +106,12 @@ typedef uint16_t member_set_t;
 
 extern int librte_member_logtype;
 
-#define RTE_MEMBER_LOG(level, fmt, args...) \
-rte_log(RTE_LOG_ ## level, librte_member_logtype, "%s(): " fmt, \
-	__func__, ## args)
+#define RTE_MEMBER_LOG(level, ...) \
+	rte_log(RTE_LOG_ ## level, \
+		librte_member_logtype, \
+		RTE_FMT("%s(): " RTE_FMT_HEAD(__VA_ARGS__,), \
+			__func__, \
+			RTE_FMT_TAIL(__VA_ARGS__,)))
 
 /** @internal setsummary structure. */
 struct rte_member_setsum;
-- 
2.11.0

[dpdk-dev] [PATCH v1 5/6] fix missing includes in exported headers

[Adrien Mazarguil]

Many exported headers rely on definitions found in rte_config.h without
including it, as shown by the following command:

 grep -L '^#include <rte_config.h>' -- \
  $(grep -Rl \
    $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
      build/include/rte_config.h) \
    -- build/include/)

We cannot assume external applications will include rte_config.h on their
own, neither directly nor through a -include parameter like DPDK does
internally.

This not only causes obvious compilation failures that can be reproduced
with check-includes.sh such as:

 [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
     this scope
  #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
                                            ^

It also results in less visible issues, for instance rte_hash_crc.h relying
on RTE_ARCH_X86_64's presence to provide dedicated inline functions.

This patch partially reverts the commit below and adds missing include
lines to the remaining files.

Fixes: f1a7a5c5f404 ("remove include of generated config header")
Cc: Thomas Monjalon <thomas@monjalon.net>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/avp/rte_avp_common.h                              | 1 +
 lib/librte_cmdline/cmdline_cirbuf.h                           | 2 ++
 lib/librte_cryptodev/rte_cryptodev.h                          | 1 +
 lib/librte_cryptodev/rte_cryptodev_pmd.h                      | 1 +
 lib/librte_eal/common/include/arch/x86/rte_atomic.h           | 1 +
 lib/librte_eal/common/include/arch/x86/rte_byteorder.h        | 1 +
 lib/librte_eal/common/include/arch/x86/rte_cycles.h           | 1 +
 lib/librte_eal/common/include/arch/x86/rte_memcpy.h           | 1 +
 lib/librte_eal/common/include/arch/x86/rte_vect.h             | 1 +
 lib/librte_eal/common/include/generic/rte_byteorder.h         | 1 +
 lib/librte_eal/common/include/rte_bitmap.h                    | 1 +
 lib/librte_eal/common/include/rte_common.h                    | 2 ++
 lib/librte_eal/common/include/rte_dev.h                       | 1 +
 lib/librte_eal/common/include/rte_eal.h                       | 1 +
 lib/librte_eal/common/include/rte_eal_memconfig.h             | 1 +
 lib/librte_eal/common/include/rte_keepalive.h                 | 1 +
 lib/librte_eal/common/include/rte_lcore.h                     | 1 +
 lib/librte_eal/common/include/rte_log.h                       | 1 +
 lib/librte_eal/common/include/rte_memory.h                    | 1 +
 lib/librte_eal/common/include/rte_service.h                   | 1 +
 lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h | 1 +
 lib/librte_ether/rte_ethdev.h                                 | 1 +
 lib/librte_ether/rte_ethdev_pci.h                             | 1 +
 lib/librte_ether/rte_ethdev_vdev.h                            | 1 +
 lib/librte_eventdev/rte_eventdev.h                            | 1 +
 lib/librte_eventdev/rte_eventdev_pmd.h                        | 1 +
 lib/librte_eventdev/rte_eventdev_pmd_pci.h                    | 1 +
 lib/librte_eventdev/rte_eventdev_pmd_vdev.h                   | 1 +
 lib/librte_hash/rte_fbk_hash.h                                | 1 +
 lib/librte_hash/rte_hash_crc.h                                | 1 +
 lib/librte_hash/rte_jhash.h                                   | 1 +
 lib/librte_hash/rte_thash.h                                   | 1 +
 lib/librte_ip_frag/rte_ip_frag.h                              | 1 +
 lib/librte_lpm/rte_lpm.h                                      | 1 +
 lib/librte_mbuf/rte_mbuf.h                                    | 1 +
 lib/librte_member/rte_member.h                                | 1 +
 lib/librte_mempool/rte_mempool.h                              | 1 +
 lib/librte_ring/rte_ring.h                                    | 1 +
 lib/librte_table/rte_lru.h                                    | 1 +
 lib/librte_table/rte_lru_x86.h                                | 2 ++
 lib/librte_timer/rte_timer.h                                  | 1 +
 41 files changed, 44 insertions(+)

diff --git a/drivers/net/avp/rte_avp_common.h b/drivers/net/avp/rte_avp_common.h
index 54437e9a1..81dfe5ea6 100644
--- a/drivers/net/avp/rte_avp_common.h
+++ b/drivers/net/avp/rte_avp_common.h
@@ -63,6 +63,7 @@
 #else
 #include <stdint.h>
 #include <rte_common.h>
+#include <rte_config.h>
 #include <rte_memory.h>
 #include <rte_ether.h>
 #include <rte_atomic.h>
diff --git a/lib/librte_cmdline/cmdline_cirbuf.h b/lib/librte_cmdline/cmdline_cirbuf.h
index 6321dec5c..87407efa1 100644
--- a/lib/librte_cmdline/cmdline_cirbuf.h
+++ b/lib/librte_cmdline/cmdline_cirbuf.h
@@ -61,6 +61,8 @@
 #ifndef _CIRBUF_H_
 #define _CIRBUF_H_
 
+#include <rte_config.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h
index dade5548f..ed92f9822 100644
--- a/lib/librte_cryptodev/rte_cryptodev.h
+++ b/lib/librte_cryptodev/rte_cryptodev.h
@@ -49,6 +49,7 @@ extern "C" {
 #include "rte_crypto.h"
 #include "rte_dev.h"
 #include <rte_common.h>
+#include <rte_config.h>
 
 extern const char **rte_cyptodev_names;
 
diff --git a/lib/librte_cryptodev/rte_cryptodev_pmd.h b/lib/librte_cryptodev/rte_cryptodev_pmd.h
index 744405e2f..c3bf91c3c 100644
--- a/lib/librte_cryptodev/rte_cryptodev_pmd.h
+++ b/lib/librte_cryptodev/rte_cryptodev_pmd.h
@@ -46,6 +46,7 @@ extern "C" {
 
 #include <string.h>
 
+#include <rte_config.h>
 #include <rte_dev.h>
 #include <rte_malloc.h>
 #include <rte_mbuf.h>
diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic.h b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
index 4eac66631..ea665d521 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic.h
@@ -40,6 +40,7 @@ extern "C" {
 
 #include <stdint.h>
 #include <rte_common.h>
+#include <rte_config.h>
 #include <emmintrin.h>
 #include "generic/rte_atomic.h"
 
diff --git a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
index 251f11b4d..126f29e04 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_byteorder.h
@@ -40,6 +40,7 @@ extern "C" {
 
 #include <stdint.h>
 #include <rte_common.h>
+#include <rte_config.h>
 #include "generic/rte_byteorder.h"
 
 #ifndef RTE_BYTE_ORDER
diff --git a/lib/librte_eal/common/include/arch/x86/rte_cycles.h b/lib/librte_eal/common/include/arch/x86/rte_cycles.h
index 1bb3e1dbe..b95beced9 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_cycles.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_cycles.h
@@ -47,6 +47,7 @@ extern int rte_cycles_vmware_tsc_map;
 #include <rte_branch_prediction.h>
 #endif
 #include <rte_common.h>
+#include <rte_config.h>
 
 static inline uint64_t
 rte_rdtsc(void)
diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
index 74c280c2c..596d77791 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
@@ -45,6 +45,7 @@
 #include <string.h>
 #include <rte_vect.h>
 #include <rte_common.h>
+#include <rte_config.h>
 
 #ifdef __cplusplus
 extern "C" {
diff --git a/lib/librte_eal/common/include/arch/x86/rte_vect.h b/lib/librte_eal/common/include/arch/x86/rte_vect.h
index 03fc991ed..893b81200 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_vect.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_vect.h
@@ -41,6 +41,7 @@
  */
 
 #include <stdint.h>
+#include <rte_config.h>
 #include "generic/rte_vect.h"
 
 #if (defined(__ICC) || (__GNUC__ == 4 &&  __GNUC_MINOR__ < 4))
diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
index e5e820d36..29e70c968 100644
--- a/lib/librte_eal/common/include/generic/rte_byteorder.h
+++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
@@ -51,6 +51,7 @@
 #endif
 
 #include <rte_common.h>
+#include <rte_config.h>
 
 /*
  * Compile-time endianness detection
diff --git a/lib/librte_eal/common/include/rte_bitmap.h b/lib/librte_eal/common/include/rte_bitmap.h
index 010d752c2..b9067e67f 100644
--- a/lib/librte_eal/common/include/rte_bitmap.h
+++ b/lib/librte_eal/common/include/rte_bitmap.h
@@ -66,6 +66,7 @@ extern "C" {
 
 #include <string.h>
 #include <rte_common.h>
+#include <rte_config.h>
 #include <rte_debug.h>
 #include <rte_memory.h>
 #include <rte_branch_prediction.h>
diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index de853e164..f1d24b86d 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -51,6 +51,8 @@ extern "C" {
 #include <errno.h>
 #include <limits.h>
 
+#include <rte_config.h>
+
 #ifndef typeof
 #define typeof __typeof__
 #endif
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 9342e0cbd..8088dcc53 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -49,6 +49,7 @@ extern "C" {
 #include <stdio.h>
 #include <sys/queue.h>
 
+#include <rte_config.h>
 #include <rte_log.h>
 
 __attribute__((format(printf, 2, 0)))
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 8e4e71cc1..78b0e0051 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -43,6 +43,7 @@
 #include <stdint.h>
 #include <sched.h>
 
+#include <rte_config.h>
 #include <rte_per_lcore.h>
 #include <rte_bus.h>
 
diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index b9eee702e..c963b450d 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -34,6 +34,7 @@
 #ifndef _RTE_EAL_MEMCONFIG_H_
 #define _RTE_EAL_MEMCONFIG_H_
 
+#include <rte_config.h>
 #include <rte_tailq.h>
 #include <rte_memory.h>
 #include <rte_memzone.h>
diff --git a/lib/librte_eal/common/include/rte_keepalive.h b/lib/librte_eal/common/include/rte_keepalive.h
index 88ad8e487..e9f8f083a 100644
--- a/lib/librte_eal/common/include/rte_keepalive.h
+++ b/lib/librte_eal/common/include/rte_keepalive.h
@@ -39,6 +39,7 @@
 #ifndef _KEEPALIVE_H_
 #define _KEEPALIVE_H_
 
+#include <rte_config.h>
 #include <rte_memory.h>
 
 #ifndef RTE_KEEPALIVE_MAXCORES
diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index c89e6bab1..3735da0c7 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -40,6 +40,7 @@
  * API for lcore and socket manipulation
  *
  */
+#include <rte_config.h>
 #include <rte_per_lcore.h>
 #include <rte_eal.h>
 #include <rte_launch.h>
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 6c2d35667..0bb106896 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -51,6 +51,7 @@ extern "C" {
 #include <stdarg.h>
 
 #include <rte_common.h>
+#include <rte_config.h>
 
 struct rte_log_dynamic_type;
 
diff --git a/lib/librte_eal/common/include/rte_memory.h b/lib/librte_eal/common/include/rte_memory.h
index 14aacea54..80a8fc023 100644
--- a/lib/librte_eal/common/include/rte_memory.h
+++ b/lib/librte_eal/common/include/rte_memory.h
@@ -49,6 +49,7 @@ extern "C" {
 #endif
 
 #include <rte_common.h>
+#include <rte_config.h>
 
 __extension__
 enum rte_page_sizes {
diff --git a/lib/librte_eal/common/include/rte_service.h b/lib/librte_eal/common/include/rte_service.h
index 927244065..5d9f8b34b 100644
--- a/lib/librte_eal/common/include/rte_service.h
+++ b/lib/librte_eal/common/include/rte_service.h
@@ -59,6 +59,7 @@ extern "C" {
 #include <stdint.h>
 #include <sys/queue.h>
 
+#include <rte_config.h>
 #include <rte_lcore.h>
 
 #define RTE_SERVICE_NAME_MAX 32
diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
index 2ac879fdd..794cd4f78 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
@@ -64,6 +64,7 @@
 #define RTE_STD_C11
 #else
 #include <rte_common.h>
+#include <rte_config.h>
 #endif
 
 /**
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 341c2d624..8c68c604f 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -181,6 +181,7 @@ extern "C" {
 #include <rte_devargs.h>
 #include <rte_errno.h>
 #include <rte_common.h>
+#include <rte_config.h>
 
 #include "rte_ether.h"
 #include "rte_eth_ctrl.h"
diff --git a/lib/librte_ether/rte_ethdev_pci.h b/lib/librte_ether/rte_ethdev_pci.h
index 722075e09..ad64a169c 100644
--- a/lib/librte_ether/rte_ethdev_pci.h
+++ b/lib/librte_ether/rte_ethdev_pci.h
@@ -37,6 +37,7 @@
 #include <rte_malloc.h>
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
+#include <rte_config.h>
 #include <rte_ethdev.h>
 
 /**
diff --git a/lib/librte_ether/rte_ethdev_vdev.h b/lib/librte_ether/rte_ethdev_vdev.h
index ff92e6ed0..feb5a3eb0 100644
--- a/lib/librte_ether/rte_ethdev_vdev.h
+++ b/lib/librte_ether/rte_ethdev_vdev.h
@@ -34,6 +34,7 @@
 #ifndef _RTE_ETHDEV_VDEV_H_
 #define _RTE_ETHDEV_VDEV_H_
 
+#include <rte_config.h>
 #include <rte_malloc.h>
 #include <rte_bus_vdev.h>
 #include <rte_ethdev.h>
diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
index f1949ff7d..9134b4984 100644
--- a/lib/librte_eventdev/rte_eventdev.h
+++ b/lib/librte_eventdev/rte_eventdev.h
@@ -239,6 +239,7 @@ extern "C" {
 #endif
 
 #include <rte_common.h>
+#include <rte_config.h>
 #include <rte_memory.h>
 #include <rte_errno.h>
 
diff --git a/lib/librte_eventdev/rte_eventdev_pmd.h b/lib/librte_eventdev/rte_eventdev_pmd.h
index 7a206c56f..b78423db4 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd.h
@@ -47,6 +47,7 @@ extern "C" {
 #include <string.h>
 
 #include <rte_common.h>
+#include <rte_config.h>
 #include <rte_dev.h>
 #include <rte_log.h>
 #include <rte_malloc.h>
diff --git a/lib/librte_eventdev/rte_eventdev_pmd_pci.h b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
index ade32b5db..8da40be04 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd_pci.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd_pci.h
@@ -47,6 +47,7 @@ extern "C" {
 
 #include <string.h>
 
+#include <rte_config.h>
 #include <rte_eal.h>
 #include <rte_lcore.h>
 #include <rte_pci.h>
diff --git a/lib/librte_eventdev/rte_eventdev_pmd_vdev.h b/lib/librte_eventdev/rte_eventdev_pmd_vdev.h
index 56232dec4..f77d59bd9 100644
--- a/lib/librte_eventdev/rte_eventdev_pmd_vdev.h
+++ b/lib/librte_eventdev/rte_eventdev_pmd_vdev.h
@@ -46,6 +46,7 @@ extern "C" {
 
 #include <string.h>
 
+#include <rte_config.h>
 #include <rte_debug.h>
 #include <rte_eal.h>
 #include <rte_bus_vdev.h>
diff --git a/lib/librte_hash/rte_fbk_hash.h b/lib/librte_hash/rte_fbk_hash.h
index c39c09765..e0d42b454 100644
--- a/lib/librte_hash/rte_fbk_hash.h
+++ b/lib/librte_hash/rte_fbk_hash.h
@@ -54,6 +54,7 @@ extern "C" {
 
 #include <string.h>
 
+#include <rte_config.h>
 #ifndef RTE_FBK_HASH_FUNC_DEFAULT
 #if defined(RTE_ARCH_X86) || defined(RTE_MACHINE_CPUFLAG_CRC32)
 #include <rte_hash_crc.h>
diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 4f815aea8..93188c290 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -45,6 +45,7 @@ extern "C" {
 #endif
 
 #include <stdint.h>
+#include <rte_config.h>
 #include <rte_cpuflags.h>
 #include <rte_branch_prediction.h>
 #include <rte_common.h>
diff --git a/lib/librte_hash/rte_jhash.h b/lib/librte_hash/rte_jhash.h
index 3eca13858..42c45685b 100644
--- a/lib/librte_hash/rte_jhash.h
+++ b/lib/librte_hash/rte_jhash.h
@@ -48,6 +48,7 @@ extern "C" {
 #include <string.h>
 #include <limits.h>
 
+#include <rte_config.h>
 #include <rte_log.h>
 #include <rte_byteorder.h>
 
diff --git a/lib/librte_hash/rte_thash.h b/lib/librte_hash/rte_thash.h
index 4fa5e07a5..a6ddb7bf7 100644
--- a/lib/librte_hash/rte_thash.h
+++ b/lib/librte_hash/rte_thash.h
@@ -53,6 +53,7 @@ extern "C" {
 
 #include <stdint.h>
 #include <rte_byteorder.h>
+#include <rte_config.h>
 #include <rte_ip.h>
 #include <rte_common.h>
 
diff --git a/lib/librte_ip_frag/rte_ip_frag.h b/lib/librte_ip_frag/rte_ip_frag.h
index 9f8cede8d..014416414 100644
--- a/lib/librte_ip_frag/rte_ip_frag.h
+++ b/lib/librte_ip_frag/rte_ip_frag.h
@@ -48,6 +48,7 @@ extern "C" {
 #include <stdint.h>
 #include <stdio.h>
 
+#include <rte_config.h>
 #include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_ip.h>
diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 682865e4c..650c51d1d 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -45,6 +45,7 @@
 #include <stdlib.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
+#include <rte_config.h>
 #include <rte_memory.h>
 #include <rte_common.h>
 #include <rte_vect.h>
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index ce8a05ddf..e5d7643cf 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -62,6 +62,7 @@
 
 #include <stdint.h>
 #include <rte_common.h>
+#include <rte_config.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
 #include <rte_atomic.h>
diff --git a/lib/librte_member/rte_member.h b/lib/librte_member/rte_member.h
index fa0871122..56dd3b3a9 100644
--- a/lib/librte_member/rte_member.h
+++ b/lib/librte_member/rte_member.h
@@ -81,6 +81,7 @@ extern "C" {
 #include <stdint.h>
 
 #include <rte_common.h>
+#include <rte_config.h>
 
 /** The set ID type that stored internally in hash table based set summary. */
 typedef uint16_t member_set_t;
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 721227f6d..e21026aa7 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -69,6 +69,7 @@
 #include <inttypes.h>
 #include <sys/queue.h>
 
+#include <rte_config.h>
 #include <rte_spinlock.h>
 #include <rte_log.h>
 #include <rte_debug.h>
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index e92443813..7069d52e2 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -96,6 +96,7 @@ extern "C" {
 #include <sys/queue.h>
 #include <errno.h>
 #include <rte_common.h>
+#include <rte_config.h>
 #include <rte_memory.h>
 #include <rte_lcore.h>
 #include <rte_atomic.h>
diff --git a/lib/librte_table/rte_lru.h b/lib/librte_table/rte_lru.h
index 93258ef46..a73b2ffe4 100644
--- a/lib/librte_table/rte_lru.h
+++ b/lib/librte_table/rte_lru.h
@@ -38,6 +38,7 @@
 extern "C" {
 #endif
 
+#include <rte_config.h>
 #ifdef RTE_ARCH_X86_64
 #include "rte_lru_x86.h"
 #elif defined(RTE_ARCH_ARM64)
diff --git a/lib/librte_table/rte_lru_x86.h b/lib/librte_table/rte_lru_x86.h
index 10f513cd2..b4f605257 100644
--- a/lib/librte_table/rte_lru_x86.h
+++ b/lib/librte_table/rte_lru_x86.h
@@ -40,6 +40,8 @@ extern "C" {
 
 #include <stdint.h>
 
+#include <rte_config.h>
+
 #ifndef RTE_TABLE_HASH_LRU_STRATEGY
 #define RTE_TABLE_HASH_LRU_STRATEGY                        2
 #endif
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index a276a7361..3b50155c1 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -67,6 +67,7 @@
 #include <stdint.h>
 #include <stddef.h>
 #include <rte_common.h>
+#include <rte_config.h>
 
 #ifdef __cplusplus
 extern "C" {
-- 
2.11.0

[dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc

[Adrien Mazarguil]

Applications can't combine either net/ethernet.h or netinet/ether.h
together with rte_ether.h due to the redefinition of struct ether_addr and
various macros by the latter.

This patch adapts rte_ether.h to rely on system definitions while
maintaining DPDK additions.

An unforeseen consequence of involving more system header files compilation
issues with some base drivers (i40e, ixgbe) defining their own conflicting
types (e.g. __le64). This is addressed by explicitly including rte_ether.h
where missing to ensure system definitions always come first.

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Olivier Matz <olivier.matz@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>
---
This patch should work but wasn't validated on FreeBSD, any volunteers?
---
 drivers/net/i40e/base/i40e_osdep.h   |  1 +
 drivers/net/ixgbe/base/ixgbe_osdep.h |  1 +
 drivers/net/qede/base/bcm_osal.h     |  1 -
 lib/librte_net/rte_ether.h           | 30 ++++++++++++++++++++----------
 4 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
index 8e5c593c9..0f53d8f66 100644
--- a/drivers/net/i40e/base/i40e_osdep.h
+++ b/drivers/net/i40e/base/i40e_osdep.h
@@ -40,6 +40,7 @@
 #include <stdarg.h>
 
 #include <rte_common.h>
+#include <rte_ether.h>
 #include <rte_memcpy.h>
 #include <rte_byteorder.h>
 #include <rte_cycles.h>
diff --git a/drivers/net/ixgbe/base/ixgbe_osdep.h b/drivers/net/ixgbe/base/ixgbe_osdep.h
index bb5dfd2af..a59d2a63f 100644
--- a/drivers/net/ixgbe/base/ixgbe_osdep.h
+++ b/drivers/net/ixgbe/base/ixgbe_osdep.h
@@ -41,6 +41,7 @@
 #include <stdarg.h>
 #include <rte_common.h>
 #include <rte_debug.h>
+#include <rte_ether.h>
 #include <rte_cycles.h>
 #include <rte_log.h>
 #include <rte_byteorder.h>
diff --git a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
index 52c2f0ec9..c4c8e179e 100644
--- a/drivers/net/qede/base/bcm_osal.h
+++ b/drivers/net/qede/base/bcm_osal.h
@@ -334,7 +334,6 @@ u32 qede_find_first_zero_bit(unsigned long *, u32);
 	qede_find_first_zero_bit(bitmap, length)
 
 #define OSAL_BUILD_BUG_ON(cond)		nothing
-#define ETH_ALEN			ETHER_ADDR_LEN
 
 #define OSAL_BITMAP_WEIGHT(bitmap, count) 0
 
diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 06d7b486c..19e62ea89 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -44,6 +44,7 @@
 extern "C" {
 #endif
 
+#include <net/ethernet.h>
 #include <stdint.h>
 #include <stdio.h>
 
@@ -52,15 +53,7 @@ extern "C" {
 #include <rte_mbuf.h>
 #include <rte_byteorder.h>
 
-#define ETHER_ADDR_LEN  6 /**< Length of Ethernet address. */
-#define ETHER_TYPE_LEN  2 /**< Length of Ethernet type field. */
-#define ETHER_CRC_LEN   4 /**< Length of Ethernet CRC. */
-#define ETHER_HDR_LEN   \
-	(ETHER_ADDR_LEN * 2 + ETHER_TYPE_LEN) /**< Length of Ethernet header. */
-#define ETHER_MIN_LEN   64    /**< Minimum frame len, including CRC. */
-#define ETHER_MAX_LEN   1518  /**< Maximum frame len, including CRC. */
-#define ETHER_MTU       \
-	(ETHER_MAX_LEN - ETHER_HDR_LEN - ETHER_CRC_LEN) /**< Ethernet MTU. */
+#define ETHER_MTU ETHERMTU /**< Deprecated, defined for compatibility. */
 
 #define ETHER_MAX_VLAN_FRAME_LEN \
 	(ETHER_MAX_LEN + 4) /**< Maximum VLAN frame length, including CRC. */
@@ -72,8 +65,11 @@ extern "C" {
 
 #define ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
 
+#ifdef __DOXYGEN__
+
 /**
- * Ethernet address:
+ * Ethernet address.
+ *
  * A universally administered address is uniquely assigned to a device by its
  * manufacturer. The first three octets (in transmission order) contain the
  * Organizationally Unique Identifier (OUI). The following three (MAC-48 and
@@ -82,11 +78,25 @@ extern "C" {
  * A locally administered address is assigned to a device by a network
  * administrator and does not contain OUIs.
  * See http://standards.ieee.org/regauth/groupmac/tutorial.html
+ *
+ * This structure is defined system-wide by "net/ethernet.h", however since
+ * the name of its data field is OS-dependent, a macro named "addr_bytes" is
+ * defined as an alias for the convenience of DPDK applications.
+ *
+ * The following definition is only for documentation purposes.
  */
 struct ether_addr {
 	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
 } __attribute__((__packed__));
 
+#endif /* __DOXYGEN__ */
+
+#if defined(__FreeBSD__)
+#define addr_bytes octet
+#else
+#define addr_bytes ether_addr_octet
+#endif
+
 #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
 #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH v1 5/6] fix missing includes in exported headers

[Bruce Richardson]

On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote:
> Many exported headers rely on definitions found in rte_config.h without
> including it, as shown by the following command:
> 
>  grep -L '^#include <rte_config.h>' -- \
>   $(grep -Rl \
>     $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
>       build/include/rte_config.h) \
>     -- build/include/)
> 
> We cannot assume external applications will include rte_config.h on their
> own, neither directly nor through a -include parameter like DPDK does
> internally.
> 
> This not only causes obvious compilation failures that can be reproduced
> with check-includes.sh such as:
> 
>  [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
>      this scope
>   #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
>                                             ^
> 
> It also results in less visible issues, for instance rte_hash_crc.h relying
> on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
> 
> This patch partially reverts the commit below and adds missing include
> lines to the remaining files.
> 
> Fixes: f1a7a5c5f404 ("remove include of generated config header")
> Cc: Thomas Monjalon <thomas@monjalon.net>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---

Hi Adrien,

Just FYI, when we move to the new DPDK build system and pass the
necessary build meta-data to the application using pkg-config, this
should be a non-issue, as the pkg-config information will include the
"-include rte_config.h" parameter.

When investigating that, I also tried this approach of adding rte_config
to files explicitly but it did not work for me as expected, as there
were cases where the build was depending upon the rte_config.h always
being the first include in the file. Normally, the rte_* headers should
be last included, so putting it at the top just didn't seem right to me.
I don't remember the specifics, but it was something like using the RTE_
defines to determine which system header file to use e.g. BSD vs Linux.
However, this may be an internal DPDK-build restriction rather than one
that would affect user-apps our or examples.

So, with transitioning to meson and pkg-config, this issue becomes less
significant.

/Bruce

Re: [dpdk-dev] [PATCH v1 5/6] fix missing includes in exported headers

[Adrien Mazarguil]

On Thu, Dec 21, 2017 at 02:12:57PM +0000, Bruce Richardson wrote:
> On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote:
> > Many exported headers rely on definitions found in rte_config.h without
> > including it, as shown by the following command:
> > 
> >  grep -L '^#include <rte_config.h>' -- \
> >   $(grep -Rl \
> >     $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
> >       build/include/rte_config.h) \
> >     -- build/include/)
> > 
> > We cannot assume external applications will include rte_config.h on their
> > own, neither directly nor through a -include parameter like DPDK does
> > internally.
> > 
> > This not only causes obvious compilation failures that can be reproduced
> > with check-includes.sh such as:
> > 
> >  [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
> >      this scope
> >   #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
> >                                             ^
> > 
> > It also results in less visible issues, for instance rte_hash_crc.h relying
> > on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
> > 
> > This patch partially reverts the commit below and adds missing include
> > lines to the remaining files.
> > 
> > Fixes: f1a7a5c5f404 ("remove include of generated config header")
> > Cc: Thomas Monjalon <thomas@monjalon.net>
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> 
> Hi Adrien,
> 
> Just FYI, when we move to the new DPDK build system and pass the
> necessary build meta-data to the application using pkg-config, this
> should be a non-issue, as the pkg-config information will include the
> "-include rte_config.h" parameter.

Right, actually -include rte_config.h is also provided to applications that
rely on the current DPDK build system, those are already safe.

The motivation behind this patch is other applications that casually
#include things without even depending on the build features of DPDK (not
even going through pkg-config manually), in order to meet a safety level
worthy of /usr/include.

In my opinion it's fine if this usage is not supported and a prior #include
<rte_config.h> is required, however in that case we need our header files to
#error out. Adding #include directly as done in this patch was in fact just
as easy.

> When investigating that, I also tried this approach of adding rte_config
> to files explicitly but it did not work for me as expected, as there
> were cases where the build was depending upon the rte_config.h always
> being the first include in the file. Normally, the rte_* headers should
> be last included, so putting it at the top just didn't seem right to me.
> I don't remember the specifics, but it was something like using the RTE_
> defines to determine which system header file to use e.g. BSD vs Linux.
> However, this may be an internal DPDK-build restriction rather than one
> that would affect user-apps our or examples.

Was it perhaps before we added consistency to all public headers?

check-includes.sh was added a few releases ago to make sure each of them
could be included on its own, to ensure they properly fetched all their
dependencies instead of triggering compilation errors.

Rule of thumb being if you need something, #include the file providing it
first. I find it strange rte_config.h is somehow an exception.

> So, with transitioning to meson and pkg-config, this issue becomes less
> significant.

Agreed, however I still think something needs to be done, so:

- Do we want to let applications not rely on our build flags yet still use
  our exported headers?

- Is a missing #include <rte_config.h> supposed to trigger an explicit
  #error for safety?

- How about putting back #include <rte_config.h> in our exported header
  files directly given the above?

Note this discussion is only about exported headers. The rest remains
unaffected and can safely continue to rely on -include rte_config.h.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v1 5/6] fix missing includes in exported headers

[Bruce Richardson]

On Thu, Dec 21, 2017 at 03:50:31PM +0100, Adrien Mazarguil wrote:
> On Thu, Dec 21, 2017 at 02:12:57PM +0000, Bruce Richardson wrote:
> > On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote:
> > > Many exported headers rely on definitions found in rte_config.h without
> > > including it, as shown by the following command:
> > > 
> > >  grep -L '^#include <rte_config.h>' -- \
> > >   $(grep -Rl \
> > >     $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
> > >       build/include/rte_config.h) \
> > >     -- build/include/)
> > > 
> > > We cannot assume external applications will include rte_config.h on their
> > > own, neither directly nor through a -include parameter like DPDK does
> > > internally.
> > > 
> > > This not only causes obvious compilation failures that can be reproduced
> > > with check-includes.sh such as:
> > > 
> > >  [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
> > >      this scope
> > >   #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
> > >                                             ^
> > > 
> > > It also results in less visible issues, for instance rte_hash_crc.h relying
> > > on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
> > > 
> > > This patch partially reverts the commit below and adds missing include
> > > lines to the remaining files.
> > > 
> > > Fixes: f1a7a5c5f404 ("remove include of generated config header")
> > > Cc: Thomas Monjalon <thomas@monjalon.net>
> > > 
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > ---
> > 
> > Hi Adrien,
> > 
> > Just FYI, when we move to the new DPDK build system and pass the
> > necessary build meta-data to the application using pkg-config, this
> > should be a non-issue, as the pkg-config information will include the
> > "-include rte_config.h" parameter.
> 
> Right, actually -include rte_config.h is also provided to applications that
> rely on the current DPDK build system, those are already safe.
> 
> The motivation behind this patch is other applications that casually
> #include things without even depending on the build features of DPDK (not
> even going through pkg-config manually), in order to meet a safety level
> worthy of /usr/include.
> 
> In my opinion it's fine if this usage is not supported and a prior #include
> <rte_config.h> is required, however in that case we need our header files to
> #error out. Adding #include directly as done in this patch was in fact just
> as easy.
> 
> > When investigating that, I also tried this approach of adding rte_config
> > to files explicitly but it did not work for me as expected, as there
> > were cases where the build was depending upon the rte_config.h always
> > being the first include in the file. Normally, the rte_* headers should
> > be last included, so putting it at the top just didn't seem right to me.
> > I don't remember the specifics, but it was something like using the RTE_
> > defines to determine which system header file to use e.g. BSD vs Linux.
> > However, this may be an internal DPDK-build restriction rather than one
> > that would affect user-apps our or examples.
> 
> Was it perhaps before we added consistency to all public headers?
> 
> check-includes.sh was added a few releases ago to make sure each of them
> could be included on its own, to ensure they properly fetched all their
> dependencies instead of triggering compilation errors.
> 
> Rule of thumb being if you need something, #include the file providing it
> first. I find it strange rte_config.h is somehow an exception.
> 
> > So, with transitioning to meson and pkg-config, this issue becomes less
> > significant.
> 
> Agreed, however I still think something needs to be done, so:
> 
> - Do we want to let applications not rely on our build flags yet still use
>   our exported headers?
> 
> - Is a missing #include <rte_config.h> supposed to trigger an explicit
>   #error for safety?
> 
> - How about putting back #include <rte_config.h> in our exported header
>   files directly given the above?
> 
> Note this discussion is only about exported headers. The rest remains
> unaffected and can safely continue to rely on -include rte_config.h.
> 
Sure, and to be clear I have no objection to having this patch applied.
Having the include on the compiler cmdline as well as having it the file
is fine, so there is no real conflict. As you say, it will make
individual headers more consumable for those not looking to use all of
DPDK.

/Bruce

Re: [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc

[Olivier MATZ]

Hi Adrien,

On Thu, Dec 21, 2017 at 02:00:06PM +0100, Adrien Mazarguil wrote:
> Applications can't combine either net/ethernet.h or netinet/ether.h
> together with rte_ether.h due to the redefinition of struct ether_addr and
> various macros by the latter.
> 
> This patch adapts rte_ether.h to rely on system definitions while
> maintaining DPDK additions.
> 
> An unforeseen consequence of involving more system header files compilation
> issues with some base drivers (i40e, ixgbe) defining their own conflicting
> types (e.g. __le64). This is addressed by explicitly including rte_ether.h
> where missing to ensure system definitions always come first.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Cc: Bruce Richardson <bruce.richardson@intel.com>

[...]

> --- a/drivers/net/qede/base/bcm_osal.h
> +++ b/drivers/net/qede/base/bcm_osal.h
> @@ -334,7 +334,6 @@ u32 qede_find_first_zero_bit(unsigned long *, u32);
>  	qede_find_first_zero_bit(bitmap, length)
>  
>  #define OSAL_BUILD_BUG_ON(cond)		nothing
> -#define ETH_ALEN			ETHER_ADDR_LEN
>  
>  #define OSAL_BITMAP_WEIGHT(bitmap, count) 0
>  

Not sure we can update code in a 'base' driver as easily.
It should be checked how it can be done with the qede maintainer.


> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 06d7b486c..19e62ea89 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -44,6 +44,7 @@
>  extern "C" {
>  #endif
>  
> +#include <net/ethernet.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  
> @@ -52,15 +53,7 @@ extern "C" {
>  #include <rte_mbuf.h>
>  #include <rte_byteorder.h>
>  
> -#define ETHER_ADDR_LEN  6 /**< Length of Ethernet address. */
> -#define ETHER_TYPE_LEN  2 /**< Length of Ethernet type field. */
> -#define ETHER_CRC_LEN   4 /**< Length of Ethernet CRC. */
> -#define ETHER_HDR_LEN   \
> -	(ETHER_ADDR_LEN * 2 + ETHER_TYPE_LEN) /**< Length of Ethernet header. */
> -#define ETHER_MIN_LEN   64    /**< Minimum frame len, including CRC. */
> -#define ETHER_MAX_LEN   1518  /**< Maximum frame len, including CRC. */
> -#define ETHER_MTU       \
> -	(ETHER_MAX_LEN - ETHER_HDR_LEN - ETHER_CRC_LEN) /**< Ethernet MTU. */
> +#define ETHER_MTU ETHERMTU /**< Deprecated, defined for compatibility. */
>  
>  #define ETHER_MAX_VLAN_FRAME_LEN \
>  	(ETHER_MAX_LEN + 4) /**< Maximum VLAN frame length, including CRC. */
> @@ -72,8 +65,11 @@ extern "C" {
>  
>  #define ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
>  
> +#ifdef __DOXYGEN__
> +
>  /**
> - * Ethernet address:
> + * Ethernet address.
> + *
>   * A universally administered address is uniquely assigned to a device by its
>   * manufacturer. The first three octets (in transmission order) contain the
>   * Organizationally Unique Identifier (OUI). The following three (MAC-48 and
> @@ -82,11 +78,25 @@ extern "C" {
>   * A locally administered address is assigned to a device by a network
>   * administrator and does not contain OUIs.
>   * See http://standards.ieee.org/regauth/groupmac/tutorial.html
> + *
> + * This structure is defined system-wide by "net/ethernet.h", however since
> + * the name of its data field is OS-dependent, a macro named "addr_bytes" is
> + * defined as an alias for the convenience of DPDK applications.
> + *
> + * The following definition is only for documentation purposes.
>   */
>  struct ether_addr {
>  	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
>  } __attribute__((__packed__));
>  
> +#endif /* __DOXYGEN__ */
> +
> +#if defined(__FreeBSD__)
> +#define addr_bytes octet
> +#else
> +#define addr_bytes ether_addr_octet
> +#endif
> +
>  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
>  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */

This kind of #define looks a bit dangerous to me: it can trigger
strange bugs because it will replace all occurences of addr_bytes
after this header is included.

Wouldn't it be a good opportunity to think about adding the rte_ prefix
to all variables/functions of rte_ether.h?

Re: [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc

[Adrien Mazarguil]

Hi Olivier,

On Fri, Dec 22, 2017 at 02:34:21PM +0100, Olivier MATZ wrote:
> Hi Adrien,
> 
> On Thu, Dec 21, 2017 at 02:00:06PM +0100, Adrien Mazarguil wrote:
> > Applications can't combine either net/ethernet.h or netinet/ether.h
> > together with rte_ether.h due to the redefinition of struct ether_addr and
> > various macros by the latter.
> > 
> > This patch adapts rte_ether.h to rely on system definitions while
> > maintaining DPDK additions.
> > 
> > An unforeseen consequence of involving more system header files compilation
> > issues with some base drivers (i40e, ixgbe) defining their own conflicting
> > types (e.g. __le64). This is addressed by explicitly including rte_ether.h
> > where missing to ensure system definitions always come first.
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: Olivier Matz <olivier.matz@6wind.com>
> > Cc: Bruce Richardson <bruce.richardson@intel.com>
> 
> [...]
> 
> > --- a/drivers/net/qede/base/bcm_osal.h
> > +++ b/drivers/net/qede/base/bcm_osal.h
> > @@ -334,7 +334,6 @@ u32 qede_find_first_zero_bit(unsigned long *, u32);
> >  	qede_find_first_zero_bit(bitmap, length)
> >  
> >  #define OSAL_BUILD_BUG_ON(cond)		nothing
> > -#define ETH_ALEN			ETHER_ADDR_LEN
> >  
> >  #define OSAL_BITMAP_WEIGHT(bitmap, count) 0
> >  
> 
> Not sure we can update code in a 'base' driver as easily.
> It should be checked how it can be done with the qede maintainer.

Sure, although I have to send an update for this chunk already: ETH_ALEN
seems only defined in Linux, not under FreeBSD. I intend to enclose the
above within #ifdef ETH_ALEN.

Besides, updating this file shouldn't be a problem, it's already tailored
for DPDK as it includes and uses several DPDK headers.

> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index 06d7b486c..19e62ea89 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -44,6 +44,7 @@
> >  extern "C" {
> >  #endif
> >  
> > +#include <net/ethernet.h>
> >  #include <stdint.h>
> >  #include <stdio.h>
> >  
> > @@ -52,15 +53,7 @@ extern "C" {
> >  #include <rte_mbuf.h>
> >  #include <rte_byteorder.h>
> >  
> > -#define ETHER_ADDR_LEN  6 /**< Length of Ethernet address. */
> > -#define ETHER_TYPE_LEN  2 /**< Length of Ethernet type field. */
> > -#define ETHER_CRC_LEN   4 /**< Length of Ethernet CRC. */
> > -#define ETHER_HDR_LEN   \
> > -	(ETHER_ADDR_LEN * 2 + ETHER_TYPE_LEN) /**< Length of Ethernet header. */
> > -#define ETHER_MIN_LEN   64    /**< Minimum frame len, including CRC. */
> > -#define ETHER_MAX_LEN   1518  /**< Maximum frame len, including CRC. */
> > -#define ETHER_MTU       \
> > -	(ETHER_MAX_LEN - ETHER_HDR_LEN - ETHER_CRC_LEN) /**< Ethernet MTU. */
> > +#define ETHER_MTU ETHERMTU /**< Deprecated, defined for compatibility. */
> >  
> >  #define ETHER_MAX_VLAN_FRAME_LEN \
> >  	(ETHER_MAX_LEN + 4) /**< Maximum VLAN frame length, including CRC. */
> > @@ -72,8 +65,11 @@ extern "C" {
> >  
> >  #define ETHER_MIN_MTU 68 /**< Minimum MTU for IPv4 packets, see RFC 791. */
> >  
> > +#ifdef __DOXYGEN__
> > +
> >  /**
> > - * Ethernet address:
> > + * Ethernet address.
> > + *
> >   * A universally administered address is uniquely assigned to a device by its
> >   * manufacturer. The first three octets (in transmission order) contain the
> >   * Organizationally Unique Identifier (OUI). The following three (MAC-48 and
> > @@ -82,11 +78,25 @@ extern "C" {
> >   * A locally administered address is assigned to a device by a network
> >   * administrator and does not contain OUIs.
> >   * See http://standards.ieee.org/regauth/groupmac/tutorial.html
> > + *
> > + * This structure is defined system-wide by "net/ethernet.h", however since
> > + * the name of its data field is OS-dependent, a macro named "addr_bytes" is
> > + * defined as an alias for the convenience of DPDK applications.
> > + *
> > + * The following definition is only for documentation purposes.
> >   */
> >  struct ether_addr {
> >  	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
> >  } __attribute__((__packed__));
> >  
> > +#endif /* __DOXYGEN__ */
> > +
> > +#if defined(__FreeBSD__)
> > +#define addr_bytes octet
> > +#else
> > +#define addr_bytes ether_addr_octet
> > +#endif
> > +
> >  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
> >  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
> 
> This kind of #define looks a bit dangerous to me: it can trigger
> strange bugs because it will replace all occurences of addr_bytes
> after this header is included.

Understandable, I checked before settling on this macro though, there's no
other usage of addr_bytes inside DPDK.

As for applications, there's no way to be completely sure. If we consider
they have to explicitly include rte_ether.h to get this definition, there
are chances addr_bytes is exclusively used with MAC addresses.

This change results in an API change (addr_bytes now documented as a
reserved macro) but has no ABI impact. I think it's a rather harmless
workaround pending your next suggestion:

> Wouldn't it be a good opportunity to think about adding the rte_ prefix
> to all variables/functions of rte_ether.h?

That would be ideal, however since almost all definitions in librte_net
(rte_ip.h, rte_udp.h etc.) also lack this prefix and can still technically
trigger conflicts with outside definitions (I'm aware work was done to avoid
that, but it doesn't change the fact they're not in the official DPDK
namespace), a massive API change would be needed for consistency.

Such a change is unlikely to be accepted for 18.02 in any case, therefore if
the proposed workaround is deemed too risky, I offer to remove this patch
from the series. What do you suggest?

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v1 2/6] net/i40e: fix issue in exported header

[Xing, Beilei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Thursday, December 21, 2017 9:00 PM
> To: dev@dpdk.org
> Cc: Chilikin, Andrey <andrey.chilikin@intel.com>
> Subject: [dpdk-dev] [PATCH v1 2/6] net/i40e: fix issue in exported header
> 
> Reported by check-includes.sh:
> 
>  [...]/rte_pmd_i40e.h:97:30: error: ISO C restricts enumerator values to
>      range of `int' [-Werror=pedantic]
>    RTE_PMD_I40E_PKG_INFO_MAX = 0xFFFFFFFF
>                                ^
> 
> Fixes: edeab742edac ("net/i40e: get information about DDP profile")
> Cc: Andrey Chilikin <andrey.chilikin@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/i40e/rte_pmd_i40e.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> b/drivers/net/i40e/rte_pmd_i40e.h index 580ca4ae9..49f4427a5 100644
> --- a/drivers/net/i40e/rte_pmd_i40e.h
> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> @@ -94,7 +94,7 @@ enum rte_pmd_i40e_package_info {
>  	RTE_PMD_I40E_PKG_INFO_PCTYPE_LIST,
>  	RTE_PMD_I40E_PKG_INFO_PTYPE_NUM,
>  	RTE_PMD_I40E_PKG_INFO_PTYPE_LIST,
> -	RTE_PMD_I40E_PKG_INFO_MAX = 0xFFFFFFFF
> +	RTE_PMD_I40E_PKG_INFO_MAX = (int)0xFFFFFFFF
>  };
> 
>  /**
> --
> 2.11.0

Acked-by: Beilei Xing <beilei.xing@intel.com>

Re: [dpdk-dev] [PATCH v1 3/6] flow_classify: fix issue in exported header

[Iremonger, Bernard]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, December 21, 2017 1:00 PM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Subject: [PATCH v1 3/6] flow_classify: fix issue in exported header
> 
> Reported by check-includes.sh:
> 
>  [...]/rte_flow_classify.h:85:47: error: ISO C does not permit named
>      variadic macros [-Werror=variadic-macros]
>   #define RTE_FLOW_CLASSIFY_LOG(level, fmt, args...) \
>                                                 ^
> 
> Fixes: be41ac2a330f ("flow_classify: introduce flow classify library")
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Acked-by: Bernard Iremonger <Bernard.iremonger@intel.com>

Re: [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc

[Olivier Matz]

Hi Adrien,

On Fri, Dec 22, 2017 at 03:25:27PM +0100, Adrien Mazarguil wrote:
> On Fri, Dec 22, 2017 at 02:34:21PM +0100, Olivier MATZ wrote:
> > On Thu, Dec 21, 2017 at 02:00:06PM +0100, Adrien Mazarguil wrote:

...

> > > +#if defined(__FreeBSD__)
> > > +#define addr_bytes octet
> > > +#else
> > > +#define addr_bytes ether_addr_octet
> > > +#endif
> > > +
> > >  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
> > >  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
> > 
> > This kind of #define looks a bit dangerous to me: it can trigger
> > strange bugs because it will replace all occurences of addr_bytes
> > after this header is included.
> 
> Understandable, I checked before settling on this macro though, there's no
> other usage of addr_bytes inside DPDK.
> 
> As for applications, there's no way to be completely sure. If we consider
> they have to explicitly include rte_ether.h to get this definition, there
> are chances addr_bytes is exclusively used with MAC addresses.
> 
> This change results in an API change (addr_bytes now documented as a
> reserved macro) but has no ABI impact. I think it's a rather harmless
> workaround pending your next suggestion:
> 
> > Wouldn't it be a good opportunity to think about adding the rte_ prefix
> > to all variables/functions of rte_ether.h?
> 
> That would be ideal, however since almost all definitions in librte_net
> (rte_ip.h, rte_udp.h etc.) also lack this prefix and can still technically
> trigger conflicts with outside definitions (I'm aware work was done to avoid
> that, but it doesn't change the fact they're not in the official DPDK
> namespace), a massive API change would be needed for consistency.
> 
> Such a change is unlikely to be accepted for 18.02 in any case, therefore if
> the proposed workaround is deemed too risky, I offer to remove this patch
> from the series. What do you suggest?

I think the only good long term solution is to have a proper namespace
for all rte_net structures and definitions. If there is no blocking issue
requiring this patch now, we can consider the following:

- 18.02: announce an api change for 18.05
- 18.05:
  - add new net structures and definitions with rte_ prefix
  - mark the old ones as deprecated
  - removes the structures or definitions that conflicts with system headers
- 18.08: remove the old structures and definitions

Re: [dpdk-dev] [PATCH v1 0/6] Address various issues with exported headers

[Thomas Monjalon]

21/12/2017 13:59, Adrien Mazarguil:
> Adrien Mazarguil (6):
>   devtools: update check-includes exceptions
>   net/i40e: fix issue in exported header
>   flow_classify: fix issue in exported header
>   member: fix issue in exported header
>   fix missing includes in exported headers
>   net: fix rte_ether conflicts with libc

Series applied except the last patch, thanks

Re: [dpdk-dev] [PATCH v1 6/6] net: fix rte_ether conflicts with libc

[Thomas Monjalon]

22/12/2017 15:25, Adrien Mazarguil:
> Such a change is unlikely to be accepted for 18.02 in any case, therefore if
> the proposed workaround is deemed too risky, I offer to remove this patch
> from the series. What do you suggest?

Series applied without this patch.

Please let's fix it in 18.05.