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

[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 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