[dpdk-dev] [PATCH v2] net/pcap: support snaplen option to truncate packet

[dpdk-dev] [PATCH v2] net/pcap: support snaplen option to truncate packet

[Zhike Wang]

Introduced "snaplen=<length>" option. It is convenient to truncate
large packets to only capture necessary headers.

Signed-off-by: Zhike Wang <wangzhike@jd.com>
---
 app/pdump/main.c                | 32 +++++++++++++++++++++++++++++++-
 drivers/net/pcap/rte_eth_pcap.c | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c38c537..241346b 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -41,10 +41,12 @@
 #define PDUMP_RING_SIZE_ARG "ring-size"
 #define PDUMP_MSIZE_ARG "mbuf-size"
 #define PDUMP_NUM_MBUFS_ARG "total-num-mbufs"
+#define PDUMP_SNAPLEN_ARG "snaplen"
 
 #define VDEV_NAME_FMT "net_pcap_%s_%d"
 #define VDEV_PCAP_ARGS_FMT "tx_pcap=%s"
 #define VDEV_IFACE_ARGS_FMT "tx_iface=%s"
+#define VDEV_SNAPLEN_ARGS_FMT "snaplen=%d"
 #define TX_STREAM_SIZE 64
 
 #define MP_NAME "pdump_pool_%d"
@@ -97,6 +99,7 @@ enum pdump_by {
 	PDUMP_RING_SIZE_ARG,
 	PDUMP_MSIZE_ARG,
 	PDUMP_NUM_MBUFS_ARG,
+	PDUMP_SNAPLEN_ARG,
 	NULL
 };
 
@@ -116,6 +119,7 @@ struct pdump_tuples {
 	uint32_t ring_size;
 	uint16_t mbuf_data_size;
 	uint32_t total_num_mbufs;
+	uint16_t snaplen;
 
 	/* params for library API call */
 	uint32_t dir;
@@ -160,7 +164,8 @@ struct parse_val {
 			" tx-dev=<iface or pcap file>,"
 			"[ring-size=<ring size>default:16384],"
 			"[mbuf-size=<mbuf data size>default:2176],"
-			"[total-num-mbufs=<number of mbufs>default:65535]'\n",
+			"[total-num-mbufs=<number of mbufs>default:65535],",
+			"[snaplen=<snap length>default:0, meaning no truncation]'\n",
 			prgname);
 }
 
@@ -370,6 +375,19 @@ struct parse_val {
 	} else
 		pt->total_num_mbufs = MBUFS_PER_POOL;
 
+	/* snaplen parsing and validation */
+	cnt1 = rte_kvargs_count(kvlist, PDUMP_SNAPLEN_ARG);
+	if (cnt1 == 1) {
+		v.min = 1;
+		v.max = UINT16_MAX;
+		ret = rte_kvargs_process(kvlist, PDUMP_SNAPLEN_ARG,
+						&parse_uint_value, &v);
+		if (ret < 0)
+			goto free_kvlist;
+		pt->snaplen = (uint16_t) v.val;
+	} else
+		pt->snaplen = 0;
+
 	num_tuples++;
 
 free_kvlist:
@@ -692,6 +710,9 @@ struct parse_val {
 				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
 			snprintf(vdev_args, sizeof(vdev_args),
 				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
+			snprintf(vdev_args + strlen(vdev_args),
+				 sizeof(vdev_args) - strlen(vdev_args),
+				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 			if (rte_eal_hotplug_add("vdev", vdev_name,
 						vdev_args) < 0) {
 				cleanup_rings();
@@ -722,6 +743,9 @@ struct parse_val {
 					 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
 				snprintf(vdev_args, sizeof(vdev_args),
 					 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
+				snprintf(vdev_args + strlen(vdev_args),
+					 sizeof(vdev_args) - strlen(vdev_args),
+					 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 				if (rte_eal_hotplug_add("vdev", vdev_name,
 							vdev_args) < 0) {
 					cleanup_rings();
@@ -762,6 +786,9 @@ struct parse_val {
 				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
 			snprintf(vdev_args, sizeof(vdev_args),
 				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
+			snprintf(vdev_args + strlen(vdev_args),
+				 sizeof(vdev_args) - strlen(vdev_args),
+				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 			if (rte_eal_hotplug_add("vdev", vdev_name,
 						vdev_args) < 0) {
 				cleanup_rings();
@@ -799,6 +826,9 @@ struct parse_val {
 				 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
 			snprintf(vdev_args, sizeof(vdev_args),
 				 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
+			snprintf(vdev_args + strlen(vdev_args),
+				 sizeof(vdev_args) - strlen(vdev_args),
+				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 			if (rte_eal_hotplug_add("vdev", vdev_name,
 						vdev_args) < 0) {
 				cleanup_rings();
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 13a3d0a..d51ea48 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -40,6 +40,7 @@
 #define ETH_PCAP_IFACE_ARG    "iface"
 #define ETH_PCAP_PHY_MAC_ARG  "phy_mac"
 #define ETH_PCAP_INFINITE_RX_ARG  "infinite_rx"
+#define ETH_PCAP_SNAPLEN_ARG  "snaplen"
 
 #define ETH_PCAP_ARG_MAXLEN	64
 
@@ -92,6 +93,7 @@ struct pmd_process_private {
 	pcap_t *rx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
 	pcap_t *tx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
 	pcap_dumper_t *tx_dumper[RTE_PMD_PCAP_MAX_QUEUES];
+	int snaplen;
 };
 
 struct pmd_devargs {
@@ -114,6 +116,7 @@ struct pmd_devargs_all {
 	unsigned int is_rx_pcap;
 	unsigned int is_rx_iface;
 	unsigned int infinite_rx;
+	int snaplen;
 };
 
 static const char *valid_arguments[] = {
@@ -125,6 +128,7 @@ struct pmd_devargs_all {
 	ETH_PCAP_IFACE_ARG,
 	ETH_PCAP_PHY_MAC_ARG,
 	ETH_PCAP_INFINITE_RX_ARG,
+	ETH_PCAP_SNAPLEN_ARG,
 	NULL
 };
 
@@ -339,6 +343,9 @@ struct pmd_devargs_all {
 			caplen = sizeof(temp_data);
 		}
 
+		if ((pp->snaplen > 0) && (caplen > pp->snaplen))
+			caplen = pp->snaplen;
+
 		calculate_timestamp(&header.ts);
 		header.len = len;
 		header.caplen = caplen;
@@ -949,6 +956,7 @@ struct pmd_devargs_all {
 {
 	const char *pcap_filename = value;
 	struct pmd_devargs *dumpers = extra_args;
+	int snaplen;
 	pcap_dumper_t *dumper;
 
 	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
@@ -1083,6 +1091,19 @@ struct pmd_devargs_all {
 }
 
 static int
+get_snaplen_arg(const char *key __rte_unused,
+		const char *value, void *extra_args)
+{
+	if (extra_args) {
+		const int snaplen = atoi(value);
+		int *snaplen_p = extra_args;
+
+		*snaplen_p = snaplen;
+	}
+	return 0;
+}
+
+static int
 pmd_init_internals(struct rte_vdev_device *vdev,
 		const unsigned int nb_rx_queues,
 		const unsigned int nb_tx_queues,
@@ -1335,6 +1356,7 @@ struct pmd_devargs_all {
 	struct rte_eth_dev *eth_dev =  NULL;
 	struct pmd_internals *internal;
 	int ret = 0;
+	unsigned int snaplen_cnt;
 
 	struct pmd_devargs_all devargs_all = {
 		.single_iface = 0,
@@ -1412,6 +1434,15 @@ struct pmd_devargs_all {
 		rte_kvargs_count(kvlist, ETH_PCAP_TX_IFACE_ARG) ? 1 : 0;
 	dumpers.num_of_queue = 0;
 
+	snaplen_cnt = rte_kvargs_count(kvlist,
+				ETH_PCAP_SNAPLEN_ARG);
+	if (snaplen_cnt == 1) {
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
+				&get_snaplen_arg, &devargs_all.snaplen);
+		if (ret < 0)
+			goto free_kvlist;
+	}
+
 	if (devargs_all.is_rx_pcap) {
 		/*
 		 * We check whether we want to infinitely rx the pcap file.
@@ -1518,6 +1549,7 @@ struct pmd_devargs_all {
 			pp->tx_pcap[i] = dumpers.queue[i].pcap;
 		}
 
+		pp->snaplen = devargs_all.snaplen;
 		eth_dev->process_private = pp;
 		eth_dev->rx_pkt_burst = eth_pcap_rx;
 		if (devargs_all.is_tx_pcap)
@@ -1587,7 +1619,8 @@ struct pmd_devargs_all {
 	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
 	ETH_PCAP_IFACE_ARG "=<ifc> "
 	ETH_PCAP_PHY_MAC_ARG "=<int>"
-	ETH_PCAP_INFINITE_RX_ARG "=<0|1>");
+	ETH_PCAP_INFINITE_RX_ARG "=<0|1>"
+	ETH_PCAP_SNAPLEN_ARG "=<int>");
 
 RTE_INIT(eth_pcap_init_log)
 {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/pcap: support snaplen option to truncate packet

[Ferruh Yigit]

On 7/3/2020 7:47 AM, Zhike Wang wrote:
> Introduced "snaplen=<length>" option. It is convenient to truncate
> large packets to only capture necessary headers.
> 
> Signed-off-by: Zhike Wang <wangzhike@jd.com>
> ---
>  app/pdump/main.c                | 32 +++++++++++++++++++++++++++++++-
>  drivers/net/pcap/rte_eth_pcap.c | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index c38c537..241346b 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -41,10 +41,12 @@
>  #define PDUMP_RING_SIZE_ARG "ring-size"
>  #define PDUMP_MSIZE_ARG "mbuf-size"
>  #define PDUMP_NUM_MBUFS_ARG "total-num-mbufs"
> +#define PDUMP_SNAPLEN_ARG "snaplen"
>  
>  #define VDEV_NAME_FMT "net_pcap_%s_%d"
>  #define VDEV_PCAP_ARGS_FMT "tx_pcap=%s"
>  #define VDEV_IFACE_ARGS_FMT "tx_iface=%s"
> +#define VDEV_SNAPLEN_ARGS_FMT "snaplen=%d"
>  #define TX_STREAM_SIZE 64
>  
>  #define MP_NAME "pdump_pool_%d"
> @@ -97,6 +99,7 @@ enum pdump_by {
>  	PDUMP_RING_SIZE_ARG,
>  	PDUMP_MSIZE_ARG,
>  	PDUMP_NUM_MBUFS_ARG,
> +	PDUMP_SNAPLEN_ARG,
>  	NULL
>  };
>  
> @@ -116,6 +119,7 @@ struct pdump_tuples {
>  	uint32_t ring_size;
>  	uint16_t mbuf_data_size;
>  	uint32_t total_num_mbufs;
> +	uint16_t snaplen;
>  
>  	/* params for library API call */
>  	uint32_t dir;
> @@ -160,7 +164,8 @@ struct parse_val {
>  			" tx-dev=<iface or pcap file>,"
>  			"[ring-size=<ring size>default:16384],"
>  			"[mbuf-size=<mbuf data size>default:2176],"
> -			"[total-num-mbufs=<number of mbufs>default:65535]'\n",
> +			"[total-num-mbufs=<number of mbufs>default:65535],",
> +			"[snaplen=<snap length>default:0, meaning no truncation]'\n",
>  			prgname);
>  }
>  
> @@ -370,6 +375,19 @@ struct parse_val {
>  	} else
>  		pt->total_num_mbufs = MBUFS_PER_POOL;
>  
> +	/* snaplen parsing and validation */
> +	cnt1 = rte_kvargs_count(kvlist, PDUMP_SNAPLEN_ARG);
> +	if (cnt1 == 1) {
> +		v.min = 1;
> +		v.max = UINT16_MAX;
> +		ret = rte_kvargs_process(kvlist, PDUMP_SNAPLEN_ARG,
> +						&parse_uint_value, &v);
> +		if (ret < 0)
> +			goto free_kvlist;
> +		pt->snaplen = (uint16_t) v.val;
> +	} else
> +		pt->snaplen = 0;
> +
>  	num_tuples++;
>  
>  free_kvlist:
> @@ -692,6 +710,9 @@ struct parse_val {
>  				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
>  			snprintf(vdev_args, sizeof(vdev_args),
>  				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
> +			snprintf(vdev_args + strlen(vdev_args),
> +				 sizeof(vdev_args) - strlen(vdev_args),
> +				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  			if (rte_eal_hotplug_add("vdev", vdev_name,
>  						vdev_args) < 0) {
>  				cleanup_rings();
> @@ -722,6 +743,9 @@ struct parse_val {
>  					 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
>  				snprintf(vdev_args, sizeof(vdev_args),
>  					 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
> +				snprintf(vdev_args + strlen(vdev_args),
> +					 sizeof(vdev_args) - strlen(vdev_args),
> +					 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  				if (rte_eal_hotplug_add("vdev", vdev_name,
>  							vdev_args) < 0) {
>  					cleanup_rings();
> @@ -762,6 +786,9 @@ struct parse_val {
>  				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
>  			snprintf(vdev_args, sizeof(vdev_args),
>  				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
> +			snprintf(vdev_args + strlen(vdev_args),
> +				 sizeof(vdev_args) - strlen(vdev_args),
> +				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  			if (rte_eal_hotplug_add("vdev", vdev_name,
>  						vdev_args) < 0) {
>  				cleanup_rings();
> @@ -799,6 +826,9 @@ struct parse_val {
>  				 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
>  			snprintf(vdev_args, sizeof(vdev_args),
>  				 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
> +			snprintf(vdev_args + strlen(vdev_args),
> +				 sizeof(vdev_args) - strlen(vdev_args),
> +				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  			if (rte_eal_hotplug_add("vdev", vdev_name,
>  						vdev_args) < 0) {
>  				cleanup_rings();
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 13a3d0a..d51ea48 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -40,6 +40,7 @@
>  #define ETH_PCAP_IFACE_ARG    "iface"
>  #define ETH_PCAP_PHY_MAC_ARG  "phy_mac"
>  #define ETH_PCAP_INFINITE_RX_ARG  "infinite_rx"
> +#define ETH_PCAP_SNAPLEN_ARG  "snaplen"
>  
>  #define ETH_PCAP_ARG_MAXLEN	64
>  
> @@ -92,6 +93,7 @@ struct pmd_process_private {
>  	pcap_t *rx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
>  	pcap_t *tx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
>  	pcap_dumper_t *tx_dumper[RTE_PMD_PCAP_MAX_QUEUES];
> +	int snaplen;

As it shouldn't be negative, choosing "unsigned int" or "size_t" type can be better.

Also is the variable added to 'process_private' memmory intentionally? Why not
in the shared device data ('pmd_internals') memory?

>  };
>  
>  struct pmd_devargs {
> @@ -114,6 +116,7 @@ struct pmd_devargs_all {
>  	unsigned int is_rx_pcap;
>  	unsigned int is_rx_iface;
>  	unsigned int infinite_rx;
> +	int snaplen;
>  };
>  
>  static const char *valid_arguments[] = {
> @@ -125,6 +128,7 @@ struct pmd_devargs_all {
>  	ETH_PCAP_IFACE_ARG,
>  	ETH_PCAP_PHY_MAC_ARG,
>  	ETH_PCAP_INFINITE_RX_ARG,
> +	ETH_PCAP_SNAPLEN_ARG,
>  	NULL
>  };
>  
> @@ -339,6 +343,9 @@ struct pmd_devargs_all {
>  			caplen = sizeof(temp_data);
>  		}
>  
> +		if ((pp->snaplen > 0) && (caplen > pp->snaplen))
> +			caplen = pp->snaplen;

This "is 'snaplen' set" check done per packet, and it will hit both 'snaplen'
set or not, I wonder if there is a way to get rid of per packet check?

>  		calculate_timestamp(&header.ts);
>  		header.len = len;
>  		header.caplen = caplen;
> @@ -949,6 +956,7 @@ struct pmd_devargs_all {
>  {
>  	const char *pcap_filename = value;
>  	struct pmd_devargs *dumpers = extra_args;
> +	int snaplen;

This is not used at all, I guess added by mistake.

>  	pcap_dumper_t *dumper;
>  
>  	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
> @@ -1083,6 +1091,19 @@ struct pmd_devargs_all {
>  }
>  
>  static int
> +get_snaplen_arg(const char *key __rte_unused,
> +		const char *value, void *extra_args)
> +{
> +	if (extra_args) {
> +		const int snaplen = atoi(value);

'const' qualifier is not needed.

And some error checks can be better.

> +		int *snaplen_p = extra_args;
> +
> +		*snaplen_p = snaplen;
> +	}
> +	return 0;
> +}
> +
> +static int
>  pmd_init_internals(struct rte_vdev_device *vdev,
>  		const unsigned int nb_rx_queues,
>  		const unsigned int nb_tx_queues,
> @@ -1335,6 +1356,7 @@ struct pmd_devargs_all {
>  	struct rte_eth_dev *eth_dev =  NULL;
>  	struct pmd_internals *internal;
>  	int ret = 0;
> +	unsigned int snaplen_cnt;
>  
>  	struct pmd_devargs_all devargs_all = {
>  		.single_iface = 0,
> @@ -1412,6 +1434,15 @@ struct pmd_devargs_all {
>  		rte_kvargs_count(kvlist, ETH_PCAP_TX_IFACE_ARG) ? 1 : 0;
>  	dumpers.num_of_queue = 0;
>  
> +	snaplen_cnt = rte_kvargs_count(kvlist,
> +				ETH_PCAP_SNAPLEN_ARG);
> +	if (snaplen_cnt == 1) {

No need to get the count of the devargs. Just calling 'rte_kvargs_process()'
should be enough.

> +		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
> +				&get_snaplen_arg, &devargs_all.snaplen);

'snaplen' make sense only for 'is_tx_pcap' case, so you can process the
'ETH_PCAP_SNAPLEN_ARG' arg only for 'is_tx_pcap' case.

> +		if (ret < 0)
> +			goto free_kvlist;
> +	}
> +
>  	if (devargs_all.is_rx_pcap) {
>  		/*
>  		 * We check whether we want to infinitely rx the pcap file.
> @@ -1518,6 +1549,7 @@ struct pmd_devargs_all {
>  			pp->tx_pcap[i] = dumpers.queue[i].pcap;
>  		}
>  
> +		pp->snaplen = devargs_all.snaplen;

This is inside the "rte_eal_process_type() == RTE_PROC_SECONDARY" block, why
'snaplen' only used for secondary process?

>  		eth_dev->process_private = pp;
>  		eth_dev->rx_pkt_burst = eth_pcap_rx;
>  		if (devargs_all.is_tx_pcap)
> @@ -1587,7 +1619,8 @@ struct pmd_devargs_all {
>  	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_PHY_MAC_ARG "=<int>"
> -	ETH_PCAP_INFINITE_RX_ARG "=<0|1>");
> +	ETH_PCAP_INFINITE_RX_ARG "=<0|1>"
> +	ETH_PCAP_SNAPLEN_ARG "=<int>");
>  
>  RTE_INIT(eth_pcap_init_log)
>  {
> 

[dpdk-dev] [PATCH v4] net/pcap: support snaplen option to truncate packet

[Zhike Wang]

Introduced "snaplen=<length>" option. It is convenient to truncate
large packets to only capture necessary headers.

Signed-off-by: Zhike Wang <wangzhike@jd.com>
---
 app/pdump/main.c                       | 32 ++++++++++++++++++++++++++-
 doc/guides/rel_notes/release_20_08.rst |  4 ++++
 doc/guides/tools/pdump.rst             |  5 ++++-
 drivers/net/pcap/rte_eth_pcap.c        | 40 ++++++++++++++++++++++++++++++++--
 4 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index c38c537..1f87310 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -41,10 +41,12 @@
 #define PDUMP_RING_SIZE_ARG "ring-size"
 #define PDUMP_MSIZE_ARG "mbuf-size"
 #define PDUMP_NUM_MBUFS_ARG "total-num-mbufs"
+#define PDUMP_SNAPLEN_ARG "snaplen"
 
 #define VDEV_NAME_FMT "net_pcap_%s_%d"
 #define VDEV_PCAP_ARGS_FMT "tx_pcap=%s"
 #define VDEV_IFACE_ARGS_FMT "tx_iface=%s"
+#define VDEV_SNAPLEN_ARGS_FMT "snaplen=%d"
 #define TX_STREAM_SIZE 64
 
 #define MP_NAME "pdump_pool_%d"
@@ -97,6 +99,7 @@ enum pdump_by {
 	PDUMP_RING_SIZE_ARG,
 	PDUMP_MSIZE_ARG,
 	PDUMP_NUM_MBUFS_ARG,
+	PDUMP_SNAPLEN_ARG,
 	NULL
 };
 
@@ -116,6 +119,7 @@ struct pdump_tuples {
 	uint32_t ring_size;
 	uint16_t mbuf_data_size;
 	uint32_t total_num_mbufs;
+	uint16_t snaplen;
 
 	/* params for library API call */
 	uint32_t dir;
@@ -160,7 +164,8 @@ struct parse_val {
 			" tx-dev=<iface or pcap file>,"
 			"[ring-size=<ring size>default:16384],"
 			"[mbuf-size=<mbuf data size>default:2176],"
-			"[total-num-mbufs=<number of mbufs>default:65535]'\n",
+			"[total-num-mbufs=<number of mbufs>default:65535],"
+			"[snaplen=<snap length>default:0, meaning no truncation]'\n",
 			prgname);
 }
 
@@ -370,6 +375,19 @@ struct parse_val {
 	} else
 		pt->total_num_mbufs = MBUFS_PER_POOL;
 
+	/* snaplen parsing and validation */
+	cnt1 = rte_kvargs_count(kvlist, PDUMP_SNAPLEN_ARG);
+	if (cnt1 == 1) {
+		v.min = 1;
+		v.max = UINT16_MAX;
+		ret = rte_kvargs_process(kvlist, PDUMP_SNAPLEN_ARG,
+						&parse_uint_value, &v);
+		if (ret < 0)
+			goto free_kvlist;
+		pt->snaplen = (uint16_t) v.val;
+	} else
+		pt->snaplen = 0;
+
 	num_tuples++;
 
 free_kvlist:
@@ -692,6 +710,9 @@ struct parse_val {
 				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
 			snprintf(vdev_args, sizeof(vdev_args),
 				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
+			snprintf(vdev_args + strlen(vdev_args),
+				 sizeof(vdev_args) - strlen(vdev_args),
+				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 			if (rte_eal_hotplug_add("vdev", vdev_name,
 						vdev_args) < 0) {
 				cleanup_rings();
@@ -722,6 +743,9 @@ struct parse_val {
 					 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
 				snprintf(vdev_args, sizeof(vdev_args),
 					 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
+				snprintf(vdev_args + strlen(vdev_args),
+					 sizeof(vdev_args) - strlen(vdev_args),
+					 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 				if (rte_eal_hotplug_add("vdev", vdev_name,
 							vdev_args) < 0) {
 					cleanup_rings();
@@ -762,6 +786,9 @@ struct parse_val {
 				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
 			snprintf(vdev_args, sizeof(vdev_args),
 				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
+			snprintf(vdev_args + strlen(vdev_args),
+				 sizeof(vdev_args) - strlen(vdev_args),
+				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 			if (rte_eal_hotplug_add("vdev", vdev_name,
 						vdev_args) < 0) {
 				cleanup_rings();
@@ -799,6 +826,9 @@ struct parse_val {
 				 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
 			snprintf(vdev_args, sizeof(vdev_args),
 				 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
+			snprintf(vdev_args + strlen(vdev_args),
+				 sizeof(vdev_args) - strlen(vdev_args),
+				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
 			if (rte_eal_hotplug_add("vdev", vdev_name,
 						vdev_args) < 0) {
 				cleanup_rings();
diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
index 17d70e7..462c4f3 100644
--- a/doc/guides/rel_notes/release_20_08.rst
+++ b/doc/guides/rel_notes/release_20_08.rst
@@ -171,6 +171,10 @@ New Features
   See the :doc:`../sample_app_ug/l2_forward_real_virtual` for more
   details of this parameter usage.
 
+* **Added a devarg to truncate the dumped packets for PCAP vdev.**
+
+  A new devarg ``snaplen`` was introduced to allow users to truncate the
+  dumped packets, and is convenient for capturing with large packet size.
 
 Removed Items
 -------------
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 8a499c6..27b9102 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -45,7 +45,8 @@ The tool has a number of command line options:
                                     tx-dev=<iface or pcap file>),
                                    [ring-size=<ring size>],
                                    [mbuf-size=<mbuf data size>],
-                                   [total-num-mbufs=<number of mbufs>]'
+                                   [total-num-mbufs=<number of mbufs>],
+                                   [snaplen=<snap length>]'
 
 The ``--multi`` command line option is optional argument. If passed, capture
 will be running on unique cores for all ``--pdump`` options. If ignored,
@@ -114,6 +115,8 @@ default size 2176.
 Total number mbufs in mempool. This is used internally for mempool creation. This is an optional parameter with default
 value 65535.
 
+``snaplen``:
+Truncate snaplen bytes of data from each packet. This is an optional parameter with default value 0, meaning no truncation.
 
 Example
 -------
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 668cbd1..0d2a4b3 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -40,6 +40,7 @@
 #define ETH_PCAP_IFACE_ARG    "iface"
 #define ETH_PCAP_PHY_MAC_ARG  "phy_mac"
 #define ETH_PCAP_INFINITE_RX_ARG  "infinite_rx"
+#define ETH_PCAP_SNAPLEN_ARG  "snaplen"
 
 #define ETH_PCAP_ARG_MAXLEN	64
 
@@ -86,6 +87,7 @@ struct pmd_internals {
 	int single_iface;
 	int phy_mac;
 	unsigned int infinite_rx;
+	unsigned int snaplen;
 };
 
 struct pmd_process_private {
@@ -114,6 +116,7 @@ struct pmd_devargs_all {
 	unsigned int is_rx_pcap;
 	unsigned int is_rx_iface;
 	unsigned int infinite_rx;
+	unsigned int snaplen;
 };
 
 static const char *valid_arguments[] = {
@@ -125,6 +128,7 @@ struct pmd_devargs_all {
 	ETH_PCAP_IFACE_ARG,
 	ETH_PCAP_PHY_MAC_ARG,
 	ETH_PCAP_INFINITE_RX_ARG,
+	ETH_PCAP_SNAPLEN_ARG,
 	NULL
 };
 
@@ -322,11 +326,13 @@ struct pmd_devargs_all {
 	pcap_dumper_t *dumper;
 	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
 	size_t len, caplen;
+	struct pmd_internals *internal;
 
 	pp = rte_eth_devices[dumper_q->port_id].process_private;
 	dumper = pp->tx_dumper[dumper_q->queue_id];
+	internal = rte_eth_devices[dumper_q->port_id].data->dev_private;
 
-	if (dumper == NULL || nb_pkts == 0)
+	if (dumper == NULL || nb_pkts == 0 || internal == NULL)
 		return 0;
 
 	/* writes the nb_pkts packets to the previously opened pcap file
@@ -339,6 +345,9 @@ struct pmd_devargs_all {
 			caplen = sizeof(temp_data);
 		}
 
+		if (caplen > internal->snaplen)
+			caplen = internal->snaplen;
+
 		calculate_timestamp(&header.ts);
 		header.len = len;
 		header.caplen = caplen;
@@ -1083,6 +1092,21 @@ struct pmd_devargs_all {
 }
 
 static int
+get_snaplen_arg(const char *key __rte_unused,
+		const char *value, void *extra_args)
+{
+	if (extra_args) {
+		unsigned int snaplen = (unsigned int)atoi(value);
+		unsigned int *snaplen_p = extra_args;
+
+		if (snaplen == 0)
+			snaplen = RTE_ETH_PCAP_SNAPLEN;
+		*snaplen_p = snaplen;
+	}
+	return 0;
+}
+
+static int
 pmd_init_internals(struct rte_vdev_device *vdev,
 		const unsigned int nb_rx_queues,
 		const unsigned int nb_tx_queues,
@@ -1291,6 +1315,9 @@ struct pmd_devargs_all {
 	/* store weather we are using a single interface for rx/tx or not */
 	internals->single_iface = single_iface;
 
+	if (devargs_all->is_tx_pcap)
+		internals->snaplen = devargs_all->snaplen;
+
 	if (single_iface) {
 		internals->if_index = if_nametoindex(rx_queues->queue[0].name);
 
@@ -1341,6 +1368,7 @@ struct pmd_devargs_all {
 		.is_tx_pcap = 0,
 		.is_tx_iface = 0,
 		.infinite_rx = 0,
+		.snaplen = RTE_ETH_PCAP_SNAPLEN,
 	};
 
 	name = rte_vdev_device_name(dev);
@@ -1464,6 +1492,13 @@ struct pmd_devargs_all {
 	if (ret < 0)
 		goto free_kvlist;
 
+	if (devargs_all.is_tx_pcap) {
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
+				&get_snaplen_arg, &devargs_all.snaplen);
+		if (ret < 0)
+			goto free_kvlist;
+	}
+
 	/*
 	 * We check whether we want to open a TX stream to a real NIC,
 	 * a pcap file, or drop packets on tx
@@ -1587,4 +1622,5 @@ struct pmd_devargs_all {
 	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
 	ETH_PCAP_IFACE_ARG "=<ifc> "
 	ETH_PCAP_PHY_MAC_ARG "=<int>"
-	ETH_PCAP_INFINITE_RX_ARG "=<0|1>");
+	ETH_PCAP_INFINITE_RX_ARG "=<0|1>"
+	ETH_PCAP_SNAPLEN_ARG "=<int>");
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] net/pcap: support snaplen option to truncate packet

[王志克]

Thanks for review.

Sent v4.

Br,
Zhike Wang 
JDCloud, Product Development, IaaS   
------------------------------------------------------------------------------------------------
Mobile／+86 13466719566
E- mail／wangzhike@jd.com
Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
Https://JDCloud.com
------------------------------------------------------------------------------------------------



-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] 
Sent: Saturday, July 11, 2020 9:56 AM
To: 王志克; dev@dpdk.org
Cc: reshma.pattan@intel.com
Subject: Re: [PATCH v2] net/pcap: support snaplen option to truncate packet

On 7/3/2020 7:47 AM, Zhike Wang wrote:
> Introduced "snaplen=<length>" option. It is convenient to truncate
> large packets to only capture necessary headers.
> 
> Signed-off-by: Zhike Wang <wangzhike@jd.com>
> ---
>  app/pdump/main.c                | 32 +++++++++++++++++++++++++++++++-
>  drivers/net/pcap/rte_eth_pcap.c | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index c38c537..241346b 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -41,10 +41,12 @@
>  #define PDUMP_RING_SIZE_ARG "ring-size"
>  #define PDUMP_MSIZE_ARG "mbuf-size"
>  #define PDUMP_NUM_MBUFS_ARG "total-num-mbufs"
> +#define PDUMP_SNAPLEN_ARG "snaplen"
>  
>  #define VDEV_NAME_FMT "net_pcap_%s_%d"
>  #define VDEV_PCAP_ARGS_FMT "tx_pcap=%s"
>  #define VDEV_IFACE_ARGS_FMT "tx_iface=%s"
> +#define VDEV_SNAPLEN_ARGS_FMT "snaplen=%d"
>  #define TX_STREAM_SIZE 64
>  
>  #define MP_NAME "pdump_pool_%d"
> @@ -97,6 +99,7 @@ enum pdump_by {
>  	PDUMP_RING_SIZE_ARG,
>  	PDUMP_MSIZE_ARG,
>  	PDUMP_NUM_MBUFS_ARG,
> +	PDUMP_SNAPLEN_ARG,
>  	NULL
>  };
>  
> @@ -116,6 +119,7 @@ struct pdump_tuples {
>  	uint32_t ring_size;
>  	uint16_t mbuf_data_size;
>  	uint32_t total_num_mbufs;
> +	uint16_t snaplen;
>  
>  	/* params for library API call */
>  	uint32_t dir;
> @@ -160,7 +164,8 @@ struct parse_val {
>  			" tx-dev=<iface or pcap file>,"
>  			"[ring-size=<ring size>default:16384],"
>  			"[mbuf-size=<mbuf data size>default:2176],"
> -			"[total-num-mbufs=<number of mbufs>default:65535]'\n",
> +			"[total-num-mbufs=<number of mbufs>default:65535],",
> +			"[snaplen=<snap length>default:0, meaning no truncation]'\n",
>  			prgname);
>  }
>  
> @@ -370,6 +375,19 @@ struct parse_val {
>  	} else
>  		pt->total_num_mbufs = MBUFS_PER_POOL;
>  
> +	/* snaplen parsing and validation */
> +	cnt1 = rte_kvargs_count(kvlist, PDUMP_SNAPLEN_ARG);
> +	if (cnt1 == 1) {
> +		v.min = 1;
> +		v.max = UINT16_MAX;
> +		ret = rte_kvargs_process(kvlist, PDUMP_SNAPLEN_ARG,
> +						&parse_uint_value, &v);
> +		if (ret < 0)
> +			goto free_kvlist;
> +		pt->snaplen = (uint16_t) v.val;
> +	} else
> +		pt->snaplen = 0;
> +
>  	num_tuples++;
>  
>  free_kvlist:
> @@ -692,6 +710,9 @@ struct parse_val {
>  				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
>  			snprintf(vdev_args, sizeof(vdev_args),
>  				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
> +			snprintf(vdev_args + strlen(vdev_args),
> +				 sizeof(vdev_args) - strlen(vdev_args),
> +				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  			if (rte_eal_hotplug_add("vdev", vdev_name,
>  						vdev_args) < 0) {
>  				cleanup_rings();
> @@ -722,6 +743,9 @@ struct parse_val {
>  					 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
>  				snprintf(vdev_args, sizeof(vdev_args),
>  					 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
> +				snprintf(vdev_args + strlen(vdev_args),
> +					 sizeof(vdev_args) - strlen(vdev_args),
> +					 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  				if (rte_eal_hotplug_add("vdev", vdev_name,
>  							vdev_args) < 0) {
>  					cleanup_rings();
> @@ -762,6 +786,9 @@ struct parse_val {
>  				 VDEV_IFACE_ARGS_FMT, pt->rx_dev) :
>  			snprintf(vdev_args, sizeof(vdev_args),
>  				 VDEV_PCAP_ARGS_FMT, pt->rx_dev);
> +			snprintf(vdev_args + strlen(vdev_args),
> +				 sizeof(vdev_args) - strlen(vdev_args),
> +				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  			if (rte_eal_hotplug_add("vdev", vdev_name,
>  						vdev_args) < 0) {
>  				cleanup_rings();
> @@ -799,6 +826,9 @@ struct parse_val {
>  				 VDEV_IFACE_ARGS_FMT, pt->tx_dev) :
>  			snprintf(vdev_args, sizeof(vdev_args),
>  				 VDEV_PCAP_ARGS_FMT, pt->tx_dev);
> +			snprintf(vdev_args + strlen(vdev_args),
> +				 sizeof(vdev_args) - strlen(vdev_args),
> +				 ","VDEV_SNAPLEN_ARGS_FMT, pt->snaplen);
>  			if (rte_eal_hotplug_add("vdev", vdev_name,
>  						vdev_args) < 0) {
>  				cleanup_rings();
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 13a3d0a..d51ea48 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -40,6 +40,7 @@
>  #define ETH_PCAP_IFACE_ARG    "iface"
>  #define ETH_PCAP_PHY_MAC_ARG  "phy_mac"
>  #define ETH_PCAP_INFINITE_RX_ARG  "infinite_rx"
> +#define ETH_PCAP_SNAPLEN_ARG  "snaplen"
>  
>  #define ETH_PCAP_ARG_MAXLEN	64
>  
> @@ -92,6 +93,7 @@ struct pmd_process_private {
>  	pcap_t *rx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
>  	pcap_t *tx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
>  	pcap_dumper_t *tx_dumper[RTE_PMD_PCAP_MAX_QUEUES];
> +	int snaplen;

As it shouldn't be negative, choosing "unsigned int" or "size_t" type can be better.

Also is the variable added to 'process_private' memmory intentionally? Why not
in the shared device data ('pmd_internals') memory?

>  };
>  
>  struct pmd_devargs {
> @@ -114,6 +116,7 @@ struct pmd_devargs_all {
>  	unsigned int is_rx_pcap;
>  	unsigned int is_rx_iface;
>  	unsigned int infinite_rx;
> +	int snaplen;
>  };
>  
>  static const char *valid_arguments[] = {
> @@ -125,6 +128,7 @@ struct pmd_devargs_all {
>  	ETH_PCAP_IFACE_ARG,
>  	ETH_PCAP_PHY_MAC_ARG,
>  	ETH_PCAP_INFINITE_RX_ARG,
> +	ETH_PCAP_SNAPLEN_ARG,
>  	NULL
>  };
>  
> @@ -339,6 +343,9 @@ struct pmd_devargs_all {
>  			caplen = sizeof(temp_data);
>  		}
>  
> +		if ((pp->snaplen > 0) && (caplen > pp->snaplen))
> +			caplen = pp->snaplen;

This "is 'snaplen' set" check done per packet, and it will hit both 'snaplen'
set or not, I wonder if there is a way to get rid of per packet check?

>  		calculate_timestamp(&header.ts);
>  		header.len = len;
>  		header.caplen = caplen;
> @@ -949,6 +956,7 @@ struct pmd_devargs_all {
>  {
>  	const char *pcap_filename = value;
>  	struct pmd_devargs *dumpers = extra_args;
> +	int snaplen;

This is not used at all, I guess added by mistake.

>  	pcap_dumper_t *dumper;
>  
>  	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
> @@ -1083,6 +1091,19 @@ struct pmd_devargs_all {
>  }
>  
>  static int
> +get_snaplen_arg(const char *key __rte_unused,
> +		const char *value, void *extra_args)
> +{
> +	if (extra_args) {
> +		const int snaplen = atoi(value);

'const' qualifier is not needed.

And some error checks can be better.

> +		int *snaplen_p = extra_args;
> +
> +		*snaplen_p = snaplen;
> +	}
> +	return 0;
> +}
> +
> +static int
>  pmd_init_internals(struct rte_vdev_device *vdev,
>  		const unsigned int nb_rx_queues,
>  		const unsigned int nb_tx_queues,
> @@ -1335,6 +1356,7 @@ struct pmd_devargs_all {
>  	struct rte_eth_dev *eth_dev =  NULL;
>  	struct pmd_internals *internal;
>  	int ret = 0;
> +	unsigned int snaplen_cnt;
>  
>  	struct pmd_devargs_all devargs_all = {
>  		.single_iface = 0,
> @@ -1412,6 +1434,15 @@ struct pmd_devargs_all {
>  		rte_kvargs_count(kvlist, ETH_PCAP_TX_IFACE_ARG) ? 1 : 0;
>  	dumpers.num_of_queue = 0;
>  
> +	snaplen_cnt = rte_kvargs_count(kvlist,
> +				ETH_PCAP_SNAPLEN_ARG);
> +	if (snaplen_cnt == 1) {

No need to get the count of the devargs. Just calling 'rte_kvargs_process()'
should be enough.

> +		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
> +				&get_snaplen_arg, &devargs_all.snaplen);

'snaplen' make sense only for 'is_tx_pcap' case, so you can process the
'ETH_PCAP_SNAPLEN_ARG' arg only for 'is_tx_pcap' case.

> +		if (ret < 0)
> +			goto free_kvlist;
> +	}
> +
>  	if (devargs_all.is_rx_pcap) {
>  		/*
>  		 * We check whether we want to infinitely rx the pcap file.
> @@ -1518,6 +1549,7 @@ struct pmd_devargs_all {
>  			pp->tx_pcap[i] = dumpers.queue[i].pcap;
>  		}
>  
> +		pp->snaplen = devargs_all.snaplen;

This is inside the "rte_eal_process_type() == RTE_PROC_SECONDARY" block, why
'snaplen' only used for secondary process?

>  		eth_dev->process_private = pp;
>  		eth_dev->rx_pkt_burst = eth_pcap_rx;
>  		if (devargs_all.is_tx_pcap)
> @@ -1587,7 +1619,8 @@ struct pmd_devargs_all {
>  	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_PHY_MAC_ARG "=<int>"
> -	ETH_PCAP_INFINITE_RX_ARG "=<0|1>");
> +	ETH_PCAP_INFINITE_RX_ARG "=<0|1>"
> +	ETH_PCAP_SNAPLEN_ARG "=<int>");
>  
>  RTE_INIT(eth_pcap_init_log)
>  {
> 

Re: [dpdk-dev] [PATCH v4] net/pcap: support snaplen option to truncate packet

[Pattan, Reshma]

> -----Original Message-----
> From: wangzk320@163.com <wangzk320@163.com> On Behalf Of Zhike Wang
> +			"[snaplen=<snap length>default:0, meaning no
> truncation]'\n",

From pdump changes, below are couple of comments.

Bit more descriptive would be nice,  how about, disables truncation of captured packets.?

> +	if (cnt1 == 1) {
> +		v.min = 1;

Min should be 0. User still can pass 0 value.

> +		v.max = UINT16_MAX;

Instead of  allowing big max number, isn't it good to have some restricted max value?

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v4] net/pcap: support snaplen option to truncate packet

[Ferruh Yigit]

On 7/14/2020 9:26 AM, Zhike Wang wrote:
> Introduced "snaplen=<length>" option. It is convenient to truncate
> large packets to only capture necessary headers.
> 
> Signed-off-by: Zhike Wang <wangzhike@jd.com>

<...>

> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 668cbd1..0d2a4b3 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -40,6 +40,7 @@
>  #define ETH_PCAP_IFACE_ARG    "iface"
>  #define ETH_PCAP_PHY_MAC_ARG  "phy_mac"
>  #define ETH_PCAP_INFINITE_RX_ARG  "infinite_rx"
> +#define ETH_PCAP_SNAPLEN_ARG  "snaplen"
>  
>  #define ETH_PCAP_ARG_MAXLEN	64
>  
> @@ -86,6 +87,7 @@ struct pmd_internals {
>  	int single_iface;
>  	int phy_mac;
>  	unsigned int infinite_rx;
> +	unsigned int snaplen;
>  };
>  
>  struct pmd_process_private {
> @@ -114,6 +116,7 @@ struct pmd_devargs_all {
>  	unsigned int is_rx_pcap;
>  	unsigned int is_rx_iface;
>  	unsigned int infinite_rx;
> +	unsigned int snaplen;
>  };
>  
>  static const char *valid_arguments[] = {
> @@ -125,6 +128,7 @@ struct pmd_devargs_all {
>  	ETH_PCAP_IFACE_ARG,
>  	ETH_PCAP_PHY_MAC_ARG,
>  	ETH_PCAP_INFINITE_RX_ARG,
> +	ETH_PCAP_SNAPLEN_ARG,
>  	NULL
>  };
>  
> @@ -322,11 +326,13 @@ struct pmd_devargs_all {
>  	pcap_dumper_t *dumper;
>  	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
>  	size_t len, caplen;
> +	struct pmd_internals *internal;
>  
>  	pp = rte_eth_devices[dumper_q->port_id].process_private;
>  	dumper = pp->tx_dumper[dumper_q->queue_id];
> +	internal = rte_eth_devices[dumper_q->port_id].data->dev_private;

Instead of accesing 'internal' through the 'rte_eth_devices' can you put a
reference to the queue struct and access from there?

It is not good to access to the global array 'rte_eth_devices' and we should
avoid it as much as possible. We have to do this for 'process_private' but can
avoid for the 'internal' structure.

>  
> -	if (dumper == NULL || nb_pkts == 0)
> +	if (dumper == NULL || nb_pkts == 0 || internal == NULL)

Can 'internal' be NULL at this stage? I don't think so.

>  		return 0;
>  
>  	/* writes the nb_pkts packets to the previously opened pcap file
> @@ -339,6 +345,9 @@ struct pmd_devargs_all {
>  			caplen = sizeof(temp_data);
>  		}
>  
> +		if (caplen > internal->snaplen)
> +			caplen = internal->snaplen;
> +
>  		calculate_timestamp(&header.ts);
>  		header.len = len;
>  		header.caplen = caplen;
> @@ -1083,6 +1092,21 @@ struct pmd_devargs_all {
>  }
>  
>  static int
> +get_snaplen_arg(const char *key __rte_unused,
> +		const char *value, void *extra_args)
> +{
> +	if (extra_args) {
> +		unsigned int snaplen = (unsigned int)atoi(value);

Not sure this is what we want, this may lead very unexpected 'snaplen' values.
Please verify the values from users before using them.

Please check if the value is negative and return error in that case, if positive
you can cast it to unsigned.

Also does any value "snaplen >= RTE_ETH_PCAP_SNAPLEN" make sense? That case can
be ignored quitely and set 'snaplen =RTE_ETH_PCAP_SNAPLEN" I think.

> +		unsigned int *snaplen_p = extra_args;
> +
> +		if (snaplen == 0)
> +			snaplen = RTE_ETH_PCAP_SNAPLEN;

Why silently ignoring the value '0', if the user explicitly provided the
'snaplen' devarg, and explicitly set it to '0', I think better to give an error
if '0' is not a valid value.
Also this requirement can be higlighted below 'RTE_PMD_REGISTER_PARAM_STRING',
something like:
"ETH_PCAP_SNAPLEN_ARG "=X where X > 0");

> +		*snaplen_p = snaplen;
> +	}
> +	return 0;
> +}
> +
> +static int
>  pmd_init_internals(struct rte_vdev_device *vdev,
>  		const unsigned int nb_rx_queues,
>  		const unsigned int nb_tx_queues,
> @@ -1291,6 +1315,9 @@ struct pmd_devargs_all {
>  	/* store weather we are using a single interface for rx/tx or not */
>  	internals->single_iface = single_iface;
>  
> +	if (devargs_all->is_tx_pcap)
> +		internals->snaplen = devargs_all->snaplen;
> +

Please don't add this in the middle of the 'single_iface' related block, in the
below, just above the 'infinite_rx' assingment can be better place.

And not sure if 'devargs_all->is_tx_pcap' check has a value here, I think can
assign it directly without check.

>  	if (single_iface) {
>  		internals->if_index = if_nametoindex(rx_queues->queue[0].name);
>  
> @@ -1341,6 +1368,7 @@ struct pmd_devargs_all {
>  		.is_tx_pcap = 0,
>  		.is_tx_iface = 0,
>  		.infinite_rx = 0,
> +		.snaplen = RTE_ETH_PCAP_SNAPLEN,
>  	};
>  
>  	name = rte_vdev_device_name(dev);
> @@ -1464,6 +1492,13 @@ struct pmd_devargs_all {
>  	if (ret < 0)
>  		goto free_kvlist;
>  
> +	if (devargs_all.is_tx_pcap) {
> +		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
> +				&get_snaplen_arg, &devargs_all.snaplen);
> +		if (ret < 0)
> +			goto free_kvlist;
> +	}
> +

Why creating a new "if (devargs_all.is_tx_pcap) {" block, this check already
exists a few lines below, what do you think adding 'ETH_PCAP_SNAPLEN_ARG'
process withing that existing block?

>  	/*
>  	 * We check whether we want to open a TX stream to a real NIC,
>  	 * a pcap file, or drop packets on tx
> @@ -1587,4 +1622,5 @@ struct pmd_devargs_all {
>  	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_PHY_MAC_ARG "=<int>"
> -	ETH_PCAP_INFINITE_RX_ARG "=<0|1>");
> +	ETH_PCAP_INFINITE_RX_ARG "=<0|1>"
> +	ETH_PCAP_SNAPLEN_ARG "=<int>");
> 

Re: [dpdk-dev] [PATCH v4] net/pcap: support snaplen option to truncate packet

[Stephen Hemminger]

On Tue, 14 Jul 2020 11:46:00 +0000
"Pattan, Reshma" <reshma.pattan@intel.com> wrote:

> > -----Original Message-----
> > From: wangzk320@163.com <wangzk320@163.com> On Behalf Of Zhike Wang
> > +			"[snaplen=<snap length>default:0, meaning no
> > truncation]'\n",  
> 
> From pdump changes, below are couple of comments.
> 
> Bit more descriptive would be nice,  how about, disables truncation of captured packets.?
> 
> > +	if (cnt1 == 1) {
> > +		v.min = 1;  
> 
> Min should be 0. User still can pass 0 value.
> 
> > +		v.max = UINT16_MAX;  
> 
> Instead of  allowing big max number, isn't it good to have some restricted max value?
> 
> Thanks,
> Reshma

An easier way to do the same thing. Just set the mbuf pool for the copied packets
to be sized to the snaplen, then modify pdump to only copy the truncated header.