diff mbox series

[v2] net/af_packet: add string error for system errors

Message ID 20190710144630.14926-1-kkanas@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] net/af_packet: add string error for system errors |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Krzysztof Kanas July 10, 2019, 2:46 p.m. UTC 
  From: Krzysztof Kanas <kkanas@marvell.com>

Print system error to make easier diagnosis of errors with af_packet.

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 53 ++++++++++-------------
 1 file changed, 24 insertions(+), 29 deletions(-)

Comments

Stephen Hemminger July 10, 2019, 4:04 p.m. UTC | #1 
On Wed, 10 Jul 2019 16:46:30 +0200
<kkanas@marvell.com> wrote:

>  
> +#define PMD_LOG_ERRNO(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, af_packet_logtype, \
> +		"%s(): " fmt ":%s\n", __func__, ##args, rte_strerror(errno))
> +
>  static uint16_t
>  eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  {
> @@ -603,9 +609,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		return -1;
>  	}
>  	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
> -		PMD_LOG(ERR,
> -			"%s: ioctl failed (SIOCGIFINDEX)",
> -		        name);
> +		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
>  		return -1;

This is wrong.
The ioctl sets errno not rte_errno.

Why not?
		PMD_LOG(ERR, "%s: ioctl failed (SIOCGIFINDEX): %s",
			name, strerror(errno));
Krzysztof Kanas July 11, 2019, 9:56 a.m. UTC | #2 
On 19-07-10 09:04, Stephen Hemminger wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 10 Jul 2019 16:46:30 +0200
> <kkanas@marvell.com> wrote:
> 
> >  
> > +#define PMD_LOG_ERRNO(level, fmt, args...) \
> > +	rte_log(RTE_LOG_ ## level, af_packet_logtype, \
> > +		"%s(): " fmt ":%s\n", __func__, ##args, rte_strerror(errno))
> > +
> >  static uint16_t
> >  eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> >  {
> > @@ -603,9 +609,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		return -1;
> >  	}
> >  	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
> > -		PMD_LOG(ERR,
> > -			"%s: ioctl failed (SIOCGIFINDEX)",
> > -		        name);
> > +		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
> >  		return -1;
> 
> This is wrong.
> The ioctl sets errno not rte_errno.
I was following the documentation on rte_strerror, which states:

For non-RTE-specific error codes, this function returns the value from 
the libc strerror function.
>
> 
> Why not?
> 		PMD_LOG(ERR, "%s: ioctl failed (SIOCGIFINDEX): %s",
> 			name, strerror(errno));
Ok, but rte_strerror uses strerror_r, although I am unsure this is 
required here.
Stephen Hemminger July 11, 2019, 4:10 p.m. UTC | #3 
On Thu, 11 Jul 2019 09:56:32 +0000
Krzysztof Kanas <kkanas@marvell.com> wrote:

> On 19-07-10 09:04, Stephen Hemminger wrote:
> > External Email
> > 
> > ----------------------------------------------------------------------
> > On Wed, 10 Jul 2019 16:46:30 +0200
> > <kkanas@marvell.com> wrote:
> >   
> > >  
> > > +#define PMD_LOG_ERRNO(level, fmt, args...) \
> > > +	rte_log(RTE_LOG_ ## level, af_packet_logtype, \
> > > +		"%s(): " fmt ":%s\n", __func__, ##args, rte_strerror(errno))
> > > +
> > >  static uint16_t
> > >  eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> > >  {
> > > @@ -603,9 +609,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> > >  		return -1;
> > >  	}
> > >  	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
> > > -		PMD_LOG(ERR,
> > > -			"%s: ioctl failed (SIOCGIFINDEX)",
> > > -		        name);
> > > +		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
> > >  		return -1;  
> > 
> > This is wrong.
> > The ioctl sets errno not rte_errno.  
> I was following the documentation on rte_strerror, which states:
> 
> For non-RTE-specific error codes, this function returns the value from 
> the libc strerror function

That document is correct, but the intended usage is to do rte_strerror(rte_errno)
and since rte_errno values correspond to errno values for system errors it would work.

In conclusion, your code will work but is a little confusing.
diff mbox series

Patch

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index db1a751cef5b..d4c899526b3c 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -13,7 +13,9 @@ 
 #include <rte_malloc.h>
 #include <rte_kvargs.h>
 #include <rte_bus_vdev.h>
+#include <rte_errno.h>
 
+#include <errno.h>
 #include <linux/if_ether.h>
 #include <linux/if_packet.h>
 #include <arpa/inet.h>
@@ -103,6 +105,10 @@  static int af_packet_logtype;
 	rte_log(RTE_LOG_ ## level, af_packet_logtype, \
 		"%s(): " fmt "\n", __func__, ##args)
 
+#define PMD_LOG_ERRNO(level, fmt, args...) \
+	rte_log(RTE_LOG_ ## level, af_packet_logtype, \
+		"%s(): " fmt ":%s\n", __func__, ##args, rte_strerror(errno))
+
 static uint16_t
 eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
@@ -603,9 +609,7 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 		return -1;
 	}
 	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
-		PMD_LOG(ERR,
-			"%s: ioctl failed (SIOCGIFINDEX)",
-		        name);
+		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
 		return -1;
 	}
 	(*internals)->if_name = strdup(pair->value);
@@ -614,9 +618,7 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 	(*internals)->if_index = ifr.ifr_ifindex;
 
 	if (ioctl(sockfd, SIOCGIFHWADDR, &ifr) == -1) {
-		PMD_LOG(ERR,
-			"%s: ioctl failed (SIOCGIFHWADDR)",
-		        name);
+		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFHWADDR)", name);
 		return -1;
 	}
 	memcpy(&(*internals)->eth_addr, ifr.ifr_hwaddr.sa_data, ETH_ALEN);
@@ -638,9 +640,8 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 		/* Open an AF_PACKET socket for this queue... */
 		qsockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
 		if (qsockfd == -1) {
-			PMD_LOG(ERR,
-				"%s: could not open AF_PACKET socket",
-			        name);
+			PMD_LOG_ERRNO(ERR, "%s: could not open AF_PACKET socket",
+				      name);
 			return -1;
 		}
 
@@ -648,9 +649,8 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_VERSION,
 				&tpver, sizeof(tpver));
 		if (rc == -1) {
-			PMD_LOG(ERR,
-				"%s: could not set PACKET_VERSION on AF_PACKET socket for %s",
-				name, pair->value);
+			PMD_LOG_ERRNO(ERR, "%s: could not set PACKET_VERSION on AF_PACKET socket for %s:",
+				      name, pair->value);
 			goto error;
 		}
 
@@ -658,9 +658,8 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_LOSS,
 				&discard, sizeof(discard));
 		if (rc == -1) {
-			PMD_LOG(ERR,
-				"%s: could not set PACKET_LOSS on AF_PACKET socket for %s",
-				name, pair->value);
+			PMD_LOG_ERRNO(ERR, "%s: could not set PACKET_LOSS on AF_PACKET socket for %s",
+				      name, pair->value);
 			goto error;
 		}
 
@@ -668,9 +667,8 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_QDISC_BYPASS,
 				&qdisc_bypass, sizeof(qdisc_bypass));
 		if (rc == -1) {
-			PMD_LOG(ERR,
-				"%s: could not set PACKET_QDISC_BYPASS on AF_PACKET socket for %s",
-				name, pair->value);
+			PMD_LOG_ERRNO(ERR, "%s: could not set PACKET_QDISC_BYPASS on AF_PACKET socket for %s",
+				      name, pair->value);
 			goto error;
 		}
 #else
@@ -679,15 +677,14 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_RX_RING, req, sizeof(*req));
 		if (rc == -1) {
-			PMD_LOG(ERR,
-				"%s: could not set PACKET_RX_RING on AF_PACKET socket for %s",
-				name, pair->value);
+			PMD_LOG_ERRNO(ERR, "%s: could not set PACKET_RX_RING on AF_PACKE socket for %s",
+				      name, pair->value);
 			goto error;
 		}
 
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_TX_RING, req, sizeof(*req));
 		if (rc == -1) {
-			PMD_LOG(ERR,
+			PMD_LOG_ERRNO(ERR,
 				"%s: could not set PACKET_TX_RING on AF_PACKET "
 				"socket for %s", name, pair->value);
 			goto error;
@@ -700,7 +697,7 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 				    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_LOCKED,
 				    qsockfd, 0);
 		if (rx_queue->map == MAP_FAILED) {
-			PMD_LOG(ERR,
+			PMD_LOG_ERRNO(ERR,
 				"%s: call to mmap failed on AF_PACKET socket for %s",
 				name, pair->value);
 			goto error;
@@ -737,9 +734,8 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 
 		rc = bind(qsockfd, (const struct sockaddr*)&sockaddr, sizeof(sockaddr));
 		if (rc == -1) {
-			PMD_LOG(ERR,
-				"%s: could not bind AF_PACKET socket to %s",
-			        name, pair->value);
+			PMD_LOG_ERRNO(ERR, "%s: could not bind AF_PACKET socket to %s",
+				name, pair->value);
 			goto error;
 		}
 
@@ -747,9 +743,8 @@  rte_pmd_init_internals(struct rte_vdev_device *dev,
 		rc = setsockopt(qsockfd, SOL_PACKET, PACKET_FANOUT,
 				&fanout_arg, sizeof(fanout_arg));
 		if (rc == -1) {
-			PMD_LOG(ERR,
-				"%s: could not set PACKET_FANOUT on AF_PACKET socket "
-				"for %s", name, pair->value);
+			PMD_LOG_ERRNO(ERR, "%s: could not set PACKET_FANOUT on AF_PACKET socket for %s",
+				name, pair->value);
 			goto error;
 		}
 #endif