[v4] net/pcap: support snaplen option to truncate packet
Checks
Commit Message
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(-)
Comments
> -----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
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>");
>
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.
@@ -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();
@@ -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
-------------
@@ -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
-------
@@ -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>");