Re: [PATCH 1/2] net/pcap: add snaplen argument

Re: [PATCH 1/2] net/pcap: add snaplen argument

[Ferruh Yigit]

On 3/7/2022 8:30 PM, Tianli Lai wrote:
> snaplen argument would set the length of each packet
> that will save to pcap file.
> 

Hi Tianli,

Overall +1 to add snaplen argument, but please find below comments.

Also we are close to finalize the release and this is a new feature,
so this can be considered for next release.

> Signed-off-by: Tianli Lai <laitianli@tom.com>
> ---
>   drivers/net/pcap/pcap_ethdev.c | 63 ++++++++++++++++++++++++++--------

please document new devargs in pmd documentation:
doc/guides/nics/pcap_ring.rst

>   1 file changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index ec29fd6bc5..8aea6d66ee 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -33,10 +33,12 @@
>   #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
>   
>   #define RTE_PMD_PCAP_MAX_QUEUES 16
> +#define RTE_PCAP_MIN_SNAPLEN    64
>   
>   static char errbuf[PCAP_ERRBUF_SIZE];
>   static struct timespec start_time;
> @@ -99,6 +101,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_internals' is better place than 'struct pmd_process_private'.

'struct pmd_process_private' is for the values that can
differ for primary and secondary processes, 'snaplen' does not.

>   };
>   
>   struct pmd_devargs {
> @@ -110,6 +113,7 @@ struct pmd_devargs {
>   		const char *type;
>   	} queue[RTE_PMD_PCAP_MAX_QUEUES];
>   	int phy_mac;
> +	int snaplen;
>   };
>   
>   struct pmd_devargs_all {
> @@ -132,6 +136,7 @@ static const char *valid_arguments[] = {
>   	ETH_PCAP_IFACE_ARG,
>   	ETH_PCAP_PHY_MAC_ARG,
>   	ETH_PCAP_INFINITE_RX_ARG,
> +	ETH_PCAP_SNAPLEN_ARG,
>   	NULL
>   };
>   
> @@ -404,6 +409,12 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   		calculate_timestamp(&header.ts);
>   		header.len = len;
>   		header.caplen = caplen;
> +		if (pp->snaplen >= RTE_PCAP_MIN_SNAPLEN) {

why not make 'snaplen' always a valid value, so it saves some checks here

This can be done by 'snaplen' default value is 'RTE_ETH_PCAP_SNAPSHOT_LEN'
and it can be overwritten with *valid* user input, so the variable
can be used here directly.

> +			if ((typeof(pp->snaplen))header.caplen > pp->snaplen) {
> +				header.caplen = pp->snaplen;

Why not do the check before above 'header.caplen = caplen;' line,
so won't need to set here again.

> +				caplen = pp->snaplen;
> +			}
> +		}
>   		/* rte_pktmbuf_read() returns a pointer to the data directly
>   		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
>   		 * a pointer to temp_data after copying into it.
> @@ -512,8 +523,11 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>    * pcap_open_live wrapper function
>    */
>   static inline int
> -open_iface_live(const char *iface, pcap_t **pcap) {
> -	*pcap = pcap_open_live(iface, RTE_ETH_PCAP_SNAPLEN,
> +open_iface_live(const char *iface, pcap_t **pcap, int snaplen) {
> +	int caplen = RTE_ETH_PCAP_SNAPLEN;
> +	if (snaplen >= RTE_PCAP_MIN_SNAPLEN)

As said above these checks can be eliminated by making
'snaplen' always a valid value at this stage.

> +		caplen = snaplen;
> +	*pcap = pcap_open_live(iface, caplen,
>   			RTE_ETH_PCAP_PROMISC, RTE_ETH_PCAP_TIMEOUT, errbuf);
>   
>   	if (*pcap == NULL) {
> @@ -525,9 +539,9 @@ open_iface_live(const char *iface, pcap_t **pcap) {
>   }
>   
>   static int
> -open_single_iface(const char *iface, pcap_t **pcap)
> +open_single_iface(const char *iface, pcap_t **pcap, int snaplen)
>   {
> -	if (open_iface_live(iface, pcap) < 0) {
> +	if (open_iface_live(iface, pcap, snaplen) < 0) {
>   		PMD_LOG(ERR, "Couldn't open interface %s", iface);
>   		return -1;
>   	}
> @@ -536,17 +550,19 @@ open_single_iface(const char *iface, pcap_t **pcap)
>   }
>   
>   static int
> -open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper)
> +open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper, int snaplen)
>   {
>   	pcap_t *tx_pcap;
> -
> +	int caplen = RTE_ETH_PCAP_SNAPSHOT_LEN;
> +	if (snaplen >= RTE_PCAP_MIN_SNAPLEN)
> +		caplen = snaplen;

ditto

>   	/*
>   	 * We need to create a dummy empty pcap_t to use it
>   	 * with pcap_dump_open(). We create big enough an Ethernet
>   	 * pcap holder.
>   	 */
>   	tx_pcap = pcap_open_dead_with_tstamp_precision(DLT_EN10MB,
> -			RTE_ETH_PCAP_SNAPSHOT_LEN, PCAP_TSTAMP_PRECISION_NANO);
> +			caplen, PCAP_TSTAMP_PRECISION_NANO);
>   	if (tx_pcap == NULL) {
>   		PMD_LOG(ERR, "Couldn't create dead pcap");
>   		return -1;
> @@ -612,7 +628,7 @@ eth_dev_start(struct rte_eth_dev *dev)
>   
>   		if (!pp->tx_pcap[0] &&
>   			strcmp(tx->type, ETH_PCAP_IFACE_ARG) == 0) {
> -			if (open_single_iface(tx->name, &pp->tx_pcap[0]) < 0)
> +			if (open_single_iface(tx->name, &pp->tx_pcap[0], pp->snaplen) < 0)
>   				return -1;
>   			pp->rx_pcap[0] = pp->tx_pcap[0];
>   		}
> @@ -627,11 +643,11 @@ eth_dev_start(struct rte_eth_dev *dev)
>   		if (!pp->tx_dumper[i] &&
>   				strcmp(tx->type, ETH_PCAP_TX_PCAP_ARG) == 0) {
>   			if (open_single_tx_pcap(tx->name,
> -				&pp->tx_dumper[i]) < 0)
> +				&pp->tx_dumper[i], pp->snaplen) < 0)
>   				return -1;
>   		} else if (!pp->tx_pcap[i] &&
>   				strcmp(tx->type, ETH_PCAP_TX_IFACE_ARG) == 0) {
> -			if (open_single_iface(tx->name, &pp->tx_pcap[i]) < 0)
> +			if (open_single_iface(tx->name, &pp->tx_pcap[i], pp->snaplen) < 0)
>   				return -1;
>   		}
>   	}
> @@ -647,7 +663,7 @@ eth_dev_start(struct rte_eth_dev *dev)
>   			if (open_single_rx_pcap(rx->name, &pp->rx_pcap[i]) < 0)
>   				return -1;
>   		} else if (strcmp(rx->type, ETH_PCAP_RX_IFACE_ARG) == 0) {
> -			if (open_single_iface(rx->name, &pp->rx_pcap[i]) < 0)
> +			if (open_single_iface(rx->name, &pp->rx_pcap[i], pp->snaplen) < 0)
>   				return -1;
>   		}
>   	}
> @@ -1055,7 +1071,7 @@ open_tx_pcap(const char *key, const char *value, void *extra_args)
>   	struct pmd_devargs *dumpers = extra_args;
>   	pcap_dumper_t *dumper;
>   
> -	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
> +	if (open_single_tx_pcap(pcap_filename, &dumper, dumpers->snaplen) < 0)
>   		return -1;
>   
>   	if (add_queue(dumpers, pcap_filename, key, NULL, dumper) < 0) {
> @@ -1066,6 +1082,16 @@ open_tx_pcap(const char *key, const char *value, void *extra_args)
>   	return 0;
>   }
>   
> +static int
> +parse_uint_value(const char *key, const char *value, void *extra_args)
> +{
> +	(void)key;

please use '__rte_unused' keyword, instead of assigning to (void)

> +	char *end;
> +	int *val = extra_args;> +	*val = strtoul(value, &end, 10);
> +	return 0;
> +}
> +
>   /*
>    * Opens an interface for reading and writing
>    */
> @@ -1076,7 +1102,7 @@ open_rx_tx_iface(const char *key, const char *value, void *extra_args)
>   	struct pmd_devargs *tx = extra_args;
>   	pcap_t *pcap = NULL;
>   
> -	if (open_single_iface(iface, &pcap) < 0)
> +	if (open_single_iface(iface, &pcap, tx->snaplen) < 0)
>   		return -1;
>   
>   	tx->queue[0].pcap = pcap;
> @@ -1108,7 +1134,7 @@ open_iface(const char *key, const char *value, void *extra_args)
>   	struct pmd_devargs *pmd = extra_args;
>   	pcap_t *pcap = NULL;
>   
> -	if (open_single_iface(iface, &pcap) < 0)
> +	if (open_single_iface(iface, &pcap, pmd->snaplen) < 0)

When this function (open_iface) is called by 'open_rx_iface',
'pmd' will be rx_queues ('pcaps'), and I guess 'pmd->snaplen'
will be 0 in that case, won't this cause a problem?

>   		return -1;
>   	if (add_queue(pmd, iface, key, pcap, NULL) < 0) {
>   		pcap_close(pcap);
> @@ -1403,6 +1429,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>   	struct rte_eth_dev *eth_dev =  NULL;
>   	struct pmd_internals *internal;
>   	int ret = 0;
> +	int snaplen = 0;

Why not use 'dumpers.snaplen' directly, instead of having a
temporary variable?

>   
>   	struct pmd_devargs_all devargs_all = {
>   		.single_iface = 0,
> @@ -1444,6 +1471,12 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>   			return -1;
>   	}
>   
> +	if (rte_kvargs_count(kvlist, ETH_PCAP_SNAPLEN_ARG) == 1) {
> +		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
> +				&parse_uint_value, &snaplen)
should handle 'ret < 0' case.

Also input value needs to be verified here, before accepted,
I guess it should be
RTE_PCAP_MIN_SNAPLEN <= snaplen <= RTE_ETH_PCAP_SNAPLEN

> +		dumpers.snaplen = snaplen;
> +	}
> +
>   	/*
>   	 * If iface argument is passed we open the NICs and use them for
>   	 * reading / writing
> @@ -1593,7 +1626,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>   			pp->tx_dumper[i] = dumpers.queue[i].dumper;
>   			pp->tx_pcap[i] = dumpers.queue[i].pcap;
>   		}
> -
> +		pp->snaplen = snaplen;
>   		eth_dev->process_private = pp;
>   		eth_dev->rx_pkt_burst = eth_pcap_rx;
>   		if (devargs_all.is_tx_pcap)

[PATCH 1/2] net/pcap: add snaplen argument

[Tianli Lai]

snaplen argument would set the length of each packet
that will save to pcap file.

Signed-off-by: Tianli Lai <laitianli@tom.com>
---
 drivers/net/pcap/pcap_ethdev.c | 63 ++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 15 deletions(-)

diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
index ec29fd6bc5..8aea6d66ee 100644
--- a/drivers/net/pcap/pcap_ethdev.c
+++ b/drivers/net/pcap/pcap_ethdev.c
@@ -33,10 +33,12 @@
 #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
 
 #define RTE_PMD_PCAP_MAX_QUEUES 16
+#define RTE_PCAP_MIN_SNAPLEN    64
 
 static char errbuf[PCAP_ERRBUF_SIZE];
 static struct timespec start_time;
@@ -99,6 +101,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 {
@@ -110,6 +113,7 @@ struct pmd_devargs {
 		const char *type;
 	} queue[RTE_PMD_PCAP_MAX_QUEUES];
 	int phy_mac;
+	int snaplen;
 };
 
 struct pmd_devargs_all {
@@ -132,6 +136,7 @@ static const char *valid_arguments[] = {
 	ETH_PCAP_IFACE_ARG,
 	ETH_PCAP_PHY_MAC_ARG,
 	ETH_PCAP_INFINITE_RX_ARG,
+	ETH_PCAP_SNAPLEN_ARG,
 	NULL
 };
 
@@ -404,6 +409,12 @@ eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		calculate_timestamp(&header.ts);
 		header.len = len;
 		header.caplen = caplen;
+		if (pp->snaplen >= RTE_PCAP_MIN_SNAPLEN) {
+			if ((typeof(pp->snaplen))header.caplen > pp->snaplen) {
+				header.caplen = pp->snaplen;
+				caplen = pp->snaplen;
+			}
+		}
 		/* rte_pktmbuf_read() returns a pointer to the data directly
 		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
 		 * a pointer to temp_data after copying into it.
@@ -512,8 +523,11 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
  * pcap_open_live wrapper function
  */
 static inline int
-open_iface_live(const char *iface, pcap_t **pcap) {
-	*pcap = pcap_open_live(iface, RTE_ETH_PCAP_SNAPLEN,
+open_iface_live(const char *iface, pcap_t **pcap, int snaplen) {
+	int caplen = RTE_ETH_PCAP_SNAPLEN;
+	if (snaplen >= RTE_PCAP_MIN_SNAPLEN)
+		caplen = snaplen;
+	*pcap = pcap_open_live(iface, caplen,
 			RTE_ETH_PCAP_PROMISC, RTE_ETH_PCAP_TIMEOUT, errbuf);
 
 	if (*pcap == NULL) {
@@ -525,9 +539,9 @@ open_iface_live(const char *iface, pcap_t **pcap) {
 }
 
 static int
-open_single_iface(const char *iface, pcap_t **pcap)
+open_single_iface(const char *iface, pcap_t **pcap, int snaplen)
 {
-	if (open_iface_live(iface, pcap) < 0) {
+	if (open_iface_live(iface, pcap, snaplen) < 0) {
 		PMD_LOG(ERR, "Couldn't open interface %s", iface);
 		return -1;
 	}
@@ -536,17 +550,19 @@ open_single_iface(const char *iface, pcap_t **pcap)
 }
 
 static int
-open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper)
+open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper, int snaplen)
 {
 	pcap_t *tx_pcap;
-
+	int caplen = RTE_ETH_PCAP_SNAPSHOT_LEN;
+	if (snaplen >= RTE_PCAP_MIN_SNAPLEN)
+		caplen = snaplen;
 	/*
 	 * We need to create a dummy empty pcap_t to use it
 	 * with pcap_dump_open(). We create big enough an Ethernet
 	 * pcap holder.
 	 */
 	tx_pcap = pcap_open_dead_with_tstamp_precision(DLT_EN10MB,
-			RTE_ETH_PCAP_SNAPSHOT_LEN, PCAP_TSTAMP_PRECISION_NANO);
+			caplen, PCAP_TSTAMP_PRECISION_NANO);
 	if (tx_pcap == NULL) {
 		PMD_LOG(ERR, "Couldn't create dead pcap");
 		return -1;
@@ -612,7 +628,7 @@ eth_dev_start(struct rte_eth_dev *dev)
 
 		if (!pp->tx_pcap[0] &&
 			strcmp(tx->type, ETH_PCAP_IFACE_ARG) == 0) {
-			if (open_single_iface(tx->name, &pp->tx_pcap[0]) < 0)
+			if (open_single_iface(tx->name, &pp->tx_pcap[0], pp->snaplen) < 0)
 				return -1;
 			pp->rx_pcap[0] = pp->tx_pcap[0];
 		}
@@ -627,11 +643,11 @@ eth_dev_start(struct rte_eth_dev *dev)
 		if (!pp->tx_dumper[i] &&
 				strcmp(tx->type, ETH_PCAP_TX_PCAP_ARG) == 0) {
 			if (open_single_tx_pcap(tx->name,
-				&pp->tx_dumper[i]) < 0)
+				&pp->tx_dumper[i], pp->snaplen) < 0)
 				return -1;
 		} else if (!pp->tx_pcap[i] &&
 				strcmp(tx->type, ETH_PCAP_TX_IFACE_ARG) == 0) {
-			if (open_single_iface(tx->name, &pp->tx_pcap[i]) < 0)
+			if (open_single_iface(tx->name, &pp->tx_pcap[i], pp->snaplen) < 0)
 				return -1;
 		}
 	}
@@ -647,7 +663,7 @@ eth_dev_start(struct rte_eth_dev *dev)
 			if (open_single_rx_pcap(rx->name, &pp->rx_pcap[i]) < 0)
 				return -1;
 		} else if (strcmp(rx->type, ETH_PCAP_RX_IFACE_ARG) == 0) {
-			if (open_single_iface(rx->name, &pp->rx_pcap[i]) < 0)
+			if (open_single_iface(rx->name, &pp->rx_pcap[i], pp->snaplen) < 0)
 				return -1;
 		}
 	}
@@ -1055,7 +1071,7 @@ open_tx_pcap(const char *key, const char *value, void *extra_args)
 	struct pmd_devargs *dumpers = extra_args;
 	pcap_dumper_t *dumper;
 
-	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
+	if (open_single_tx_pcap(pcap_filename, &dumper, dumpers->snaplen) < 0)
 		return -1;
 
 	if (add_queue(dumpers, pcap_filename, key, NULL, dumper) < 0) {
@@ -1066,6 +1082,16 @@ open_tx_pcap(const char *key, const char *value, void *extra_args)
 	return 0;
 }
 
+static int
+parse_uint_value(const char *key, const char *value, void *extra_args)
+{
+	(void)key;
+	char *end;
+	int *val = extra_args;
+	*val = strtoul(value, &end, 10);
+	return 0;
+}
+
 /*
  * Opens an interface for reading and writing
  */
@@ -1076,7 +1102,7 @@ open_rx_tx_iface(const char *key, const char *value, void *extra_args)
 	struct pmd_devargs *tx = extra_args;
 	pcap_t *pcap = NULL;
 
-	if (open_single_iface(iface, &pcap) < 0)
+	if (open_single_iface(iface, &pcap, tx->snaplen) < 0)
 		return -1;
 
 	tx->queue[0].pcap = pcap;
@@ -1108,7 +1134,7 @@ open_iface(const char *key, const char *value, void *extra_args)
 	struct pmd_devargs *pmd = extra_args;
 	pcap_t *pcap = NULL;
 
-	if (open_single_iface(iface, &pcap) < 0)
+	if (open_single_iface(iface, &pcap, pmd->snaplen) < 0)
 		return -1;
 	if (add_queue(pmd, iface, key, pcap, NULL) < 0) {
 		pcap_close(pcap);
@@ -1403,6 +1429,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
 	struct rte_eth_dev *eth_dev =  NULL;
 	struct pmd_internals *internal;
 	int ret = 0;
+	int snaplen = 0;
 
 	struct pmd_devargs_all devargs_all = {
 		.single_iface = 0,
@@ -1444,6 +1471,12 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
 			return -1;
 	}
 
+	if (rte_kvargs_count(kvlist, ETH_PCAP_SNAPLEN_ARG) == 1) {
+		ret = rte_kvargs_process(kvlist, ETH_PCAP_SNAPLEN_ARG,
+				&parse_uint_value, &snaplen);
+		dumpers.snaplen = snaplen;
+	}
+
 	/*
 	 * If iface argument is passed we open the NICs and use them for
 	 * reading / writing
@@ -1593,7 +1626,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
 			pp->tx_dumper[i] = dumpers.queue[i].dumper;
 			pp->tx_pcap[i] = dumpers.queue[i].pcap;
 		}
-
+		pp->snaplen = snaplen;
 		eth_dev->process_private = pp;
 		eth_dev->rx_pkt_burst = eth_pcap_rx;
 		if (devargs_all.is_tx_pcap)
-- 
2.27.0

Re: [PATCH 1/2] net/pcap: add snaplen argument

[Stephen Hemminger]

On Tue,  8 Mar 2022 04:30:12 +0800
Tianli Lai <laitianli@tom.com> wrote:

> snaplen argument would set the length of each packet
> that will save to pcap file.
> 
> Signed-off-by: Tianli Lai <laitianli@tom.com>

Dropping this patch since dpdk-dumpcap can already do all this
and more.