[PATCH 1/6] app/dumpcap: add additional dump info

[PATCH 1/6] app/dumpcap: add additional dump info

[Ben Magistro]

This change increases the information returned when listing interfaces
to include link state and promiscuous mode. Additionally, the
information is formatted to be easily consumed by a machine or a person.

This change is/was utilized while troubleshooting and developing the
subsequent patches to address the issues identified on the mailing
list [1].

[1] http://mails.dpdk.org/archives/dev/2022-December/258317.html

Signed-off-by: Ben Magistro <koncept1@gmail.com>
---
 app/dumpcap/main.c           | 27 ++++++++++++++++++++++++++-
 doc/guides/tools/dumpcap.rst |  9 ++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 2eb8414efa..b9096f050c 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -266,11 +266,36 @@ static void dump_interfaces(void)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 	uint16_t p;
+	struct rte_eth_link link_info;
+	char link_state[8];
+	char promisc_mode[9];
 
+	printf("%-4s\t%-40s\t%-8s\t%-12s\n",
+		"Port", "Name", "Link", "Promiscuous");
 	RTE_ETH_FOREACH_DEV(p) {
 		if (rte_eth_dev_get_name_by_port(p, name) < 0)
 			continue;
-		printf("%u. %s\n", p, name);
+
+		if (rte_eth_link_get_nowait(p, &link_info) < 0) {
+			rte_strscpy(link_state, "Unknown", sizeof(link_state));
+		}
+		else if (link_info.link_status == RTE_ETH_LINK_UP) {
+			rte_strscpy(link_state, "Up", sizeof(link_state));
+		}
+		else {
+			rte_strscpy(link_state, "Down", sizeof(link_state));
+		}
+
+		// not checking error here; should only error if given an invalid port id
+		if (rte_eth_promiscuous_get(p) == 1) {
+			rte_strscpy(promisc_mode, "Enabled", sizeof(promisc_mode));
+		}
+		else {
+			rte_strscpy(promisc_mode, "Disabled", sizeof(promisc_mode));
+		}
+
+		printf("%-4u\t%-40s\t%-8s\t%-12s\n",
+			p, name, link_state, promisc_mode);
 	}
 
 	exit(0);
diff --git a/doc/guides/tools/dumpcap.rst b/doc/guides/tools/dumpcap.rst
index d8a137b1cd..0f89e6c5ca 100644
--- a/doc/guides/tools/dumpcap.rst
+++ b/doc/guides/tools/dumpcap.rst
@@ -39,7 +39,7 @@ If ``-w`` option is specified, then that file is used.
 Running the Application
 -----------------------
 
-To list interfaces available for capture, use ``--list-interfaces``.
+To list interfaces available for capture, use ``-D`` or ``--list-interfaces``.
 
 To filter packets in style of *tshark*, use the ``-f`` flag.
 
@@ -52,8 +52,11 @@ Example
 .. code-block:: console
 
    # <build_dir>/app/dpdk-dumpcap --list-interfaces
-   0. 000:00:03.0
-   1. 000:00:03.1
+   Port    Name                                        Link        Promiscuous
+   0       0000:00:03.0                                Up          Enabled
+   1       0000:00:03.1                                Up          Disabled
+   2       0000:00:03.2                                Down        Disabled
+   3       0000:00:03.3                                Down        Disabled
 
    # <build_dir>/app/dpdk-dumpcap -i 0000:00:03.0 -c 6 -w /tmp/sample.pcapng
    Packets captured: 6
-- 
2.27.0

[PATCH 2/6] app/dumpcap: fix storing port identifier

[Ben Magistro]

When dumpcap adds an interface, the port was not being preserved. This
results in the structure being initialized and the port field being set
to 0 regardless of what port was actually selected. This unset field is
then used in both the enable and cleanup calls. This could result in the
capture occurring on the wrong interface.

Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
Cc: stephen@networkplumber.org
Cc: stable@dpdk.org

Signed-off-by: Ben Magistro <koncept1@gmail.com>
---
 app/dumpcap/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index b9096f050c..aaee9349b1 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -202,6 +202,7 @@ static void add_interface(uint16_t port, const char *name)
 		rte_exit(EXIT_FAILURE, "no memory for interface\n");
 
 	memset(intf, 0, sizeof(*intf));
+	intf->port = port;
 	rte_strscpy(intf->name, name, sizeof(intf->name));
 
 	printf("Capturing on '%s'\n", name);
-- 
2.27.0

Re: [PATCH 2/6] app/dumpcap: fix storing port identifier

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:37 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> When dumpcap adds an interface, the port was not being preserved. This
> results in the structure being initialized and the port field being set
> to 0 regardless of what port was actually selected. This unset field is
> then used in both the enable and cleanup calls. This could result in the
> capture occurring on the wrong interface.
> 
> Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
> Cc: stephen@networkplumber.org
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>
> ---
>  app/dumpcap/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index b9096f050c..aaee9349b1 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -202,6 +202,7 @@ static void add_interface(uint16_t port, const char *name)
>  		rte_exit(EXIT_FAILURE, "no memory for interface\n");
>  
>  	memset(intf, 0, sizeof(*intf));
> +	intf->port = port;
>  	rte_strscpy(intf->name, name, sizeof(intf->name));
>  
>  	printf("Capturing on '%s'\n", name);

Will be superseded by my fixes to port setup.

Re: [PATCH 2/6] app/dumpcap: fix storing port identifier

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:37 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> When dumpcap adds an interface, the port was not being preserved. This
> results in the structure being initialized and the port field being set
> to 0 regardless of what port was actually selected. This unset field is
> then used in both the enable and cleanup calls. This could result in the
> capture occurring on the wrong interface.
> 
> Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
> Cc: stephen@networkplumber.org
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>
> ---
>  app/dumpcap/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index b9096f050c..aaee9349b1 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -202,6 +202,7 @@ static void add_interface(uint16_t port, const char *name)
>  		rte_exit(EXIT_FAILURE, "no memory for interface\n");
>  
>  	memset(intf, 0, sizeof(*intf));
> +	intf->port = port;
>  	rte_strscpy(intf->name, name, sizeof(intf->name));
>  
>  	printf("Capturing on '%s'\n", name);

LGTM
Already set in the fix patch set

[PATCH 3/6] app/dumpcap: fix preserving promiscuous mode

[Ben Magistro]

When stopping dumpcap and the main application set an interface to
promiscuous mode, it would be disabled when dumpcap performed its
cleanup.  This results in a change of behavior for the main application
after running/utilizing dumpcap. The initial promiscuous mode is now
stored and compared when cleaning up allowing that to be preserved.

Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
Cc: stephen@networkplumber.org
Cc: stable@dpdk.org

Signed-off-by: Ben Magistro <koncept1@gmail.com>
---
 app/dumpcap/main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index aaee9349b1..26c641df61 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -84,6 +84,7 @@ struct interface {
 	TAILQ_ENTRY(interface) next;
 	uint16_t port;
 	char name[RTE_ETH_NAME_MAX_LEN];
+	int promiscuous_exit; /* 1 when promicuous is set prior to starting dumpcap */
 
 	struct rte_rxtx_callback *rx_cb[RTE_MAX_QUEUES_PER_PORT];
 };
@@ -204,6 +205,8 @@ static void add_interface(uint16_t port, const char *name)
 	memset(intf, 0, sizeof(*intf));
 	intf->port = port;
 	rte_strscpy(intf->name, name, sizeof(intf->name));
+	// not checking error here; should only error if given an invalid port id
+	intf->promiscuous_exit = rte_eth_promiscuous_get(port);
 
 	printf("Capturing on '%s'\n", name);
 
@@ -462,7 +465,7 @@ cleanup_pdump_resources(void)
 	TAILQ_FOREACH(intf, &interfaces, next) {
 		rte_pdump_disable(intf->port,
 				  RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
-		if (promiscuous_mode)
+		if (!intf->promiscuous_exit && promiscuous_mode)
 			rte_eth_promiscuous_disable(intf->port);
 	}
 }
-- 
2.27.0

Re: [PATCH 3/6] app/dumpcap: fix preserving promiscuous mode

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:38 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> When stopping dumpcap and the main application set an interface to
> promiscuous mode, it would be disabled when dumpcap performed its
> cleanup.  This results in a change of behavior for the main application
> after running/utilizing dumpcap. The initial promiscuous mode is now
> stored and compared when cleaning up allowing that to be preserved.
> 
> Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
> Cc: stephen@networkplumber.org
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>
> ---
>  app/dumpcap/main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index aaee9349b1..26c641df61 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -84,6 +84,7 @@ struct interface {
>  	TAILQ_ENTRY(interface) next;
>  	uint16_t port;
>  	char name[RTE_ETH_NAME_MAX_LEN];
> +	int promiscuous_exit; /* 1 when promicuous is set prior to starting dumpcap */
>  
>  	struct rte_rxtx_callback *rx_cb[RTE_MAX_QUEUES_PER_PORT];
>  };
> @@ -204,6 +205,8 @@ static void add_interface(uint16_t port, const char *name)
>  	memset(intf, 0, sizeof(*intf));
>  	intf->port = port;
>  	rte_strscpy(intf->name, name, sizeof(intf->name));
> +	// not checking error here; should only error if given an invalid port id
> +	intf->promiscuous_exit = rte_eth_promiscuous_get(port);
>  
>  	printf("Capturing on '%s'\n", name);
>  
> @@ -462,7 +465,7 @@ cleanup_pdump_resources(void)
>  	TAILQ_FOREACH(intf, &interfaces, next) {
>  		rte_pdump_disable(intf->port,
>  				  RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
> -		if (promiscuous_mode)
> +		if (!intf->promiscuous_exit && promiscuous_mode)
>  			rte_eth_promiscuous_disable(intf->port);
>  	}
>  }

Thanks, could you test mine instead.

Re: [PATCH 3/6] app/dumpcap: fix preserving promiscuous mode

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:38 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> When stopping dumpcap and the main application set an interface to
> promiscuous mode, it would be disabled when dumpcap performed its
> cleanup.  This results in a change of behavior for the main application
> after running/utilizing dumpcap. The initial promiscuous mode is now
> stored and compared when cleaning up allowing that to be preserved.
> 
> Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
> Cc: stephen@networkplumber.org
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>

What I did in the fix patch set add promisc_mode flag
to existing interface array.

[PATCH 4/6] app/dumpcap: fix capturing on multiple interfaces

[Ben Magistro]

When selecting interfaces to capture traffic on, only the last interface
specified was being preserved for selection. This turns the interface
argument into an array allowing multiple interfaces to be selected.

This also adds checks for the capture format so that if multiple interfaces
are selected, the interface information is included in the capture file.

Fixes: 7f3623a ("app/dumpcap: fix select interface")
Cc: arshdeep.kaur@intel.com
Cc: stephen@networkplumber.org
Cc: stable@dpdk.org

Signed-off-by: Ben Magistro <koncept1@gmail.com>
---
 app/dumpcap/main.c | 47 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 26c641df61..dc4d69ff6b 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -65,8 +65,8 @@ static const char *file_prefix;
 static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
 static bool dump_bpf;
 static bool show_interfaces;
-static bool select_interfaces;
-const char *interface_arg;
+static uint8_t interface_arg_count = 0; /* count of interfaces configured via -i */
+static const char *interface_arg[RTE_MAX_ETHPORTS]; /*array of interface parameters */
 
 static struct {
 	uint64_t  duration;	/* nanoseconds */
@@ -242,7 +242,6 @@ static void set_default_interface(void)
 		add_interface(p, name);
 		return;
 	}
-	rte_exit(EXIT_FAILURE, "No usable interfaces found\n");
 }
 
 /* Lookup interface by name or port and add it to the list */
@@ -265,6 +264,32 @@ static void select_interface(const char *arg)
 	}
 }
 
+/**
+ * builds list of interfacs that capture will occur on
+ * this also validates the save format based on the number of interfaces
+ */
+static void collect_interfaces(void)
+{
+	uint8_t active;
+	struct interface *intf;
+
+	active = 0;
+
+	if (interface_arg_count == 0)
+		set_default_interface();
+	else
+		for (uint8_t i = 0; i < interface_arg_count; ++i)
+			select_interface(interface_arg[i]);
+
+	TAILQ_FOREACH(intf, &interfaces, next)
+		active++;
+
+	if ((!use_pcapng) && (active > 1)) {
+		printf("Requested pcap format, but options require pcapng; overriding\n");
+		use_pcapng = true;
+	}
+}
+
 /* Display list of possible interfaces that can be used. */
 static void dump_interfaces(void)
 {
@@ -406,8 +431,8 @@ static void parse_opts(int argc, char **argv)
 			usage();
 			exit(0);
 		case 'i':
-			select_interfaces = true;
-			interface_arg = optarg;
+			interface_arg_count++;
+			interface_arg[interface_arg_count - 1] = optarg;
 			break;
 		case 'n':
 			use_pcapng = true;
@@ -840,21 +865,17 @@ int main(int argc, char **argv)
 	parse_opts(argc, argv);
 	dpdk_init();
 
+	if (rte_eth_dev_count_avail() == 0)
+		rte_exit(EXIT_FAILURE, "No usable ports found\n");
+
 	if (show_interfaces)
 		dump_interfaces();
 
-	if (rte_eth_dev_count_avail() == 0)
-		rte_exit(EXIT_FAILURE, "No Ethernet ports found\n");
-
-	if (select_interfaces)
-		select_interface(interface_arg);
+	collect_interfaces();
 
 	if (filter_str)
 		compile_filter();
 
-	if (TAILQ_EMPTY(&interfaces))
-		set_default_interface();
-
 	r = create_ring();
 	mp = create_mempool();
 	out = create_output();
-- 
2.27.0

Re: [PATCH 4/6] app/dumpcap: fix capturing on multiple interfaces

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:39 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> When selecting interfaces to capture traffic on, only the last interface
> specified was being preserved for selection. This turns the interface
> argument into an array allowing multiple interfaces to be selected.
> 
> This also adds checks for the capture format so that if multiple interfaces
> are selected, the interface information is included in the capture file.
> 
> Fixes: 7f3623a ("app/dumpcap: fix select interface")
> Cc: arshdeep.kaur@intel.com
> Cc: stephen@networkplumber.org
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>
> ---
>  app/dumpcap/main.c | 47 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index 26c641df61..dc4d69ff6b 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -65,8 +65,8 @@ static const char *file_prefix;
>  static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
>  static bool dump_bpf;
>  static bool show_interfaces;
> -static bool select_interfaces;
> -const char *interface_arg;
> +static uint8_t interface_arg_count = 0; /* count of interfaces configured via -i */
> +static const char *interface_arg[RTE_MAX_ETHPORTS]; /*array of interface parameters */

I think it is simpler to just use the existing interfaces
array rather than introducing a new data structure.

PS: run you patches through check patch, lots of little style issues
especially in #1

[PATCH 5/6] app/dumpcap: improve per interface arg parsing

[Ben Magistro]

This change improves the argument parsing to align closer to that of Wireshark
dumpcap allowing for per interface settings on promiscuous mode and the filter
string.

Cc: stephen@networkplumber.org

Signed-off-by: Ben Magistro <koncept1@gmail.com>
---
 app/dumpcap/main.c           | 149 +++++++++++++++++++++++------------
 doc/guides/tools/dumpcap.rst |  17 +++-
 2 files changed, 112 insertions(+), 54 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index dc4d69ff6b..1dc4a38adb 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -55,18 +55,32 @@ static const char *progname;
 static bool quit_signal;
 static bool group_read;
 static bool quiet;
-static bool promiscuous_mode = true;
 static bool use_pcapng = true;
 static char *output_name;
-static const char *filter_str;
 static unsigned int ring_size = 2048;
 static const char *capture_comment;
 static const char *file_prefix;
-static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
 static bool dump_bpf;
 static bool show_interfaces;
 static uint8_t interface_arg_count = 0; /* count of interfaces configured via -i */
-static const char *interface_arg[RTE_MAX_ETHPORTS]; /*array of interface parameters */
+
+/* struct representing args for each interface */
+struct interface_opts {
+	const char *intf_arg;
+	const char *filter_str;
+	bool promiscuous_mode;
+	uint32_t snaplen;
+};
+/* default parameters for interfaces */
+static struct interface_opts interface_defaults = {
+	.promiscuous_mode = true,
+	.snaplen = RTE_MBUF_DEFAULT_BUF_SIZE
+};
+//TODO this can be specified per interface but there are a few places
+// that need more review and possibly refactoring so leaving this one out
+uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
+/* array of interface parameters */
+static struct interface_opts interface_args[RTE_MAX_ETHPORTS];
 
 static struct {
 	uint64_t  duration;	/* nanoseconds */
@@ -75,7 +89,6 @@ static struct {
 } stop;
 
 /* Running state */
-static struct rte_bpf_prm *bpf_prm;
 static uint64_t start_time, end_time;
 static uint64_t packets_received;
 static size_t file_size;
@@ -85,6 +98,8 @@ struct interface {
 	uint16_t port;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int promiscuous_exit; /* 1 when promicuous is set prior to starting dumpcap */
+	struct interface_opts *start_opts; /* cli parameters associated with interface */
+	struct rte_bpf_prm *bpf_prm;
 
 	struct rte_rxtx_callback *rx_cb[RTE_MAX_QUEUES_PER_PORT];
 };
@@ -194,7 +209,7 @@ static void auto_stop(char *opt)
 }
 
 /* Add interface to list of interfaces to capture */
-static void add_interface(uint16_t port, const char *name)
+static void add_interface(uint16_t port, const char *name, struct interface_opts *opts)
 {
 	struct interface *intf;
 
@@ -207,6 +222,7 @@ static void add_interface(uint16_t port, const char *name)
 	rte_strscpy(intf->name, name, sizeof(intf->name));
 	// not checking error here; should only error if given an invalid port id
 	intf->promiscuous_exit = rte_eth_promiscuous_get(port);
+	intf->start_opts = opts;
 
 	printf("Capturing on '%s'\n", name);
 
@@ -215,7 +231,7 @@ static void add_interface(uint16_t port, const char *name)
 }
 
 /* Select all valid DPDK interfaces */
-static void select_all_interfaces(void)
+static void select_all_interfaces(struct interface_opts *opts)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 	uint16_t p;
@@ -223,7 +239,7 @@ static void select_all_interfaces(void)
 	RTE_ETH_FOREACH_DEV(p) {
 		if (rte_eth_dev_get_name_by_port(p, name) < 0)
 			continue;
-		add_interface(p, name);
+		add_interface(p, name, opts);
 	}
 }
 
@@ -231,7 +247,7 @@ static void select_all_interfaces(void)
  * Choose interface to capture if no -i option given.
  * Select the first DPDK port, this matches what dumpcap does.
  */
-static void set_default_interface(void)
+static void set_default_interface(struct interface_opts *opts)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 	uint16_t p;
@@ -239,28 +255,28 @@ static void set_default_interface(void)
 	RTE_ETH_FOREACH_DEV(p) {
 		if (rte_eth_dev_get_name_by_port(p, name) < 0)
 			continue;
-		add_interface(p, name);
+		add_interface(p, name, opts);
 		return;
 	}
 }
 
 /* Lookup interface by name or port and add it to the list */
-static void select_interface(const char *arg)
+static void select_interface(struct interface_opts *opts)
 {
 	uint16_t port;
 
-	if (strcmp(arg, "*") == 0)
-		select_all_interfaces();
-	else if (rte_eth_dev_get_port_by_name(arg, &port) == 0)
-		add_interface(port, arg);
+	if (strcmp(opts->intf_arg, "*") == 0)
+		select_all_interfaces(opts);
+	else if (rte_eth_dev_get_port_by_name(opts->intf_arg, &port) == 0)
+		add_interface(port, opts->intf_arg, opts);
 	else {
 		char name[RTE_ETH_NAME_MAX_LEN];
 
-		port = get_uint(arg, "port_number", UINT16_MAX);
+		port = get_uint(opts->intf_arg, "port_number", UINT16_MAX);
 		if (rte_eth_dev_get_name_by_port(port, name) < 0)
 			rte_exit(EXIT_FAILURE, "Invalid port number %u\n",
 				 port);
-		add_interface(port, name);
+		add_interface(port, name, opts);
 	}
 }
 
@@ -276,10 +292,10 @@ static void collect_interfaces(void)
 	active = 0;
 
 	if (interface_arg_count == 0)
-		set_default_interface();
+		set_default_interface(&interface_defaults);
 	else
 		for (uint8_t i = 0; i < interface_arg_count; ++i)
-			select_interface(interface_arg[i]);
+			select_interface(&interface_args[i]);
 
 	TAILQ_FOREACH(intf, &interfaces, next)
 		active++;
@@ -330,37 +346,48 @@ static void dump_interfaces(void)
 	exit(0);
 }
 
-static void compile_filter(void)
+static void compile_filters(void)
 {
-	struct bpf_program bf;
-	pcap_t *pcap;
+	struct interface *intf;
 
-	pcap = pcap_open_dead(DLT_EN10MB, snaplen);
-	if (!pcap)
-		rte_exit(EXIT_FAILURE, "can not open pcap\n");
+	TAILQ_FOREACH(intf, &interfaces, next) {
+		if (!intf->start_opts->filter_str)
+			continue;
 
-	if (pcap_compile(pcap, &bf, filter_str,
-			 1, PCAP_NETMASK_UNKNOWN) != 0)
-		rte_exit(EXIT_FAILURE, "pcap filter string not valid (%s)\n",
-			 pcap_geterr(pcap));
+		struct bpf_program bf;
+		pcap_t *pcap;
 
-	bpf_prm = rte_bpf_convert(&bf);
-	if (bpf_prm == NULL)
-		rte_exit(EXIT_FAILURE,
-			 "bpf convert failed: %s(%d)\n",
-			 rte_strerror(rte_errno), rte_errno);
-
-	if (dump_bpf) {
-		printf("cBPF program (%u insns)\n", bf.bf_len);
-		bpf_dump(&bf, 1);
-		printf("\neBPF program (%u insns)\n", bpf_prm->nb_ins);
-		rte_bpf_dump(stdout, bpf_prm->ins, bpf_prm->nb_ins);
-		exit(0);
+		// pcap = pcap_open_dead(DLT_EN10MB, intf->start_opts->snaplen);
+		pcap = pcap_open_dead(DLT_EN10MB, snaplen);
+		if (!pcap)
+			rte_exit(EXIT_FAILURE, "can not open pcap\n");
+
+		if (pcap_compile(pcap, &bf, intf->start_opts->filter_str,
+				 1, PCAP_NETMASK_UNKNOWN) != 0)
+			rte_exit(EXIT_FAILURE, "pcap filter string not valid (%s)\n",
+				 pcap_geterr(pcap));
+
+		intf->bpf_prm = rte_bpf_convert(&bf);
+		if (intf->bpf_prm == NULL)
+			rte_exit(EXIT_FAILURE,
+				 "bpf convert failed: %s(%d)\n",
+				 rte_strerror(rte_errno), rte_errno);
+
+		if (dump_bpf) {
+			printf("\nPort: %d; Name: %s\n", intf->port, intf->name);
+			printf("cBPF program (%u insns)\n", bf.bf_len);
+			bpf_dump(&bf, 1);
+			printf("\neBPF program (%u insns)\n", intf->bpf_prm->nb_ins);
+			rte_bpf_dump(stdout, intf->bpf_prm->ins, intf->bpf_prm->nb_ins);
+		}
+
+		/* Don't care about original program any more */
+		pcap_freecode(&bf);
+		pcap_close(pcap);
 	}
 
-	/* Don't care about original program any more */
-	pcap_freecode(&bf);
-	pcap_close(pcap);
+	if (dump_bpf)
+		exit(0);
 }
 
 /*
@@ -421,7 +448,10 @@ static void parse_opts(int argc, char **argv)
 			show_interfaces = true;
 			break;
 		case 'f':
-			filter_str = optarg;
+			if (interface_arg_count == 0 )
+				interface_defaults.filter_str = optarg;
+			else
+				interface_args[interface_arg_count - 1].filter_str = optarg;
 			break;
 		case 'g':
 			group_read = true;
@@ -432,7 +462,13 @@ static void parse_opts(int argc, char **argv)
 			exit(0);
 		case 'i':
 			interface_arg_count++;
-			interface_arg[interface_arg_count - 1] = optarg;
+			interface_args[interface_arg_count - 1].intf_arg = optarg;
+			// set interface to global parameters when new interface is configured
+			interface_args[interface_arg_count - 1].filter_str =
+				interface_defaults.filter_str;
+			interface_args[interface_arg_count - 1].promiscuous_mode =
+				interface_defaults.promiscuous_mode;
+			// interface_args[interface_arg_count - 1].snaplen = interface_defaults.snaplen;
 			break;
 		case 'n':
 			use_pcapng = true;
@@ -441,7 +477,10 @@ static void parse_opts(int argc, char **argv)
 			ring_size = get_uint(optarg, "packet_limit", 0);
 			break;
 		case 'p':
-			promiscuous_mode = false;
+			if (interface_arg_count == 0 )
+				interface_defaults.promiscuous_mode = false;
+			else
+				interface_args[interface_arg_count - 1].promiscuous_mode = false;
 			break;
 		case 'P':
 			use_pcapng = false;
@@ -451,6 +490,12 @@ static void parse_opts(int argc, char **argv)
 			break;
 		case 's':
 			snaplen = get_uint(optarg, "snap_len", 0);
+			/*
+			if (interface_arg_count == 0 )
+				interface_defaults.snaplen = get_uint(optarg, "snap_len", 0);
+			else
+				interface_args[interface_arg_count - 1].snaplen = get_uint(optarg, "snap_len", 0);
+			*/
 			break;
 		case 'w':
 			output_name = optarg;
@@ -490,7 +535,7 @@ cleanup_pdump_resources(void)
 	TAILQ_FOREACH(intf, &interfaces, next) {
 		rte_pdump_disable(intf->port,
 				  RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
-		if (!intf->promiscuous_exit && promiscuous_mode)
+		if (!intf->promiscuous_exit && intf->start_opts->promiscuous_mode)
 			rte_eth_promiscuous_disable(intf->port);
 	}
 }
@@ -750,7 +795,7 @@ static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
 		flags |= RTE_PDUMP_FLAG_PCAPNG;
 
 	TAILQ_FOREACH(intf, &interfaces, next) {
-		if (promiscuous_mode) {
+		if (intf->start_opts->promiscuous_mode) {
 			ret = rte_eth_promiscuous_enable(intf->port);
 			if (ret != 0)
 				fprintf(stderr,
@@ -759,8 +804,9 @@ static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
 		}
 
 		ret = rte_pdump_enable_bpf(intf->port, RTE_PDUMP_ALL_QUEUES,
+					   // flags, intf->start_opts->snaplen,
 					   flags, snaplen,
-					   r, mp, bpf_prm);
+					   r, mp, intf->bpf_prm);
 		if (ret < 0)
 			rte_exit(EXIT_FAILURE,
 				 "Packet dump enable failed: %s\n",
@@ -873,8 +919,7 @@ int main(int argc, char **argv)
 
 	collect_interfaces();
 
-	if (filter_str)
-		compile_filter();
+	compile_filters();
 
 	r = create_ring();
 	mp = create_mempool();
diff --git a/doc/guides/tools/dumpcap.rst b/doc/guides/tools/dumpcap.rst
index 0f89e6c5ca..0538f5833b 100644
--- a/doc/guides/tools/dumpcap.rst
+++ b/doc/guides/tools/dumpcap.rst
@@ -41,10 +41,19 @@ Running the Application
 
 To list interfaces available for capture, use ``-D`` or ``--list-interfaces``.
 
-To filter packets in style of *tshark*, use the ``-f`` flag.
-
 To capture on multiple interfaces at once, use multiple ``-i`` flags.
 
+To filter packets in style of *tshark*, use the ``-f`` flag. This flag
+can be specified multiple times. If this flag is specified prior to ``-i``
+it sets a default filter that will be used with all interfaces. If this
+flag is specified after ``-i`` it defines a filter for that interface only.
+
+To control the promiscuous mode of an interface, use the ``-p`` flag. This flag
+can be specified multiple times. If this flag is specified prior to ``-i`` it
+sets the default mode for all interfaces. If this flag is specified after ``-i``
+it sets the mode for that interface. If you want to allow some interfaces to
+remain in promiscuous mode, this must flag must be associated with an interface.
+
 
 Example
 -------
@@ -70,6 +79,10 @@ Example
 Limitations
 -----------
 
+The following option of Wireshark ``dumpcap`` has a different behavior:
+
+   * ``-s`` -- snaplen is not per interface
+
 The following option of Wireshark ``dumpcap`` is not yet implemented:
 
    * ``-b|--ring-buffer`` -- more complex file management.
-- 
2.27.0

Re: [PATCH 5/6] app/dumpcap: improve per interface arg parsing

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:40 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> +
> +/* struct representing args for each interface */
> +struct interface_opts {
> +	const char *intf_arg;
> +	const char *filter_str;
> +	bool promiscuous_mode;
> +	uint32_t snaplen;
> +};
> +/* default parameters for interfaces */
> +static struct interface_opts interface_defaults = {
> +	.promiscuous_mode = true,
> +	.snaplen = RTE_MBUF_DEFAULT_BUF_SIZE
> +};
> +//TODO this can be specified per interface but there are a few places
> +// that need more review and possibly refactoring so leaving this one out
> +uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
> +/* array of interface parameters */
> +static struct interface_opts interface_args[RTE_MAX_ETHPORTS];

There is no need to split the interface_opts into a new data structure.
See my fixes patch.

[PATCH 6/6] app/dumpcap: refactor add all and default

[Ben Magistro]

This refactors the add all and default interface functions to reduce code
duplication.

Cc: stephen@networkplumber.org

Signed-off-by: Ben Magistro <koncept1@gmail.com>
---
 app/dumpcap/main.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 1dc4a38adb..d85839a550 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -230,33 +230,20 @@ static void add_interface(uint16_t port, const char *name, struct interface_opts
 	TAILQ_INSERT_TAIL(&interfaces, intf, next);
 }
 
-/* Select all valid DPDK interfaces */
-static void select_all_interfaces(struct interface_opts *opts)
+/* Select available DPDK interfaces up to the limit  */
+static void select_available_interfaces(uint8_t limit, struct interface_opts *opts)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 	uint16_t p;
+	uint8_t added = 0;
 
 	RTE_ETH_FOREACH_DEV(p) {
 		if (rte_eth_dev_get_name_by_port(p, name) < 0)
 			continue;
 		add_interface(p, name, opts);
-	}
-}
-
-/*
- * Choose interface to capture if no -i option given.
- * Select the first DPDK port, this matches what dumpcap does.
- */
-static void set_default_interface(struct interface_opts *opts)
-{
-	char name[RTE_ETH_NAME_MAX_LEN];
-	uint16_t p;
-
-	RTE_ETH_FOREACH_DEV(p) {
-		if (rte_eth_dev_get_name_by_port(p, name) < 0)
-			continue;
-		add_interface(p, name, opts);
-		return;
+		added++;
+		if (added == limit)
+			break;
 	}
 }
 
@@ -266,7 +253,7 @@ static void select_interface(struct interface_opts *opts)
 	uint16_t port;
 
 	if (strcmp(opts->intf_arg, "*") == 0)
-		select_all_interfaces(opts);
+		select_available_interfaces(RTE_MAX_ETHPORTS, opts);
 	else if (rte_eth_dev_get_port_by_name(opts->intf_arg, &port) == 0)
 		add_interface(port, opts->intf_arg, opts);
 	else {
@@ -292,7 +279,7 @@ static void collect_interfaces(void)
 	active = 0;
 
 	if (interface_arg_count == 0)
-		set_default_interface(&interface_defaults);
+		select_available_interfaces(1, &interface_defaults);
 	else
 		for (uint8_t i = 0; i < interface_arg_count; ++i)
 			select_interface(&interface_args[i]);
-- 
2.27.0

Re: [PATCH 1/6] app/dumpcap: add additional dump info

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:36 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> This change increases the information returned when listing interfaces
> to include link state and promiscuous mode. Additionally, the
> information is formatted to be easily consumed by a machine or a person.
> 
> This change is/was utilized while troubleshooting and developing the
> subsequent patches to address the issues identified on the mailing
> list [1].
> 
> [1] http://mails.dpdk.org/archives/dev/2022-December/258317.html
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>

The output format was designed to be the same as dumpcap,
if you want more info use procinfo.


$ dumpcap -D
1. enp2s0
2. any
3. lo (Loopback)
4. wlo1
5. bluetooth0
6. bluetooth-monitor
7. nflog
8. nfqueue
9. dbus-system
10. dbus-session

Testing a fix now for multiple interfaces.

[RFT] dumpcap: fix multiple interface and promiscious handling

[Stephen Hemminger]

The code to handle multiple interfaces needs to setup
the interface list when command line is parsed, otherwise multiple
interfaces won't work.

Dumpcap enables promiscuous on a port, and then always resets
it on exit. If the device was already in promiscuous mode,
then don't change it.

The handling of the -p flag should match what dumpcap does.

Fixes: 499b1cbcf9d3 ("app/dumpcap: check for failure to set promiscuous")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 98 ++++++++++++++++++++++++----------------------
 1 file changed, 52 insertions(+), 46 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 2eb8414efaa5..33125710428e 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -65,8 +65,6 @@ static const char *file_prefix;
 static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
 static bool dump_bpf;
 static bool show_interfaces;
-static bool select_interfaces;
-const char *interface_arg;
 
 static struct {
 	uint64_t  duration;	/* nanoseconds */
@@ -83,6 +81,7 @@ static size_t file_size;
 struct interface {
 	TAILQ_ENTRY(interface) next;
 	uint16_t port;
+	bool promisc_mode;
 	char name[RTE_ETH_NAME_MAX_LEN];
 
 	struct rte_rxtx_callback *rx_cb[RTE_MAX_QUEUES_PER_PORT];
@@ -90,7 +89,6 @@ struct interface {
 
 TAILQ_HEAD(interface_list, interface);
 static struct interface_list interfaces = TAILQ_HEAD_INITIALIZER(interfaces);
-static struct interface *port2intf[RTE_MAX_ETHPORTS];
 
 /* Can do either pcap or pcapng format output */
 typedef union {
@@ -197,29 +195,36 @@ static void add_interface(uint16_t port, const char *name)
 {
 	struct interface *intf;
 
+	if (strlen(name) >= RTE_ETH_NAME_MAX_LEN)
+		rte_exit(EXIT_FAILURE, "invalid name for interface: '%s'\n", name);
+
 	intf = malloc(sizeof(*intf));
 	if (!intf)
 		rte_exit(EXIT_FAILURE, "no memory for interface\n");
 
 	memset(intf, 0, sizeof(*intf));
 	rte_strscpy(intf->name, name, sizeof(intf->name));
+	intf->promisc_mode = promiscuous_mode;
+	intf->port = port;	/* set later */
 
-	printf("Capturing on '%s'\n", name);
-
-	port2intf[port] = intf;
 	TAILQ_INSERT_TAIL(&interfaces, intf, next);
 }
 
-/* Select all valid DPDK interfaces */
-static void select_all_interfaces(void)
+/* Name has been set but need to lookup port after eal_init */
+static void find_interfaces(void)
 {
-	char name[RTE_ETH_NAME_MAX_LEN];
-	uint16_t p;
+	struct interface *intf;
 
-	RTE_ETH_FOREACH_DEV(p) {
-		if (rte_eth_dev_get_name_by_port(p, name) < 0)
+	TAILQ_FOREACH(intf, &interfaces, next) {
+		/* if name is valid then just record port */
+		if (rte_eth_dev_get_port_by_name(intf->name, &intf->port) == 0)
 			continue;
-		add_interface(p, name);
+
+		/* maybe got passed port number string as name */
+		intf->port = get_uint(intf->name, "port_number", UINT16_MAX);
+		if (rte_eth_dev_get_name_by_port(intf->port, intf->name) < 0)
+			rte_exit(EXIT_FAILURE, "Invalid port number %u\n",
+				 intf->port);
 	}
 }
 
@@ -235,32 +240,13 @@ static void set_default_interface(void)
 	RTE_ETH_FOREACH_DEV(p) {
 		if (rte_eth_dev_get_name_by_port(p, name) < 0)
 			continue;
+
 		add_interface(p, name);
 		return;
 	}
 	rte_exit(EXIT_FAILURE, "No usable interfaces found\n");
 }
 
-/* Lookup interface by name or port and add it to the list */
-static void select_interface(const char *arg)
-{
-	uint16_t port;
-
-	if (strcmp(arg, "*") == 0)
-		select_all_interfaces();
-	else if (rte_eth_dev_get_port_by_name(arg, &port) == 0)
-		add_interface(port, arg);
-	else {
-		char name[RTE_ETH_NAME_MAX_LEN];
-
-		port = get_uint(arg, "port_number", UINT16_MAX);
-		if (rte_eth_dev_get_name_by_port(port, name) < 0)
-			rte_exit(EXIT_FAILURE, "Invalid port number %u\n",
-				 port);
-		add_interface(port, name);
-	}
-}
-
 /* Display list of possible interfaces that can be used. */
 static void dump_interfaces(void)
 {
@@ -377,8 +363,7 @@ static void parse_opts(int argc, char **argv)
 			usage();
 			exit(0);
 		case 'i':
-			select_interfaces = true;
-			interface_arg = optarg;
+			add_interface(-1, optarg);
 			break;
 		case 'n':
 			use_pcapng = true;
@@ -387,7 +372,22 @@ static void parse_opts(int argc, char **argv)
 			ring_size = get_uint(optarg, "packet_limit", 0);
 			break;
 		case 'p':
-			promiscuous_mode = false;
+			/* Like dumpcap this option can occur multiple times.
+			 *
+			 * If used before the first occurrence of the -i option,
+			 * no interface will be put into the promiscuous mode.
+			 * If used after an -i option, the interface specified
+			 * by the last -i option occurring before this option
+			 * will not be put into the promiscuous mode.
+			 */
+			if (TAILQ_EMPTY(&interfaces)) {
+				promiscuous_mode = false;
+			} else {
+				struct interface *intf;
+
+				intf = TAILQ_LAST(&interfaces, interface_list);
+				intf->promisc_mode = false;
+			}
 			break;
 		case 'P':
 			use_pcapng = false;
@@ -436,7 +436,7 @@ cleanup_pdump_resources(void)
 	TAILQ_FOREACH(intf, &interfaces, next) {
 		rte_pdump_disable(intf->port,
 				  RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
-		if (promiscuous_mode)
+		if (intf->promisc_mode)
 			rte_eth_promiscuous_disable(intf->port);
 	}
 }
@@ -696,12 +696,17 @@ static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
 		flags |= RTE_PDUMP_FLAG_PCAPNG;
 
 	TAILQ_FOREACH(intf, &interfaces, next) {
-		if (promiscuous_mode) {
-			ret = rte_eth_promiscuous_enable(intf->port);
-			if (ret != 0)
-				fprintf(stderr,
-					"port %u set promiscuous enable failed: %d\n",
-					intf->port, ret);
+		if (intf->promisc_mode) {
+			if (rte_eth_promiscuous_get(intf->port) == 1) {
+				/* promiscuous already enabled */
+				intf->promisc_mode = false;
+			} else {
+				ret = rte_eth_promiscuous_enable(intf->port);
+				if (ret != 0)
+					fprintf(stderr,
+						"port %u set promiscuous enable failed: %d\n",
+						intf->port, ret);
+			}
 		}
 
 		ret = rte_pdump_enable_bpf(intf->port, RTE_PDUMP_ALL_QUEUES,
@@ -711,6 +716,8 @@ static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
 			rte_exit(EXIT_FAILURE,
 				 "Packet dump enable failed: %s\n",
 				 rte_strerror(-ret));
+
+		printf("Capturing on '%s'\n", intf->name);
 	}
 }
 
@@ -817,14 +824,13 @@ int main(int argc, char **argv)
 	if (rte_eth_dev_count_avail() == 0)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports found\n");
 
-	if (select_interfaces)
-		select_interface(interface_arg);
-
 	if (filter_str)
 		compile_filter();
 
 	if (TAILQ_EMPTY(&interfaces))
 		set_default_interface();
+	else
+		find_interfaces();
 
 	r = create_ring();
 	mp = create_mempool();
-- 
2.39.0

Re: [PATCH 1/6] app/dumpcap: add additional dump info

[Stephen Hemminger]

On Mon,  2 Jan 2023 16:24:36 +0000
Ben Magistro <koncept1@gmail.com> wrote:

> This change increases the information returned when listing interfaces
> to include link state and promiscuous mode. Additionally, the
> information is formatted to be easily consumed by a machine or a person.
> 
> This change is/was utilized while troubleshooting and developing the
> subsequent patches to address the issues identified on the mailing
> list [1].
> 
> [1] http://mails.dpdk.org/archives/dev/2022-December/258317.html
> 
> Signed-off-by: Ben Magistro <koncept1@gmail.com>


I prefer to keep the similar output format to wireshark dumpcap

I.e:

$ dumpcap -D
1. enp2s0
2. any
3. lo (Loopback)
4. wlo1
5. bluetooth0
6. bluetooth-monitor
7. nflog
8. nfqueue
9. dbus-system
10. dbus-session

With testpmd running with three interfaces

# dpdk-dumpcap -D
0. net_null0
1. net_null1
2. net_null2

[PATCH v2 0/6] dumpcap support multiple interfaces

[Stephen Hemminger]

This set of patches fixed the handling of multiple interfaces
in dumpcap. It also supports recording the name, description
and capture filter in the pcapng file.

The last two are not necessary for stable.

Ben Magistro (1):
  app/dumpcap: fix storing port identifier

Stephen Hemminger (5):
  app/dumpcap: remove unused variable
  app/dumpcap: check for invalid interface name
  app/dumpcap: support multiple interfaces
  pcapng: require per-interface information
  app/dumpcap: support interface name and description

 app/dumpcap/main.c      | 299 ++++++++++++++++++++++++++--------------
 app/test/test_pcapng.c  |   7 +
 lib/pcapng/rte_pcapng.c |  79 +++++++----
 lib/pcapng/rte_pcapng.h |  25 ++++
 lib/pcapng/version.map  |   1 +
 5 files changed, 283 insertions(+), 128 deletions(-)

-- 
2.39.0

[PATCH v2 1/6] app/dumpcap: fix storing port identifier

[Stephen Hemminger]

From: Ben Magistro <koncept1@gmail.com>

When dumpcap adds an interface, the port was not being preserved. This
results in the structure being initialized and the port field being set
to 0 regardless of what port was actually selected. This unset field is
then used in both the enable and cleanup calls. This could result in the
capture occurring on the wrong interface.

Fixes: d59fb4d ("app/dumpcap: add new packet capture application")
Cc: stephen@networkplumber.org
Cc: stable@dpdk.org

Signed-off-by: Ben Magistro <koncept1@gmail.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 2eb8414efaa5..4751ca26b892 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -202,6 +202,7 @@ static void add_interface(uint16_t port, const char *name)
 		rte_exit(EXIT_FAILURE, "no memory for interface\n");
 
 	memset(intf, 0, sizeof(*intf));
+	intf->port = port;
 	rte_strscpy(intf->name, name, sizeof(intf->name));
 
 	printf("Capturing on '%s'\n", name);
-- 
2.39.0

[PATCH v2 2/6] app/dumpcap: remove unused variable

[Stephen Hemminger]

The port2intf variable was set but never used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 4751ca26b892..1c47ff851252 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -90,7 +90,6 @@ struct interface {
 
 TAILQ_HEAD(interface_list, interface);
 static struct interface_list interfaces = TAILQ_HEAD_INITIALIZER(interfaces);
-static struct interface *port2intf[RTE_MAX_ETHPORTS];
 
 /* Can do either pcap or pcapng format output */
 typedef union {
@@ -207,7 +206,6 @@ static void add_interface(uint16_t port, const char *name)
 
 	printf("Capturing on '%s'\n", name);
 
-	port2intf[port] = intf;
 	TAILQ_INSERT_TAIL(&interfaces, intf, next);
 }
 
-- 
2.39.0

[PATCH v2 3/6] app/dumpcap: check for invalid interface name

[Stephen Hemminger]

Avoid any possible issue with ridiculously long name.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 1c47ff851252..a7aec021204a 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -196,6 +196,9 @@ static void add_interface(uint16_t port, const char *name)
 {
 	struct interface *intf;
 
+	if (strlen(name) >= RTE_ETH_NAME_MAX_LEN)
+		rte_exit(EXIT_FAILURE, "invalid name for interface: '%s'\n", name);
+
 	intf = malloc(sizeof(*intf));
 	if (!intf)
 		rte_exit(EXIT_FAILURE, "no memory for interface\n");
-- 
2.39.0

[PATCH v2 4/6] app/dumpcap: support multiple interfaces

[Stephen Hemminger]

The code to handle multiple interfaces did not work before.
Need to initialize the list of interfaces while parsing the command
line. Each interface should the ability to have its own options.

Fixes another bug where dumpcap would always disable promiscious
on exit, even if set by the application.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 259 +++++++++++++++++++++++++++++----------------
 1 file changed, 165 insertions(+), 94 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index a7aec021204a..86a4423b6de1 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -55,19 +55,15 @@ static const char *progname;
 static bool quit_signal;
 static bool group_read;
 static bool quiet;
-static bool promiscuous_mode = true;
 static bool use_pcapng = true;
 static char *output_name;
-static const char *filter_str;
 static unsigned int ring_size = 2048;
 static const char *capture_comment;
 static const char *file_prefix;
-static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
 static bool dump_bpf;
 static bool show_interfaces;
-static bool select_interfaces;
-const char *interface_arg;
 
+/* capture limit options */
 static struct {
 	uint64_t  duration;	/* nanoseconds */
 	unsigned long packets;  /* number of packets in file */
@@ -75,14 +71,25 @@ static struct {
 } stop;
 
 /* Running state */
-static struct rte_bpf_prm *bpf_prm;
 static uint64_t start_time, end_time;
 static uint64_t packets_received;
 static size_t file_size;
 
+/* capture options */
+struct capture_options {
+	const char *filter;
+	uint32_t snap_len;
+	bool promisc_mode;
+} capture = {
+	.snap_len = RTE_MBUF_DEFAULT_BUF_SIZE,
+	.promisc_mode = true,
+};
+
 struct interface {
 	TAILQ_ENTRY(interface) next;
 	uint16_t port;
+	struct capture_options opts;
+	struct rte_bpf_prm *bpf_prm;
 	char name[RTE_ETH_NAME_MAX_LEN];
 
 	struct rte_rxtx_callback *rx_cb[RTE_MAX_QUEUES_PER_PORT];
@@ -192,7 +199,7 @@ static void auto_stop(char *opt)
 }
 
 /* Add interface to list of interfaces to capture */
-static void add_interface(uint16_t port, const char *name)
+static struct interface *add_interface(const char *name)
 {
 	struct interface *intf;
 
@@ -204,24 +211,29 @@ static void add_interface(uint16_t port, const char *name)
 		rte_exit(EXIT_FAILURE, "no memory for interface\n");
 
 	memset(intf, 0, sizeof(*intf));
-	intf->port = port;
 	rte_strscpy(intf->name, name, sizeof(intf->name));
-
-	printf("Capturing on '%s'\n", name);
+	intf->opts = capture;
+	intf->port = -1;	/* port set later after EAL init */
 
 	TAILQ_INSERT_TAIL(&interfaces, intf, next);
+	return intf;
 }
 
-/* Select all valid DPDK interfaces */
-static void select_all_interfaces(void)
+/* Name has been set but need to lookup port after eal_init */
+static void find_interfaces(void)
 {
-	char name[RTE_ETH_NAME_MAX_LEN];
-	uint16_t p;
+	struct interface *intf;
 
-	RTE_ETH_FOREACH_DEV(p) {
-		if (rte_eth_dev_get_name_by_port(p, name) < 0)
-			continue;
-		add_interface(p, name);
+	TAILQ_FOREACH(intf, &interfaces, next) {
+		/* if name is valid then just record port */
+		if (rte_eth_dev_get_port_by_name(intf->name, &intf->port) == 0)
+ 			continue;
+
+		/* maybe got passed port number string as name */
+		intf->port = get_uint(intf->name, "port_number", UINT16_MAX);
+		if (rte_eth_dev_get_name_by_port(intf->port, intf->name) < 0)
+			rte_exit(EXIT_FAILURE, "Invalid port number %u\n",
+				 intf->port);
 	}
 }
 
@@ -231,38 +243,21 @@ static void select_all_interfaces(void)
  */
 static void set_default_interface(void)
 {
+	struct interface *intf;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	uint16_t p;
 
 	RTE_ETH_FOREACH_DEV(p) {
 		if (rte_eth_dev_get_name_by_port(p, name) < 0)
 			continue;
-		add_interface(p, name);
+
+		intf = add_interface(name);
+		intf->port = p;
 		return;
 	}
 	rte_exit(EXIT_FAILURE, "No usable interfaces found\n");
 }
 
-/* Lookup interface by name or port and add it to the list */
-static void select_interface(const char *arg)
-{
-	uint16_t port;
-
-	if (strcmp(arg, "*") == 0)
-		select_all_interfaces();
-	else if (rte_eth_dev_get_port_by_name(arg, &port) == 0)
-		add_interface(port, arg);
-	else {
-		char name[RTE_ETH_NAME_MAX_LEN];
-
-		port = get_uint(arg, "port_number", UINT16_MAX);
-		if (rte_eth_dev_get_name_by_port(port, name) < 0)
-			rte_exit(EXIT_FAILURE, "Invalid port number %u\n",
-				 port);
-		add_interface(port, name);
-	}
-}
-
 /* Display list of possible interfaces that can be used. */
 static void dump_interfaces(void)
 {
@@ -278,37 +273,50 @@ static void dump_interfaces(void)
 	exit(0);
 }
 
-static void compile_filter(void)
+static void compile_filters(void)
 {
-	struct bpf_program bf;
-	pcap_t *pcap;
+	struct interface *intf;
 
-	pcap = pcap_open_dead(DLT_EN10MB, snaplen);
-	if (!pcap)
-		rte_exit(EXIT_FAILURE, "can not open pcap\n");
+	TAILQ_FOREACH(intf, &interfaces, next) {
+		struct rte_bpf_prm *bpf_prm;
+		struct bpf_program bf;
+		pcap_t *pcap;
 
-	if (pcap_compile(pcap, &bf, filter_str,
-			 1, PCAP_NETMASK_UNKNOWN) != 0)
-		rte_exit(EXIT_FAILURE, "pcap filter string not valid (%s)\n",
-			 pcap_geterr(pcap));
+		pcap = pcap_open_dead(DLT_EN10MB, intf->opts.snap_len);
+		if (!pcap)
+			rte_exit(EXIT_FAILURE, "can not open pcap\n");
 
-	bpf_prm = rte_bpf_convert(&bf);
-	if (bpf_prm == NULL)
-		rte_exit(EXIT_FAILURE,
-			 "bpf convert failed: %s(%d)\n",
-			 rte_strerror(rte_errno), rte_errno);
-
-	if (dump_bpf) {
-		printf("cBPF program (%u insns)\n", bf.bf_len);
-		bpf_dump(&bf, 1);
-		printf("\neBPF program (%u insns)\n", bpf_prm->nb_ins);
-		rte_bpf_dump(stdout, bpf_prm->ins, bpf_prm->nb_ins);
-		exit(0);
-	}
+		if (pcap_compile(pcap, &bf, intf->opts.filter,
+				 1, PCAP_NETMASK_UNKNOWN) != 0) {
+			fprintf(stderr,
+				"Invalid capture filter \"%s\": for interface '%s'\n",
+				intf->opts.filter, intf->name);
+			rte_exit(EXIT_FAILURE, "\n%s\n",
+				 pcap_geterr(pcap));
+		}
 
-	/* Don't care about original program any more */
-	pcap_freecode(&bf);
-	pcap_close(pcap);
+		bpf_prm = rte_bpf_convert(&bf);
+		if (bpf_prm == NULL)
+			rte_exit(EXIT_FAILURE,
+				 "BPF convert interface '%s'\n%s(%d)\n",
+				 intf->name,
+				 rte_strerror(rte_errno), rte_errno);
+
+		if (dump_bpf) {
+			printf("cBPF program (%u insns)\n", bf.bf_len);
+			bpf_dump(&bf, 1);
+			printf("\neBPF program (%u insns)\n",
+			       bpf_prm->nb_ins);
+			rte_bpf_dump(stdout, bpf_prm->ins, bpf_prm->nb_ins);
+			exit(0);
+		}
+
+		intf->bpf_prm = bpf_prm;
+
+		/* Don't care about original program any more */
+		pcap_freecode(&bf);
+		pcap_close(pcap);
+	}
 }
 
 /*
@@ -332,6 +340,8 @@ static void parse_opts(int argc, char **argv)
 		{ NULL },
 	};
 	int option_index, c;
+	struct interface *last_intf = NULL;
+	uint32_t len;
 
 	for (;;) {
 		c = getopt_long(argc, argv, "a:b:c:dDf:ghi:nN:pPqs:vw:",
@@ -369,7 +379,10 @@ static void parse_opts(int argc, char **argv)
 			show_interfaces = true;
 			break;
 		case 'f':
-			filter_str = optarg;
+			if (last_intf == NULL)
+				capture.filter = optarg;
+			else
+				last_intf->opts.filter = optarg;
 			break;
 		case 'g':
 			group_read = true;
@@ -379,8 +392,7 @@ static void parse_opts(int argc, char **argv)
 			usage();
 			exit(0);
 		case 'i':
-			select_interfaces = true;
-			interface_arg = optarg;
+			last_intf = add_interface(optarg);
 			break;
 		case 'n':
 			use_pcapng = true;
@@ -389,7 +401,18 @@ static void parse_opts(int argc, char **argv)
 			ring_size = get_uint(optarg, "packet_limit", 0);
 			break;
 		case 'p':
-			promiscuous_mode = false;
+			/* Like dumpcap this option can occur multiple times.
+			 *
+			 * If used before the first occurrence of the -i option,
+			 * no interface will be put into the promiscuous mode.
+			 * If used after an -i option, the interface specified
+			 * by the last -i option occurring before this option
+			 * will not be put into the promiscuous mode.
+			 */
+			if (last_intf == NULL)
+				capture.promisc_mode = false;
+			else
+				last_intf->opts.promisc_mode = false;
 			break;
 		case 'P':
 			use_pcapng = false;
@@ -398,7 +421,12 @@ static void parse_opts(int argc, char **argv)
 			quiet = true;
 			break;
 		case 's':
-			snaplen = get_uint(optarg, "snap_len", 0);
+			len = get_uint(optarg, "snap_len", 0);
+			if (last_intf == NULL)
+				capture.snap_len = len;
+			else
+				last_intf->opts.snap_len = len;
+
 			break;
 		case 'w':
 			output_name = optarg;
@@ -438,7 +466,7 @@ cleanup_pdump_resources(void)
 	TAILQ_FOREACH(intf, &interfaces, next) {
 		rte_pdump_disable(intf->port,
 				  RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
-		if (promiscuous_mode)
+		if (intf->opts.promisc_mode)
 			rte_eth_promiscuous_disable(intf->port);
 	}
 }
@@ -582,17 +610,27 @@ static struct rte_ring *create_ring(void)
 
 static struct rte_mempool *create_mempool(void)
 {
+	const struct interface *intf;
 	static const char pool_name[] = "capture_mbufs";
 	size_t num_mbufs = 2 * ring_size;
 	struct rte_mempool *mp;
+	uint32_t data_size = 128;
 
 	mp = rte_mempool_lookup(pool_name);
 	if (mp)
 		return mp;
 
+	/* Common pool so size mbuf for biggest snap length */
+	TAILQ_FOREACH(intf, &interfaces, next) {
+		uint32_t mbuf_size = rte_pcapng_mbuf_size(intf->opts.snap_len);
+
+		if (mbuf_size > data_size)
+			data_size = mbuf_size;
+	}
+
 	mp = rte_pktmbuf_pool_create_by_ops(pool_name, num_mbufs,
 					    MBUF_POOL_CACHE_SIZE, 0,
-					    rte_pcapng_mbuf_size(snaplen),
+					    data_size,
 					    rte_socket_id(), "ring_mp_sc");
 	if (mp == NULL)
 		rte_exit(EXIT_FAILURE,
@@ -673,7 +711,8 @@ static dumpcap_out_t create_output(void)
 	} else {
 		pcap_t *pcap;
 
-		pcap = pcap_open_dead_with_tstamp_precision(DLT_EN10MB, snaplen,
+		pcap = pcap_open_dead_with_tstamp_precision(DLT_EN10MB,
+							    capture.snap_len,
 							    PCAP_TSTAMP_PRECISION_NANO);
 		if (pcap == NULL)
 			rte_exit(EXIT_FAILURE, "pcap_open_dead failed\n");
@@ -690,6 +729,7 @@ static dumpcap_out_t create_output(void)
 static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
 {
 	struct interface *intf;
+	unsigned int count = 0;
 	uint32_t flags;
 	int ret;
 
@@ -698,22 +738,55 @@ static void enable_pdump(struct rte_ring *r, struct rte_mempool *mp)
 		flags |= RTE_PDUMP_FLAG_PCAPNG;
 
 	TAILQ_FOREACH(intf, &interfaces, next) {
-		if (promiscuous_mode) {
-			ret = rte_eth_promiscuous_enable(intf->port);
-			if (ret != 0)
-				fprintf(stderr,
-					"port %u set promiscuous enable failed: %d\n",
-					intf->port, ret);
-		}
-
 		ret = rte_pdump_enable_bpf(intf->port, RTE_PDUMP_ALL_QUEUES,
-					   flags, snaplen,
-					   r, mp, bpf_prm);
-		if (ret < 0)
+					   flags, intf->opts.snap_len,
+					   r, mp, intf->bpf_prm);
+		if (ret < 0) {
+			const struct interface *intf2;
+
+			/* unwind any previous enables */
+			TAILQ_FOREACH(intf2, &interfaces, next) {
+				if (intf == intf2)
+					break;
+				rte_pdump_disable(intf2->port,
+						  RTE_PDUMP_ALL_QUEUES, RTE_PDUMP_FLAG_RXTX);
+				if (intf2->opts.promisc_mode)
+					rte_eth_promiscuous_disable(intf2->port);
+			}
 			rte_exit(EXIT_FAILURE,
-				 "Packet dump enable failed: %s\n",
-				 rte_strerror(-ret));
+				"Packet dump enable on %u:%s failed %s\n",
+				intf->port, intf->name,
+				rte_strerror(-ret));
+		}
+
+		if (intf->opts.promisc_mode) {
+			if (rte_eth_promiscuous_get(intf->port) == 1) {
+				/* promiscuous already enabled */
+				intf->opts.promisc_mode = false;
+			} else {
+				ret = rte_eth_promiscuous_enable(intf->port);
+				if (ret != 0)
+					fprintf(stderr,
+						"port %u set promiscuous enable failed: %d\n",
+						intf->port, ret);
+				intf->opts.promisc_mode = false;
+			}
+		}
+		++count;
+	}
+
+	fputs("Capturing on ", stdout);
+	TAILQ_FOREACH(intf, &interfaces, next) {
+		if (intf != TAILQ_FIRST(&interfaces)) {
+			if (count > 2)
+				putchar(',');
+			putchar(' ');
+			if (TAILQ_NEXT(intf, next) == NULL)
+				fputs("and ", stdout);
+		}
+		printf("'%s'", intf->name);
 	}
+	putchar('\n');
 }
 
 /*
@@ -736,7 +809,7 @@ static ssize_t
 pcap_write_packets(pcap_dumper_t *dumper,
 		   struct rte_mbuf *pkts[], uint16_t n)
 {
-	uint8_t temp_data[snaplen];
+	uint8_t temp_data[RTE_MBUF_DEFAULT_BUF_SIZE];
 	struct pcap_pkthdr header;
 	uint16_t i;
 	size_t total = 0;
@@ -747,7 +820,7 @@ pcap_write_packets(pcap_dumper_t *dumper,
 		struct rte_mbuf *m = pkts[i];
 
 		header.len = rte_pktmbuf_pkt_len(m);
-		header.caplen = RTE_MIN(header.len, snaplen);
+		header.caplen = RTE_MIN(header.len, sizeof(temp_data));
 
 		pcap_dump((u_char *)dumper, &header,
 			  rte_pktmbuf_read(m, 0, header.caplen, temp_data));
@@ -819,14 +892,12 @@ int main(int argc, char **argv)
 	if (rte_eth_dev_count_avail() == 0)
 		rte_exit(EXIT_FAILURE, "No Ethernet ports found\n");
 
-	if (select_interfaces)
-		select_interface(interface_arg);
-
-	if (filter_str)
-		compile_filter();
-
 	if (TAILQ_EMPTY(&interfaces))
 		set_default_interface();
+	else
+		find_interfaces();
+
+	compile_filters();
 
 	r = create_ring();
 	mp = create_mempool();
-- 
2.39.0

[PATCH v2 5/6] pcapng: require per-interface information

[Stephen Hemminger]

This changes the API for how pcapng is used.
Before each interface was automatically added to the capture
file.  Now the application must add each interface.
Note: API changes are allowed because this is an experimental
interface.

This allows application to specify extra meta data like
interface name, description and packet filter.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_pcapng.c  |  7 ++++
 lib/pcapng/rte_pcapng.c | 79 ++++++++++++++++++++++++++---------------
 lib/pcapng/rte_pcapng.h | 25 +++++++++++++
 lib/pcapng/version.map  |  1 +
 4 files changed, 84 insertions(+), 28 deletions(-)

diff --git a/app/test/test_pcapng.c b/app/test/test_pcapng.c
index a7acbdc0586d..edba46d1fe0f 100644
--- a/app/test/test_pcapng.c
+++ b/app/test/test_pcapng.c
@@ -109,6 +109,13 @@ test_setup(void)
 		return -1;
 	}
 
+	/* Add interface to the file */
+	if (rte_pcapng_add_interface(pcapng, port_id,
+				     NULL, NULL, NULL) != 0) {
+		fprintf(stderr, "can not add port %u\n", port_id);
+		return -1;
+	}
+
 	/* Make a pool for cloned packets */
 	mp = rte_pktmbuf_pool_create_by_ops("pcapng_test_pool", IOV_MAX + NUM_PACKETS,
 					    0, 0,
diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c
index 80d08e1a3bde..ea004939e63e 100644
--- a/lib/pcapng/rte_pcapng.c
+++ b/lib/pcapng/rte_pcapng.c
@@ -32,6 +32,9 @@
 /* Format of the capture file handle */
 struct rte_pcapng {
 	int  outfd;		/* output file */
+
+	unsigned int ports;	/* number of interfaces added */
+
 	/* DPDK port id to interface index in file */
 	uint32_t port_index[RTE_MAX_ETHPORTS];
 };
@@ -185,8 +188,10 @@ pcapng_section_block(rte_pcapng_t *self,
 }
 
 /* Write an interface block for a DPDK port */
-static int
-pcapng_add_interface(rte_pcapng_t *self, uint16_t port)
+int
+rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
+			 const char *ifname, const char *ifdescr,
+			 const char *filter)
 {
 	struct pcapng_interface_block *hdr;
 	struct rte_eth_dev_info dev_info;
@@ -197,7 +202,7 @@ pcapng_add_interface(rte_pcapng_t *self, uint16_t port)
 	const uint8_t tsresol = 9;	/* nanosecond resolution */
 	uint32_t len;
 	void *buf;
-	char ifname[IF_NAMESIZE];
+	char ifname_buf[IF_NAMESIZE];
 	char ifhw[256];
 	uint64_t speed = 0;
 
@@ -205,8 +210,14 @@ pcapng_add_interface(rte_pcapng_t *self, uint16_t port)
 		return -1;
 
 	/* make something like an interface name */
-	if (if_indextoname(dev_info.if_index, ifname) == NULL)
-		snprintf(ifname, IF_NAMESIZE, "dpdk:%u", port);
+	if (ifname == NULL) {
+		/* Use kernel name if available */
+		ifname = if_indextoname(dev_info.if_index, ifname_buf);
+		if (ifname == NULL) {
+			snprintf(ifname_buf, IF_NAMESIZE, "dpdk:%u", port);
+			ifname = ifname_buf;
+		}
+	}
 
 	/* make a useful device hardware string */
 	dev = dev_info.device;
@@ -230,10 +241,14 @@ pcapng_add_interface(rte_pcapng_t *self, uint16_t port)
 	len += pcapng_optlen(sizeof(tsresol));	/* timestamp */
 	len += pcapng_optlen(strlen(ifname));	/* ifname */
 
+	if (ifdescr)
+		len += pcapng_optlen(strlen(ifdescr));
 	if (ea)
 		len += pcapng_optlen(RTE_ETHER_ADDR_LEN); /* macaddr */
 	if (speed != 0)
 		len += pcapng_optlen(sizeof(uint64_t));
+	if (filter)
+		len += pcapng_optlen(strlen(filter) + 1);
 	if (dev)
 		len += pcapng_optlen(strlen(ifhw));
 
@@ -256,6 +271,9 @@ pcapng_add_interface(rte_pcapng_t *self, uint16_t port)
 				&tsresol, sizeof(tsresol));
 	opt = pcapng_add_option(opt, PCAPNG_IFB_NAME,
 				ifname, strlen(ifname));
+	if (ifdescr)
+		opt = pcapng_add_option(opt, PCAPNG_IFB_DESCRIPTION,
+					ifdescr, strlen(ifdescr));
 	if (ea)
 		opt = pcapng_add_option(opt, PCAPNG_IFB_MACADDR,
 					ea, RTE_ETHER_ADDR_LEN);
@@ -265,31 +283,29 @@ pcapng_add_interface(rte_pcapng_t *self, uint16_t port)
 	if (dev)
 		opt = pcapng_add_option(opt, PCAPNG_IFB_HARDWARE,
 					 ifhw, strlen(ifhw));
+	if (filter) {
+		/* Encoding is that the first octet indicates string vs BPF */
+		size_t len;
+		char *buf;
+
+		len = strlen(filter) + 1;
+		buf = alloca(len);
+		*buf = '\0';
+		memcpy(buf + 1, filter, len);
+
+		opt = pcapng_add_option(opt, PCAPNG_IFB_FILTER,
+					buf, len);
+	}
+
 	opt = pcapng_add_option(opt, PCAPNG_OPT_END, NULL, 0);
 
 	/* clone block_length after optionsa */
 	memcpy(opt, &hdr->block_length, sizeof(uint32_t));
 
-	return write(self->outfd, buf, len);
-}
-
-/*
- * Write the list of possible interfaces at the start
- * of the file.
- */
-static int
-pcapng_interfaces(rte_pcapng_t *self)
-{
-	uint16_t port_id;
-	uint16_t index = 0;
+	/* remember the file index */
+	self->port_index[port] = self->ports++;
 
-	RTE_ETH_FOREACH_DEV(port_id) {
-		/* The list if ports in pcapng needs to be contiguous */
-		self->port_index[port_id] = index++;
-		if (pcapng_add_interface(self, port_id) < 0)
-			return -1;
-	}
-	return 0;
+	return write(self->outfd, buf, len);
 }
 
 /*
@@ -598,6 +614,13 @@ rte_pcapng_write_packets(rte_pcapng_t *self,
 			return -1;
 		}
 
+		/* check that this interface was added. */
+		epb->interface_id = self->port_index[m->port];
+		if (unlikely(epb->interface_id > RTE_MAX_ETHPORTS)) {
+			rte_errno = EINVAL;
+			return -1;
+		}
+
 		/*
 		 * Handle case of highly fragmented and large burst size
 		 * Note: this assumes that max segments per mbuf < IOV_MAX
@@ -616,7 +639,6 @@ rte_pcapng_write_packets(rte_pcapng_t *self,
 		 * The DPDK port is recorded during pcapng_copy.
 		 * Map that to PCAPNG interface in file.
 		 */
-		epb->interface_id = self->port_index[m->port];
 		do {
 			iov[cnt].iov_base = rte_pktmbuf_mtod(m, void *);
 			iov[cnt].iov_len = rte_pktmbuf_data_len(m);
@@ -638,6 +660,7 @@ rte_pcapng_fdopen(int fd,
 		  const char *osname, const char *hardware,
 		  const char *appname, const char *comment)
 {
+	unsigned int i;
 	rte_pcapng_t *self;
 
 	self = malloc(sizeof(*self));
@@ -647,13 +670,13 @@ rte_pcapng_fdopen(int fd,
 	}
 
 	self->outfd = fd;
+	self->ports = 0;
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
+		self->port_index[i] = UINT32_MAX;
 
 	if (pcapng_section_block(self, osname, hardware, appname, comment) < 0)
 		goto fail;
 
-	if (pcapng_interfaces(self) < 0)
-		goto fail;
-
 	return self;
 fail:
 	free(self);
diff --git a/lib/pcapng/rte_pcapng.h b/lib/pcapng/rte_pcapng.h
index 7d2697c647ef..86b7996e291e 100644
--- a/lib/pcapng/rte_pcapng.h
+++ b/lib/pcapng/rte_pcapng.h
@@ -70,6 +70,31 @@ __rte_experimental
 void
 rte_pcapng_close(rte_pcapng_t *self);
 
+/**
+ * Add interface information to the capture file
+ *
+ * @param self
+ *  The handle to the packet capture file
+ * @param port
+ *  The Ethernet port to report stats on.
+ * @param ifname (optional)
+ *  Interface name to record in the file.
+ *  If not specified, name will be constructed from port
+ * @param ifdescr (optional)
+ *  Interface description to record in the file.
+ * @param filter
+ *  Capture filter to record in the file.
+ *
+ * Interfaces must be added to the output file after opening
+ * and before any packet record. All ports used in packet capture
+ * must be added.
+ */
+__rte_experimental
+int
+rte_pcapng_add_interface(rte_pcapng_t *self, uint16_t port,
+			 const char *ifname, const char *ifdescr,
+			 const char *filter);
+
 /**
  * Direction flag
  * These should match Enhanced Packet Block flag bits
diff --git a/lib/pcapng/version.map b/lib/pcapng/version.map
index 05a9c86a7d91..e98e71038ee6 100644
--- a/lib/pcapng/version.map
+++ b/lib/pcapng/version.map
@@ -7,6 +7,7 @@ EXPERIMENTAL {
 	rte_pcapng_mbuf_size;
 	rte_pcapng_write_packets;
 	rte_pcapng_write_stats;
+	rte_pcapng_add_interface;
 
 	local: *;
 };
-- 
2.39.0

[PATCH v2 6/6] app/dumpcap: support interface name and description

[Stephen Hemminger]

Support setting --ifname and --ifdescr options to record information
in the start of the pcapng interface description block.
Also, records filter (if any) used in the file.

This also makes sure only the interfaces being recorded in
the capture file are in the interface block.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/dumpcap/main.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 86a4423b6de1..fdabc14b02c6 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -93,6 +93,8 @@ struct interface {
 	char name[RTE_ETH_NAME_MAX_LEN];
 
 	struct rte_rxtx_callback *rx_cb[RTE_MAX_QUEUES_PER_PORT];
+	const char *ifname;
+	const char *ifdescr;
 };
 
 TAILQ_HEAD(interface_list, interface);
@@ -110,6 +112,9 @@ static void usage(void)
 	printf("Capture Interface:\n"
 	       "  -i <interface>           name or port index of interface\n"
 	       "  -f <capture filter>      packet filter in libpcap filter syntax\n");
+	printf("  --ifname <name>          name to use in the capture file\n");
+	printf("  --ifdescr <description>\n");
+	printf("                           description to use in the capture file\n");
 	printf("  -s <snaplen>, --snapshot-length <snaplen>\n"
 	       "                           packet snapshot length (def: %u)\n",
 	       RTE_MBUF_DEFAULT_BUF_SIZE);
@@ -330,6 +335,8 @@ static void parse_opts(int argc, char **argv)
 		{ "capture-comment", required_argument, NULL, 0 },
 		{ "file-prefix",     required_argument, NULL, 0 },
 		{ "help",            no_argument,       NULL, 'h' },
+		{ "ifdescr",	     required_argument, NULL, 0 },
+		{ "ifname",	     required_argument, NULL, 0 },
 		{ "interface",       required_argument, NULL, 'i' },
 		{ "list-interfaces", no_argument,       NULL, 'D' },
 		{ "no-promiscuous-mode", no_argument,   NULL, 'p' },
@@ -350,18 +357,30 @@ static void parse_opts(int argc, char **argv)
 			break;
 
 		switch (c) {
-		case 0:
-			if (!strcmp(long_options[option_index].name,
-				    "capture-comment")) {
+		case 0: {
+			const char *longopt
+				= long_options[option_index].name;
+
+			if (!strcmp(longopt, "capture-comment")) {
 				capture_comment = optarg;
-			} else if (!strcmp(long_options[option_index].name,
-					   "file-prefix")) {
+			} else if (!strcmp(longopt,"file-prefix")) {
 				file_prefix = optarg;
+			} else if (!strcmp(longopt, "ifdescr")) {
+				if (last_intf == NULL)
+					rte_exit(EXIT_FAILURE,
+						 "--ifdescr must be specified after a -i option\n");
+				last_intf->ifdescr = optarg;
+			} else if (!strcmp(longopt, "ifname")) {
+				if (last_intf == NULL)
+					rte_exit(EXIT_FAILURE,
+						 "--ifname must be specified after a -i option\n");
+				last_intf->ifname = optarg;
 			} else {
 				usage();
 				exit(1);
 			}
 			break;
+		}
 		case 'a':
 			auto_stop(optarg);
 			break;
@@ -700,6 +719,7 @@ static dumpcap_out_t create_output(void)
 	}
 
 	if (use_pcapng) {
+		struct interface *intf;
 		char *os = get_os_info();
 
 		ret.pcapng = rte_pcapng_fdopen(fd, os, NULL,
@@ -708,6 +728,12 @@ static dumpcap_out_t create_output(void)
 			rte_exit(EXIT_FAILURE, "pcapng_fdopen failed: %s\n",
 				 strerror(rte_errno));
 		free(os);
+
+		TAILQ_FOREACH(intf, &interfaces, next) {
+			rte_pcapng_add_interface(ret.pcapng, intf->port,
+						 intf->ifname, intf->ifdescr,
+						 intf->opts.filter);
+		}
 	} else {
 		pcap_t *pcap;
 
-- 
2.39.0

Re: [PATCH v2 0/6] dumpcap support multiple interfaces

[Thomas Monjalon]

> Ben Magistro (1):
>   app/dumpcap: fix storing port identifier
> 
> Stephen Hemminger (5):
>   app/dumpcap: remove unused variable
>   app/dumpcap: check for invalid interface name
>   app/dumpcap: support multiple interfaces
>   pcapng: require per-interface information
>   app/dumpcap: support interface name and description

Applied, thanks.

It required a bit of rebase, please check nothing is broken.