[PATCH v3 00/40] Fix build on gcc8 and various bugs

[PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

The following series gets current master able to build
itself, and allow lagopus to build against it, on Fedora 28 +
x86_64 using gcc 8.0.1.

The first 17 patches have already been through two spins and
this time are corrected for all the comment (thanks to
everybody who commented) since v2, and have tested-by /
acked-bys applied.  The first workaround patch for the hash
function cast problem is dropped since something has already
been applied in master since yesterday to address it.

The additional 23 patches are fixes for problems found
actually trying to build lagopus using current master.
These are almost entirely related to signed / unsigned
or truncation without explicit casts inside dpdk
headers.

---

Andy Green (40):
      drivers/bus/pci: fix strncpy dangerous code
      drivers/bus/dpaa: fix inconsistent struct alignment
      drivers/net/axgbe: fix broken eeprom string comp
      drivers/net/nfp/nfpcore: fix strncpy misuse
      drivers/net/nfp/nfpcore: fix off-by-one and no NUL on strncpy use
      drivers/net/nfp: don't memcpy out of source range
      drivers/net/nfp: fix buffer overflow in fw_name
      drivers/net/qede: fix strncpy constant and NUL
      drivers/net/qede: fix broken strncpy
      drivers/net/sfc: fix strncpy length
      drivers/net/sfc: fix strncpy size and NUL
      drivers/net/vdev: readlink inputs cannot be aliased
      drivers/net/vdev: fix 3 x strncpy misuse
      app/test-pmd: can't find include
      app/proc-info: fix sprintf overrun bug
      app/test-bbdev: test-bbdev: strcpy ok for allocated string
      app/test-bbdev: strcpy ok for allocated string
      rte_common.h: cast gcc builtin result to avoid complaints
      rte_memcpy.h: explicit tmp cast
      lib/librte_eal/common/include/rte_lcore.h: explicit cast for signed change
      /lib/librte_eal/common/include/rte_random.h: stage cast from uint64_t to long
      rte_spinlock.h: stack declarations before code
      rte_ring_generic.h: stack declarations before code
      rte_ring.h: remove signed type flipflopping
      rte_dev.h: stack declaration at top of own basic block
      rte_mbuf.h: avoid truncation warnings from inadvertant int16_t to int promotion
      rte_mbuf.h: explicit casts for flipping between int16_t and uint16_t
      rte_mbuf.h: make sure RTE_MIN compares same types
      rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
      rte_mbuf.h: explicit cast for size_t to uint32_t
      rte_mbuf.h: explicit casts to uint16_t to avoid truncation warnings
      rte_byteorder.h: explicit cast for return promotion
      rte_ether.h: explicit cast avoiding truncation warning
      rte_ether.h: stack vars declared at top of function
      rte_ethdev.h: fix sign and scope of temp var
      rte_ethdev.h: explicit cast for return type
      rte_ethdev.h: explicit cast for truncation
      rte_hash_crc.h: stack vars declared at top of function
      rte_hash_crc.h: explicit casts for truncation
      rte_string_fns.h: explicit cast for int return to size_t


 app/proc-info/main.c                               |    9 ++++-
 app/test-bbdev/test_bbdev_vector.c                 |    5 ++-
 app/test-pmd/Makefile                              |    1 +
 drivers/bus/dpaa/base/qbman/qman.c                 |   14 ++++----
 drivers/bus/dpaa/include/fsl_qman.h                |   24 +++++++------
 drivers/bus/pci/linux/pci.c                        |    2 +
 drivers/net/axgbe/axgbe_phy_impl.c                 |    4 +-
 drivers/net/nfp/nfp_net.c                          |    6 ++-
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c         |    4 ++
 drivers/net/nfp/nfpcore/nfp_resource.c             |   10 +++---
 drivers/net/qede/base/ecore_int.c                  |    8 +++-
 drivers/net/qede/qede_main.c                       |    7 ++--
 drivers/net/sfc/sfc_ethdev.c                       |    6 ++-
 drivers/net/vdev_netvsc/vdev_netvsc.c              |   15 +++++---
 .../common/include/arch/x86/rte_memcpy.h           |    8 ++--
 .../common/include/arch/x86/rte_spinlock.h         |    4 ++
 .../common/include/generic/rte_byteorder.h         |    2 +
 lib/librte_eal/common/include/rte_common.h         |    2 +
 lib/librte_eal/common/include/rte_dev.h            |   15 +++++---
 lib/librte_eal/common/include/rte_lcore.h          |    2 +
 lib/librte_eal/common/include/rte_random.h         |    6 ++-
 lib/librte_eal/common/include/rte_string_fns.h     |    2 +
 lib/librte_ethdev/rte_ethdev.h                     |   32 +++++++++++-------
 lib/librte_hash/rte_hash_crc.h                     |   11 +++---
 lib/librte_mbuf/rte_mbuf.h                         |   36 +++++++++++---------
 lib/librte_net/rte_ether.h                         |    5 ++-
 lib/librte_ring/rte_ring.h                         |    4 +-
 lib/librte_ring/rte_ring_c11_mem.h                 |    2 +
 lib/librte_ring/rte_ring_generic.h                 |   10 ++----
 29 files changed, 144 insertions(+), 112 deletions(-)

--
Signature

[PATCH v3 01/40] drivers/bus/pci: fix strncpy dangerous code

[Andy Green]

In function ‘pci_get_kernel_driver_by_path’,
    inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/
	drivers/bus/pci/linux/pci.c:317:8:
/home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error:
‘strncpy’ specified bound depends on the length of the source argument
[-Werror=stringop-overflow=]
   strncpy(dri_name, name + 1, strlen(name + 1) + 1);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/bus/pci/linux/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index 4630a8057..a73ee49c2 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -54,7 +54,7 @@ pci_get_kernel_driver_by_path(const char *filename, char *dri_name)
 
 	name = strrchr(path, '/');
 	if (name) {
-		strncpy(dri_name, name + 1, strlen(name + 1) + 1);
+		strlcpy(dri_name, name + 1, sizeof(dri_name));
 		return 0;
 	}
 

[PATCH v3 02/40] drivers/bus/dpaa: fix inconsistent struct alignment

[Andy Green]

The actual descriptor for qm_mr_entry is 64-byte aligned.

But the original code plays a trick, and puts a u8 common
to the three descriptor subtypes in the union afterwards
outside their structure definitions.

Unfortunately since they compose a struct qm_fd with
alignment 8, this trick destroys the ability of the compiler
to understand what has happened, resulting in this kind of
problem:

/home/agreen/projects/dpdk/drivers/bus/dpaa/include/
fsl_qman.h:354:3: error: alignment 1 of ‘struct <anonymous>’
is less than 8 [-Werror=packed-not-aligned]
   } __packed dcern;

on gcc 8 / Fedora 28 out of the box.

This patch moves the u8 verb into the structure definitions
composed into the union, so the alignment of the parent struct
containing the alignment 8 object can also be seen to be
alignment 8 by the compiler.  Uses of .verb are fixed up to use
.ern.verb (the same offset of +0 inside all the structs in
the union).

The final struct layout should be unchanged.

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
Tested-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 drivers/bus/dpaa/base/qbman/qman.c  |   14 +++++++-------
 drivers/bus/dpaa/include/fsl_qman.h |   24 +++++++++++++-----------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/qman.c b/drivers/bus/dpaa/base/qbman/qman.c
index 2810fdd26..27d98cc10 100644
--- a/drivers/bus/dpaa/base/qbman/qman.c
+++ b/drivers/bus/dpaa/base/qbman/qman.c
@@ -314,9 +314,9 @@ static int drain_mr_fqrni(struct qm_portal *p)
 		if (!msg)
 			return 0;
 	}
-	if ((msg->verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
+	if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
 		/* We aren't draining anything but FQRNIs */
-		pr_err("Found verb 0x%x in MR\n", msg->verb);
+		pr_err("Found verb 0x%x in MR\n", msg->ern.verb);
 		return -1;
 	}
 	qm_mr_next(p);
@@ -483,7 +483,7 @@ static inline void qm_mr_pvb_update(struct qm_portal *portal)
 	/* when accessing 'verb', use __raw_readb() to ensure that compiler
 	 * inlining doesn't try to optimise out "excess reads".
 	 */
-	if ((__raw_readb(&res->verb) & QM_MR_VERB_VBIT) == mr->vbit) {
+	if ((__raw_readb(&res->ern.verb) & QM_MR_VERB_VBIT) == mr->vbit) {
 		mr->pi = (mr->pi + 1) & (QM_MR_SIZE - 1);
 		if (!mr->pi)
 			mr->vbit ^= QM_MR_VERB_VBIT;
@@ -832,7 +832,7 @@ static u32 __poll_portal_slow(struct qman_portal *p, u32 is)
 			goto mr_done;
 		swapped_msg = *msg;
 		hw_fd_to_cpu(&swapped_msg.ern.fd);
-		verb = msg->verb & QM_MR_VERB_TYPE_MASK;
+		verb = msg->ern.verb & QM_MR_VERB_TYPE_MASK;
 		/* The message is a software ERN iff the 0x20 bit is set */
 		if (verb & 0x20) {
 			switch (verb) {
@@ -1666,7 +1666,7 @@ int qman_retire_fq(struct qman_fq *fq, u32 *flags)
 			 */
 			struct qm_mr_entry msg;
 
-			msg.verb = QM_MR_VERB_FQRNI;
+			msg.ern.verb = QM_MR_VERB_FQRNI;
 			msg.fq.fqs = mcr->alterfq.fqs;
 			msg.fq.fqid = fq->fqid;
 #ifdef CONFIG_FSL_QMAN_FQ_LOOKUP
@@ -2643,7 +2643,7 @@ int qman_shutdown_fq(u32 fqid)
 				qm_mr_pvb_update(low_p);
 				msg = qm_mr_current(low_p);
 				while (msg) {
-					if ((msg->verb &
+					if ((msg->ern.verb &
 					     QM_MR_VERB_TYPE_MASK)
 					    == QM_MR_VERB_FQRN)
 						found_fqrn = 1;
@@ -2711,7 +2711,7 @@ int qman_shutdown_fq(u32 fqid)
 			qm_mr_pvb_update(low_p);
 			msg = qm_mr_current(low_p);
 			while (msg) {
-				if ((msg->verb & QM_MR_VERB_TYPE_MASK) ==
+				if ((msg->ern.verb & QM_MR_VERB_TYPE_MASK) ==
 				    QM_MR_VERB_FQRL)
 					orl_empty = 1;
 				qm_mr_next(low_p);
diff --git a/drivers/bus/dpaa/include/fsl_qman.h b/drivers/bus/dpaa/include/fsl_qman.h
index e9793f30d..e4ad7ae48 100644
--- a/drivers/bus/dpaa/include/fsl_qman.h
+++ b/drivers/bus/dpaa/include/fsl_qman.h
@@ -284,20 +284,20 @@ static inline dma_addr_t qm_sg_addr(const struct qm_sg_entry *sg)
 	} while (0)
 
 /* See 1.5.8.1: "Enqueue Command" */
-struct qm_eqcr_entry {
+struct __rte_aligned(8) qm_eqcr_entry {
 	u8 __dont_write_directly__verb;
 	u8 dca;
 	u16 seqnum;
 	u32 orp;	/* 24-bit */
 	u32 fqid;	/* 24-bit */
 	u32 tag;
-	struct qm_fd fd;
+	struct qm_fd fd; /* this has alignment 8 */
 	u8 __reserved3[32];
 } __packed;
 
 
 /* "Frame Dequeue Response" */
-struct qm_dqrr_entry {
+struct __rte_aligned(8) qm_dqrr_entry {
 	u8 verb;
 	u8 stat;
 	u16 seqnum;	/* 15-bit */
@@ -305,7 +305,7 @@ struct qm_dqrr_entry {
 	u8 __reserved2[3];
 	u32 fqid;	/* 24-bit */
 	u32 contextB;
-	struct qm_fd fd;
+	struct qm_fd fd; /* this has alignment 8 */
 	u8 __reserved4[32];
 };
 
@@ -323,18 +323,19 @@ struct qm_dqrr_entry {
 /* "ERN Message Response" */
 /* "FQ State Change Notification" */
 struct qm_mr_entry {
-	u8 verb;
 	union {
 		struct {
+			u8 verb;
 			u8 dca;
 			u16 seqnum;
 			u8 rc;		/* Rejection Code */
 			u32 orp:24;
 			u32 fqid;	/* 24-bit */
 			u32 tag;
-			struct qm_fd fd;
-		} __packed ern;
+			struct qm_fd fd; /* this has alignment 8 */
+		} __packed __rte_aligned(8) ern;
 		struct {
+			u8 verb;
 #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
 			u8 colour:2;	/* See QM_MR_DCERN_COLOUR_* */
 			u8 __reserved1:4;
@@ -349,18 +350,19 @@ struct qm_mr_entry {
 			u32 __reserved3:24;
 			u32 fqid;	/* 24-bit */
 			u32 tag;
-			struct qm_fd fd;
-		} __packed dcern;
+			struct qm_fd fd; /* this has alignment 8 */
+		} __packed __rte_aligned(8) dcern;
 		struct {
+			u8 verb;
 			u8 fqs;		/* Frame Queue Status */
 			u8 __reserved1[6];
 			u32 fqid;	/* 24-bit */
 			u32 contextB;
 			u8 __reserved2[16];
-		} __packed fq;		/* FQRN/FQRNI/FQRL/FQPN */
+		} __packed __rte_aligned(8) fq;	/* FQRN/FQRNI/FQRL/FQPN */
 	};
 	u8 __reserved2[32];
-} __packed;
+} __packed __rte_aligned(8);
 #define QM_MR_VERB_VBIT			0x80
 /*
  * ERNs originating from direct-connect portals ("dcern") use 0x20 as a verb

[PATCH v3 03/40] drivers/net/axgbe: fix broken eeprom string comp

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/axgbe/axgbe_phy_impl.c:576:6:
error: ‘__builtin_memcmp_eq’ reading 16 bytes from a region of
size 9 [-Werror=stringop-overflow=]
  if (memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_NAME],
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      AXGBE_BEL_FUSE_VENDOR, AXGBE_SFP_BASE_VENDOR_NAME_LEN))

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/axgbe/axgbe_phy_impl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index dfa908dd8..973177f69 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -574,11 +574,11 @@ static bool axgbe_phy_belfuse_parse_quirks(struct axgbe_port *pdata)
 	struct axgbe_sfp_eeprom *sfp_eeprom = &phy_data->sfp_eeprom;
 
 	if (memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_NAME],
-		   AXGBE_BEL_FUSE_VENDOR, AXGBE_SFP_BASE_VENDOR_NAME_LEN))
+		   AXGBE_BEL_FUSE_VENDOR, strlen(AXGBE_BEL_FUSE_VENDOR)))
 		return false;
 
 	if (!memcmp(&sfp_eeprom->base[AXGBE_SFP_BASE_VENDOR_PN],
-		    AXGBE_BEL_FUSE_PARTNO, AXGBE_SFP_BASE_VENDOR_PN_LEN)) {
+		    AXGBE_BEL_FUSE_PARTNO, strlen(AXGBE_BEL_FUSE_PARTNO))) {
 		phy_data->sfp_base = AXGBE_SFP_BASE_1000_SX;
 		phy_data->sfp_cable = AXGBE_SFP_CABLE_ACTIVE;
 		phy_data->sfp_speed = AXGBE_SFP_SPEED_1000;

[PATCH v3 04/40] drivers/net/nfp/nfpcore: fix strncpy misuse

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
index 4e6c66624..52b294888 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -31,6 +31,8 @@
 #include <sys/file.h>
 #include <sys/stat.h>
 
+#include <rte_string_fns.h>
+
 #include "nfp_cpp.h"
 #include "nfp_target.h"
 #include "nfp6000/nfp6000.h"
@@ -846,7 +848,7 @@ nfp6000_init(struct nfp_cpp *cpp, const char *devname)
 
 
 	memset(desc->busdev, 0, BUSDEV_SZ);
-	strncpy(desc->busdev, devname, strlen(devname));
+	strlcpy(desc->busdev, devname, sizeof(desc->busdev));
 
 	ret = nfp_acquire_process_lock(desc);
 	if (ret)

[PATCH v3 05/40] drivers/net/nfp/nfpcore: fix off-by-one and no NUL on strncpy use

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/nfp/nfpcore/nfp_resource.c:
76:2:error: ‘strncpy’ output may be truncated copying 8 bytes from
a string of length 8 [-Werror=stringop-truncation]
  strncpy(name_pad, res->name, sizeof(name_pad));

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/nfp/nfpcore/nfp_resource.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/nfp/nfpcore/nfp_resource.c b/drivers/net/nfp/nfpcore/nfp_resource.c
index e1df2b2e1..dd41fa4de 100644
--- a/drivers/net/nfp/nfpcore/nfp_resource.c
+++ b/drivers/net/nfp/nfpcore/nfp_resource.c
@@ -7,6 +7,8 @@
 #include <time.h>
 #include <endian.h>
 
+#include <rte_string_fns.h>
+
 #include "nfp_cpp.h"
 #include "nfp6000/nfp6000.h"
 #include "nfp_resource.h"
@@ -65,22 +67,22 @@ struct nfp_resource {
 static int
 nfp_cpp_resource_find(struct nfp_cpp *cpp, struct nfp_resource *res)
 {
-	char name_pad[NFP_RESOURCE_ENTRY_NAME_SZ] = {};
+	char name_pad[NFP_RESOURCE_ENTRY_NAME_SZ + 2];
 	struct nfp_resource_entry entry;
 	uint32_t cpp_id, key;
 	int ret, i;
 
 	cpp_id = NFP_CPP_ID(NFP_RESOURCE_TBL_TARGET, 3, 0);  /* Atomic read */
 
-	memset(name_pad, 0, NFP_RESOURCE_ENTRY_NAME_SZ);
-	strncpy(name_pad, res->name, sizeof(name_pad));
+	memset(name_pad, 0, sizeof(name_pad));
+	strlcpy(name_pad, res->name, sizeof(name_pad));
 
 	/* Search for a matching entry */
 	if (!memcmp(name_pad, NFP_RESOURCE_TBL_NAME "\0\0\0\0\0\0\0\0", 8)) {
 		printf("Grabbing device lock not supported\n");
 		return -EOPNOTSUPP;
 	}
-	key = nfp_crc32_posix(name_pad, sizeof(name_pad));
+	key = nfp_crc32_posix(name_pad, NFP_RESOURCE_ENTRY_NAME_SZ);
 
 	for (i = 0; i < NFP_RESOURCE_TBL_ENTRIES; i++) {
 		uint64_t addr = NFP_RESOURCE_TBL_BASE +

[PATCH v3 06/40] drivers/net/nfp: don't memcpy out of source range

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:669:2:
error: ‘memcpy’ forming offset [5, 6] is out of the bounds
[0, 4] of object ‘tmp’ with type ‘uint32_t’ {aka ‘unsigned
int’} [-Werror=array-bounds]  memcpy(&hw->mac_addr[0],
&tmp, sizeof(struct ether_addr));

Signed-off-by: Andy Green <andy@warmcat.com>
Acked-by: Alejandro Lucero <alejandro.lucero@netronome.com>
Tested-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 048324ec9..199aac40b 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -666,7 +666,7 @@ nfp_net_vf_read_mac(struct nfp_net_hw *hw)
 	uint32_t tmp;
 
 	tmp = rte_be_to_cpu_32(nn_cfg_readl(hw, NFP_NET_CFG_MACADDR));
-	memcpy(&hw->mac_addr[0], &tmp, sizeof(struct ether_addr));
+	memcpy(&hw->mac_addr[0], &tmp, 4);
 
 	tmp = rte_be_to_cpu_32(nn_cfg_readl(hw, NFP_NET_CFG_MACADDR + 4));
 	memcpy(&hw->mac_addr[4], &tmp, 2);

[PATCH v3 07/40] drivers/net/nfp: fix buffer overflow in fw_name

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In
function ‘nfp_pf_pci_probe’:
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3160:
23: error: ‘%s’ directive writing up to 99 bytes into a
region of size 76 [-Werror=format-overflow=]
  sprintf(fw_name, "%s/%s.nffw", DEFAULT_FW_PATH, serial);

Note fw_buf still has to increase somewhat even after
restricting serial[], since otherwise:

/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c: In
function ‘nfp_pf_pci_probe’:
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:23:
error: ‘%s’ directive writing up to 99 bytes into a region
of size 76 [-Werror=format-overflow=]
  sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);
                       ^~
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3262:32:
  err = nfp_fw_upload(dev, nsp, card_desc);
                                ~~~~~~~~~
/home/agreen/projects/dpdk/drivers/net/nfp/nfp_net.c:3176:2:
note: ‘sprintf’ output between 25 and 124 bytes into a
destination of size 100
  sprintf(fw_name, "%s/%s", DEFAULT_FW_PATH, card);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/nfp/nfp_net.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 199aac40b..f114b1839 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -3144,8 +3144,8 @@ nfp_fw_upload(struct rte_pci_device *dev, struct nfp_nsp *nsp, char *card)
 	struct nfp_cpp *cpp = nsp->cpp;
 	int fw_f;
 	char *fw_buf;
-	char fw_name[100];
-	char serial[100];
+	char fw_name[125];
+	char serial[40];
 	struct stat file_stat;
 	off_t fsize, bytes;
 

[PATCH v3 08/40] drivers/net/qede: fix strncpy constant and NUL

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/qede/base/ecore_int.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qede/base/ecore_int.c b/drivers/net/qede/base/ecore_int.c
index f43781ba4..d9e22b5ed 100644
--- a/drivers/net/qede/base/ecore_int.c
+++ b/drivers/net/qede/base/ecore_int.c
@@ -6,6 +6,8 @@
  * See LICENSE.qede_pmd for copyright and licensing details.
  */
 
+#include <rte_string_fns.h>
+
 #include "bcm_osal.h"
 #include "ecore.h"
 #include "ecore_spq.h"
@@ -1104,9 +1106,9 @@ static enum _ecore_status_t ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
 							      p_aeu->bit_name,
 							      num);
 					else
-						OSAL_STRNCPY(bit_name,
-							     p_aeu->bit_name,
-							     30);
+						strlcpy(bit_name,
+							p_aeu->bit_name,
+							sizeof(bit_name));
 
 					/* We now need to pass bitmask in its
 					 * correct position.

[PATCH v3 09/40] drivers/net/qede: fix broken strncpy

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/qede/qede_main.c: In function
‘qed_slowpath_start’:
/home/agreen/projects/dpdk/drivers/net/qede/qede_main.c:307:3:
error: ‘strncpy’ output may be truncated copying 12 bytes from a
string of length 127 [-Werror=stringop-truncation]
   strncpy((char *)drv_version.name, (const char *)params->name,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    MCP_DRV_VER_STR_SIZE - 4);
    ~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/qede/qede_main.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qede/qede_main.c b/drivers/net/qede/qede_main.c
index 2333ca073..fcfc32d0d 100644
--- a/drivers/net/qede/qede_main.c
+++ b/drivers/net/qede/qede_main.c
@@ -9,6 +9,7 @@
 #include <limits.h>
 #include <time.h>
 #include <rte_alarm.h>
+#include <rte_string_fns.h>
 
 #include "qede_ethdev.h"
 
@@ -303,9 +304,9 @@ static int qed_slowpath_start(struct ecore_dev *edev,
 		drv_version.version = (params->drv_major << 24) |
 		    (params->drv_minor << 16) |
 		    (params->drv_rev << 8) | (params->drv_eng);
-		/* TBD: strlcpy() */
-		strncpy((char *)drv_version.name, (const char *)params->name,
-			MCP_DRV_VER_STR_SIZE - 4);
+		strlcpy((char *)drv_version.name, (const char *)params->name,
+			sizeof(drv_version.name));
+		drv_version.name[sizeof(drv_version.name) - 1] = '\0';
 		rc = ecore_mcp_send_drv_version(hwfn, hwfn->p_main_ptt,
 						&drv_version);
 		if (rc) {

[PATCH v3 10/40] drivers/net/sfc: fix strncpy length

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/sfc/sfc_ethdev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index e42d55350..ef5e9ecb2 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -13,6 +13,7 @@
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
 #include <rte_errno.h>
+#include <rte_string_fns.h>
 
 #include "efx.h"
 
@@ -741,9 +742,8 @@ sfc_xstats_get_names_by_id(struct rte_eth_dev *dev,
 		if ((ids == NULL) || (ids[nb_written] == nb_supported)) {
 			char *name = xstats_names[nb_written++].name;
 
-			strncpy(name, efx_mac_stat_name(sa->nic, i),
+			strlcpy(name, efx_mac_stat_name(sa->nic, i),
 				sizeof(xstats_names[0].name));
-			name[sizeof(xstats_names[0].name) - 1] = '\0';
 		}
 
 		++nb_supported;

[PATCH v3 11/40] drivers/net/sfc: fix strncpy size and NUL

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/sfc/sfc_ethdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index ef5e9ecb2..a8c0f8e19 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -664,7 +664,7 @@ sfc_xstats_get_names(struct rte_eth_dev *dev,
 	for (i = 0; i < EFX_MAC_NSTATS; ++i) {
 		if (EFX_MAC_STAT_SUPPORTED(port->mac_stats_mask, i)) {
 			if (xstats_names != NULL && nstats < xstats_count)
-				strncpy(xstats_names[nstats].name,
+				strlcpy(xstats_names[nstats].name,
 					efx_mac_stat_name(sa->nic, i),
 					sizeof(xstats_names[0].name));
 			nstats++;

[PATCH v3 12/40] drivers/net/vdev: readlink inputs cannot be aliased

[Andy Green]

/home/agreen/projects/dpdk/drivers/net/vdev_netvsc/
vdev_netvsc.c:335:2:error: passing argument 2 to restrict-
qualified parameter aliases with argument 1 [-Werror=restrict]
  ret = readlink(buf, buf, size);
  ^~~

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/vdev_netvsc/vdev_netvsc.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index c321a9f1b..dca25761d 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -327,12 +327,14 @@ static int
 vdev_netvsc_sysfs_readlink(char *buf, size_t size, const char *if_name,
 			   const char *relpath)
 {
+	char in[160];
 	int ret;
 
-	ret = snprintf(buf, size, "/sys/class/net/%s/%s", if_name, relpath);
-	if (ret == -1 || (size_t)ret >= size)
+	ret = snprintf(in, sizeof(in) - 1, "/sys/class/net/%s/%s",
+		       if_name, relpath);
+	if (ret == -1 || (size_t)ret >= sizeof(in) - 1)
 		return -ENOBUFS;
-	ret = readlink(buf, buf, size);
+	ret = readlink(in, buf, size);
 	if (ret == -1)
 		return -errno;
 	if ((size_t)ret >= size - 1)

[PATCH v3 13/40] drivers/net/vdev: fix 3 x strncpy misuse

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 drivers/net/vdev_netvsc/vdev_netvsc.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index dca25761d..f1d036152 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -35,6 +35,7 @@
 #include <rte_hypervisor.h>
 #include <rte_kvargs.h>
 #include <rte_log.h>
+#include <rte_string_fns.h>
 
 #define VDEV_NETVSC_DRIVER net_vdev_netvsc
 #define VDEV_NETVSC_DRIVER_NAME RTE_STR(VDEV_NETVSC_DRIVER)
@@ -182,7 +183,7 @@ vdev_netvsc_foreach_iface(int (*func)(const struct if_nameindex *iface,
 		is_netvsc_ret = vdev_netvsc_iface_is_netvsc(&iface[i]) ? 1 : 0;
 		if (is_netvsc ^ is_netvsc_ret)
 			continue;
-		strncpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name));
+		strlcpy(req.ifr_name, iface[i].if_name, sizeof(req.ifr_name));
 		if (ioctl(s, SIOCGIFHWADDR, &req) == -1) {
 			DRV_LOG(WARNING, "cannot retrieve information about"
 					 " interface \"%s\": %s",
@@ -384,7 +385,7 @@ vdev_netvsc_device_probe(const struct if_nameindex *iface,
 		DRV_LOG(DEBUG,
 			"NetVSC interface \"%s\" (index %u) renamed \"%s\"",
 			ctx->if_name, ctx->if_index, iface->if_name);
-		strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
+		strlcpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
 		return 0;
 	}
 	if (!is_same_ether_addr(eth_addr, &ctx->if_addr))
@@ -582,7 +583,7 @@ vdev_netvsc_netvsc_probe(const struct if_nameindex *iface,
 		goto error;
 	}
 	ctx->id = vdev_netvsc_ctx_count;
-	strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
+	strlcpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name));
 	ctx->if_index = iface->if_index;
 	ctx->if_addr = *eth_addr;
 	ctx->pipe[0] = -1;

[PATCH v3 14/40] app/test-pmd: can't find include

[Andy Green]

/home/agreen/projects/dpdk/app/test-pmd/cmdline.c:64:10:
fatal error: rte_pmd_dpaa.h: No such file or directory
 #include <rte_pmd_dpaa.h>
          ^~~~~~~~~~~~~~~~

Signed-off-by: Andy Green <andy@warmcat.com>
---
 app/test-pmd/Makefile |    1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
index 60ae9b9c1..a0fdd0e11 100644
--- a/app/test-pmd/Makefile
+++ b/app/test-pmd/Makefile
@@ -13,6 +13,7 @@ APP = testpmd
 CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += -O3
 CFLAGS += $(WERROR_FLAGS)
+CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa
 
 #
 # all source are stored in SRCS-y

[PATCH v3 15/40] app/proc-info: fix sprintf overrun bug

[Andy Green]

/home/agreen/projects/dpdk/app/proc-info/main.c: In function
‘nic_xstats_display’:
/home/agreen/projects/dpdk/app/proc-info/main.c:495:45:
error: ‘%s’ directive writing up to 255 bytes into a region
of size between 165 and 232 [-Werror=format-overflow=]
    sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
                                             ^~
     PRIu64"\n", host_id, port_id, counter_type,
                                   ~~~~~~~~~~~~
/home/agreen/projects/dpdk/app/proc-info/main.c:495:4: note:
‘sprintf’ output between 31 and 435 bytes into a destination
of size 256
    sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     PRIu64"\n", host_id, port_id, counter_type,
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     xstats_names[i].name, values[i]);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 app/proc-info/main.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 539e13243..df46c235e 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -488,14 +488,19 @@ nic_xstats_display(uint16_t port_id)
 		if (enable_collectd_format) {
 			char counter_type[MAX_STRING_LEN];
 			char buf[MAX_STRING_LEN];
+			size_t n;
 
 			collectd_resolve_cnt_type(counter_type,
 						  sizeof(counter_type),
 						  xstats_names[i].name);
-			sprintf(buf, "PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
+			n = snprintf(buf, MAX_STRING_LEN,
+				"PUTVAL %s/dpdkstat-port.%u/%s-%s N:%"
 				PRIu64"\n", host_id, port_id, counter_type,
 				xstats_names[i].name, values[i]);
-			ret = write(stdout_fd, buf, strlen(buf));
+			buf[sizeof(buf) - 1] = '\0';
+			if (n > sizeof(buf) - 1)
+				n = sizeof(buf) - 1;
+			ret = write(stdout_fd, buf, n);
 			if (ret < 0)
 				goto err;
 		} else {

[PATCH v3 16/40] app/test-bbdev: test-bbdev: strcpy ok for allocated string

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 app/test-bbdev/test_bbdev_vector.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-bbdev/test_bbdev_vector.c
index addef0572..5ad2a6535 100644
--- a/app/test-bbdev/test_bbdev_vector.c
+++ b/app/test-bbdev/test_bbdev_vector.c
@@ -890,7 +890,7 @@ test_bbdev_vector_read(const char *filename,
 		}
 
 		memset(entry, 0, strlen(line) + 1);
-		strncpy(entry, line, strlen(line));
+		strcpy(entry, line);
 
 		/* check if entry ends with , or = */
 		if (entry[strlen(entry) - 1] == ','

[PATCH v3 17/40] app/test-bbdev: strcpy ok for allocated string

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 app/test-bbdev/test_bbdev_vector.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/test-bbdev/test_bbdev_vector.c b/app/test-bbdev/test_bbdev_vector.c
index 5ad2a6535..373f94984 100644
--- a/app/test-bbdev/test_bbdev_vector.c
+++ b/app/test-bbdev/test_bbdev_vector.c
@@ -912,7 +912,8 @@ test_bbdev_vector_read(const char *filename,
 				}
 
 				entry = entry_extended;
-				strncat(entry, line, strlen(line));
+				/* entry has been allocated accordingly */
+				strcpy(&entry[strlen(entry)], line);
 
 				if (entry[strlen(entry) - 1] != ',')
 					break;

[PATCH v3 18/40] rte_common.h: cast gcc builtin result to avoid complaints

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_common.h:416:9:
warning: conversion to 'uint32_t' {aka 'unsigned int'} from
'int' may change the sign of the result [-Wsign-conversion]
  return __builtin_ctz(v);
         ^~~~~~~~~~~~~~~~

The builtin is defined to return int, but we want to
return it as uint32_t.  Its only defined valid return
values are positive integers or zero, which is OK for
uint32_t.  So just add an explicit cast.

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_eal/common/include/rte_common.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 69e5ed1e3..679f2f184 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -413,7 +413,7 @@ rte_align64prevpow2(uint64_t v)
 static inline uint32_t
 rte_bsf32(uint32_t v)
 {
-	return __builtin_ctz(v);
+	return (uint32_t)__builtin_ctz(v);
 }
 
 /**

[PATCH v3 19/40] rte_memcpy.h: explicit tmp cast

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 .../common/include/arch/x86/rte_memcpy.h           |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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 5ead68ab2..f9ea0ab69 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
@@ -597,9 +597,9 @@ __extension__ ({
         _mm_storeu_si128((__m128i *)((uint8_t *)dst + 7 * 16), _mm_alignr_epi8(xmm8, xmm7, offset));        \
         dst = (uint8_t *)dst + 128;                                                                         \
     }                                                                                                       \
-    tmp = len;                                                                                              \
+    tmp = (int)len;                                                                                         \
     len = ((len - 16 + offset) & 127) + 16 - offset;                                                        \
-    tmp -= len;                                                                                             \
+    tmp -= (int)len;                                                                                        \
     src = (const uint8_t *)src + tmp;                                                                       \
     dst = (uint8_t *)dst + tmp;                                                                             \
     if (len >= 32 + 16 - offset) {                                                                          \
@@ -613,9 +613,9 @@ __extension__ ({
             _mm_storeu_si128((__m128i *)((uint8_t *)dst + 1 * 16), _mm_alignr_epi8(xmm2, xmm1, offset));    \
             dst = (uint8_t *)dst + 32;                                                                      \
         }                                                                                                   \
-        tmp = len;                                                                                          \
+        tmp = (int)len;                                                                                     \
         len = ((len - 16 + offset) & 31) + 16 - offset;                                                     \
-        tmp -= len;                                                                                         \
+        tmp -= (int)len;                                                                                    \
         src = (const uint8_t *)src + tmp;                                                                   \
         dst = (uint8_t *)dst + tmp;                                                                         \
     }                                                                                                       \

[PATCH v3 20/40] lib/librte_eal/common/include/rte_lcore.h: explicit cast for signed change

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_lcore.h:
In function 'rte_lcore_index':
/projects/lagopus/src/dpdk/build/include/rte_lcore.h:122:14:
warning: conversion to 'int' from 'unsigned int' may change
the sign of the result [-Wsign-conversion]
   lcore_id = rte_lcore_id();

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_eal/common/include/rte_lcore.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 1a2f37eaa..6e09d9181 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -119,7 +119,7 @@ rte_lcore_index(int lcore_id)
 	if (lcore_id >= RTE_MAX_LCORE)
 		return -1;
 	if (lcore_id < 0)
-		lcore_id = rte_lcore_id();
+		lcore_id = (int)rte_lcore_id();
 	return lcore_config[lcore_id].core_index;
 }
 

[PATCH v3 21/40] /lib/librte_eal/common/include/rte_random.h: stage cast from uint64_t to long

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_random.h:
In function 'rte_srand':
/projects/lagopus/src/dpdk/build/include/rte_random.h:34:10:
warning: conversion to 'long int' from 'long unsigned int'
may change the sign of the result [-Wsign-conversion]
  srand48((long unsigned int)seedval);

/projects/lagopus/src/dpdk/build/include/rte_random.h:51:8:
warning: conversion to 'uint64_t' {aka 'long unsigned int'}
from 'long int' may change the sign of the result
[-Wsign-conversion]
  val = lrand48();
        ^~~~~~~
/projects/lagopus/src/dpdk/build/include/rte_random.h:53:6:
warning: conversion to 'long unsigned int' from 'long int'
may change the sign of the result [-Wsign-conversion]
  val += lrand48();

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_eal/common/include/rte_random.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_random.h b/lib/librte_eal/common/include/rte_random.h
index 63bb28088..96c64fdd2 100644
--- a/lib/librte_eal/common/include/rte_random.h
+++ b/lib/librte_eal/common/include/rte_random.h
@@ -31,7 +31,7 @@ extern "C" {
 static inline void
 rte_srand(uint64_t seedval)
 {
-	srand48((long unsigned int)seedval);
+	srand48((long)(unsigned long)seedval);
 }
 
 /**
@@ -48,9 +48,9 @@ static inline uint64_t
 rte_rand(void)
 {
 	uint64_t val;
-	val = lrand48();
+	val = (unsigned long)lrand48();
 	val <<= 32;
-	val += lrand48();
+	val += (unsigned long)lrand48();
 	return val;
 }
 

[PATCH v3 22/40] rte_spinlock.h: stack declarations before code

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_spinlock.h:
In function 'rte_try_tm':
/projects/lagopus/src/dpdk/build/include/rte_spinlock.h:82:2:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
  int retries = RTE_RTM_MAX_RETRIES;

Signed-off-by: Andy Green <andy@warmcat.com>
---
 .../common/include/arch/x86/rte_spinlock.h         |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/arch/x86/rte_spinlock.h b/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
index 4b16887ea..60321da02 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
@@ -76,10 +76,12 @@ static inline int rte_tm_supported(void)
 static inline int
 rte_try_tm(volatile int *lock)
 {
+	int retries;
+
 	if (!rte_rtm_supported)
 		return 0;
 
-	int retries = RTE_RTM_MAX_RETRIES;
+	retries = RTE_RTM_MAX_RETRIES;
 
 	while (likely(retries--)) {
 

[PATCH v3 23/40] rte_ring_generic.h: stack declarations before code

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
In function '__rte_ring_move_prod_head':
/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:76:3:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t cons_tail = r->cons.tail;
   ^~~~~
/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:
In function '__rte_ring_move_cons_head':
/projects/lagopus/src/dpdk/build/include/rte_ring_generic.h:147:3:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t prod_tail = r->prod.tail;

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_ring/rte_ring_generic.h |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
index 5b110425f..c2d482bc9 100644
--- a/lib/librte_ring/rte_ring_generic.h
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -73,14 +73,13 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
 		 */
 		rte_smp_rmb();
 
-		const uint32_t cons_tail = r->cons.tail;
 		/*
 		 *  The subtraction is done between two unsigned 32bits value
 		 * (the result is always modulo 32 bits even if we have
 		 * *old_head > cons_tail). So 'free_entries' is always between 0
 		 * and capacity (which is < size).
 		 */
-		*free_entries = (capacity + cons_tail - *old_head);
+		*free_entries = (capacity + r->cons.tail - *old_head);
 
 		/* check that we have enough room in ring */
 		if (unlikely(n > *free_entries))
@@ -144,13 +143,12 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
 		 */
 		rte_smp_rmb();
 
-		const uint32_t prod_tail = r->prod.tail;
 		/* The subtraction is done between two unsigned 32bits value
 		 * (the result is always modulo 32 bits even if we have
 		 * cons_head > prod_tail). So 'entries' is always between 0
 		 * and size(ring)-1.
 		 */
-		*entries = (prod_tail - *old_head);
+		*entries = (r->prod.tail - *old_head);
 
 		/* Set the actual entries for dequeue */
 		if (n > *entries)

[PATCH v3 24/40] rte_ring.h: remove signed type flipflopping

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_ring.h:350:46:
warning: conversion to 'uint32_t' {aka 'unsigned int'}
from 'int' may change the sign of the result
[-Wsign-conversion]
  update_tail(&r->prod, prod_head, prod_next, is_sp, 1);

The visible apis take unsigned int, then call a private
api taking an int, which finally calls an api taking an
unsigned int.

Convert the private api to take unsigned int removing
5 x warning similar to that shown above.

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_ring/rte_ring.h         |    4 ++--
 lib/librte_ring/rte_ring_c11_mem.h |    2 +-
 lib/librte_ring/rte_ring_generic.h |    4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index d3d3f7f97..124582251 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -335,7 +335,7 @@ void rte_ring_dump(FILE *f, const struct rte_ring *r);
 static __rte_always_inline unsigned int
 __rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table,
 		 unsigned int n, enum rte_ring_queue_behavior behavior,
-		 int is_sp, unsigned int *free_space)
+		 unsigned int is_sp, unsigned int *free_space)
 {
 	uint32_t prod_head, prod_next;
 	uint32_t free_entries;
@@ -377,7 +377,7 @@ __rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table,
 static __rte_always_inline unsigned int
 __rte_ring_do_dequeue(struct rte_ring *r, void **obj_table,
 		 unsigned int n, enum rte_ring_queue_behavior behavior,
-		 int is_sc, unsigned int *available)
+		 unsigned int is_sc, unsigned int *available)
 {
 	uint32_t cons_head, cons_next;
 	uint32_t entries;
diff --git a/lib/librte_ring/rte_ring_c11_mem.h b/lib/librte_ring/rte_ring_c11_mem.h
index 08825ea5b..cb3f82b1a 100644
--- a/lib/librte_ring/rte_ring_c11_mem.h
+++ b/lib/librte_ring/rte_ring_c11_mem.h
@@ -51,7 +51,7 @@ update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
  *   If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
  */
 static __rte_always_inline unsigned int
-__rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
+__rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 		unsigned int n, enum rte_ring_queue_behavior behavior,
 		uint32_t *old_head, uint32_t *new_head,
 		uint32_t *free_entries)
diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
index c2d482bc9..ea7dbe5b9 100644
--- a/lib/librte_ring/rte_ring_generic.h
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -53,7 +53,7 @@ update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
  *   If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
  */
 static __rte_always_inline unsigned int
-__rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
+__rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 		unsigned int n, enum rte_ring_queue_behavior behavior,
 		uint32_t *old_head, uint32_t *new_head,
 		uint32_t *free_entries)
@@ -123,7 +123,7 @@ __rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
  *     If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
  */
 static __rte_always_inline unsigned int
-__rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
+__rte_ring_move_cons_head(struct rte_ring *r, unsigned int is_sc,
 		unsigned int n, enum rte_ring_queue_behavior behavior,
 		uint32_t *old_head, uint32_t *new_head,
 		uint32_t *entries)

[PATCH v3 25/40] rte_dev.h: stack declaration at top of own basic block

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_dev.h:54:2:
warning: ISO C90 forbids mixed declarations and
code [-Wdeclaration-after-statement]
  char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_eal/common/include/rte_dev.h |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index 0955e9adb..3879ff3ca 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -51,15 +51,18 @@ rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
 
 	va_start(ap, fmt);
 
-	char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];
+	{
+		char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];
 
-	va_end(ap);
+		va_end(ap);
 
-	va_start(ap, fmt);
-	vsnprintf(buffer, sizeof(buffer), fmt, ap);
-	va_end(ap);
+		va_start(ap, fmt);
+		vsnprintf(buffer, sizeof(buffer), fmt, ap);
+		va_end(ap);
 
-	rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name, buffer);
+		rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s",
+			func_name, buffer);
+	}
 }
 
 /*

[PATCH v3 26/40] rte_mbuf.h: avoid truncation warnings from inadvertant int16_t to int promotion

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 4fd9a0d9e..a2a37a311 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -836,8 +836,9 @@ rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 	 * reference counter can occur.
 	 */
 	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
-		rte_mbuf_refcnt_set(m, 1 + value);
-		return 1 + value;
+		++value;
+		rte_mbuf_refcnt_set(m, value);
+		return value;
 	}
 
 	return __rte_mbuf_refcnt_update(m, value);
@@ -927,8 +928,9 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 	int16_t value)
 {
 	if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1)) {
-		rte_mbuf_ext_refcnt_set(shinfo, 1 + value);
-		return 1 + value;
+		++value;
+		rte_mbuf_ext_refcnt_set(shinfo, value);
+		return value;
 	}
 
 	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);

[PATCH v3 27/40] rte_mbuf.h: explicit casts for flipping between int16_t and uint16_t

[Andy Green]

differences to the atomic16 are signed, but the
atomic16 itself is unsigned.  It needs to be
made explicit with casts.

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a2a37a311..c214f1945 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -806,7 +806,7 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, new_value);
+	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
 }
 
 /* internal */
@@ -837,8 +837,8 @@ rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 	 */
 	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
 		++value;
-		rte_mbuf_refcnt_set(m, value);
-		return value;
+		rte_mbuf_refcnt_set(m, (uint16_t)value);
+		return (uint16_t)value;
 	}
 
 	return __rte_mbuf_refcnt_update(m, value);
@@ -909,7 +909,7 @@ static inline void
 rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
 	uint16_t new_value)
 {
-	rte_atomic16_set(&shinfo->refcnt_atomic, new_value);
+	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
 }
 
 /**
@@ -929,8 +929,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 {
 	if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1)) {
 		++value;
-		rte_mbuf_ext_refcnt_set(shinfo, value);
-		return value;
+		rte_mbuf_ext_refcnt_set(shinfo, (uint16_t)value);
+		return (uint16_t)value;
 	}
 
 	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);

[PATCH v3 28/40] rte_mbuf.h: make sure RTE_MIN compares same types

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_common.h:384:2:
warning: conversion from 'int' to 'uint16_t'
{aka 'short unsigned int'} may change value [-Wconversion]
  __extension__ ({ \
  ^~~~~~~~~~~~~
/projects/lagopus/src/dpdk/build/include/rte_mbuf.h:1204:16:
note: in expansion of macro 'RTE_MIN'
  m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM,
(uint16_t)m->buf_len);

RTE_PKTMBUF_HEADROOM is typ 128, so it doesn't make trouble.

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index c214f1945..a27dbb878 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1201,7 +1201,8 @@ rte_pktmbuf_priv_size(struct rte_mempool *mp)
  */
 static inline void rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
 {
-	m->data_off = RTE_MIN(RTE_PKTMBUF_HEADROOM, (uint16_t)m->buf_len);
+	m->data_off = (uint16_t)RTE_MIN((uint16_t)RTE_PKTMBUF_HEADROOM,
+					(uint16_t)m->buf_len);
 }
 
 /**

[PATCH v3 29/40] rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_common.h:141:34:
warning: conversion from 'long unsigned int' to 'uint16_t'
{aka 'short unsigned int'} may change value [-Wconversion]
 #define RTE_PTR_DIFF(ptr1, ptr2) ((uintptr_t)(ptr1) -
(uintptr_t)(ptr2))
                                  ^
/projects/lagopus/src/dpdk/build/include/rte_mbuf.h:1360:13:
note: in expansion of macro 'RTE_PTR_DIFF'
  *buf_len = RTE_PTR_DIFF(shinfo, buf_addr);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a27dbb878..0580ec8a0 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1358,7 +1358,7 @@ rte_pktmbuf_ext_shinfo_init_helper(void *buf_addr, uint16_t *buf_len,
 	shinfo->fcb_opaque = fcb_opaque;
 	rte_mbuf_ext_refcnt_set(shinfo, 1);
 
-	*buf_len = RTE_PTR_DIFF(shinfo, buf_addr);
+	*buf_len = (uint16_t)RTE_PTR_DIFF(shinfo, buf_addr);
 	return shinfo;
 }
 

[PATCH v3 30/40] rte_mbuf.h: explicit cast for size_t to uint32_t

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 0580ec8a0..169f3d3b0 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1577,7 +1577,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 		__rte_pktmbuf_free_direct(m);
 
 	priv_size = rte_pktmbuf_priv_size(mp);
-	mbuf_size = sizeof(struct rte_mbuf) + priv_size;
+	mbuf_size = (uint32_t)sizeof(struct rte_mbuf) + priv_size;
 	buf_len = rte_pktmbuf_data_room_size(mp);
 
 	m->priv_size = priv_size;

[PATCH v3 31/40] rte_mbuf.h: explicit casts to uint16_t to avoid truncation warnings

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_mbuf/rte_mbuf.h |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 169f3d3b0..3cd76abbc 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1580,7 +1580,7 @@ static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 	mbuf_size = (uint32_t)sizeof(struct rte_mbuf) + priv_size;
 	buf_len = rte_pktmbuf_data_room_size(mp);
 
-	m->priv_size = priv_size;
+	m->priv_size = (uint16_t)priv_size;
 	m->buf_addr = (char *)m + mbuf_size;
 	m->buf_iova = rte_mempool_virt2iova(m) + mbuf_size;
 	m->buf_len = (uint16_t)buf_len;
@@ -1905,7 +1905,7 @@ static inline char *rte_pktmbuf_prepend(struct rte_mbuf *m,
 	if (unlikely(len > rte_pktmbuf_headroom(m)))
 		return NULL;
 
-	m->data_off -= len;
+	m->data_off = (uint16_t)(m->data_off - len);
 	m->data_len = (uint16_t)(m->data_len + len);
 	m->pkt_len  = (m->pkt_len + len);
 
@@ -1966,7 +1966,7 @@ static inline char *rte_pktmbuf_adj(struct rte_mbuf *m, uint16_t len)
 		return NULL;
 
 	m->data_len = (uint16_t)(m->data_len - len);
-	m->data_off += len;
+	m->data_off = (uint16_t)(m->data_off + len);
 	m->pkt_len  = (m->pkt_len - len);
 	return (char *)m->buf_addr + m->data_off;
 }
@@ -2079,7 +2079,7 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
 	cur_tail->next = tail;
 
 	/* accumulate number of segments and total length. */
-	head->nb_segs += tail->nb_segs;
+	head->nb_segs = (uint16_t)(head->nb_segs + tail->nb_segs);
 	head->pkt_len += tail->pkt_len;
 
 	/* pkt_len is only set in the head */
@@ -2109,7 +2109,8 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 		return 0;
 
 	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
-		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
+		inner_l3_offset += (unsigned int)(m->outer_l2_len +
+						  m->outer_l3_len);
 
 	/* Headers are fragmented */
 	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
@@ -2154,7 +2155,7 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 static inline int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 {
-	int seg_len, copy_len;
+	size_t seg_len, copy_len;
 	struct rte_mbuf *m;
 	struct rte_mbuf *m_next;
 	char *buffer;
@@ -2169,7 +2170,7 @@ rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 		return -1;
 
 	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+	mbuf->data_len = (uint16_t)mbuf->pkt_len;
 
 	/* Append data from next segments to the first one */
 	m = mbuf->next;

[PATCH v3 32/40] rte_byteorder.h: explicit cast for return promotion

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 .../common/include/generic/rte_byteorder.h         |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
index 9bed85cca..8ffbac394 100644
--- a/lib/librte_eal/common/include/generic/rte_byteorder.h
+++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
@@ -123,7 +123,7 @@ typedef uint64_t rte_le64_t; /**< 64-bit little-endian value. */
 static inline uint16_t
 rte_constant_bswap16(uint16_t x)
 {
-	return RTE_STATIC_BSWAP16(x);
+	return (uint16_t)RTE_STATIC_BSWAP16((uint16_t)x);
 }
 
 /*

[PATCH v3 33/40] rte_ether.h: explicit cast avoiding truncation warning

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_ether.h:213:13:
warning: conversion from 'int' to 'uint8_t'
{aka 'unsigned char'} may change value [-Wconversion]
  addr[0] &= ~ETHER_GROUP_ADDR;
/* clear multicast bit */

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_net/rte_ether.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 95d0a533f..01d57f0ae 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -210,7 +210,7 @@ static inline void eth_random_addr(uint8_t *addr)
 	uint8_t *p = (uint8_t *)&rand;
 
 	rte_memcpy(addr, p, ETHER_ADDR_LEN);
-	addr[0] &= ~ETHER_GROUP_ADDR;       /* clear multicast bit */
+	addr[0] &= (uint8_t)~ETHER_GROUP_ADDR;       /* clear multicast bit */
 	addr[0] |= ETHER_LOCAL_ADMIN_ADDR;  /* set local assignment bit */
 }
 

[PATCH v3 34/40] rte_ether.h: stack vars declared at top of function

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_ether.h:
In function 'rte_vlan_strip':
/projects/lagopus/src/dpdk/build/include/rte_ether.h:357:2:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
  struct vlan_hdr *vh = (struct vlan_hdr *)(eh + 1);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_net/rte_ether.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 01d57f0ae..bee2b34f0 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -350,11 +350,12 @@ static inline int rte_vlan_strip(struct rte_mbuf *m)
 {
 	struct ether_hdr *eh
 		 = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	struct vlan_hdr *vh;
 
 	if (eh->ether_type != rte_cpu_to_be_16(ETHER_TYPE_VLAN))
 		return -1;
 
-	struct vlan_hdr *vh = (struct vlan_hdr *)(eh + 1);
+	vh = (struct vlan_hdr *)(eh + 1);
 	m->ol_flags |= PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED;
 	m->vlan_tci = rte_be_to_cpu_16(vh->vlan_tci);
 

[PATCH v3 35/40] rte_ethdev.h: fix sign and scope of temp var

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_ethdev/rte_ethdev.h |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 7ccf4bae6..2487e1d2d 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3801,6 +3801,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 		 struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
+	uint16_t nb_rx;
 
 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
@@ -3811,18 +3812,22 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 		return 0;
 	}
 #endif
-	int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
-			rx_pkts, nb_pkts);
+	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
+				     rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
-
-	if (unlikely(cb != NULL)) {
-		do {
-			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
-						nb_pkts, cb->param);
-			cb = cb->next;
-		} while (cb != NULL);
+	{
+		struct rte_eth_rxtx_callback *cb =
+				dev->post_rx_burst_cbs[queue_id];
+
+		if (unlikely(cb != NULL)) {
+			do {
+				nb_rx = cb->fn.rx(port_id, queue_id,
+						  rx_pkts, nb_rx,
+						  nb_pkts, cb->param);
+				cb = cb->next;
+			} while (cb != NULL);
+		}
 	}
 #endif
 

[PATCH v3 36/40] rte_ethdev.h: explicit cast for return type

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_ethdev.h:3860:10:
warning: conversion to 'int' from 'uint32_t' {aka 'unsigned int'}
may change the sign of the result [-Wsign-conversion]
  return (*dev->dev_ops->rx_queue_count)(dev, queue_id);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_ethdev/rte_ethdev.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 2487e1d2d..c84dc44b8 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3857,7 +3857,7 @@ rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
 	if (queue_id >= dev->data->nb_rx_queues)
 		return -EINVAL;
 
-	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
+	return (int)(*dev->dev_ops->rx_queue_count)(dev, queue_id);
 }
 
 /**

[PATCH v3 37/40] rte_ethdev.h: explicit cast for truncation

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_ethdev.h:
In function 'rte_eth_tx_buffer_flush':
/projects/lagopus/src/dpdk/build/include/rte_ethdev.h:4248:55:
warning: conversion from 'int' to 'uint16_t'
{aka 'short unsigned int'} may change value [-Wconversion]
   buffer->error_callback(&buffer->pkts[sent], to_send - sent,

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_ethdev/rte_ethdev.h |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index c84dc44b8..a7c0b3025 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4245,8 +4245,9 @@ rte_eth_tx_buffer_flush(uint16_t port_id, uint16_t queue_id,
 
 	/* All packets sent, or to be dealt with by callback below */
 	if (unlikely(sent != to_send))
-		buffer->error_callback(&buffer->pkts[sent], to_send - sent,
-				buffer->error_userdata);
+		buffer->error_callback(&buffer->pkts[sent],
+				       (uint16_t)(to_send - sent),
+				       buffer->error_userdata);
 
 	return sent;
 }

[PATCH v3 38/40] rte_hash_crc.h: stack vars declared at top of function

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_hash_crc.h:
In function 'crc32c_2words':
/projects/lagopus/src/dpdk/build/include/rte_hash_crc.h:347:2:
warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
  uint32_t crc, term1, term2;

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_hash/rte_hash_crc.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 479f84b14..5f5fb3db1 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -338,14 +338,13 @@ crc32c_1word(uint32_t data, uint32_t init_val)
 static inline uint32_t
 crc32c_2words(uint64_t data, uint32_t init_val)
 {
+	uint32_t crc, term1, term2;
 	union {
 		uint64_t u64;
 		uint32_t u32[2];
 	} d;
 	d.u64 = data;
 
-	uint32_t crc, term1, term2;
-
 	crc = init_val;
 	crc ^= d.u32[0];
 

[PATCH v3 39/40] rte_hash_crc.h: explicit casts for truncation

[Andy Green]

/projects/lagopus/src/dpdk/build/include/rte_hash_crc.h:
In function 'crc32c_sse42_u64_mimic':
/projects/lagopus/src/dpdk/build/include/rte_hash_crc.h:402:40:
warning: conversion from 'uint64_t' {aka 'long unsigned int'}
to 'uint32_t' {aka 'unsigned int'} may change value [-Wconversion]
  init_val = crc32c_sse42_u32(d.u32[0], init_val);

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_hash/rte_hash_crc.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 5f5fb3db1..cf28031b3 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -398,9 +398,9 @@ crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
 	} d;
 
 	d.u64 = data;
-	init_val = crc32c_sse42_u32(d.u32[0], init_val);
-	init_val = crc32c_sse42_u32(d.u32[1], init_val);
-	return init_val;
+	init_val = crc32c_sse42_u32(d.u32[0], (uint32_t)init_val);
+	init_val = crc32c_sse42_u32(d.u32[1], (uint32_t)init_val);
+	return (uint32_t)init_val;
 }
 #endif
 
@@ -412,7 +412,7 @@ crc32c_sse42_u64(uint64_t data, uint64_t init_val)
 			"crc32q %[data], %[init_val];"
 			: [init_val] "+r" (init_val)
 			: [data] "rm" (data));
-	return init_val;
+	return (uint32_t)init_val;
 }
 #endif
 

[PATCH v3 40/40] rte_string_fns.h: explicit cast for int return to size_t

[Andy Green]

Signed-off-by: Andy Green <andy@warmcat.com>
---
 lib/librte_eal/common/include/rte_string_fns.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
index fcbb42e00..51413a55e 100644
--- a/lib/librte_eal/common/include/rte_string_fns.h
+++ b/lib/librte_eal/common/include/rte_string_fns.h
@@ -55,7 +55,7 @@ rte_strsplit(char *string, int stringlen,
 static inline size_t
 rte_strlcpy(char *dst, const char *src, size_t size)
 {
-	return snprintf(dst, size, "%s", src);
+	return (size_t)(unsigned int)snprintf(dst, size, "%s", src);
 }
 
 /* pull in a strlcpy function */

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 10 May 2018 10:46:18 +0800
> From: Andy Green <andy@warmcat.com>
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> User-Agent: StGit/unknown-version
> 
> The following series gets current master able to build
> itself, and allow lagopus to build against it, on Fedora 28 +
> x86_64 using gcc 8.0.1.
> 
> The first 17 patches have already been through two spins and
> this time are corrected for all the comment (thanks to
> everybody who commented) since v2, and have tested-by /
> acked-bys applied.  The first workaround patch for the hash
> function cast problem is dropped since something has already
> been applied in master since yesterday to address it.
> 
> The additional 23 patches are fixes for problems found
> actually trying to build lagopus using current master.
> These are almost entirely related to signed / unsigned
> or truncation without explicit casts inside dpdk
> headers.


Tested this series on gcc version 8.1.0

Found following error:
/export/dpdk.org/test/test/test_table_pipeline.c: In function
‘test_table_pipeline’:
/export/dpdk.org/test/test/test_table_pipeline.c:521:3: error: cast
between incompatible function types from ‘int (* (*)(struct rte_pipeline
*, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry **,
void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry **, void *))(struct rte_pipeline *, struct
rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
*)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
   (rte_pipeline_table_action_handler_miss) table_action_stub_miss;
   ^
/export/dpdk.org/test/test/test_table_pipeline.c:557:3: error: cast
between incompatible function types from ‘int (* (*)(struct rte_pipeline
*, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry **,
void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry **, void *))(struct rte_pipeline *, struct
rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
*)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
   (rte_pipeline_table_action_handler_miss)table_action_stub_miss;


➜ [master][dpdk.org] $ gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/8.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr
--libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man
--infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++ --enable-shared
--enable-threads=posix --enable-libmpx --with-system-zlib --with-isl
--enable-__cxa_atexit --disable-libunwind-exceptions
--enable-clocale=gnu --disable-libstdcxx-pch --disable-libssp
--enable-gnu-unique-object --enable-linker-build-id --enable-lto
--enable-plugin --enable-install-libiberty --with-linker-hash-style=gnu
--enable-gnu-indirect-function --enable-multilib --disable-werror
--enable-checking=release --enable-default-pie --enable-default-ssp
Thread model: posix
gcc version 8.1.0 (GCC) 


> 
> ---
> 
> Andy Green (40):
>       drivers/bus/pci: fix strncpy dangerous code
>       drivers/bus/dpaa: fix inconsistent struct alignment
>       drivers/net/axgbe: fix broken eeprom string comp
>       drivers/net/nfp/nfpcore: fix strncpy misuse
>       drivers/net/nfp/nfpcore: fix off-by-one and no NUL on strncpy use
>       drivers/net/nfp: don't memcpy out of source range
>       drivers/net/nfp: fix buffer overflow in fw_name
>       drivers/net/qede: fix strncpy constant and NUL
>       drivers/net/qede: fix broken strncpy
>       drivers/net/sfc: fix strncpy length
>       drivers/net/sfc: fix strncpy size and NUL
>       drivers/net/vdev: readlink inputs cannot be aliased
>       drivers/net/vdev: fix 3 x strncpy misuse
>       app/test-pmd: can't find include
>       app/proc-info: fix sprintf overrun bug
>       app/test-bbdev: test-bbdev: strcpy ok for allocated string
>       app/test-bbdev: strcpy ok for allocated string
>       rte_common.h: cast gcc builtin result to avoid complaints
>       rte_memcpy.h: explicit tmp cast
>       lib/librte_eal/common/include/rte_lcore.h: explicit cast for signed change
>       /lib/librte_eal/common/include/rte_random.h: stage cast from uint64_t to long
>       rte_spinlock.h: stack declarations before code
>       rte_ring_generic.h: stack declarations before code
>       rte_ring.h: remove signed type flipflopping
>       rte_dev.h: stack declaration at top of own basic block
>       rte_mbuf.h: avoid truncation warnings from inadvertant int16_t to int promotion
>       rte_mbuf.h: explicit casts for flipping between int16_t and uint16_t
>       rte_mbuf.h: make sure RTE_MIN compares same types
>       rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
>       rte_mbuf.h: explicit cast for size_t to uint32_t
>       rte_mbuf.h: explicit casts to uint16_t to avoid truncation warnings
>       rte_byteorder.h: explicit cast for return promotion
>       rte_ether.h: explicit cast avoiding truncation warning
>       rte_ether.h: stack vars declared at top of function
>       rte_ethdev.h: fix sign and scope of temp var
>       rte_ethdev.h: explicit cast for return type
>       rte_ethdev.h: explicit cast for truncation
>       rte_hash_crc.h: stack vars declared at top of function
>       rte_hash_crc.h: explicit casts for truncation
>       rte_string_fns.h: explicit cast for int return to size_t
> 
> 
>  app/proc-info/main.c                               |    9 ++++-
>  app/test-bbdev/test_bbdev_vector.c                 |    5 ++-
>  app/test-pmd/Makefile                              |    1 +
>  drivers/bus/dpaa/base/qbman/qman.c                 |   14 ++++----
>  drivers/bus/dpaa/include/fsl_qman.h                |   24 +++++++------
>  drivers/bus/pci/linux/pci.c                        |    2 +
>  drivers/net/axgbe/axgbe_phy_impl.c                 |    4 +-
>  drivers/net/nfp/nfp_net.c                          |    6 ++-
>  drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c         |    4 ++
>  drivers/net/nfp/nfpcore/nfp_resource.c             |   10 +++---
>  drivers/net/qede/base/ecore_int.c                  |    8 +++-
>  drivers/net/qede/qede_main.c                       |    7 ++--
>  drivers/net/sfc/sfc_ethdev.c                       |    6 ++-
>  drivers/net/vdev_netvsc/vdev_netvsc.c              |   15 +++++---
>  .../common/include/arch/x86/rte_memcpy.h           |    8 ++--
>  .../common/include/arch/x86/rte_spinlock.h         |    4 ++
>  .../common/include/generic/rte_byteorder.h         |    2 +
>  lib/librte_eal/common/include/rte_common.h         |    2 +
>  lib/librte_eal/common/include/rte_dev.h            |   15 +++++---
>  lib/librte_eal/common/include/rte_lcore.h          |    2 +
>  lib/librte_eal/common/include/rte_random.h         |    6 ++-
>  lib/librte_eal/common/include/rte_string_fns.h     |    2 +
>  lib/librte_ethdev/rte_ethdev.h                     |   32 +++++++++++-------
>  lib/librte_hash/rte_hash_crc.h                     |   11 +++---
>  lib/librte_mbuf/rte_mbuf.h                         |   36 +++++++++++---------
>  lib/librte_net/rte_ether.h                         |    5 ++-
>  lib/librte_ring/rte_ring.h                         |    4 +-
>  lib/librte_ring/rte_ring_c11_mem.h                 |    2 +
>  lib/librte_ring/rte_ring_generic.h                 |   10 ++----
>  29 files changed, 144 insertions(+), 112 deletions(-)
> 
> --
> Signature

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 10 May 2018 10:46:18 +0800
> From: Andy Green <andy@warmcat.com>
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> User-Agent: StGit/unknown-version
> 
> The following series gets current master able to build
> itself, and allow lagopus to build against it, on Fedora 28 +
> x86_64 using gcc 8.0.1.
> 
> The first 17 patches have already been through two spins and
> this time are corrected for all the comment (thanks to
> everybody who commented) since v2, and have tested-by /
> acked-bys applied.  The first workaround patch for the hash
> function cast problem is dropped since something has already
> been applied in master since yesterday to address it.
> 
> The additional 23 patches are fixes for problems found
> actually trying to build lagopus using current master.
> These are almost entirely related to signed / unsigned
> or truncation without explicit casts inside dpdk
> headers.
> 
> ---

Please run the following scripts before submitting the patch. There are plenty of errors.
./devtools/checkpatches.sh
./devtools/check-git-log.sh

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 02:17 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 10 May 2018 10:46:18 +0800
>> From: Andy Green <andy@warmcat.com>
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>> User-Agent: StGit/unknown-version
>>
>> The following series gets current master able to build
>> itself, and allow lagopus to build against it, on Fedora 28 +
>> x86_64 using gcc 8.0.1.
>>
>> The first 17 patches have already been through two spins and
>> this time are corrected for all the comment (thanks to
>> everybody who commented) since v2, and have tested-by /
>> acked-bys applied.  The first workaround patch for the hash
>> function cast problem is dropped since something has already
>> been applied in master since yesterday to address it.
>>
>> The additional 23 patches are fixes for problems found
>> actually trying to build lagopus using current master.
>> These are almost entirely related to signed / unsigned
>> or truncation without explicit casts inside dpdk
>> headers.
>>
>> ---
> 
> Please run the following scripts before submitting the patch. There are plenty of errors.
> ./devtools/checkpatches.sh

Actually I checked them already with kernel checkpatch.pl.

There is only one patch where the existing code I am patching does not 
pass checkpatch rules already, this kind of thing.

WARNING:LONG_LINE: line over 80 characters
#19: FILE: lib/librte_eal/common/include/arch/x86/rte_memcpy.h:600:
+    tmp = (int)len; 
                                     \

WARNING:LEADING_SPACE: please, no spaces at the start of a line
#19: FILE: lib/librte_eal/common/include/arch/x86/rte_memcpy.h:600:
+    tmp = (int)len; 
                                     \$


So what should I better do when the project doesn't follow its own 
super-pedantic rules?  I chose to follow the formatting that was already 
there.

> ./devtools/check-git-log.sh

Ugh...

Wrong headline format:
	drivers/net/nfp: fix buffer overflow in fw_name

... snip something "wrong" about every patch title...

It's just doing this

# check headline format (spacing, no punctuation, no code)
bad=$(echo "$headlines" | grep --color=always \
         -e '    ' \
         -e '^ ' \
         -e ' $' \
         -e '\.$' \
         -e '[,;!?&|]' \
         -e ':.*_' \
         -e '^[^:]\+$' \
         -e ':[^ ]' \
         -e ' :' \
         | sed 's,^,\t,')
[ -z "$bad" ] || printf "Wrong headline format:\n$bad\n"

It probably seems to whoever wrote it that adds "quality", but actually 
inflexible rules like this do nothing to help quality of the patch 
payload and actively put off contribution.

So on this first one it's hitting the rule ':.*_', ie, this project 
believes there should never be a patch title mentioning anything with an 
underscore after a colon.

Can you help me understand in what way banning mentioning relevant 
strings in the patch title is a good idea?  It's actively reducing the 
value of the patch title, isn't it?

-Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 02:12 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 10 May 2018 10:46:18 +0800
>> From: Andy Green <andy@warmcat.com>
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>> User-Agent: StGit/unknown-version
>>
>> The following series gets current master able to build
>> itself, and allow lagopus to build against it, on Fedora 28 +
>> x86_64 using gcc 8.0.1.
>>
>> The first 17 patches have already been through two spins and
>> this time are corrected for all the comment (thanks to
>> everybody who commented) since v2, and have tested-by /
>> acked-bys applied.  The first workaround patch for the hash
>> function cast problem is dropped since something has already
>> been applied in master since yesterday to address it.
>>
>> The additional 23 patches are fixes for problems found
>> actually trying to build lagopus using current master.
>> These are almost entirely related to signed / unsigned
>> or truncation without explicit casts inside dpdk
>> headers.
> 
> 
> Tested this series on gcc version 8.1.0
> 
> Found following error:

On gcc 8.0.1 on Fedora 28, where this doesn't appear using x86_64 
defconfig.  This is with the basis the current master HEAD 
8ea41438832a360aed2b7ba49fb75e310a2ff1dc

I assume 8.1 just got better at spotting the problem.

> /export/dpdk.org/test/test/test_table_pipeline.c: In function
> ‘test_table_pipeline’:
> /export/dpdk.org/test/test/test_table_pipeline.c:521:3: error: cast
> between incompatible function types from ‘int (* (*)(struct rte_pipeline
> *, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry **,
> void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
> rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
> rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> rte_pipeline_table_entry **, void *))(struct rte_pipeline *, struct
> rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
> *)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
> struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
> rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
>     (rte_pipeline_table_action_handler_miss) table_action_stub_miss;
>     ^
> /export/dpdk.org/test/test/test_table_pipeline.c:557:3: error: cast
> between incompatible function types from ‘int (* (*)(struct rte_pipeline
> *, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry **,
> void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
> rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
> rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> rte_pipeline_table_entry **, void *))(struct rte_pipeline *, struct
> rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
> *)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
> struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
> rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
>     (rte_pipeline_table_action_handler_miss)table_action_stub_miss;

Hum...

That seems to be declared:

rte_pipeline_table_action_handler_miss action_handler_miss = NULL;

which is

typedef int (*rte_pipeline_table_action_handler_miss)(
         struct rte_pipeline *p,
         struct rte_mbuf **pkts,
         uint64_t pkts_mask,
         struct rte_pipeline_table_entry *entry,    <<<<=====
         void *arg);

We're doing

action_handler_miss =
    (rte_pipeline_table_action_handler_miss)table_action_stub_miss;

The prototype for that is

rte_pipeline_table_action_handler_miss
table_action_stub_miss(struct rte_pipeline *p,
			struct rte_mbuf **pkts,
			uint64_t pkts_mask,
			struct rte_pipeline_table_entry **entry,  <<====
			void *arg);

It's true, the fourth arg has a different level of indirection... it 
seems another real pre-existing problem.

However the stub doesn't use the fourth arg "entry"

rte_pipeline_table_action_handler_miss
table_action_stub_miss(struct rte_pipeline *p,
         __attribute__((unused)) struct rte_mbuf **pkts,
         uint64_t pkts_mask,
         __attribute__((unused)) struct rte_pipeline_table_entry **entry,
         __attribute__((unused)) void *arg)
{
         printf("STUB Table Action Miss - setting mask to 0x%"PRIx64"\n",
                 override_miss_mask);
         pkts_mask = (~override_miss_mask) & 0x3;
         rte_pipeline_ah_packet_drop(p, pkts_mask);
         return 0;
}

Therefore, since I can't get the error here, can you check if this 
solves it?

diff --git a/test/test/test_table_pipeline.c 
b/test/test/test_table_pipeline.c
index 055a1a4e7..d007d55ce 100644
--- a/test/test/test_table_pipeline.c
+++ b/test/test/test_table_pipeline.c
@@ -71,7 +71,7 @@ table_action_stub_hit(struct rte_pipeline *p, struct 
rte_mbuf **pkts,

  rte_pipeline_table_action_handler_miss
  table_action_stub_miss(struct rte_pipeline *p, struct rte_mbuf **pkts,
-       uint64_t pkts_mask, struct rte_pipeline_table_entry **entry, 
void *arg);
+       uint64_t pkts_mask, struct rte_pipeline_table_entry *entry, void 
*arg);

  rte_pipeline_table_action_handler_hit
  table_action_0x00(__attribute__((unused)) struct rte_pipeline *p,
@@ -105,7 +105,7 @@ rte_pipeline_table_action_handler_miss
  table_action_stub_miss(struct rte_pipeline *p,
         __attribute__((unused)) struct rte_mbuf **pkts,
         uint64_t pkts_mask,
-       __attribute__((unused)) struct rte_pipeline_table_entry **entry,
+       __attribute__((unused)) struct rte_pipeline_table_entry *entry,
         __attribute__((unused)) void *arg)
  {
         printf("STUB Table Action Miss - setting mask to 0x%"PRIx64"\n",


At least I confirmed it still builds without error on 8.0.1 with my 
series and that patch.

-Andy

> 
> ➜ [master][dpdk.org] $ gcc -v
> Using built-in specs.
> COLLECT_GCC=/usr/bin/gcc
> COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/8.1.0/lto-wrapper
> Target: x86_64-pc-linux-gnu
> Configured with: /build/gcc/src/gcc/configure --prefix=/usr
> --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man
> --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/
> --enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++ --enable-shared
> --enable-threads=posix --enable-libmpx --with-system-zlib --with-isl
> --enable-__cxa_atexit --disable-libunwind-exceptions
> --enable-clocale=gnu --disable-libstdcxx-pch --disable-libssp
> --enable-gnu-unique-object --enable-linker-build-id --enable-lto
> --enable-plugin --enable-install-libiberty --with-linker-hash-style=gnu
> --enable-gnu-indirect-function --enable-multilib --disable-werror
> --enable-checking=release --enable-default-pie --enable-default-ssp
> Thread model: posix
> gcc version 8.1.0 (GCC)
> 
> 
>>
>> ---
>>
>> Andy Green (40):
>>        drivers/bus/pci: fix strncpy dangerous code
>>        drivers/bus/dpaa: fix inconsistent struct alignment
>>        drivers/net/axgbe: fix broken eeprom string comp
>>        drivers/net/nfp/nfpcore: fix strncpy misuse
>>        drivers/net/nfp/nfpcore: fix off-by-one and no NUL on strncpy use
>>        drivers/net/nfp: don't memcpy out of source range
>>        drivers/net/nfp: fix buffer overflow in fw_name
>>        drivers/net/qede: fix strncpy constant and NUL
>>        drivers/net/qede: fix broken strncpy
>>        drivers/net/sfc: fix strncpy length
>>        drivers/net/sfc: fix strncpy size and NUL
>>        drivers/net/vdev: readlink inputs cannot be aliased
>>        drivers/net/vdev: fix 3 x strncpy misuse
>>        app/test-pmd: can't find include
>>        app/proc-info: fix sprintf overrun bug
>>        app/test-bbdev: test-bbdev: strcpy ok for allocated string
>>        app/test-bbdev: strcpy ok for allocated string
>>        rte_common.h: cast gcc builtin result to avoid complaints
>>        rte_memcpy.h: explicit tmp cast
>>        lib/librte_eal/common/include/rte_lcore.h: explicit cast for signed change
>>        /lib/librte_eal/common/include/rte_random.h: stage cast from uint64_t to long
>>        rte_spinlock.h: stack declarations before code
>>        rte_ring_generic.h: stack declarations before code
>>        rte_ring.h: remove signed type flipflopping
>>        rte_dev.h: stack declaration at top of own basic block
>>        rte_mbuf.h: avoid truncation warnings from inadvertant int16_t to int promotion
>>        rte_mbuf.h: explicit casts for flipping between int16_t and uint16_t
>>        rte_mbuf.h: make sure RTE_MIN compares same types
>>        rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
>>        rte_mbuf.h: explicit cast for size_t to uint32_t
>>        rte_mbuf.h: explicit casts to uint16_t to avoid truncation warnings
>>        rte_byteorder.h: explicit cast for return promotion
>>        rte_ether.h: explicit cast avoiding truncation warning
>>        rte_ether.h: stack vars declared at top of function
>>        rte_ethdev.h: fix sign and scope of temp var
>>        rte_ethdev.h: explicit cast for return type
>>        rte_ethdev.h: explicit cast for truncation
>>        rte_hash_crc.h: stack vars declared at top of function
>>        rte_hash_crc.h: explicit casts for truncation
>>        rte_string_fns.h: explicit cast for int return to size_t
>>
>>
>>   app/proc-info/main.c                               |    9 ++++-
>>   app/test-bbdev/test_bbdev_vector.c                 |    5 ++-
>>   app/test-pmd/Makefile                              |    1 +
>>   drivers/bus/dpaa/base/qbman/qman.c                 |   14 ++++----
>>   drivers/bus/dpaa/include/fsl_qman.h                |   24 +++++++------
>>   drivers/bus/pci/linux/pci.c                        |    2 +
>>   drivers/net/axgbe/axgbe_phy_impl.c                 |    4 +-
>>   drivers/net/nfp/nfp_net.c                          |    6 ++-
>>   drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c         |    4 ++
>>   drivers/net/nfp/nfpcore/nfp_resource.c             |   10 +++---
>>   drivers/net/qede/base/ecore_int.c                  |    8 +++-
>>   drivers/net/qede/qede_main.c                       |    7 ++--
>>   drivers/net/sfc/sfc_ethdev.c                       |    6 ++-
>>   drivers/net/vdev_netvsc/vdev_netvsc.c              |   15 +++++---
>>   .../common/include/arch/x86/rte_memcpy.h           |    8 ++--
>>   .../common/include/arch/x86/rte_spinlock.h         |    4 ++
>>   .../common/include/generic/rte_byteorder.h         |    2 +
>>   lib/librte_eal/common/include/rte_common.h         |    2 +
>>   lib/librte_eal/common/include/rte_dev.h            |   15 +++++---
>>   lib/librte_eal/common/include/rte_lcore.h          |    2 +
>>   lib/librte_eal/common/include/rte_random.h         |    6 ++-
>>   lib/librte_eal/common/include/rte_string_fns.h     |    2 +
>>   lib/librte_ethdev/rte_ethdev.h                     |   32 +++++++++++-------
>>   lib/librte_hash/rte_hash_crc.h                     |   11 +++---
>>   lib/librte_mbuf/rte_mbuf.h                         |   36 +++++++++++---------
>>   lib/librte_net/rte_ether.h                         |    5 ++-
>>   lib/librte_ring/rte_ring.h                         |    4 +-
>>   lib/librte_ring/rte_ring_c11_mem.h                 |    2 +
>>   lib/librte_ring/rte_ring_generic.h                 |   10 ++----
>>   29 files changed, 144 insertions(+), 112 deletions(-)
>>
>> --
>> Signature

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 10 May 2018 14:46:42 +0800
> From: Andy Green <andy@warmcat.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>  Thunderbird/52.7.0
> 
> 
> 
> On 05/10/2018 02:17 PM, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Thu, 10 May 2018 10:46:18 +0800
> > > From: Andy Green <andy@warmcat.com>
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> > > User-Agent: StGit/unknown-version
> > > 
> > ./devtools/check-git-log.sh
> 
> Ugh...
> 
> Wrong headline format:
> 	drivers/net/nfp: fix buffer overflow in fw_name
> 
> ... snip something "wrong" about every patch title...
> 
> It's just doing this
> 
> # check headline format (spacing, no punctuation, no code)
> bad=$(echo "$headlines" | grep --color=always \
>         -e '    ' \
>         -e '^ ' \
>         -e ' $' \
>         -e '\.$' \
>         -e '[,;!?&|]' \
>         -e ':.*_' \
>         -e '^[^:]\+$' \
>         -e ':[^ ]' \
>         -e ' :' \
>         | sed 's,^,\t,')
> [ -z "$bad" ] || printf "Wrong headline format:\n$bad\n"
> 
> It probably seems to whoever wrote it that adds "quality", but actually
> inflexible rules like this do nothing to help quality of the patch payload
> and actively put off contribution.
> 
> So on this first one it's hitting the rule ':.*_', ie, this project believes
> there should never be a patch title mentioning anything with an underscore
> after a colon.
> 
> Can you help me understand in what way banning mentioning relevant strings
> in the patch title is a good idea?  It's actively reducing the value of the
> patch title, isn't it?

I think, the underscore check is to make sure that the subject should not have
C symbols.

Change to following will work:

net/nfp: fix buffer overflow

> 
> -Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 10 May 2018 15:11:11 +0800
> From: Andy Green <andy@warmcat.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>  Thunderbird/52.7.0
> 
> 
> 
> On 05/10/2018 02:12 PM, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Thu, 10 May 2018 10:46:18 +0800
> > > From: Andy Green <andy@warmcat.com>
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> > > User-Agent: StGit/unknown-version
> > > 
> > > The following series gets current master able to build
> > > itself, and allow lagopus to build against it, on Fedora 28 +
> > > x86_64 using gcc 8.0.1.
> > > 
> > > The first 17 patches have already been through two spins and
> > > this time are corrected for all the comment (thanks to
> > > everybody who commented) since v2, and have tested-by /
> > > acked-bys applied.  The first workaround patch for the hash
> > > function cast problem is dropped since something has already
> > > been applied in master since yesterday to address it.
> > > 
> > > The additional 23 patches are fixes for problems found
> > > actually trying to build lagopus using current master.
> > > These are almost entirely related to signed / unsigned
> > > or truncation without explicit casts inside dpdk
> > > headers.
> > 
> > 
> > Tested this series on gcc version 8.1.0
> > 
> > Found following error:
> 
> On gcc 8.0.1 on Fedora 28, where this doesn't appear using x86_64 defconfig.
> This is with the basis the current master HEAD
> 8ea41438832a360aed2b7ba49fb75e310a2ff1dc

I use the same change set. I think, In GCC 8.1, they have added 
strict function type casting check.

I see a lot failure in examples with GCC 8.1, Are you tested the examples directory?

> 
> I assume 8.1 just got better at spotting the problem.
> 
> > /export/dpdk.org/test/test/test_table_pipeline.c: In function
> > ‘test_table_pipeline’:
> > /export/dpdk.org/test/test/test_table_pipeline.c:521:3: error: cast
> > between incompatible function types from ‘int (* (*)(struct rte_pipeline
> > *, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry **,
> > void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
> > rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
> > rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> > rte_pipeline_table_entry **, void *))(struct rte_pipeline *, struct
> > rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
> > *)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
> > struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
> > rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> > rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
> >     (rte_pipeline_table_action_handler_miss) table_action_stub_miss;
> >     ^
> > /export/dpdk.org/test/test/test_table_pipeline.c:557:3: error: cast
> > between incompatible function types from ‘int (* (*)(struct rte_pipeline
> > *, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry **,
> > void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
> > rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
> > rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> > rte_pipeline_table_entry **, void *))(struct rte_pipeline *, struct
> > rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
> > *)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
> > struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
> > rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
> > rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
> >     (rte_pipeline_table_action_handler_miss)table_action_stub_miss;
> 
> Hum...
> 
> That seems to be declared:
> 
> rte_pipeline_table_action_handler_miss action_handler_miss = NULL;
> 
> which is
> 
> typedef int (*rte_pipeline_table_action_handler_miss)(
>         struct rte_pipeline *p,
>         struct rte_mbuf **pkts,
>         uint64_t pkts_mask,
>         struct rte_pipeline_table_entry *entry,    <<<<=====
>         void *arg);
> 
> We're doing
> 
> action_handler_miss =
>    (rte_pipeline_table_action_handler_miss)table_action_stub_miss;
> 
> The prototype for that is
> 
> rte_pipeline_table_action_handler_miss
> table_action_stub_miss(struct rte_pipeline *p,
> 			struct rte_mbuf **pkts,
> 			uint64_t pkts_mask,
> 			struct rte_pipeline_table_entry **entry,  <<====
> 			void *arg);
> 
> It's true, the fourth arg has a different level of indirection... it seems
> another real pre-existing problem.
> 
> However the stub doesn't use the fourth arg "entry"
> 
> rte_pipeline_table_action_handler_miss
> table_action_stub_miss(struct rte_pipeline *p,
>         __attribute__((unused)) struct rte_mbuf **pkts,
>         uint64_t pkts_mask,
>         __attribute__((unused)) struct rte_pipeline_table_entry **entry,
>         __attribute__((unused)) void *arg)
> {
>         printf("STUB Table Action Miss - setting mask to 0x%"PRIx64"\n",
>                 override_miss_mask);
>         pkts_mask = (~override_miss_mask) & 0x3;
>         rte_pipeline_ah_packet_drop(p, pkts_mask);
>         return 0;
> }
> 
> Therefore, since I can't get the error here, can you check if this solves
> it?
> 
> diff --git a/test/test/test_table_pipeline.c
> b/test/test/test_table_pipeline.c
> index 055a1a4e7..d007d55ce 100644
> --- a/test/test/test_table_pipeline.c
> +++ b/test/test/test_table_pipeline.c
> @@ -71,7 +71,7 @@ table_action_stub_hit(struct rte_pipeline *p, struct
> rte_mbuf **pkts,
> 
>  rte_pipeline_table_action_handler_miss
>  table_action_stub_miss(struct rte_pipeline *p, struct rte_mbuf **pkts,
> -       uint64_t pkts_mask, struct rte_pipeline_table_entry **entry, void
> *arg);
> +       uint64_t pkts_mask, struct rte_pipeline_table_entry *entry, void
> *arg);
> 
>  rte_pipeline_table_action_handler_hit
>  table_action_0x00(__attribute__((unused)) struct rte_pipeline *p,
> @@ -105,7 +105,7 @@ rte_pipeline_table_action_handler_miss
>  table_action_stub_miss(struct rte_pipeline *p,
>         __attribute__((unused)) struct rte_mbuf **pkts,
>         uint64_t pkts_mask,
> -       __attribute__((unused)) struct rte_pipeline_table_entry **entry,
> +       __attribute__((unused)) struct rte_pipeline_table_entry *entry,
>         __attribute__((unused)) void *arg)
>  {
>         printf("STUB Table Action Miss - setting mask to 0x%"PRIx64"\n",
> 
> 
> At least I confirmed it still builds without error on 8.0.1 with my series
> and that patch.

OK.

The above patch does not help, GCC 8.1 simply don't like any from
function typecasting. see ^^^^^ in the quote below.


/export/dpdk.org/test/test/test_table_pipeline.c: In function
‘test_table_pipeline’:
/export/dpdk.org/test/test/test_table_pipeline.c:521:3: error: cast
between incompatible function types from ‘int (* (*)(struct rte_pipeline
*, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry *,
void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
                                             ^^^^^^^^

rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry *, void *))(struct rte_pipeline *, struct
rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
*)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
         ^^^^^^^^^


struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
   (rte_pipeline_table_action_handler_miss) table_action_stub_miss;
   ^
/export/dpdk.org/test/test/test_table_pipeline.c:557:3: error: cast
between incompatible function types from ‘int (* (*)(struct rte_pipeline
*, struct rte_mbuf **, uint64_t,  struct rte_pipeline_table_entry *,
void *))(struct rte_pipeline *, struct rte_mbuf **, uint64_t,  struct
rte_pipeline_table_entry *, void *)’ {aka ‘int (* (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry *, void *))(struct rte_pipeline *, struct
rte_mbuf **, long unsigned int,  struct rte_pipeline_table_entry *, void
*)’} to ‘int (*)(struct rte_pipeline *, struct rte_mbuf **, uint64_t,
struct rte_pipeline_table_entry *, void *)’ {aka ‘int (*)(struct
rte_pipeline *, struct rte_mbuf **, long unsigned int,  struct
rte_pipeline_table_entry *, void *)’} [-Werror=cast-function-type]
   (rte_pipeline_table_action_handler_miss)table_action_stub_miss;
   ^
cc1: all warnings being treated as errors

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[De Lara Guarch, Pablo]

Hi Andy,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Thursday, May 10, 2018 3:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> 
> The following series gets current master able to build itself, and allow lagopus to
> build against it, on Fedora 28 +
> x86_64 using gcc 8.0.1.
> 
> The first 17 patches have already been through two spins and this time are
> corrected for all the comment (thanks to everybody who commented) since v2,
> and have tested-by / acked-bys applied.  The first workaround patch for the hash
> function cast problem is dropped since something has already been applied in
> master since yesterday to address it.
> 
> The additional 23 patches are fixes for problems found actually trying to build
> lagopus using current master.
> These are almost entirely related to signed / unsigned or truncation without
> explicit casts inside dpdk headers.

I think it would be a good idea to split this patchset into two.
One for the gcc 8 fixes, and another one for the rest of them.

We should first focus on fixes for just DPDK, knowing that the release is close.

Could you also share what's triggering the other issues, when ou build lagopus?

Thanks,
Pablo

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Luca Boccassi]

On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
> The following series gets current master able to build
> itself, and allow lagopus to build against it, on Fedora 28 +
> x86_64 using gcc 8.0.1.
> 
> The first 17 patches have already been through two spins and
> this time are corrected for all the comment (thanks to
> everybody who commented) since v2, and have tested-by /
> acked-bys applied.  The first workaround patch for the hash
> function cast problem is dropped since something has already
> been applied in master since yesterday to address it.
> 
> The additional 23 patches are fixes for problems found
> actually trying to build lagopus using current master.
> These are almost entirely related to signed / unsigned
> or truncation without explicit casts inside dpdk
> headers.
> 
> ---
> 
> Andy Green (40):
>       drivers/bus/pci: fix strncpy dangerous code
>       drivers/bus/dpaa: fix inconsistent struct alignment
>       drivers/net/axgbe: fix broken eeprom string comp
>       drivers/net/nfp/nfpcore: fix strncpy misuse
>       drivers/net/nfp/nfpcore: fix off-by-one and no NUL on strncpy
> use
>       drivers/net/nfp: don't memcpy out of source range
>       drivers/net/nfp: fix buffer overflow in fw_name
>       drivers/net/qede: fix strncpy constant and NUL
>       drivers/net/qede: fix broken strncpy
>       drivers/net/sfc: fix strncpy length
>       drivers/net/sfc: fix strncpy size and NUL
>       drivers/net/vdev: readlink inputs cannot be aliased
>       drivers/net/vdev: fix 3 x strncpy misuse
>       app/test-pmd: can't find include
>       app/proc-info: fix sprintf overrun bug
>       app/test-bbdev: test-bbdev: strcpy ok for allocated string
>       app/test-bbdev: strcpy ok for allocated string
>       rte_common.h: cast gcc builtin result to avoid complaints
>       rte_memcpy.h: explicit tmp cast
>       lib/librte_eal/common/include/rte_lcore.h: explicit cast for
> signed change
>       /lib/librte_eal/common/include/rte_random.h: stage cast from
> uint64_t to long
>       rte_spinlock.h: stack declarations before code
>       rte_ring_generic.h: stack declarations before code
>       rte_ring.h: remove signed type flipflopping
>       rte_dev.h: stack declaration at top of own basic block
>       rte_mbuf.h: avoid truncation warnings from inadvertant int16_t
> to int promotion
>       rte_mbuf.h: explicit casts for flipping between int16_t and
> uint16_t
>       rte_mbuf.h: make sure RTE_MIN compares same types
>       rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
>       rte_mbuf.h: explicit cast for size_t to uint32_t
>       rte_mbuf.h: explicit casts to uint16_t to avoid truncation
> warnings
>       rte_byteorder.h: explicit cast for return promotion
>       rte_ether.h: explicit cast avoiding truncation warning
>       rte_ether.h: stack vars declared at top of function
>       rte_ethdev.h: fix sign and scope of temp var
>       rte_ethdev.h: explicit cast for return type
>       rte_ethdev.h: explicit cast for truncation
>       rte_hash_crc.h: stack vars declared at top of function
>       rte_hash_crc.h: explicit casts for truncation
>       rte_string_fns.h: explicit cast for int return to size_t

Hi,

I've built-tested this series on Debian Stretch (gcc 6.3) and Debian
Sid (gcc 8.1).

The series builds fine with the default config, but the bnx2x and mlx5
PMDs still have errors with gcc-8:

/tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function 'bnx2x_alloc_hsi_mem':
/tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s' directive writing up to 31 bytes into a region of size between 15 and 25 [-Werror=format-overflow=]
   sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
                             ^~
/tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
   if (bnx2x_dma_alloc(sc, sizeof(union bnx2x_host_hc_status_block),
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf' output between 10 and 66 bytes into a destination of size 32
   sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    rte_get_timer_cycles());
    ~~~~~~~~~~~~~~~~~~~~~~~
/tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s' directive writing up to 31 bytes into a region of size between 23 and 25 [-Werror=format-overflow=]
   sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
                             ^~
/tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
   if (bnx2x_dma_alloc(sc, sizeof(union bnx2x_host_hc_status_block),
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf' output between 10 and 58 bytes into a destination of size 32
   sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    rte_get_timer_cycles());


/tmp/dpdk/drivers/net/mlx5/mlx5.c: In function 'mlx5_pci_probe':
/tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   config.vf = vf;

Hope this can be useful.

-- 
Kind regards,
Luca Boccassi

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 05:11 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 10 May 2018 14:46:42 +0800
>> From: Andy Green <andy@warmcat.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>   Thunderbird/52.7.0
>>
>>
>>
>> On 05/10/2018 02:17 PM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Thu, 10 May 2018 10:46:18 +0800
>>>> From: Andy Green <andy@warmcat.com>
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>>>> User-Agent: StGit/unknown-version
>>>>
>>> ./devtools/check-git-log.sh
>>
>> Ugh...
>>
>> Wrong headline format:
>> 	drivers/net/nfp: fix buffer overflow in fw_name
>>
>> ... snip something "wrong" about every patch title...
>>
>> It's just doing this
>>
>> # check headline format (spacing, no punctuation, no code)
>> bad=$(echo "$headlines" | grep --color=always \
>>          -e '    ' \
>>          -e '^ ' \
>>          -e ' $' \
>>          -e '\.$' \
>>          -e '[,;!?&|]' \
>>          -e ':.*_' \
>>          -e '^[^:]\+$' \
>>          -e ':[^ ]' \
>>          -e ' :' \
>>          | sed 's,^,\t,')
>> [ -z "$bad" ] || printf "Wrong headline format:\n$bad\n"
>>
>> It probably seems to whoever wrote it that adds "quality", but actually
>> inflexible rules like this do nothing to help quality of the patch payload
>> and actively put off contribution.
>>
>> So on this first one it's hitting the rule ':.*_', ie, this project believes
>> there should never be a patch title mentioning anything with an underscore
>> after a colon.
>>
>> Can you help me understand in what way banning mentioning relevant strings
>> in the patch title is a good idea?  It's actively reducing the value of the
>> patch title, isn't it?
> 
> I think, the underscore check is to make sure that the subject should not have
> C symbols.

Right, that seems to be the intention.

But if the patch is entirely about doing something to a specific C 
symbol or function, it's not a bad thing if it mentions that in the 
title is it?  In itself, most projects would consider it a good thing to 
concisely explain what it's doing.  Eg, "fix NULL pointer exception in 
my_function if unconfigured" is illegal for this project.  It's strange 
actually.

I don't understand what negative outcome the check is saving us from. 
If nothing, maybe it should be patched to not do that any more.

> Change to following will work:
> 
> net/nfp: fix buffer overflow

Sure, I studied the script to find out what its problem was.  I just 
don't think its problem is reasonable.

If nobody cares, sure I will go through removing useful information from 
my patch titles to keep this script happy.

-Andy

>>
>> -Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 05:52 PM, De Lara Guarch, Pablo wrote:
> Hi Andy,
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
>> Sent: Thursday, May 10, 2018 3:46 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>>
>> The following series gets current master able to build itself, and allow lagopus to
>> build against it, on Fedora 28 +
>> x86_64 using gcc 8.0.1.
>>
>> The first 17 patches have already been through two spins and this time are
>> corrected for all the comment (thanks to everybody who commented) since v2,
>> and have tested-by / acked-bys applied.  The first workaround patch for the hash
>> function cast problem is dropped since something has already been applied in
>> master since yesterday to address it.
>>
>> The additional 23 patches are fixes for problems found actually trying to build
>> lagopus using current master.
>> These are almost entirely related to signed / unsigned or truncation without
>> explicit casts inside dpdk headers.
> 
> I think it would be a good idea to split this patchset into two.

OK, but...

> One for the gcc 8 fixes, and another one for the rest of them.
> 
> We should first focus on fixes for just DPDK, knowing that the release is close.
> 
> Could you also share what's triggering the other issues, when ou build lagopus?

... gcc8.0.1 on Fedora 28 the same.

I just force their dpdk submodule to dpdk master + my series and build 
with defaults.  I'm not doing anything to make it more strict or nondefault.

The quoted compiler warnings and errors are coming from lagopus actually 
using dpdk with x86_64 + gcc8.

I have a pile of lagopus fixes also required to get this far, that pile 
is still growing.

-Andy

> Thanks,
> Pablo
> 

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Jerin Jacob]

-----Original Message-----
> Date: Thu, 10 May 2018 19:44:34 +0800
> From: Andy Green <andy@warmcat.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>  Thunderbird/52.7.0
> 
> 
> 
> On 05/10/2018 05:11 PM, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Thu, 10 May 2018 14:46:42 +0800
> > > From: Andy Green <andy@warmcat.com>
> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > CC: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> > > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
> > >   Thunderbird/52.7.0
> > > 
> > > 
> > > 
> > > On 05/10/2018 02:17 PM, Jerin Jacob wrote:
> > > > -----Original Message-----
> > > > > Date: Thu, 10 May 2018 10:46:18 +0800
> > > > > From: Andy Green <andy@warmcat.com>
> > > > > To: dev@dpdk.org
> > > > > Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
> > > > > User-Agent: StGit/unknown-version
> > > > > 
> > > > ./devtools/check-git-log.sh
> > > 
> > > Ugh...
> > > 
> > > Wrong headline format:
> > > 	drivers/net/nfp: fix buffer overflow in fw_name
> > > 
> > > ... snip something "wrong" about every patch title...
> > > 
> > > It's just doing this
> > > 
> > > # check headline format (spacing, no punctuation, no code)
> > > bad=$(echo "$headlines" | grep --color=always \
> > >          -e '    ' \
> > >          -e '^ ' \
> > >          -e ' $' \
> > >          -e '\.$' \
> > >          -e '[,;!?&|]' \
> > >          -e ':.*_' \
> > >          -e '^[^:]\+$' \
> > >          -e ':[^ ]' \
> > >          -e ' :' \
> > >          | sed 's,^,\t,')
> > > [ -z "$bad" ] || printf "Wrong headline format:\n$bad\n"
> > > 
> > > It probably seems to whoever wrote it that adds "quality", but actually
> > > inflexible rules like this do nothing to help quality of the patch payload
> > > and actively put off contribution.
> > > 
> > > So on this first one it's hitting the rule ':.*_', ie, this project believes
> > > there should never be a patch title mentioning anything with an underscore
> > > after a colon.
> > > 
> > > Can you help me understand in what way banning mentioning relevant strings
> > > in the patch title is a good idea?  It's actively reducing the value of the
> > > patch title, isn't it?
> > 
> > I think, the underscore check is to make sure that the subject should not have
> > C symbols.
> 
> Right, that seems to be the intention.
> 
> But if the patch is entirely about doing something to a specific C symbol or
> function, it's not a bad thing if it mentions that in the title is it?  In
> itself, most projects would consider it a good thing to concisely explain
> what it's doing.  Eg, "fix NULL pointer exception in my_function if
> unconfigured" is illegal for this project.  It's strange actually.

I think, the rational is
# In subject you have minimal information
# In commit log, you can have DETAILED info

That translated to following in your example:

module: fix a NULL pointer exception

fix a NULL pointer exception in my_function if unconfigured due to
so and so reason

> 
> I don't understand what negative outcome the check is saving us from. If
> nothing, maybe it should be patched to not do that any more.
> 
> > Change to following will work:
> > 
> > net/nfp: fix buffer overflow
> 
> Sure, I studied the script to find out what its problem was.  I just don't
> think its problem is reasonable.
> 
> If nobody cares, sure I will go through removing useful information from my
> patch titles to keep this script happy.

IMO, Keep all useful information in description not in subject.

> 
> -Andy
> 
> > > 
> > > -Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 07:58 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 10 May 2018 19:44:34 +0800
>> From: Andy Green <andy@warmcat.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>   Thunderbird/52.7.0
>>
>>
>>
>> On 05/10/2018 05:11 PM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Thu, 10 May 2018 14:46:42 +0800
>>>> From: Andy Green <andy@warmcat.com>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> CC: dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>>>    Thunderbird/52.7.0
>>>>
>>>>
>>>>
>>>> On 05/10/2018 02:17 PM, Jerin Jacob wrote:
>>>>> -----Original Message-----
>>>>>> Date: Thu, 10 May 2018 10:46:18 +0800
>>>>>> From: Andy Green <andy@warmcat.com>
>>>>>> To: dev@dpdk.org
>>>>>> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>>>>>> User-Agent: StGit/unknown-version
>>>>>>
>>>>> ./devtools/check-git-log.sh
>>>>
>>>> Ugh...
>>>>
>>>> Wrong headline format:
>>>> 	drivers/net/nfp: fix buffer overflow in fw_name
>>>>
>>>> ... snip something "wrong" about every patch title...
>>>>
>>>> It's just doing this
>>>>
>>>> # check headline format (spacing, no punctuation, no code)
>>>> bad=$(echo "$headlines" | grep --color=always \
>>>>           -e '    ' \
>>>>           -e '^ ' \
>>>>           -e ' $' \
>>>>           -e '\.$' \
>>>>           -e '[,;!?&|]' \
>>>>           -e ':.*_' \
>>>>           -e '^[^:]\+$' \
>>>>           -e ':[^ ]' \
>>>>           -e ' :' \
>>>>           | sed 's,^,\t,')
>>>> [ -z "$bad" ] || printf "Wrong headline format:\n$bad\n"
>>>>
>>>> It probably seems to whoever wrote it that adds "quality", but actually
>>>> inflexible rules like this do nothing to help quality of the patch payload
>>>> and actively put off contribution.
>>>>
>>>> So on this first one it's hitting the rule ':.*_', ie, this project believes
>>>> there should never be a patch title mentioning anything with an underscore
>>>> after a colon.
>>>>
>>>> Can you help me understand in what way banning mentioning relevant strings
>>>> in the patch title is a good idea?  It's actively reducing the value of the
>>>> patch title, isn't it?
>>>
>>> I think, the underscore check is to make sure that the subject should not have
>>> C symbols.
>>
>> Right, that seems to be the intention.
>>
>> But if the patch is entirely about doing something to a specific C symbol or
>> function, it's not a bad thing if it mentions that in the title is it?  In
>> itself, most projects would consider it a good thing to concisely explain
>> what it's doing.  Eg, "fix NULL pointer exception in my_function if
>> unconfigured" is illegal for this project.  It's strange actually.
> 
> I think, the rational is
> # In subject you have minimal information
> # In commit log, you can have DETAILED info
> 
> That translated to following in your example:
> 
> module: fix a NULL pointer exception
> 
> fix a NULL pointer exception in my_function if unconfigured due to
> so and so reason
> 
>>
>> I don't understand what negative outcome the check is saving us from. If
>> nothing, maybe it should be patched to not do that any more.
>>
>>> Change to following will work:
>>>
>>> net/nfp: fix buffer overflow
>>
>> Sure, I studied the script to find out what its problem was.  I just don't
>> think its problem is reasonable.
>>
>> If nobody cares, sure I will go through removing useful information from my
>> patch titles to keep this script happy.
> 
> IMO, Keep all useful information in description not in subject.

I appreciate the reply.

But why bother having a subject line at all if it is going to be 
mechanically enforced that nothing in it is allowed to be "useful"? 
That really doesn't make sense does it.

Stuff like line-length enforcement automatically is generally good I 
think, whitespace checking is also good.  Obviously compilers, lint, 
coverity making complaints are all good, and the tools can judge the 
things they look at better than I can most times.

But what is expressive, concise, meaningful or "useful" in a commit 
subject line can't be judged by grep IMHO.

Anyway no worries as I say if nobody cares in the next try I will nobly 
save the project from being contaminated by anything useful in my 
subject lines.

-Andy

>>
>> -Andy
>>
>>>>
>>>> -Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 06:21 PM, Luca Boccassi wrote:
> On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
>> The following series gets current master able to build
>> itself, and allow lagopus to build against it, on Fedora 28 +
>> x86_64 using gcc 8.0.1.
>>
>> The first 17 patches have already been through two spins and
>> this time are corrected for all the comment (thanks to
>> everybody who commented) since v2, and have tested-by /
>> acked-bys applied.  The first workaround patch for the hash
>> function cast problem is dropped since something has already
>> been applied in master since yesterday to address it.
>>
>> The additional 23 patches are fixes for problems found
>> actually trying to build lagopus using current master.
>> These are almost entirely related to signed / unsigned
>> or truncation without explicit casts inside dpdk
>> headers.
>>
>> ---
>>
>> Andy Green (40):
>>        drivers/bus/pci: fix strncpy dangerous code
>>        drivers/bus/dpaa: fix inconsistent struct alignment
>>        drivers/net/axgbe: fix broken eeprom string comp
>>        drivers/net/nfp/nfpcore: fix strncpy misuse
>>        drivers/net/nfp/nfpcore: fix off-by-one and no NUL on strncpy
>> use
>>        drivers/net/nfp: don't memcpy out of source range
>>        drivers/net/nfp: fix buffer overflow in fw_name
>>        drivers/net/qede: fix strncpy constant and NUL
>>        drivers/net/qede: fix broken strncpy
>>        drivers/net/sfc: fix strncpy length
>>        drivers/net/sfc: fix strncpy size and NUL
>>        drivers/net/vdev: readlink inputs cannot be aliased
>>        drivers/net/vdev: fix 3 x strncpy misuse
>>        app/test-pmd: can't find include
>>        app/proc-info: fix sprintf overrun bug
>>        app/test-bbdev: test-bbdev: strcpy ok for allocated string
>>        app/test-bbdev: strcpy ok for allocated string
>>        rte_common.h: cast gcc builtin result to avoid complaints
>>        rte_memcpy.h: explicit tmp cast
>>        lib/librte_eal/common/include/rte_lcore.h: explicit cast for
>> signed change
>>        /lib/librte_eal/common/include/rte_random.h: stage cast from
>> uint64_t to long
>>        rte_spinlock.h: stack declarations before code
>>        rte_ring_generic.h: stack declarations before code
>>        rte_ring.h: remove signed type flipflopping
>>        rte_dev.h: stack declaration at top of own basic block
>>        rte_mbuf.h: avoid truncation warnings from inadvertant int16_t
>> to int promotion
>>        rte_mbuf.h: explicit casts for flipping between int16_t and
>> uint16_t
>>        rte_mbuf.h: make sure RTE_MIN compares same types
>>        rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
>>        rte_mbuf.h: explicit cast for size_t to uint32_t
>>        rte_mbuf.h: explicit casts to uint16_t to avoid truncation
>> warnings
>>        rte_byteorder.h: explicit cast for return promotion
>>        rte_ether.h: explicit cast avoiding truncation warning
>>        rte_ether.h: stack vars declared at top of function
>>        rte_ethdev.h: fix sign and scope of temp var
>>        rte_ethdev.h: explicit cast for return type
>>        rte_ethdev.h: explicit cast for truncation
>>        rte_hash_crc.h: stack vars declared at top of function
>>        rte_hash_crc.h: explicit casts for truncation
>>        rte_string_fns.h: explicit cast for int return to size_t
> 
> Hi,
> 
> I've built-tested this series on Debian Stretch (gcc 6.3) and Debian
> Sid (gcc 8.1).
> 
> The series builds fine with the default config, but the bnx2x and mlx5
> PMDs still have errors with gcc-8:

Yes I just built it with defconfig for x86_64 on Fedora 28 with default 
tools and cleared out everything that came up.

> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function 'bnx2x_alloc_hsi_mem':
> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s' directive writing up to 31 bytes into a region of size between 15 and 25 [-Werror=format-overflow=]
>     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
>                               ^~
> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>     if (bnx2x_dma_alloc(sc, sizeof(union bnx2x_host_hc_status_block),
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf' output between 10 and 66 bytes into a destination of size 32
>     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      rte_get_timer_cycles());
>      ~~~~~~~~~~~~~~~~~~~~~~~
> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s' directive writing up to 31 bytes into a region of size between 23 and 25 [-Werror=format-overflow=]
>     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
>                               ^~
> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>     if (bnx2x_dma_alloc(sc, sizeof(union bnx2x_host_hc_status_block),
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf' output between 10 and 58 bytes into a destination of size 32
>     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      rte_get_timer_cycles());
> 
> 
> /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function 'mlx5_pci_probe':
> /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     config.vf = vf;
> 
> Hope this can be useful.

I think gcc 8.0.1 is capable to show that and I am willing to look at 
them.  But can you help me with exactly what changes you made so these 
things built and made trouble, compared to the defconfig I have used 
until now?

-Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Luca Boccassi]

On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote:
> 
> On 05/10/2018 06:21 PM, Luca Boccassi wrote:
> > On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
> > > The following series gets current master able to build
> > > itself, and allow lagopus to build against it, on Fedora 28 +
> > > x86_64 using gcc 8.0.1.
> > > 
> > > The first 17 patches have already been through two spins and
> > > this time are corrected for all the comment (thanks to
> > > everybody who commented) since v2, and have tested-by /
> > > acked-bys applied.  The first workaround patch for the hash
> > > function cast problem is dropped since something has already
> > > been applied in master since yesterday to address it.
> > > 
> > > The additional 23 patches are fixes for problems found
> > > actually trying to build lagopus using current master.
> > > These are almost entirely related to signed / unsigned
> > > or truncation without explicit casts inside dpdk
> > > headers.
> > > 
> > > ---
> > > 
> > > Andy Green (40):
> > >        drivers/bus/pci: fix strncpy dangerous code
> > >        drivers/bus/dpaa: fix inconsistent struct alignment
> > >        drivers/net/axgbe: fix broken eeprom string comp
> > >        drivers/net/nfp/nfpcore: fix strncpy misuse
> > >        drivers/net/nfp/nfpcore: fix off-by-one and no NUL on
> > > strncpy
> > > use
> > >        drivers/net/nfp: don't memcpy out of source range
> > >        drivers/net/nfp: fix buffer overflow in fw_name
> > >        drivers/net/qede: fix strncpy constant and NUL
> > >        drivers/net/qede: fix broken strncpy
> > >        drivers/net/sfc: fix strncpy length
> > >        drivers/net/sfc: fix strncpy size and NUL
> > >        drivers/net/vdev: readlink inputs cannot be aliased
> > >        drivers/net/vdev: fix 3 x strncpy misuse
> > >        app/test-pmd: can't find include
> > >        app/proc-info: fix sprintf overrun bug
> > >        app/test-bbdev: test-bbdev: strcpy ok for allocated string
> > >        app/test-bbdev: strcpy ok for allocated string
> > >        rte_common.h: cast gcc builtin result to avoid complaints
> > >        rte_memcpy.h: explicit tmp cast
> > >        lib/librte_eal/common/include/rte_lcore.h: explicit cast
> > > for
> > > signed change
> > >        /lib/librte_eal/common/include/rte_random.h: stage cast
> > > from
> > > uint64_t to long
> > >        rte_spinlock.h: stack declarations before code
> > >        rte_ring_generic.h: stack declarations before code
> > >        rte_ring.h: remove signed type flipflopping
> > >        rte_dev.h: stack declaration at top of own basic block
> > >        rte_mbuf.h: avoid truncation warnings from inadvertant
> > > int16_t
> > > to int promotion
> > >        rte_mbuf.h: explicit casts for flipping between int16_t
> > > and
> > > uint16_t
> > >        rte_mbuf.h: make sure RTE_MIN compares same types
> > >        rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
> > >        rte_mbuf.h: explicit cast for size_t to uint32_t
> > >        rte_mbuf.h: explicit casts to uint16_t to avoid truncation
> > > warnings
> > >        rte_byteorder.h: explicit cast for return promotion
> > >        rte_ether.h: explicit cast avoiding truncation warning
> > >        rte_ether.h: stack vars declared at top of function
> > >        rte_ethdev.h: fix sign and scope of temp var
> > >        rte_ethdev.h: explicit cast for return type
> > >        rte_ethdev.h: explicit cast for truncation
> > >        rte_hash_crc.h: stack vars declared at top of function
> > >        rte_hash_crc.h: explicit casts for truncation
> > >        rte_string_fns.h: explicit cast for int return to size_t
> > 
> > Hi,
> > 
> > I've built-tested this series on Debian Stretch (gcc 6.3) and
> > Debian
> > Sid (gcc 8.1).
> > 
> > The series builds fine with the default config, but the bnx2x and
> > mlx5
> > PMDs still have errors with gcc-8:
> 
> Yes I just built it with defconfig for x86_64 on Fedora 28 with
> default 
> tools and cleared out everything that came up.
> 
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function
> > 'bnx2x_alloc_hsi_mem':
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s' directive
> > writing up to 31 bytes into a region of size between 15 and 25 [-
> > Werror=format-overflow=]
> >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
> >                               ^~
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
> >     if (bnx2x_dma_alloc(sc, sizeof(union
> > bnx2x_host_hc_status_block),
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ~~
> >         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf' output
> > between 10 and 66 bytes into a destination of size 32
> >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >      rte_get_timer_cycles());
> >      ~~~~~~~~~~~~~~~~~~~~~~~
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s' directive
> > writing up to 31 bytes into a region of size between 23 and 25 [-
> > Werror=format-overflow=]
> >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
> >                               ^~
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
> >     if (bnx2x_dma_alloc(sc, sizeof(union
> > bnx2x_host_hc_status_block),
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ~~
> >         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf' output
> > between 10 and 58 bytes into a destination of size 32
> >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >      rte_get_timer_cycles());
> > 
> > 
> > /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function 'mlx5_pci_probe':
> > /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be used
> > uninitialized in this function [-Werror=maybe-uninitialized]
> >     config.vf = vf;
> > 
> > Hope this can be useful.
> 
> I think gcc 8.0.1 is capable to show that and I am willing to look
> at 
> them.  But can you help me with exactly what changes you made so
> these 
> things built and made trouble, compared to the defconfig I have used 
> until now?

If you already have a build directory you are using, the simplest way
is to edit the .config file in there and change the following from =n
to =y:

CONFIG_RTE_LIBRTE_MLX4_PMD
CONFIG_RTE_LIBRTE_MLX5_PMD
CONFIG_RTE_LIBRTE_BNX2X_PMD

Then rebuild and you should see the errors.

-- 
Kind regards,
Luca Boccassi

Re: [PATCH v3 01/40] drivers/bus/pci: fix strncpy dangerous code

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Thursday, May 10, 2018 3:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 01/40] drivers/bus/pci: fix strncpy dangerous
> code
> 
> In function ‘pci_get_kernel_driver_by_path’,
>     inlined from ‘pci_scan_one.isra.1’ at /home/agreen/projects/dpdk/
> 	drivers/bus/pci/linux/pci.c:317:8:
> /home/agreen/projects/dpdk/drivers/bus/pci/linux/pci.c:57:3: error:
> ‘strncpy’ specified bound depends on the length of the source argument [-
> Werror=stringop-overflow=]
>    strncpy(dri_name, name + 1, strlen(name + 1) + 1);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Bruce Richardson]

On Thu, May 10, 2018 at 01:35:49PM +0100, Luca Boccassi wrote:
> On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote:
> > 
> > On 05/10/2018 06:21 PM, Luca Boccassi wrote:
> > > On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
> > > > The following series gets current master able to build
> > > > itself, and allow lagopus to build against it, on Fedora 28 +
> > > > x86_64 using gcc 8.0.1.
> > > > 
> > > > The first 17 patches have already been through two spins and
> > > > this time are corrected for all the comment (thanks to
> > > > everybody who commented) since v2, and have tested-by /
> > > > acked-bys applied.  The first workaround patch for the hash
> > > > function cast problem is dropped since something has already
> > > > been applied in master since yesterday to address it.
> > > > 
> > > > The additional 23 patches are fixes for problems found
> > > > actually trying to build lagopus using current master.
> > > > These are almost entirely related to signed / unsigned
> > > > or truncation without explicit casts inside dpdk
> > > > headers.
> > > > 
> > > > ---
> > > > 
> > > > Andy Green (40):
> > > >        drivers/bus/pci: fix strncpy dangerous code
> > > >        drivers/bus/dpaa: fix inconsistent struct alignment
> > > >        drivers/net/axgbe: fix broken eeprom string comp
> > > >        drivers/net/nfp/nfpcore: fix strncpy misuse
> > > >        drivers/net/nfp/nfpcore: fix off-by-one and no NUL on
> > > > strncpy
> > > > use
> > > >        drivers/net/nfp: don't memcpy out of source range
> > > >        drivers/net/nfp: fix buffer overflow in fw_name
> > > >        drivers/net/qede: fix strncpy constant and NUL
> > > >        drivers/net/qede: fix broken strncpy
> > > >        drivers/net/sfc: fix strncpy length
> > > >        drivers/net/sfc: fix strncpy size and NUL
> > > >        drivers/net/vdev: readlink inputs cannot be aliased
> > > >        drivers/net/vdev: fix 3 x strncpy misuse
> > > >        app/test-pmd: can't find include
> > > >        app/proc-info: fix sprintf overrun bug
> > > >        app/test-bbdev: test-bbdev: strcpy ok for allocated string
> > > >        app/test-bbdev: strcpy ok for allocated string
> > > >        rte_common.h: cast gcc builtin result to avoid complaints
> > > >        rte_memcpy.h: explicit tmp cast
> > > >        lib/librte_eal/common/include/rte_lcore.h: explicit cast
> > > > for
> > > > signed change
> > > >        /lib/librte_eal/common/include/rte_random.h: stage cast
> > > > from
> > > > uint64_t to long
> > > >        rte_spinlock.h: stack declarations before code
> > > >        rte_ring_generic.h: stack declarations before code
> > > >        rte_ring.h: remove signed type flipflopping
> > > >        rte_dev.h: stack declaration at top of own basic block
> > > >        rte_mbuf.h: avoid truncation warnings from inadvertant
> > > > int16_t
> > > > to int promotion
> > > >        rte_mbuf.h: explicit casts for flipping between int16_t
> > > > and
> > > > uint16_t
> > > >        rte_mbuf.h: make sure RTE_MIN compares same types
> > > >        rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
> > > >        rte_mbuf.h: explicit cast for size_t to uint32_t
> > > >        rte_mbuf.h: explicit casts to uint16_t to avoid truncation
> > > > warnings
> > > >        rte_byteorder.h: explicit cast for return promotion
> > > >        rte_ether.h: explicit cast avoiding truncation warning
> > > >        rte_ether.h: stack vars declared at top of function
> > > >        rte_ethdev.h: fix sign and scope of temp var
> > > >        rte_ethdev.h: explicit cast for return type
> > > >        rte_ethdev.h: explicit cast for truncation
> > > >        rte_hash_crc.h: stack vars declared at top of function
> > > >        rte_hash_crc.h: explicit casts for truncation
> > > >        rte_string_fns.h: explicit cast for int return to size_t
> > > 
> > > Hi,
> > > 
> > > I've built-tested this series on Debian Stretch (gcc 6.3) and
> > > Debian
> > > Sid (gcc 8.1).
> > > 
> > > The series builds fine with the default config, but the bnx2x and
> > > mlx5
> > > PMDs still have errors with gcc-8:
> > 
> > Yes I just built it with defconfig for x86_64 on Fedora 28 with
> > default 
> > tools and cleared out everything that came up.
> > 
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function
> > > 'bnx2x_alloc_hsi_mem':
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s' directive
> > > writing up to 31 bytes into a region of size between 15 and 25 [-
> > > Werror=format-overflow=]
> > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
> > >                               ^~
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
> > >     if (bnx2x_dma_alloc(sc, sizeof(union
> > > bnx2x_host_hc_status_block),
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ~~
> > >         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf' output
> > > between 10 and 66 bytes into a destination of size 32
> > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
> > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >      rte_get_timer_cycles());
> > >      ~~~~~~~~~~~~~~~~~~~~~~~
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s' directive
> > > writing up to 31 bytes into a region of size between 23 and 25 [-
> > > Werror=format-overflow=]
> > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
> > >                               ^~
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
> > >     if (bnx2x_dma_alloc(sc, sizeof(union
> > > bnx2x_host_hc_status_block),
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ~~
> > >         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
> > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf' output
> > > between 10 and 58 bytes into a destination of size 32
> > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
> > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >      rte_get_timer_cycles());
> > > 
> > > 
> > > /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function 'mlx5_pci_probe':
> > > /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be used
> > > uninitialized in this function [-Werror=maybe-uninitialized]
> > >     config.vf = vf;
> > > 
> > > Hope this can be useful.
> > 
> > I think gcc 8.0.1 is capable to show that and I am willing to look
> > at 
> > them.  But can you help me with exactly what changes you made so
> > these 
> > things built and made trouble, compared to the defconfig I have used 
> > until now?
> 
> If you already have a build directory you are using, the simplest way
> is to edit the .config file in there and change the following from =n
> to =y:
> 
> CONFIG_RTE_LIBRTE_MLX4_PMD
> CONFIG_RTE_LIBRTE_MLX5_PMD
> CONFIG_RTE_LIBRTE_BNX2X_PMD
> 
> Then rebuild and you should see the errors.
> 
Personally, though I wouldn't view it as necessary to get those extra fixes
into this set. The set is big enough as it is, so I'd like to see the
existing gcc 8 fixes we have merged to make some progress, rather than
constantly spinning ever bigger sets to try and fix them all in one go.

My 2c.

/Bruce

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Luca Boccassi]

On Thu, 2018-05-10 at 14:36 +0100, Bruce Richardson wrote:
> On Thu, May 10, 2018 at 01:35:49PM +0100, Luca Boccassi wrote:
> > On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote:
> > > 
> > > On 05/10/2018 06:21 PM, Luca Boccassi wrote:
> > > > On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
> > > > > The following series gets current master able to build
> > > > > itself, and allow lagopus to build against it, on Fedora 28 +
> > > > > x86_64 using gcc 8.0.1.
> > > > > 
> > > > > The first 17 patches have already been through two spins and
> > > > > this time are corrected for all the comment (thanks to
> > > > > everybody who commented) since v2, and have tested-by /
> > > > > acked-bys applied.  The first workaround patch for the hash
> > > > > function cast problem is dropped since something has already
> > > > > been applied in master since yesterday to address it.
> > > > > 
> > > > > The additional 23 patches are fixes for problems found
> > > > > actually trying to build lagopus using current master.
> > > > > These are almost entirely related to signed / unsigned
> > > > > or truncation without explicit casts inside dpdk
> > > > > headers.
> > > > > 
> > > > > ---
> > > > > 
> > > > > Andy Green (40):
> > > > >        drivers/bus/pci: fix strncpy dangerous code
> > > > >        drivers/bus/dpaa: fix inconsistent struct alignment
> > > > >        drivers/net/axgbe: fix broken eeprom string comp
> > > > >        drivers/net/nfp/nfpcore: fix strncpy misuse
> > > > >        drivers/net/nfp/nfpcore: fix off-by-one and no NUL on
> > > > > strncpy
> > > > > use
> > > > >        drivers/net/nfp: don't memcpy out of source range
> > > > >        drivers/net/nfp: fix buffer overflow in fw_name
> > > > >        drivers/net/qede: fix strncpy constant and NUL
> > > > >        drivers/net/qede: fix broken strncpy
> > > > >        drivers/net/sfc: fix strncpy length
> > > > >        drivers/net/sfc: fix strncpy size and NUL
> > > > >        drivers/net/vdev: readlink inputs cannot be aliased
> > > > >        drivers/net/vdev: fix 3 x strncpy misuse
> > > > >        app/test-pmd: can't find include
> > > > >        app/proc-info: fix sprintf overrun bug
> > > > >        app/test-bbdev: test-bbdev: strcpy ok for allocated
> > > > > string
> > > > >        app/test-bbdev: strcpy ok for allocated string
> > > > >        rte_common.h: cast gcc builtin result to avoid
> > > > > complaints
> > > > >        rte_memcpy.h: explicit tmp cast
> > > > >        lib/librte_eal/common/include/rte_lcore.h: explicit
> > > > > cast
> > > > > for
> > > > > signed change
> > > > >        /lib/librte_eal/common/include/rte_random.h: stage
> > > > > cast
> > > > > from
> > > > > uint64_t to long
> > > > >        rte_spinlock.h: stack declarations before code
> > > > >        rte_ring_generic.h: stack declarations before code
> > > > >        rte_ring.h: remove signed type flipflopping
> > > > >        rte_dev.h: stack declaration at top of own basic block
> > > > >        rte_mbuf.h: avoid truncation warnings from inadvertant
> > > > > int16_t
> > > > > to int promotion
> > > > >        rte_mbuf.h: explicit casts for flipping between
> > > > > int16_t
> > > > > and
> > > > > uint16_t
> > > > >        rte_mbuf.h: make sure RTE_MIN compares same types
> > > > >        rte_mbuf.h: explicit cast restricting ptrdiff to
> > > > > uint16_t
> > > > >        rte_mbuf.h: explicit cast for size_t to uint32_t
> > > > >        rte_mbuf.h: explicit casts to uint16_t to avoid
> > > > > truncation
> > > > > warnings
> > > > >        rte_byteorder.h: explicit cast for return promotion
> > > > >        rte_ether.h: explicit cast avoiding truncation warning
> > > > >        rte_ether.h: stack vars declared at top of function
> > > > >        rte_ethdev.h: fix sign and scope of temp var
> > > > >        rte_ethdev.h: explicit cast for return type
> > > > >        rte_ethdev.h: explicit cast for truncation
> > > > >        rte_hash_crc.h: stack vars declared at top of function
> > > > >        rte_hash_crc.h: explicit casts for truncation
> > > > >        rte_string_fns.h: explicit cast for int return to
> > > > > size_t
> > > > 
> > > > Hi,
> > > > 
> > > > I've built-tested this series on Debian Stretch (gcc 6.3) and
> > > > Debian
> > > > Sid (gcc 8.1).
> > > > 
> > > > The series builds fine with the default config, but the bnx2x
> > > > and
> > > > mlx5
> > > > PMDs still have errors with gcc-8:
> > > 
> > > Yes I just built it with defconfig for x86_64 on Fedora 28 with
> > > default 
> > > tools and cleared out everything that came up.
> > > 
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function
> > > > 'bnx2x_alloc_hsi_mem':
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s'
> > > > directive
> > > > writing up to 31 bytes into a region of size between 15 and 25
> > > > [-
> > > > Werror=format-overflow=]
> > > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device,
> > > > msg,
> > > >                               ^~
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
> > > >     if (bnx2x_dma_alloc(sc, sizeof(union
> > > > bnx2x_host_hc_status_block),
> > > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ~~~~
> > > > ~~
> > > >         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
> > > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf'
> > > > output
> > > > between 10 and 66 bytes into a destination of size 32
> > > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device,
> > > > msg,
> > > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ~~
> > > >      rte_get_timer_cycles());
> > > >      ~~~~~~~~~~~~~~~~~~~~~~~
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s'
> > > > directive
> > > > writing up to 31 bytes into a region of size between 23 and 25
> > > > [-
> > > > Werror=format-overflow=]
> > > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc),
> > > > msg,
> > > >                               ^~
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
> > > >     if (bnx2x_dma_alloc(sc, sizeof(union
> > > > bnx2x_host_hc_status_block),
> > > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ~~~~
> > > > ~~
> > > >         &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
> > > >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf'
> > > > output
> > > > between 10 and 58 bytes into a destination of size 32
> > > >     sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc),
> > > > msg,
> > > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ~~
> > > >      rte_get_timer_cycles());
> > > > 
> > > > 
> > > > /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function
> > > > 'mlx5_pci_probe':
> > > > /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be
> > > > used
> > > > uninitialized in this function [-Werror=maybe-uninitialized]
> > > >     config.vf = vf;
> > > > 
> > > > Hope this can be useful.
> > > 
> > > I think gcc 8.0.1 is capable to show that and I am willing to
> > > look
> > > at 
> > > them.  But can you help me with exactly what changes you made so
> > > these 
> > > things built and made trouble, compared to the defconfig I have
> > > used 
> > > until now?
> > 
> > If you already have a build directory you are using, the simplest
> > way
> > is to edit the .config file in there and change the following from
> > =n
> > to =y:
> > 
> > CONFIG_RTE_LIBRTE_MLX4_PMD
> > CONFIG_RTE_LIBRTE_MLX5_PMD
> > CONFIG_RTE_LIBRTE_BNX2X_PMD
> > 
> > Then rebuild and you should see the errors.
> > 
> 
> Personally, though I wouldn't view it as necessary to get those extra
> fixes
> into this set. The set is big enough as it is, so I'd like to see the
> existing gcc 8 fixes we have merged to make some progress, rather
> than
> constantly spinning ever bigger sets to try and fix them all in one
> go.
> 
> My 2c.
> 
> /Bruce

Yes I agree, they can be done separately, I just reported them because
I had been asked to give the patchset a spin, and I have those PMDs
enabled so I noticed those errors. 

-- 
Kind regards,
Luca Boccassi

Re: [PATCH v3 14/40] app/test-pmd: can't find include

[De Lara Guarch, Pablo]

Hi Andy,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andy Green
> Sent: Thursday, May 10, 2018 3:47 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 14/40] app/test-pmd: can't find include
> 
> /home/agreen/projects/dpdk/app/test-pmd/cmdline.c:64:10:
> fatal error: rte_pmd_dpaa.h: No such file or directory  #include
> <rte_pmd_dpaa.h>
>           ^~~~~~~~~~~~~~~~
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  app/test-pmd/Makefile |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile index
> 60ae9b9c1..a0fdd0e11 100644
> --- a/app/test-pmd/Makefile
> +++ b/app/test-pmd/Makefile
> @@ -13,6 +13,7 @@ APP = testpmd
>  CFLAGS += -DALLOW_EXPERIMENTAL_API
>  CFLAGS += -O3
>  CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa
> 
>  #
>  # all source are stored in SRCS-y

I don't think this is necessary. This is including files directly from the source code,
but when they get copied in build/include/, so if you are not seeing that include,
it is because the driver is not getting compiled (I assume that this was because of the broken dpaa bus).

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 09:49 PM, Luca Boccassi wrote:
> On Thu, 2018-05-10 at 14:36 +0100, Bruce Richardson wrote:
>> On Thu, May 10, 2018 at 01:35:49PM +0100, Luca Boccassi wrote:
>>> On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote:
>>>>
>>>> On 05/10/2018 06:21 PM, Luca Boccassi wrote:
>>>>> On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
>>>>>> The following series gets current master able to build
>>>>>> itself, and allow lagopus to build against it, on Fedora 28 +
>>>>>> x86_64 using gcc 8.0.1.
>>>>>>
>>>>>> The first 17 patches have already been through two spins and
>>>>>> this time are corrected for all the comment (thanks to
>>>>>> everybody who commented) since v2, and have tested-by /
>>>>>> acked-bys applied.  The first workaround patch for the hash
>>>>>> function cast problem is dropped since something has already
>>>>>> been applied in master since yesterday to address it.
>>>>>>
>>>>>> The additional 23 patches are fixes for problems found
>>>>>> actually trying to build lagopus using current master.
>>>>>> These are almost entirely related to signed / unsigned
>>>>>> or truncation without explicit casts inside dpdk
>>>>>> headers.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Andy Green (40):
>>>>>>         drivers/bus/pci: fix strncpy dangerous code
>>>>>>         drivers/bus/dpaa: fix inconsistent struct alignment
>>>>>>         drivers/net/axgbe: fix broken eeprom string comp
>>>>>>         drivers/net/nfp/nfpcore: fix strncpy misuse
>>>>>>         drivers/net/nfp/nfpcore: fix off-by-one and no NUL on
>>>>>> strncpy
>>>>>> use
>>>>>>         drivers/net/nfp: don't memcpy out of source range
>>>>>>         drivers/net/nfp: fix buffer overflow in fw_name
>>>>>>         drivers/net/qede: fix strncpy constant and NUL
>>>>>>         drivers/net/qede: fix broken strncpy
>>>>>>         drivers/net/sfc: fix strncpy length
>>>>>>         drivers/net/sfc: fix strncpy size and NUL
>>>>>>         drivers/net/vdev: readlink inputs cannot be aliased
>>>>>>         drivers/net/vdev: fix 3 x strncpy misuse
>>>>>>         app/test-pmd: can't find include
>>>>>>         app/proc-info: fix sprintf overrun bug
>>>>>>         app/test-bbdev: test-bbdev: strcpy ok for allocated
>>>>>> string
>>>>>>         app/test-bbdev: strcpy ok for allocated string
>>>>>>         rte_common.h: cast gcc builtin result to avoid
>>>>>> complaints
>>>>>>         rte_memcpy.h: explicit tmp cast
>>>>>>         lib/librte_eal/common/include/rte_lcore.h: explicit
>>>>>> cast
>>>>>> for
>>>>>> signed change
>>>>>>         /lib/librte_eal/common/include/rte_random.h: stage
>>>>>> cast
>>>>>> from
>>>>>> uint64_t to long
>>>>>>         rte_spinlock.h: stack declarations before code
>>>>>>         rte_ring_generic.h: stack declarations before code
>>>>>>         rte_ring.h: remove signed type flipflopping
>>>>>>         rte_dev.h: stack declaration at top of own basic block
>>>>>>         rte_mbuf.h: avoid truncation warnings from inadvertant
>>>>>> int16_t
>>>>>> to int promotion
>>>>>>         rte_mbuf.h: explicit casts for flipping between
>>>>>> int16_t
>>>>>> and
>>>>>> uint16_t
>>>>>>         rte_mbuf.h: make sure RTE_MIN compares same types
>>>>>>         rte_mbuf.h: explicit cast restricting ptrdiff to
>>>>>> uint16_t
>>>>>>         rte_mbuf.h: explicit cast for size_t to uint32_t
>>>>>>         rte_mbuf.h: explicit casts to uint16_t to avoid
>>>>>> truncation
>>>>>> warnings
>>>>>>         rte_byteorder.h: explicit cast for return promotion
>>>>>>         rte_ether.h: explicit cast avoiding truncation warning
>>>>>>         rte_ether.h: stack vars declared at top of function
>>>>>>         rte_ethdev.h: fix sign and scope of temp var
>>>>>>         rte_ethdev.h: explicit cast for return type
>>>>>>         rte_ethdev.h: explicit cast for truncation
>>>>>>         rte_hash_crc.h: stack vars declared at top of function
>>>>>>         rte_hash_crc.h: explicit casts for truncation
>>>>>>         rte_string_fns.h: explicit cast for int return to
>>>>>> size_t
>>>>>
>>>>> Hi,
>>>>>
>>>>> I've built-tested this series on Debian Stretch (gcc 6.3) and
>>>>> Debian
>>>>> Sid (gcc 8.1).
>>>>>
>>>>> The series builds fine with the default config, but the bnx2x
>>>>> and
>>>>> mlx5
>>>>> PMDs still have errors with gcc-8:
>>>>
>>>> Yes I just built it with defconfig for x86_64 on Fedora 28 with
>>>> default
>>>> tools and cleared out everything that came up.
>>>>
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function
>>>>> 'bnx2x_alloc_hsi_mem':
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s'
>>>>> directive
>>>>> writing up to 31 bytes into a region of size between 15 and 25
>>>>> [-
>>>>> Werror=format-overflow=]
>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device,
>>>>> msg,
>>>>>                                ^~
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>>>>>      if (bnx2x_dma_alloc(sc, sizeof(union
>>>>> bnx2x_host_hc_status_block),
>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> ~~~~
>>>>> ~~
>>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf'
>>>>> output
>>>>> between 10 and 66 bytes into a destination of size 32
>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device,
>>>>> msg,
>>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> ~~
>>>>>       rte_get_timer_cycles());
>>>>>       ~~~~~~~~~~~~~~~~~~~~~~~
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s'
>>>>> directive
>>>>> writing up to 31 bytes into a region of size between 23 and 25
>>>>> [-
>>>>> Werror=format-overflow=]
>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc),
>>>>> msg,
>>>>>                                ^~
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>>>>>      if (bnx2x_dma_alloc(sc, sizeof(union
>>>>> bnx2x_host_hc_status_block),
>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> ~~~~
>>>>> ~~
>>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf'
>>>>> output
>>>>> between 10 and 58 bytes into a destination of size 32
>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc),
>>>>> msg,
>>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> ~~
>>>>>       rte_get_timer_cycles());
>>>>>
>>>>>
>>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function
>>>>> 'mlx5_pci_probe':
>>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be
>>>>> used
>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>      config.vf = vf;
>>>>>
>>>>> Hope this can be useful.
>>>>
>>>> I think gcc 8.0.1 is capable to show that and I am willing to
>>>> look
>>>> at
>>>> them.  But can you help me with exactly what changes you made so
>>>> these
>>>> things built and made trouble, compared to the defconfig I have
>>>> used
>>>> until now?
>>>
>>> If you already have a build directory you are using, the simplest
>>> way
>>> is to edit the .config file in there and change the following from
>>> =n
>>> to =y:
>>>
>>> CONFIG_RTE_LIBRTE_MLX4_PMD
>>> CONFIG_RTE_LIBRTE_MLX5_PMD
>>> CONFIG_RTE_LIBRTE_BNX2X_PMD
>>>
>>> Then rebuild and you should see the errors.
>>>
>>
>> Personally, though I wouldn't view it as necessary to get those extra
>> fixes
>> into this set. The set is big enough as it is, so I'd like to see the
>> existing gcc 8 fixes we have merged to make some progress, rather
>> than
>> constantly spinning ever bigger sets to try and fix them all in one
>> go.
>>
>> My 2c.
>>
>> /Bruce
> 
> Yes I agree, they can be done separately, I just reported them because
> I had been asked to give the patchset a spin, and I have those PMDs
> enabled so I noticed those errors.

If they want to release known-broken stuff, they can fix your broken 
stuff separately how they like.

But your problems are caused by a pile of real breakages (and attempts 
to sticking-plaster over them with illegal casts) in this codebase.

I dunno why you find yourself agreeing that isn't the same priority as 
fixing anything else with this stuff.

-Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 09:36 PM, Bruce Richardson wrote:
> On Thu, May 10, 2018 at 01:35:49PM +0100, Luca Boccassi wrote:
>> On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote:
>>>
>>> On 05/10/2018 06:21 PM, Luca Boccassi wrote:
>>>> On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
>>>>> The following series gets current master able to build
>>>>> itself, and allow lagopus to build against it, on Fedora 28 +
>>>>> x86_64 using gcc 8.0.1.
>>>>>
>>>>> The first 17 patches have already been through two spins and
>>>>> this time are corrected for all the comment (thanks to
>>>>> everybody who commented) since v2, and have tested-by /
>>>>> acked-bys applied.  The first workaround patch for the hash
>>>>> function cast problem is dropped since something has already
>>>>> been applied in master since yesterday to address it.
>>>>>
>>>>> The additional 23 patches are fixes for problems found
>>>>> actually trying to build lagopus using current master.
>>>>> These are almost entirely related to signed / unsigned
>>>>> or truncation without explicit casts inside dpdk
>>>>> headers.
>>>>>
>>>>> ---
>>>>>
>>>>> Andy Green (40):
>>>>>         drivers/bus/pci: fix strncpy dangerous code
>>>>>         drivers/bus/dpaa: fix inconsistent struct alignment
>>>>>         drivers/net/axgbe: fix broken eeprom string comp
>>>>>         drivers/net/nfp/nfpcore: fix strncpy misuse
>>>>>         drivers/net/nfp/nfpcore: fix off-by-one and no NUL on
>>>>> strncpy
>>>>> use
>>>>>         drivers/net/nfp: don't memcpy out of source range
>>>>>         drivers/net/nfp: fix buffer overflow in fw_name
>>>>>         drivers/net/qede: fix strncpy constant and NUL
>>>>>         drivers/net/qede: fix broken strncpy
>>>>>         drivers/net/sfc: fix strncpy length
>>>>>         drivers/net/sfc: fix strncpy size and NUL
>>>>>         drivers/net/vdev: readlink inputs cannot be aliased
>>>>>         drivers/net/vdev: fix 3 x strncpy misuse
>>>>>         app/test-pmd: can't find include
>>>>>         app/proc-info: fix sprintf overrun bug
>>>>>         app/test-bbdev: test-bbdev: strcpy ok for allocated string
>>>>>         app/test-bbdev: strcpy ok for allocated string
>>>>>         rte_common.h: cast gcc builtin result to avoid complaints
>>>>>         rte_memcpy.h: explicit tmp cast
>>>>>         lib/librte_eal/common/include/rte_lcore.h: explicit cast
>>>>> for
>>>>> signed change
>>>>>         /lib/librte_eal/common/include/rte_random.h: stage cast
>>>>> from
>>>>> uint64_t to long
>>>>>         rte_spinlock.h: stack declarations before code
>>>>>         rte_ring_generic.h: stack declarations before code
>>>>>         rte_ring.h: remove signed type flipflopping
>>>>>         rte_dev.h: stack declaration at top of own basic block
>>>>>         rte_mbuf.h: avoid truncation warnings from inadvertant
>>>>> int16_t
>>>>> to int promotion
>>>>>         rte_mbuf.h: explicit casts for flipping between int16_t
>>>>> and
>>>>> uint16_t
>>>>>         rte_mbuf.h: make sure RTE_MIN compares same types
>>>>>         rte_mbuf.h: explicit cast restricting ptrdiff to uint16_t
>>>>>         rte_mbuf.h: explicit cast for size_t to uint32_t
>>>>>         rte_mbuf.h: explicit casts to uint16_t to avoid truncation
>>>>> warnings
>>>>>         rte_byteorder.h: explicit cast for return promotion
>>>>>         rte_ether.h: explicit cast avoiding truncation warning
>>>>>         rte_ether.h: stack vars declared at top of function
>>>>>         rte_ethdev.h: fix sign and scope of temp var
>>>>>         rte_ethdev.h: explicit cast for return type
>>>>>         rte_ethdev.h: explicit cast for truncation
>>>>>         rte_hash_crc.h: stack vars declared at top of function
>>>>>         rte_hash_crc.h: explicit casts for truncation
>>>>>         rte_string_fns.h: explicit cast for int return to size_t
>>>>
>>>> Hi,
>>>>
>>>> I've built-tested this series on Debian Stretch (gcc 6.3) and
>>>> Debian
>>>> Sid (gcc 8.1).
>>>>
>>>> The series builds fine with the default config, but the bnx2x and
>>>> mlx5
>>>> PMDs still have errors with gcc-8:
>>>
>>> Yes I just built it with defconfig for x86_64 on Fedora 28 with
>>> default
>>> tools and cleared out everything that came up.
>>>
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function
>>>> 'bnx2x_alloc_hsi_mem':
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s' directive
>>>> writing up to 31 bytes into a region of size between 15 and 25 [-
>>>> Werror=format-overflow=]
>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
>>>>                                ^~
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>>>>      if (bnx2x_dma_alloc(sc, sizeof(union
>>>> bnx2x_host_hc_status_block),
>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> ~~
>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf' output
>>>> between 10 and 66 bytes into a destination of size 32
>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, msg,
>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>       rte_get_timer_cycles());
>>>>       ~~~~~~~~~~~~~~~~~~~~~~~
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s' directive
>>>> writing up to 31 bytes into a region of size between 23 and 25 [-
>>>> Werror=format-overflow=]
>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
>>>>                                ^~
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>>>>      if (bnx2x_dma_alloc(sc, sizeof(union
>>>> bnx2x_host_hc_status_block),
>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> ~~
>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf' output
>>>> between 10 and 58 bytes into a destination of size 32
>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), msg,
>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>       rte_get_timer_cycles());
>>>>
>>>>
>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function 'mlx5_pci_probe':
>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>      config.vf = vf;
>>>>
>>>> Hope this can be useful.
>>>
>>> I think gcc 8.0.1 is capable to show that and I am willing to look
>>> at
>>> them.  But can you help me with exactly what changes you made so
>>> these
>>> things built and made trouble, compared to the defconfig I have used
>>> until now?
>>
>> If you already have a build directory you are using, the simplest way
>> is to edit the .config file in there and change the following from =n
>> to =y:
>>
>> CONFIG_RTE_LIBRTE_MLX4_PMD
>> CONFIG_RTE_LIBRTE_MLX5_PMD
>> CONFIG_RTE_LIBRTE_BNX2X_PMD
>>
>> Then rebuild and you should see the errors.
>>
> Personally, though I wouldn't view it as necessary to get those extra fixes
> into this set. The set is big enough as it is, so I'd like to see the
> existing gcc 8 fixes we have merged to make some progress, rather than
> constantly spinning ever bigger sets to try and fix them all in one go.

Sorry, but your codebase just keeps coming up with new things to fix.

Unfortunately I have yet to see gcc8 complain about something that was 
not a real problem in the code.

If I were maintaining this, what I would do is look through the freebies 
I am getting from that awesome guy who is donating his time fixing my 
code, and anything that I could understand was useful, I would apply, 
let him rebase out the stuff that is in, reissue, until everything that 
is going in, is in.

You don't have to apply the whole series, that is just how I am posting 
them because that is how they are in my tree.  Likewise, if I post 2 x 
20 patches, instead of 1 x 40, it makes no difference what you choose to 
cherrypick.

The problems Luca pointed our are caused by problems in your code, not 
gcc8, and not me fixing more things in your code for free.

I get it you want to make a release but am I the only person throwing 
gcc8 quality-related patches at you?  Then maybe you should take a pause 
and absorb the quality improvements instead of releasing known-broken 
code (let us pretend there are not 166 coverity breakages).

> My 2c.

...and I would get rid of completely pointless "quality theater" 
roadblocks like this git subject grep stuff.

-Andy

> /Bruce
> 

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 09:53 PM, Andy Green wrote:
> 
> 
> On 05/10/2018 09:49 PM, Luca Boccassi wrote:
>> On Thu, 2018-05-10 at 14:36 +0100, Bruce Richardson wrote:
>>> On Thu, May 10, 2018 at 01:35:49PM +0100, Luca Boccassi wrote:
>>>> On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote:
>>>>>
>>>>> On 05/10/2018 06:21 PM, Luca Boccassi wrote:
>>>>>> On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote:
>>>>>>> The following series gets current master able to build
>>>>>>> itself, and allow lagopus to build against it, on Fedora 28 +
>>>>>>> x86_64 using gcc 8.0.1.
>>>>>>>
>>>>>>> The first 17 patches have already been through two spins and
>>>>>>> this time are corrected for all the comment (thanks to
>>>>>>> everybody who commented) since v2, and have tested-by /
>>>>>>> acked-bys applied.  The first workaround patch for the hash
>>>>>>> function cast problem is dropped since something has already
>>>>>>> been applied in master since yesterday to address it.
>>>>>>>
>>>>>>> The additional 23 patches are fixes for problems found
>>>>>>> actually trying to build lagopus using current master.
>>>>>>> These are almost entirely related to signed / unsigned
>>>>>>> or truncation without explicit casts inside dpdk
>>>>>>> headers.
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Andy Green (40):
>>>>>>>         drivers/bus/pci: fix strncpy dangerous code
>>>>>>>         drivers/bus/dpaa: fix inconsistent struct alignment
>>>>>>>         drivers/net/axgbe: fix broken eeprom string comp
>>>>>>>         drivers/net/nfp/nfpcore: fix strncpy misuse
>>>>>>>         drivers/net/nfp/nfpcore: fix off-by-one and no NUL on
>>>>>>> strncpy
>>>>>>> use
>>>>>>>         drivers/net/nfp: don't memcpy out of source range
>>>>>>>         drivers/net/nfp: fix buffer overflow in fw_name
>>>>>>>         drivers/net/qede: fix strncpy constant and NUL
>>>>>>>         drivers/net/qede: fix broken strncpy
>>>>>>>         drivers/net/sfc: fix strncpy length
>>>>>>>         drivers/net/sfc: fix strncpy size and NUL
>>>>>>>         drivers/net/vdev: readlink inputs cannot be aliased
>>>>>>>         drivers/net/vdev: fix 3 x strncpy misuse
>>>>>>>         app/test-pmd: can't find include
>>>>>>>         app/proc-info: fix sprintf overrun bug
>>>>>>>         app/test-bbdev: test-bbdev: strcpy ok for allocated
>>>>>>> string
>>>>>>>         app/test-bbdev: strcpy ok for allocated string
>>>>>>>         rte_common.h: cast gcc builtin result to avoid
>>>>>>> complaints
>>>>>>>         rte_memcpy.h: explicit tmp cast
>>>>>>>         lib/librte_eal/common/include/rte_lcore.h: explicit
>>>>>>> cast
>>>>>>> for
>>>>>>> signed change
>>>>>>>         /lib/librte_eal/common/include/rte_random.h: stage
>>>>>>> cast
>>>>>>> from
>>>>>>> uint64_t to long
>>>>>>>         rte_spinlock.h: stack declarations before code
>>>>>>>         rte_ring_generic.h: stack declarations before code
>>>>>>>         rte_ring.h: remove signed type flipflopping
>>>>>>>         rte_dev.h: stack declaration at top of own basic block
>>>>>>>         rte_mbuf.h: avoid truncation warnings from inadvertant
>>>>>>> int16_t
>>>>>>> to int promotion
>>>>>>>         rte_mbuf.h: explicit casts for flipping between
>>>>>>> int16_t
>>>>>>> and
>>>>>>> uint16_t
>>>>>>>         rte_mbuf.h: make sure RTE_MIN compares same types
>>>>>>>         rte_mbuf.h: explicit cast restricting ptrdiff to
>>>>>>> uint16_t
>>>>>>>         rte_mbuf.h: explicit cast for size_t to uint32_t
>>>>>>>         rte_mbuf.h: explicit casts to uint16_t to avoid
>>>>>>> truncation
>>>>>>> warnings
>>>>>>>         rte_byteorder.h: explicit cast for return promotion
>>>>>>>         rte_ether.h: explicit cast avoiding truncation warning
>>>>>>>         rte_ether.h: stack vars declared at top of function
>>>>>>>         rte_ethdev.h: fix sign and scope of temp var
>>>>>>>         rte_ethdev.h: explicit cast for return type
>>>>>>>         rte_ethdev.h: explicit cast for truncation
>>>>>>>         rte_hash_crc.h: stack vars declared at top of function
>>>>>>>         rte_hash_crc.h: explicit casts for truncation
>>>>>>>         rte_string_fns.h: explicit cast for int return to
>>>>>>> size_t
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I've built-tested this series on Debian Stretch (gcc 6.3) and
>>>>>> Debian
>>>>>> Sid (gcc 8.1).
>>>>>>
>>>>>> The series builds fine with the default config, but the bnx2x
>>>>>> and
>>>>>> mlx5
>>>>>> PMDs still have errors with gcc-8:
>>>>>
>>>>> Yes I just built it with defconfig for x86_64 on Fedora 28 with
>>>>> default
>>>>> tools and cleared out everything that came up.
>>>>>
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function
>>>>>> 'bnx2x_alloc_hsi_mem':
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s'
>>>>>> directive
>>>>>> writing up to 31 bytes into a region of size between 15 and 25
>>>>>> [-
>>>>>> Werror=format-overflow=]
>>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device,
>>>>>> msg,
>>>>>>                                ^~
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>>>>>>      if (bnx2x_dma_alloc(sc, sizeof(union
>>>>>> bnx2x_host_hc_status_block),
>>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> ~~~~
>>>>>> ~~
>>>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf'
>>>>>> output
>>>>>> between 10 and 66 bytes into a destination of size 32
>>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device,
>>>>>> msg,
>>>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> ~~
>>>>>>       rte_get_timer_cycles());
>>>>>>       ~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s'
>>>>>> directive
>>>>>> writing up to 31 bytes into a region of size between 23 and 25
>>>>>> [-
>>>>>> Werror=format-overflow=]
>>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc),
>>>>>> msg,
>>>>>>                                ^~
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7:
>>>>>>      if (bnx2x_dma_alloc(sc, sizeof(union
>>>>>> bnx2x_host_hc_status_block),
>>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> ~~~~
>>>>>> ~~
>>>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) {
>>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf'
>>>>>> output
>>>>>> between 10 and 58 bytes into a destination of size 32
>>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc),
>>>>>> msg,
>>>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> ~~
>>>>>>       rte_get_timer_cycles());
>>>>>>
>>>>>>
>>>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function
>>>>>> 'mlx5_pci_probe':
>>>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be
>>>>>> used
>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>      config.vf = vf;
>>>>>>
>>>>>> Hope this can be useful.
>>>>>
>>>>> I think gcc 8.0.1 is capable to show that and I am willing to
>>>>> look
>>>>> at
>>>>> them.  But can you help me with exactly what changes you made so
>>>>> these
>>>>> things built and made trouble, compared to the defconfig I have
>>>>> used
>>>>> until now?
>>>>
>>>> If you already have a build directory you are using, the simplest
>>>> way
>>>> is to edit the .config file in there and change the following from
>>>> =n
>>>> to =y:
>>>>
>>>> CONFIG_RTE_LIBRTE_MLX4_PMD
>>>> CONFIG_RTE_LIBRTE_MLX5_PMD
>>>> CONFIG_RTE_LIBRTE_BNX2X_PMD
>>>>
>>>> Then rebuild and you should see the errors.
>>>>
>>>
>>> Personally, though I wouldn't view it as necessary to get those extra
>>> fixes
>>> into this set. The set is big enough as it is, so I'd like to see the
>>> existing gcc 8 fixes we have merged to make some progress, rather
>>> than
>>> constantly spinning ever bigger sets to try and fix them all in one
>>> go.
>>>
>>> My 2c.
>>>
>>> /Bruce
>>
>> Yes I agree, they can be done separately, I just reported them because
>> I had been asked to give the patchset a spin, and I have those PMDs
>> enabled so I noticed those errors.
> 
> If they want to release known-broken stuff, they can fix your broken 
> stuff separately how they like.
> 
> But your problems are caused by a pile of real breakages (and attempts 
> to sticking-plaster over them with illegal casts) in this codebase.
> 
> I dunno why you find yourself agreeing that isn't the same priority as 
> fixing anything else with this stuff.

Fixes for your problems are here:

https://github.com/lws-team/dpdk/commits/master

along with the rest of my stuff.

In particular:

https://github.com/lws-team/dpdk/commit/6cead9e1947588ddccf0508acd01c17392cd849c

https://github.com/lws-team/dpdk/commit/e0063507d824bc153ae5fd3f8fe59ccbdf88ef89

https://github.com/lws-team/dpdk/commit/5daea1b9b24e1151ac8416d7b5c1e7d096389c51

https://github.com/lws-team/dpdk/commit/3e8d82bf693e1469e4143297ea4050023d50fa6b

-Andy

> -Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Stephen Hemminger]

On Thu, 10 May 2018 20:13:31 +0800
Andy Green <andy@warmcat.com> wrote:

> I appreciate the reply.
> 
> But why bother having a subject line at all if it is going to be 
> mechanically enforced that nothing in it is allowed to be "useful"? 
> That really doesn't make sense does it.


It was done because there were lots of clueless patches showing
up on the driver development list which had useless subject
lines.

Re: [PATCH v3 40/40] rte_string_fns.h: explicit cast for int return to size_t

[Stephen Hemminger]

On Thu, 10 May 2018 10:49:40 +0800
Andy Green <andy@warmcat.com> wrote:

> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  lib/librte_eal/common/include/rte_string_fns.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
> index fcbb42e00..51413a55e 100644
> --- a/lib/librte_eal/common/include/rte_string_fns.h
> +++ b/lib/librte_eal/common/include/rte_string_fns.h
> @@ -55,7 +55,7 @@ rte_strsplit(char *string, int stringlen,
>  static inline size_t
>  rte_strlcpy(char *dst, const char *src, size_t size)
>  {
> -	return snprintf(dst, size, "%s", src);
> +	return (size_t)(unsigned int)snprintf(dst, size, "%s", src);
>  }
>  
>  /* pull in a strlcpy function */
> 

I would rather see a proper version of strlcpy extracted from libbsd

Re: [PATCH v3 36/40] rte_ethdev.h: explicit cast for return type

[Stephen Hemminger]

On Thu, 10 May 2018 10:49:20 +0800
Andy Green <andy@warmcat.com> wrote:

> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h:3860:10:
> warning: conversion to 'int' from 'uint32_t' {aka 'unsigned int'}
> may change the sign of the result [-Wsign-conversion]
>   return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 2487e1d2d..c84dc44b8 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -3857,7 +3857,7 @@ rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
>  	if (queue_id >= dev->data->nb_rx_queues)
>  		return -EINVAL;
>  
> -	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
> +	return (int)(*dev->dev_ops->rx_queue_count)(dev, queue_id);
>  }
>  
>  /**
> 

Why not change rx_queue_count to int to allow drivers to return an error?

Re: [PATCH v3 36/40] rte_ethdev.h: explicit cast for return type

[Andy Green]

On 05/11/2018 03:18 AM, Stephen Hemminger wrote:
> On Thu, 10 May 2018 10:49:20 +0800
> Andy Green <andy@warmcat.com> wrote:
> 
>> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h:3860:10:
>> warning: conversion to 'int' from 'uint32_t' {aka 'unsigned int'}
>> may change the sign of the result [-Wsign-conversion]
>>    return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
>>
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.h |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 2487e1d2d..c84dc44b8 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -3857,7 +3857,7 @@ rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
>>   	if (queue_id >= dev->data->nb_rx_queues)
>>   		return -EINVAL;
>>   
>> -	return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
>> +	return (int)(*dev->dev_ops->rx_queue_count)(dev, queue_id);
>>   }
>>   
>>   /**
>>
> 
> Why not change rx_queue_count to int to allow drivers to return an error?

OK.  I have done it and will push it later.  I left the few related apis 
in ./lib/ like rte_vhost_rx_queue_count() alone, they still return a 
uint32_t.

Naturally, I want to call that patch something like "eth_dev_ops: change 
rx_queue_count to return an int"...

-Andy

Re: [PATCH v3 40/40] rte_string_fns.h: explicit cast for int return to size_t

[Andy Green]

On 05/11/2018 03:17 AM, Stephen Hemminger wrote:
> On Thu, 10 May 2018 10:49:40 +0800
> Andy Green <andy@warmcat.com> wrote:
> 
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>>   lib/librte_eal/common/include/rte_string_fns.h |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
>> index fcbb42e00..51413a55e 100644
>> --- a/lib/librte_eal/common/include/rte_string_fns.h
>> +++ b/lib/librte_eal/common/include/rte_string_fns.h
>> @@ -55,7 +55,7 @@ rte_strsplit(char *string, int stringlen,
>>   static inline size_t
>>   rte_strlcpy(char *dst, const char *src, size_t size)
>>   {
>> -	return snprintf(dst, size, "%s", src);
>> +	return (size_t)(unsigned int)snprintf(dst, size, "%s", src);
>>   }
>>   
>>   /* pull in a strlcpy function */
>>
> 
> I would rather see a proper version of strlcpy extracted from libbsd

I also have done this and will push later.  It's not inline any more.

-Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 11:01 PM, Stephen Hemminger wrote:
> On Thu, 10 May 2018 20:13:31 +0800
> Andy Green <andy@warmcat.com> wrote:
> 
>> I appreciate the reply.
>>
>> But why bother having a subject line at all if it is going to be
>> mechanically enforced that nothing in it is allowed to be "useful"?
>> That really doesn't make sense does it.
> 
> 
> It was done because there were lots of clueless patches showing
> up on the driver development list which had useless subject
> lines.

The "cure" is worse than the disease...

  - I can mention, eg, that something changed to an int.  But a size_t 
or my_type_t?  I am not allowed to mention it even if that is the whole 
reason for the patch.

  - I can mention most libc apis, but not those that happen to have an 
underscore, eg, timerfd_create(), even if that was the focus of the patch.

  - Any kind of manifest constant like MY_CONSTANT: illegal to mention, 
even if the patch's job is change MY_CONSTANT to, say, 5.  What should I 
entitle that patch?  "lib: change something to 5"?  "lib: change 
MY.CONSTANT to 5"?

  - I can mention most filenames or paths, eg, down /proc, or myfile.c. 
But not if the filepath happens to contain an underscore.  Even if the 
effect of the patch is to migrate stuff from myfile.c to my_files/

The results are arbitrary... please consider removing this now it has 
been in place a while and made its original point.

-Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/11/2018 08:29 AM, Andy Green wrote:
> 
> 
> On 05/10/2018 11:01 PM, Stephen Hemminger wrote:
>> On Thu, 10 May 2018 20:13:31 +0800
>> Andy Green <andy@warmcat.com> wrote:
>>
>>> I appreciate the reply.
>>>
>>> But why bother having a subject line at all if it is going to be
>>> mechanically enforced that nothing in it is allowed to be "useful"?
>>> That really doesn't make sense does it.
>>
>>
>> It was done because there were lots of clueless patches showing
>> up on the driver development list which had useless subject
>> lines.
> 
> The "cure" is worse than the disease...
> 
>   - I can mention, eg, that something changed to an int.  But a size_t 
> or my_type_t?  I am not allowed to mention it even if that is the whole 
> reason for the patch.
> 
>   - I can mention most libc apis, but not those that happen to have an 
> underscore, eg, timerfd_create(), even if that was the focus of the patch.
> 
>   - Any kind of manifest constant like MY_CONSTANT: illegal to mention, 
> even if the patch's job is change MY_CONSTANT to, say, 5.  What should I 
> entitle that patch?  "lib: change something to 5"?  "lib: change 
> MY.CONSTANT to 5"?
> 
>   - I can mention most filenames or paths, eg, down /proc, or myfile.c. 
> But not if the filepath happens to contain an underscore.  Even if the 
> effect of the patch is to migrate stuff from myfile.c to my_files/
> 
> The results are arbitrary... please consider removing this now it has 
> been in place a while and made its original point.

Having now used check-git-log.sh, also

1) It reports every patch has "Wrong headline uppercase" on Fedora 28, 
even, eg, "net/nfp: solve buffer overflow".  Googling around

https://stackoverflow.com/questions/11166825/grep-case-sensitive-a-z

I checked a patch on the patch checker to make it use [[:upper:]] and it 
works OK.  So there is another patch in the series adapting this.

2) It claims "app/test-pmd" is a "Wrong headline label". 
"app/proc-info" and "app/test-bbdev" are accepted.

It's because it does this, but app/test-pmd is a real directory:

# check headline label for common typos
bad=$(echo "$headlines" | grep --color=always \
         -e '^example[:/]' \
         -e '^apps/' \
         -e '^testpmd' \
         -e 'test-pmd' \
         -e '^bond:' \
         | sed 's,^,\t,')
[ -z "$bad" ] || printf "Wrong headline label:\n$bad\n"

I just left that and told it app:.

They all pass the thing now.

-Andy

> -Andy

Re: [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Thomas Monjalon]

Hi,

11/05/2018 02:29, Andy Green:
> 
> On 05/10/2018 11:01 PM, Stephen Hemminger wrote:
> > On Thu, 10 May 2018 20:13:31 +0800
> > Andy Green <andy@warmcat.com> wrote:
> > 
> >> I appreciate the reply.
> >>
> >> But why bother having a subject line at all if it is going to be
> >> mechanically enforced that nothing in it is allowed to be "useful"?
> >> That really doesn't make sense does it.
> > 
> > It was done because there were lots of clueless patches showing
> > up on the driver development list which had useless subject
> > lines.

Yes, the title should help to quickly identify the scope of the commits.
Usually, giving variable names, function names, etc are not very useful.
It is very common that some developers say "fixing function X" instead
of describing the fixed behaviour: "fixing feature Y".

> The "cure" is worse than the disease...

It is a tool showing some warnings.
We must be smart when using this tool (as any other tools)
and consider that some warnings are false positive.

>   - I can mention, eg, that something changed to an int.  But a size_t 
> or my_type_t?  I am not allowed to mention it even if that is the whole 
> reason for the patch.

Of course you can use size_t in the title if it is relevant.
Sometimes, changing a type is fixing a feature, so better to name the feature.

>   - I can mention most libc apis, but not those that happen to have an 
> underscore, eg, timerfd_create(), even if that was the focus of the patch.
> 
>   - Any kind of manifest constant like MY_CONSTANT: illegal to mention, 
> even if the patch's job is change MY_CONSTANT to, say, 5.  What should I 
> entitle that patch?  "lib: change something to 5"?  "lib: change 
> MY.CONSTANT to 5"?
> 
>   - I can mention most filenames or paths, eg, down /proc, or myfile.c. 
> But not if the filepath happens to contain an underscore.  Even if the 
> effect of the patch is to migrate stuff from myfile.c to my_files/
> 
> The results are arbitrary... please consider removing this now it has 
> been in place a while and made its original point.

I hope you got my point that the title should be a high level description
of the scope or goal of the patch.
The details are inside the commit log.
But using underscore is accepted in the title if relevant.

Every strong rules are stupids, that's why we have only guidelines
and we are flexible.