[dpdk-dev] [PATCH 0/3] Various patches

[dpdk-dev] [PATCH 0/3] Various patches

[Julien Cretin]

Hello all,

This patch set contains three unrelated patches found by running
TrustInSoft Analyzer [1] on some parts of the testpmd application:

- The first patch fixes a minor signed overflow in a constant
  expression of testpmd.

- The second patch is not a fix and concerns a suspicious loop
  condition in optimize_object_size.

- The third (and last) patch fixes sign mismatches for some printf
  arguments.

[1] TrustInSoft Analyzer (http://trust-in-soft.com) is a static
analyzer. And as such, it gives information about the execution of a
source code without actually running it. However, unlike other static
analysis tools, it has the particularity of being correct. This means
that it does not remain silent when a run-time error may happen in the
range of the analysis. In other words, it gives information about all
possible executions (defined by the analysis) of a source code without
actually running it.

Julien Cretin (3):
  app/testpmd: fix minor signed overflow in a constant
  mem: remove redundant check in optimize_object_size
  app/testpmd: fix incompatible sign for printf arguments

 app/test-pmd/config.c            | 16 ++++++++--------
 app/test-pmd/testpmd.c           |  2 +-
 app/test-pmd/txonly.c            |  4 ++--
 lib/librte_mempool/rte_mempool.c |  3 +--
 4 files changed, 12 insertions(+), 13 deletions(-)

-- 
1.9.1

[dpdk-dev] [PATCH 1/3] app/testpmd: fix minor signed overflow in a constant

[Julien Cretin]

The expression (192 << 24) has an undefined behavior since:
- the integer constant 192 has type int, and
- 192 x 2^24 is not representable as an int.

Suffixing 192 with U defines a behavior since:
- the integer constant 192U has type unsigned int, and
- the value of (192U << 24) is defined as
  (192 x 2^24) % (UINT_MAX + 1)

This minor bug was found using TrustInSoft Analyzer.

Signed-off-by: Julien Cretin <julien.cretin@trust-in-soft.com>
---
 app/test-pmd/txonly.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 1cf2574..5bbd34f 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -76,8 +76,8 @@
 #define UDP_SRC_PORT 1024
 #define UDP_DST_PORT 1024
 
-#define IP_SRC_ADDR ((192 << 24) | (168 << 16) | (0 << 8) | 1)
-#define IP_DST_ADDR ((192 << 24) | (168 << 16) | (0 << 8) | 2)
+#define IP_SRC_ADDR ((192U << 24) | (168 << 16) | (0 << 8) | 1)
+#define IP_DST_ADDR ((192U << 24) | (168 << 16) | (0 << 8) | 2)
 
 #define IP_DEFTTL  64   /* from RFC 1340. */
 #define IP_VERSION 0x40
-- 
1.9.1

[dpdk-dev] [PATCH 2/3] mem: remove redundant check in optimize_object_size

[Julien Cretin]

The second condition of this logical OR:
    (get_gcd(new_obj_size, nrank * nchan) != 1 ||
    get_gcd(nchan, new_obj_size) != 1)
is redundant with the first condition.

We can show that the first condition is equivalent to its disjunction
with the second condition using these two results:

- R1: For all conditions A and B, if B implies A, then (A || B) is
  equivalent to A.

- R2: (get_gcd(nchan, new_obj_size) != 1) implies
  (get_gcd(new_obj_size, nrank * nchan) != 1)

We can show R1 with the following truth table (0 is false, 1 is true):
        +-----+-----++----------+-----+-------------+
        |  A  |  B  || (A || B) |  A  | B implies A |
        +-----+-----++----------+-----+-------------+
        |  0  |  0  ||     0    |  0  |      1      |
        |  0  |  1  ||     1    |  0  |      0      |
        |  1  |  0  ||     1    |  1  |      1      |
        |  1  |  1  ||     1    |  1  |      1      |
        +-----+-----++----------+-----+-------------+
                Truth table of (A || B) and A

We can show R2 by looking at the code of optimize_object_size and
get_gcd.

We see that:
- S1: (nchan >= 1) and (nrank >= 1).
- S2: get_gcd returns 0 only when both arguments are 0.

Let:
- X be get_gcd(new_obj_size, nrank * nchan).
- Y be get_gcd(nchan, new_obj_size).

Suppose:
- H1: get_gcd returns the greatest common divisor of its arguments.
- H2: (nrank * nchan) does not exceed UINT_MAX.

We prove (Y != 1) implies (X != 1) with the following steps:
- Suppose L0: (Y != 1). We have to show (X != 1).
- By H1, Y is the greatest common divisor of nchan and new_obj_size.
  In particular, we have L1: Y divides nchan and new_obj_size.
- By H2, we have L2: nchan divides (nrank * nchan)
- By L1 and L2, we have L3: Y divides (nrank * nchan) and
  new_obj_size.
- By H1 and L3, we have L4: (Y <= X).
- By S1 and S2, we have L5: (Y != 0).
- By L0 and L5, we have L6: (Y > 1).
- By L4 and L6, we have (X > 1) and thus (X != 1), which concludes.

R2 was also tested for all values of new_obj_size, nrank, and nchan
between 0 and 2000.

This redundant condition was found using TrustInSoft Analyzer.

Signed-off-by: Julien Cretin <julien.cretin@trust-in-soft.com>
---
 lib/librte_mempool/rte_mempool.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index fdc1586..8e6c86a 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -114,8 +114,7 @@ static unsigned optimize_object_size(unsigned obj_size)
 
 	/* process new object size */
 	new_obj_size = (obj_size + CACHE_LINE_MASK) / CACHE_LINE_SIZE;
-	while (get_gcd(new_obj_size, nrank * nchan) != 1 ||
-			get_gcd(nchan, new_obj_size) != 1)
+	while (get_gcd(new_obj_size, nrank * nchan) != 1)
 		new_obj_size++;
 	return new_obj_size * CACHE_LINE_SIZE;
 }
-- 
1.9.1

[dpdk-dev] [PATCH 3/3] app/testpmd: fix incompatible sign for printf arguments

[Julien Cretin]

The socket_id member of struct rte_port is an unsigned int while the d
conversion specifier of printf expects an int.

The addr_bytes member of struct ether_addr is an array of uint8_t
while the X conversion specifier of printf expects an unsigned int.
Values of type uint8_t are promoted to type int when used in the
ellipsis notation of a function.

These minor bugs were found using TrustInSoft Analyzer.

Signed-off-by: Julien Cretin <julien.cretin@trust-in-soft.com>
---
 app/test-pmd/config.c  | 16 ++++++++--------
 app/test-pmd/testpmd.c |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1feb133..134bf07 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -100,12 +100,12 @@ static void
 print_ethaddr(const char *name, struct ether_addr *eth_addr)
 {
 	printf("%s%02X:%02X:%02X:%02X:%02X:%02X", name,
-	       eth_addr->addr_bytes[0],
-	       eth_addr->addr_bytes[1],
-	       eth_addr->addr_bytes[2],
-	       eth_addr->addr_bytes[3],
-	       eth_addr->addr_bytes[4],
-	       eth_addr->addr_bytes[5]);
+	       (unsigned int)eth_addr->addr_bytes[0],
+	       (unsigned int)eth_addr->addr_bytes[1],
+	       (unsigned int)eth_addr->addr_bytes[2],
+	       (unsigned int)eth_addr->addr_bytes[3],
+	       (unsigned int)eth_addr->addr_bytes[4],
+	       (unsigned int)eth_addr->addr_bytes[5]);
 }
 
 void
@@ -256,7 +256,7 @@ port_infos_display(portid_t port_id)
 	printf("\n%s Infos for port %-2d %s\n",
 	       info_border, port_id, info_border);
 	print_ethaddr("MAC address: ", &port->eth_addr);
-	printf("\nConnect to socket: %d", port->socket_id);
+	printf("\nConnect to socket: %u", port->socket_id);
 
 	if (port_numa[port_id] != NUMA_NO_CONFIG) {
 		mp = mbuf_pool_find(port_numa[port_id]);
@@ -264,7 +264,7 @@ port_infos_display(portid_t port_id)
 			printf("\nmemory allocation on the socket: %d",
 							port_numa[port_id]);
 	} else
-		printf("\nmemory allocation on the socket: %d",port->socket_id);
+		printf("\nmemory allocation on the socket: %u",port->socket_id);
 
 	printf("\nLink status: %s\n", (link.link_status) ? ("up") : ("down"));
 	printf("Link speed: %u Mbps\n", (unsigned) link.link_speed);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 9c56914..55a5ea1 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1250,7 +1250,7 @@ start_port(portid_t pid)
 		if (port->need_reconfig > 0) {
 			port->need_reconfig = 0;
 
-			printf("Configuring Port %d (socket %d)\n", pi,
+			printf("Configuring Port %d (socket %u)\n", pi,
 					port->socket_id);
 			/* configure port */
 			diag = rte_eth_dev_configure(pi, nb_rxq, nb_txq,
-- 
1.9.1

Re: [dpdk-dev] [PATCH 0/3] Various patches

[Thomas Monjalon]

2014-05-12 17:35, Julien Cretin:
> This patch set contains three unrelated patches found by running
> TrustInSoft Analyzer [1] on some parts of the testpmd application:
> 
> - The first patch fixes a minor signed overflow in a constant
>   expression of testpmd.
> 
> - The second patch is not a fix and concerns a suspicious loop
>   condition in optimize_object_size.
> 
> - The third (and last) patch fixes sign mismatches for some printf
>   arguments.
> 
> [1] TrustInSoft Analyzer (http://trust-in-soft.com) is a static
> analyzer. And as such, it gives information about the execution of a
> source code without actually running it. However, unlike other static
> analysis tools, it has the particularity of being correct. This means
> that it does not remain silent when a run-time error may happen in the
> range of the analysis. In other words, it gives information about all
> possible executions (defined by the analysis) of a source code without
> actually running it.
> 
> Julien Cretin (3):
>   app/testpmd: fix minor signed overflow in a constant
>   mem: remove redundant check in optimize_object_size
>   app/testpmd: fix incompatible sign for printf arguments

Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Applied for version 1.7.0.

Thanks for these difficult catches, especially the mempool/get_gcd one!
-- 
Thomas