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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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>");
>