[RFT] test-pmd: go back to using cmdline_interact

[RFT] test-pmd: go back to using cmdline_interact

[Stephen Hemminger]

The cmdline library poll function is broken on Windows
and was never tested, don't use it.

Instead, use sigaction() to cancel read character on Unix OS's
and a new helper to cancel I/O on Windows.

Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Bugzilla ID: 1180
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmdline.c | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c | 11 +++++++++++
 lib/cmdline/cmdline.c  | 16 ++++++++++++++++
 lib/cmdline/cmdline.h  |  1 +
 4 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc329b..072437d9bfcf 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,6 +66,7 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
+static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -13028,26 +13029,26 @@ cmdline_read_from_file(const char *filename)
 	printf("Read CLI commands from %s\n", filename);
 }
 
+void
+prompt_exit(void)
+{
+	cmdline_cancel(testpmd_cl);
+	cmdline_quit(testpmd_cl);
+}
+
 /* prompt function, called from main on MAIN lcore */
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
+		fprintf(stderr,
+			"Failed to create stdin based cmdline context\n");
 		return;
-
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
-
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
 	}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 2ce19ed47ab4..5cb6f9252395 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4469,6 +4469,7 @@ static void
 signal_handler(int signum __rte_unused)
 {
 	f_quit = 1;
+	prompt_exit();
 }
 
 int
@@ -4479,8 +4480,18 @@ main(int argc, char** argv)
 	uint16_t count;
 	int ret;
 
+#ifdef RTE_EXEC_ENV_WINDOWS
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
+#else
+	/* Want read() not to be restarted on signal */
+	struct sigaction action = {
+		.sa_handler = signal_handler,
+	};
+
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGTERM, &action, NULL);
+#endif
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
index 8ad0690d8533..69b3d9827290 100644
--- a/lib/cmdline/cmdline.c
+++ b/lib/cmdline/cmdline.c
@@ -176,6 +176,22 @@ cmdline_quit(struct cmdline *cl)
 	rdline_quit(&cl->rdl);
 }
 
+void
+cmdline_cancel(struct cmdline *cl)
+{
+	if (!cl)
+		return;
+
+#ifdef RTE_EXEC_ENV_WINDOWS
+	/* force the outstanding read on console to exit */
+	if (cl->oldterm.is_console_input) {
+		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+
+		CancelIoEx(handle, NULL);
+	}
+#endif
+}
+
 int
 cmdline_poll(struct cmdline *cl)
 {
diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h
index b14355ef5121..7dac90cfe7eb 100644
--- a/lib/cmdline/cmdline.h
+++ b/lib/cmdline/cmdline.h
@@ -58,6 +58,7 @@ cmdline_get_rdline(struct cmdline *cl);
 int cmdline_poll(struct cmdline *cl);
 
 void cmdline_interact(struct cmdline *cl);
+void cmdline_cancel(struct cmdline *cl);
 void cmdline_quit(struct cmdline *cl);
 
 #ifdef __cplusplus
-- 
2.39.2

[RFT v2] test-pmd: go back to using cmdline_interact

[Stephen Hemminger]

The cmdline library poll function is broken on Windows
and was never tested, don't use it.

Instead, use sigaction() to cancel read character on Unix OS's
and a new helper to cancel I/O on Windows.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - make cmdline_cancel an exposed API

 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.h            | 10 ++++++++++
 lib/cmdline/cmdline_os_unix.c    |  5 +++++
 lib/cmdline/cmdline_os_windows.c | 14 ++++++++++++++
 lib/cmdline/version.map          |  3 +++
 6 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc329b..072437d9bfcf 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,6 +66,7 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
+static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -13028,26 +13029,26 @@ cmdline_read_from_file(const char *filename)
 	printf("Read CLI commands from %s\n", filename);
 }
 
+void
+prompt_exit(void)
+{
+	cmdline_cancel(testpmd_cl);
+	cmdline_quit(testpmd_cl);
+}
+
 /* prompt function, called from main on MAIN lcore */
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
+		fprintf(stderr,
+			"Failed to create stdin based cmdline context\n");
 		return;
-
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
-
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
 	}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 2ce19ed47ab4..5cb6f9252395 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4469,6 +4469,7 @@ static void
 signal_handler(int signum __rte_unused)
 {
 	f_quit = 1;
+	prompt_exit();
 }
 
 int
@@ -4479,8 +4480,18 @@ main(int argc, char** argv)
 	uint16_t count;
 	int ret;
 
+#ifdef RTE_EXEC_ENV_WINDOWS
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
+#else
+	/* Want read() not to be restarted on signal */
+	struct sigaction action = {
+		.sa_handler = signal_handler,
+	};
+
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGTERM, &action, NULL);
+#endif
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h
index b14355ef5121..2a1721cf9712 100644
--- a/lib/cmdline/cmdline.h
+++ b/lib/cmdline/cmdline.h
@@ -60,6 +60,16 @@ int cmdline_poll(struct cmdline *cl);
 void cmdline_interact(struct cmdline *cl);
 void cmdline_quit(struct cmdline *cl);
 
+
+/**
+ * This function causes the read() in cmdline_interact to exit.
+ *
+ * @param cl
+ *   The command line object.
+ */
+__rte_experimental
+void cmdline_cancel(struct cmdline *cl);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 755a6141546d..356c5b3f3ca2 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -52,3 +52,8 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 {
 	return vdprintf(fd, format, op);
 }
+
+void
+cmdline_cancel(__rte_unused struct cmdline *cl)
+{
+}
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index be11409029a2..8cff3b175747 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -203,3 +203,17 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 
 	return ret;
 }
+
+void
+cmdline_cancel(struct cmdline *cl)
+{
+	if (!cl)
+		return;
+
+	/* force the outstanding read on console to exit */
+	if (cl->oldterm.is_console_input) {
+		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+
+		CancelIoEx(handle, NULL);
+	}
+}
diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
index e3d59aaf8d61..a8a417cd6ff1 100644
--- a/lib/cmdline/version.map
+++ b/lib/cmdline/version.map
@@ -84,5 +84,8 @@ EXPERIMENTAL {
 	# added in 22.07
 	cmdline_parse_check;
 
+	# added in 23.03
+	cmdline_cancel;
+
 	local: *;
 };
-- 
2.39.2

RE: [RFT v2] test-pmd: go back to using cmdline_interact

[Ling, WeiX]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, March 14, 2023 5:39 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [RFT v2] test-pmd: go back to using cmdline_interact
> 
> The cmdline library poll function is broken on Windows and was never tested,
> don't use it.
> 
> Instead, use sigaction() to cancel read character on Unix OS's and a new
> helper to cancel I/O on Windows.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Tested-by: Wei Ling <weix.ling@intel.com>

RE: [RFT v2] test-pmd: go back to using cmdline_interact

[Pier Damouny]

> Subject: [RFT v2] test-pmd: go back to using cmdline_interact
> 
> External email: Use caution opening links or attachments
> 
> 
> The cmdline library poll function is broken on Windows and was never
> tested, don't use it.
> 
> Instead, use sigaction() to cancel read character on Unix OS's and a new
> helper to cancel I/O on Windows.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Confirmed that this patch fixes Bug: 1180 on Windows.

Tested-by: Pier Damouny <pdamouny@nvidia.com>

[PATCH v3 0/3] testpmd control C fixes

[Stephen Hemminger]

Resolve issues around using control-C in testpmd.
Using cmdline_poll() is broken and won't work right so
don't use it. Instead use cmdline_interact() and fix
the signal handling.

Stephen Hemminger (3):
  testpmd: go back to using cmdline_interact
  testpmd: use sig_atomic_t for flag
  testpmd: enable interrupt in interactive mode

 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 17 ++++++++++++-----
 app/test-pmd/testpmd.h           |  3 ++-
 lib/cmdline/cmdline.h            | 10 ++++++++++
 lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
 lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
 lib/cmdline/cmdline_private.h    |  2 +-
 lib/cmdline/version.map          |  3 +++
 8 files changed, 65 insertions(+), 23 deletions(-)

-- 
2.39.2

[PATCH v3 1/3] testpmd: go back to using cmdline_interact

[Stephen Hemminger]

The cmdline library poll function is broken on Windows
and was never tested, don't use it.

Instead, use sigaction() to cancel read character on Unix OS's
and a new helper to cancel I/O on Windows.

Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Bugzilla ID: 1180
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Tested-by: Pier Damouny <pdamouny@nvidia.com>
---
 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.h            | 10 ++++++++++
 lib/cmdline/cmdline_os_unix.c    |  5 +++++
 lib/cmdline/cmdline_os_windows.c | 14 ++++++++++++++
 lib/cmdline/cmdline_private.h    |  2 +-
 lib/cmdline/version.map          |  3 +++
 7 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc329b..072437d9bfcf 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,6 +66,7 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
+static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -13028,26 +13029,26 @@ cmdline_read_from_file(const char *filename)
 	printf("Read CLI commands from %s\n", filename);
 }
 
+void
+prompt_exit(void)
+{
+	cmdline_cancel(testpmd_cl);
+	cmdline_quit(testpmd_cl);
+}
+
 /* prompt function, called from main on MAIN lcore */
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
+		fprintf(stderr,
+			"Failed to create stdin based cmdline context\n");
 		return;
-
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
-
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
 	}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 2ce19ed47ab4..5cb6f9252395 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4469,6 +4469,7 @@ static void
 signal_handler(int signum __rte_unused)
 {
 	f_quit = 1;
+	prompt_exit();
 }
 
 int
@@ -4479,8 +4480,18 @@ main(int argc, char** argv)
 	uint16_t count;
 	int ret;
 
+#ifdef RTE_EXEC_ENV_WINDOWS
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
+#else
+	/* Want read() not to be restarted on signal */
+	struct sigaction action = {
+		.sa_handler = signal_handler,
+	};
+
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGTERM, &action, NULL);
+#endif
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h
index b14355ef5121..2a1721cf9712 100644
--- a/lib/cmdline/cmdline.h
+++ b/lib/cmdline/cmdline.h
@@ -60,6 +60,16 @@ int cmdline_poll(struct cmdline *cl);
 void cmdline_interact(struct cmdline *cl);
 void cmdline_quit(struct cmdline *cl);
 
+
+/**
+ * This function causes the read() in cmdline_interact to exit.
+ *
+ * @param cl
+ *   The command line object.
+ */
+__rte_experimental
+void cmdline_cancel(struct cmdline *cl);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 64a945a34fb3..5f9839a15f98 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -51,3 +51,8 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 {
 	return vdprintf(fd, format, op);
 }
+
+void
+cmdline_cancel(__rte_unused struct cmdline *cl)
+{
+}
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index 73ed9ba290b8..80863bfc8a00 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -203,3 +203,17 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 
 	return ret;
 }
+
+void
+cmdline_cancel(struct cmdline *cl)
+{
+	if (!cl)
+		return;
+
+	/* force the outstanding read on console to exit */
+	if (cl->oldterm.is_console_input) {
+		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+
+		CancelIoEx(handle, NULL);
+	}
+}
diff --git a/lib/cmdline/cmdline_private.h b/lib/cmdline/cmdline_private.h
index a3271c76934a..7ec42ac3c78f 100644
--- a/lib/cmdline/cmdline_private.h
+++ b/lib/cmdline/cmdline_private.h
@@ -24,7 +24,7 @@
 #define RDLINE_HISTORY_MAX_LINE 64
 
 struct rdline {
-	enum rdline_status status;
+	volatile enum rdline_status status;
 	/* rdline bufs */
 	struct cirbuf left;
 	struct cirbuf right;
diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
index e3d59aaf8d61..a8a417cd6ff1 100644
--- a/lib/cmdline/version.map
+++ b/lib/cmdline/version.map
@@ -84,5 +84,8 @@ EXPERIMENTAL {
 	# added in 22.07
 	cmdline_parse_check;
 
+	# added in 23.03
+	cmdline_cancel;
+
 	local: *;
 };
-- 
2.39.2

[PATCH v3 2/3] testpmd: use sig_atomic_t for flag

[Stephen Hemminger]

Standard conforming signal handling in applications should use sig_atomic_t
for flags set by signal handler.
 https://www.gnu.org/software/libc/manual/html_node/Atomic-Data-Access.html

This is really just a fancy way of saying ''int'' but adds
to platform portablilty for DPDK.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/testpmd.c | 6 +-----
 app/test-pmd/testpmd.h | 3 ++-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 5cb6f9252395..aee926571f53 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -228,11 +228,7 @@ struct rte_eth_xstat_name *xstats_display;
 
 unsigned int xstats_display_num; /**< Size of extended statistics to show */
 
-/*
- * In container, it cannot terminate the process which running with 'stats-period'
- * option. Set flag to exit stats period loop after received SIGINT/SIGTERM.
- */
-volatile uint8_t f_quit;
+volatile sig_atomic_t f_quit;	/* Quit testpmd from signal */
 uint8_t cl_quit; /* Quit testpmd from cmdline. */
 
 /*
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index bdfbfd36d3c5..5c11d4f86301 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -6,6 +6,7 @@
 #define _TESTPMD_H_
 
 #include <stdbool.h>
+#include <signal.h>
 
 #ifdef RTE_LIB_GRO
 #include <rte_gro.h>
@@ -34,7 +35,7 @@
 #define RTE_PORT_HANDLING       (uint16_t)3
 
 extern uint8_t cl_quit;
-extern volatile uint8_t f_quit;
+extern volatile sig_atomic_t f_quit;
 
 /*
  * It is used to allocate the memory for hash key.
-- 
2.39.2

[PATCH v3 3/3] testpmd: enable interrupt in interactive mode

[Stephen Hemminger]

The setting in terminal handling for both Unix style and
Windows was not ensuring that Ctrl-C character would
cause interrupt.

This is a first release bug. Testpmd interactive mode has
always disabled control-c handling on Linux.

Fixes: af75078fece3 ("first public release")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/cmdline/cmdline_os_unix.c    | 3 ++-
 lib/cmdline/cmdline_os_windows.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 5f9839a15f98..356c5b3f3ca2 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -16,7 +16,8 @@ terminal_adjust(struct cmdline *cl)
 	tcgetattr(0, &cl->oldterm);
 
 	memcpy(&term, &cl->oldterm, sizeof(term));
-	term.c_lflag &= ~(ICANON | ECHO | ISIG);
+	term.c_lflag &= ~(ICANON | ECHO);
+	term.c_lflag |= ISIG;
 	tcsetattr(0, TCSANOW, &term);
 
 	setbuf(stdin, NULL);
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index 80863bfc8a00..8cff3b175747 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -32,10 +32,10 @@ terminal_adjust(struct cmdline *cl)
 		mode &= ~(
 			ENABLE_LINE_INPUT |      /* no line buffering */
 			ENABLE_ECHO_INPUT |      /* no echo */
-			ENABLE_PROCESSED_INPUT | /* pass Ctrl+C to program */
 			ENABLE_MOUSE_INPUT |     /* no mouse events */
 			ENABLE_WINDOW_INPUT);    /* no window resize events */
-		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT;
+		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT |
+			ENABLE_PROCESSED_INPUT; /* Ctrl C processed by the system */
 		SetConsoleMode(handle, mode);
 	}
 
-- 
2.39.2

Re: [PATCH v3 0/3] testpmd control C fixes

[Stephen Hemminger]

On Tue, 14 Mar 2023 10:36:45 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> Resolve issues around using control-C in testpmd.
> Using cmdline_poll() is broken and won't work right so
> don't use it. Instead use cmdline_interact() and fix
> the signal handling.
> 
> Stephen Hemminger (3):
>   testpmd: go back to using cmdline_interact
>   testpmd: use sig_atomic_t for flag
>   testpmd: enable interrupt in interactive mode
> 
>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>  app/test-pmd/testpmd.c           | 17 ++++++++++++-----
>  app/test-pmd/testpmd.h           |  3 ++-
>  lib/cmdline/cmdline.h            | 10 ++++++++++
>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>  lib/cmdline/cmdline_private.h    |  2 +-
>  lib/cmdline/version.map          |  3 +++
>  8 files changed, 65 insertions(+), 23 deletions(-)
> 

This patch is failing the CI tests from something that seems unrelated.
Looks like recent cyrptodev or ixgbe change broke something.


 -Idrivers/net/ice/base -I../drivers/net/ice/base -Idrivers/net/ice/../../common/iavf -I../drivers/net/ice/../../common/iavf -Idrivers/common/iavf -I../drivers/common/iavf -Idrivers/net/mlx5 -I../drivers/net/mlx5 -Idrivers/net/mlx5/linux -I../drivers/net/mlx5/linux -Idrivers/net/mlx5/hws -I../drivers/net/mlx5/hws -Idrivers/common/mlx5 -I../drivers/common/mlx5 -Idrivers/common/mlx5/linux -I../drivers/common/mlx5/linux -Idrivers/bus/auxiliary -I../drivers/bus/auxiliary -I/usr/include/libnl3 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -O3 -include rte_config.h -Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare -Wstrict-prototyp
 es -Wundef -Wwrite-strings -Wno-address-of-packed-member -Wno-packed-not-aligned -Wno-missing-field-initializers -Wno-zero-length-bounds -D_GNU_SOURCE -march=armv8-a+crc+crypto -march=armv8-a+crc -moutline-atomics -DALLOW_EXPERIMENTAL_API -Wno-deprecated-declarations -MD -MQ 'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o' -MF 'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o.d' -o 'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o' -c ../drivers/net/ixgbe/ixgbe_testpmd.c
In file included from ../drivers/net/ixgbe/ixgbe_ethdev.h:11,
from ../drivers/net/ixgbe/ixgbe_testpmd.c:9:
../drivers/net/ixgbe/base/ixgbe_type.h:2945:17: error: conflicting types for 'u64'
2945 | #define __le64  u64
|                 ^~~
In file included from ../drivers/net/ixgbe/base/ixgbe_type.h:46,
from ../drivers/net/ixgbe/ixgbe_ethdev.h:11,
from ../drivers/net/ixgbe/ixgbe_testpmd.c:9:
../drivers/net/ixgbe/base/ixgbe_osdep.h:94:18: note: previous declaration of 'u64' was here
94 | typedef uint64_t u64;
|                  ^~~
[2968/3157] Compiling C object 'app/test/3062f5d@@dpdk-test@exe/test_alarm.c.o'.
[2969/3157] Compiling C object 'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_mlx5_mlx5_testpmd.c.o'.
[2970/3157] Compiling C object 'drivers/a715181@@tmp_rte_event_cnxk@sta/event_cnxk_tx_cn10k_tx_112_127_seg.c.o'.
ninja: build stopped: subcommand failed.
==== End log output ====

Re: [PATCH v3 0/3] testpmd control C fixes

[David Marchand]

On Tue, Mar 14, 2023 at 11:40 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 14 Mar 2023 10:36:45 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> > Resolve issues around using control-C in testpmd.
> > Using cmdline_poll() is broken and won't work right so
> > don't use it. Instead use cmdline_interact() and fix
> > the signal handling.
> >
> > Stephen Hemminger (3):
> >   testpmd: go back to using cmdline_interact
> >   testpmd: use sig_atomic_t for flag
> >   testpmd: enable interrupt in interactive mode
> >
> >  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
> >  app/test-pmd/testpmd.c           | 17 ++++++++++++-----
> >  app/test-pmd/testpmd.h           |  3 ++-
> >  lib/cmdline/cmdline.h            | 10 ++++++++++
> >  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
> >  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
> >  lib/cmdline/cmdline_private.h    |  2 +-
> >  lib/cmdline/version.map          |  3 +++
> >  8 files changed, 65 insertions(+), 23 deletions(-)
> >
>
> This patch is failing the CI tests from something that seems unrelated.
> Looks like recent cyrptodev or ixgbe change broke something.


There was no recent change in ixgbe.
The build issue is due to added inclusion of signal.h in testpmd.h in patch 2.

root@eb72fb63faec:~/dpdk# git stash
Saved working directory and index state WIP on main: baf13c3135
version: 23.03-rc2
root@eb72fb63faec:~/dpdk# ninja -C build-mini-aarch64
ninja: Entering directory `build-mini-aarch64'
[104/104] Linking target app/dpdk-testpmd.
root@eb72fb63faec:~/dpdk# git stash pop
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   app/test-pmd/testpmd.h

no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (71b7fa29df19c6e8c553ca53e62cf89d1ef1ea97)
root@eb72fb63faec:~/dpdk# git diff
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index bdfbfd36d3..ada1d7b438 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -6,6 +6,7 @@
 #define _TESTPMD_H_

 #include <stdbool.h>
+#include <signal.h>

 #ifdef RTE_LIB_GRO
 #include <rte_gro.h>
root@eb72fb63faec:~/dpdk# ninja -C build-mini-aarch64
ninja: Entering directory `build-mini-aarch64'
[24/25] Compiling C object
'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o'.
FAILED: app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o
ccache aarch64-linux-gnu-gcc -Iapp/a172ced@@dpdk-testpmd@exe -Iapp
-I../app -Iapp/test-pmd -I../app/test-pmd -Ilib/ethdev -I../lib/ethdev
-I. -I../ -Iconfig -I../config -Ilib/eal/include -I../lib/eal/include
-Ilib/eal/linux/include -I../lib/eal/linux/include
-Ilib/eal/arm/include -I../lib/eal/arm/include -Ilib/eal/common
-I../lib/eal/common -Ilib/eal -I../lib/eal -Ilib/kvargs
-I../lib/kvargs -Ilib/telemetry/../metrics
-I../lib/telemetry/../metrics -Ilib/telemetry -I../lib/telemetry
-Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf -Ilib/mempool
-I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter -I../lib/meter
-Ilib/cmdline -I../lib/cmdline -Ilib/bitratestats
-I../lib/bitratestats -Ilib/metrics -I../lib/metrics -Ilib/bpf
-I../lib/bpf -Ilib/gro -I../lib/gro -Ilib/gso -I../lib/gso
-Ilib/latencystats -I../lib/latencystats -Ilib/pdump -I../lib/pdump
-Ilib/pcapng -I../lib/pcapng -Idrivers/net/ixgbe
-I../drivers/net/ixgbe -Idrivers/net/ixgbe/base
-I../drivers/net/ixgbe/base -Idrivers/bus/pci -I../drivers/bus/pci
-I../drivers/bus/pci/linux -Ilib/pci -I../lib/pci -Idrivers/bus/vdev
-I../drivers/bus/vdev -Ilib/hash -I../lib/hash -Ilib/rcu -I../lib/rcu
-Ilib/security -I../lib/security -Ilib/cryptodev -I../lib/cryptodev
-fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall
-Winvalid-pch -Wextra -O3 -include rte_config.h -Wcast-qual
-Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
-Wmissing-declarations -Wmissing-prototypes -Wnested-externs
-Wold-style-definition -Wpointer-arith -Wsign-compare
-Wstrict-prototypes -Wundef -Wwrite-strings
-Wno-address-of-packed-member -Wno-packed-not-aligned
-Wno-missing-field-initializers -D_GNU_SOURCE -march=armv8-a+crc
-moutline-atomics -DALLOW_EXPERIMENTAL_API
-Wno-deprecated-declarations -MD -MQ
'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o'
-MF 'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o.d'
-o 'app/a172ced@@dpdk-testpmd@exe/.._drivers_net_ixgbe_ixgbe_testpmd.c.o'
-c ../drivers/net/ixgbe/ixgbe_testpmd.c
In file included from ../drivers/net/ixgbe/ixgbe_ethdev.h:11,
                 from ../drivers/net/ixgbe/ixgbe_testpmd.c:9:
../drivers/net/ixgbe/base/ixgbe_type.h:2945:17: error: conflicting
types for 'u64'
 2945 | #define __le64  u64
      |                 ^~~
In file included from ../drivers/net/ixgbe/base/ixgbe_type.h:46,
                 from ../drivers/net/ixgbe/ixgbe_ethdev.h:11,
                 from ../drivers/net/ixgbe/ixgbe_testpmd.c:9:
../drivers/net/ixgbe/base/ixgbe_osdep.h:94:18: note: previous
declaration of 'u64' was here
   94 | typedef uint64_t u64;
      |                  ^~~
ninja: build stopped: subcommand failed.



-- 
David Marchand

Re: [PATCH v3 2/3] testpmd: use sig_atomic_t for flag

[Stephen Hemminger]

On Tue, 14 Mar 2023 10:36:47 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> Standard conforming signal handling in applications should use sig_atomic_t
> for flags set by signal handler.
>  https://www.gnu.org/software/libc/manual/html_node/Atomic-Data-Access.html
> 
> This is really just a fancy way of saying ''int'' but adds
> to platform portablilty for DPDK.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Apparently, this fails on some distros because signal.h ends up including
some compatibility definitions that clash with the same definitions in
the ixgbe testpmd base code. Since fixing either base code or the distro
is a PIA not worth doing.

[PATCH v4 0/2] Fix testpmd interrupt regression

[Stephen Hemminger]

Resolve issues from using control-C in testpmd.
Fixes regression from recent change to use cmdline_poll().

v4 - drop sig_atomic_t. Not required requires changes on
     some platforms.

Stephen Hemminger (2):
  testpmd: go back to using cmdline_interact
  testpmd: enable interrupt in interactive mode

 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.h            | 10 ++++++++++
 lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
 lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
 lib/cmdline/cmdline_private.h    |  2 +-
 lib/cmdline/version.map          |  3 +++
 7 files changed, 62 insertions(+), 17 deletions(-)

-- 
2.39.2

[PATCH v4 1/2] testpmd: go back to using cmdline_interact

[Stephen Hemminger]

The cmdline library poll function is broken on Windows
and was never tested, don't use it.

Instead, use sigaction() to cancel read character on Unix OS's
and a new helper to cancel I/O on Windows.

Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Bugzilla ID: 1180
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.h            | 10 ++++++++++
 lib/cmdline/cmdline_os_unix.c    |  5 +++++
 lib/cmdline/cmdline_os_windows.c | 14 ++++++++++++++
 lib/cmdline/cmdline_private.h    |  2 +-
 lib/cmdline/version.map          |  3 +++
 7 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc329b..072437d9bfcf 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,6 +66,7 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
+static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -13028,26 +13029,26 @@ cmdline_read_from_file(const char *filename)
 	printf("Read CLI commands from %s\n", filename);
 }
 
+void
+prompt_exit(void)
+{
+	cmdline_cancel(testpmd_cl);
+	cmdline_quit(testpmd_cl);
+}
+
 /* prompt function, called from main on MAIN lcore */
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
+		fprintf(stderr,
+			"Failed to create stdin based cmdline context\n");
 		return;
-
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
-
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
 	}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 2ce19ed47ab4..5cb6f9252395 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4469,6 +4469,7 @@ static void
 signal_handler(int signum __rte_unused)
 {
 	f_quit = 1;
+	prompt_exit();
 }
 
 int
@@ -4479,8 +4480,18 @@ main(int argc, char** argv)
 	uint16_t count;
 	int ret;
 
+#ifdef RTE_EXEC_ENV_WINDOWS
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
+#else
+	/* Want read() not to be restarted on signal */
+	struct sigaction action = {
+		.sa_handler = signal_handler,
+	};
+
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGTERM, &action, NULL);
+#endif
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h
index b14355ef5121..2a1721cf9712 100644
--- a/lib/cmdline/cmdline.h
+++ b/lib/cmdline/cmdline.h
@@ -60,6 +60,16 @@ int cmdline_poll(struct cmdline *cl);
 void cmdline_interact(struct cmdline *cl);
 void cmdline_quit(struct cmdline *cl);
 
+
+/**
+ * This function causes the read() in cmdline_interact to exit.
+ *
+ * @param cl
+ *   The command line object.
+ */
+__rte_experimental
+void cmdline_cancel(struct cmdline *cl);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 64a945a34fb3..5f9839a15f98 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -51,3 +51,8 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 {
 	return vdprintf(fd, format, op);
 }
+
+void
+cmdline_cancel(__rte_unused struct cmdline *cl)
+{
+}
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index 73ed9ba290b8..80863bfc8a00 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -203,3 +203,17 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 
 	return ret;
 }
+
+void
+cmdline_cancel(struct cmdline *cl)
+{
+	if (!cl)
+		return;
+
+	/* force the outstanding read on console to exit */
+	if (cl->oldterm.is_console_input) {
+		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+
+		CancelIoEx(handle, NULL);
+	}
+}
diff --git a/lib/cmdline/cmdline_private.h b/lib/cmdline/cmdline_private.h
index a3271c76934a..7ec42ac3c78f 100644
--- a/lib/cmdline/cmdline_private.h
+++ b/lib/cmdline/cmdline_private.h
@@ -24,7 +24,7 @@
 #define RDLINE_HISTORY_MAX_LINE 64
 
 struct rdline {
-	enum rdline_status status;
+	volatile enum rdline_status status;
 	/* rdline bufs */
 	struct cirbuf left;
 	struct cirbuf right;
diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
index e3d59aaf8d61..a8a417cd6ff1 100644
--- a/lib/cmdline/version.map
+++ b/lib/cmdline/version.map
@@ -84,5 +84,8 @@ EXPERIMENTAL {
 	# added in 22.07
 	cmdline_parse_check;
 
+	# added in 23.03
+	cmdline_cancel;
+
 	local: *;
 };
-- 
2.39.2

[PATCH v4 2/2] testpmd: enable interrupt in interactive mode

[Stephen Hemminger]

The setting in terminal handling for both Unix style and
Windows was not ensuring that Ctrl-C character would
cause interrupt.

This is a first release bug. Testpmd interactive mode has
always disabled control-c handling on Linux.

Fixes: af75078fece3 ("first public release")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/cmdline/cmdline_os_unix.c    | 3 ++-
 lib/cmdline/cmdline_os_windows.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 5f9839a15f98..356c5b3f3ca2 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -16,7 +16,8 @@ terminal_adjust(struct cmdline *cl)
 	tcgetattr(0, &cl->oldterm);
 
 	memcpy(&term, &cl->oldterm, sizeof(term));
-	term.c_lflag &= ~(ICANON | ECHO | ISIG);
+	term.c_lflag &= ~(ICANON | ECHO);
+	term.c_lflag |= ISIG;
 	tcsetattr(0, TCSANOW, &term);
 
 	setbuf(stdin, NULL);
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index 80863bfc8a00..8cff3b175747 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -32,10 +32,10 @@ terminal_adjust(struct cmdline *cl)
 		mode &= ~(
 			ENABLE_LINE_INPUT |      /* no line buffering */
 			ENABLE_ECHO_INPUT |      /* no echo */
-			ENABLE_PROCESSED_INPUT | /* pass Ctrl+C to program */
 			ENABLE_MOUSE_INPUT |     /* no mouse events */
 			ENABLE_WINDOW_INPUT);    /* no window resize events */
-		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT;
+		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT |
+			ENABLE_PROCESSED_INPUT; /* Ctrl C processed by the system */
 		SetConsoleMode(handle, mode);
 	}
 
-- 
2.39.2

RE: [PATCH v4 0/2] Fix testpmd interrupt regression

[Pier Damouny]

> External email: Use caution opening links or attachments
> 
> 
> Resolve issues from using control-C in testpmd.
> Fixes regression from recent change to use cmdline_poll().
> 
> v4 - drop sig_atomic_t. Not required requires changes on
>      some platforms.
> 
> Stephen Hemminger (2):
>   testpmd: go back to using cmdline_interact
>   testpmd: enable interrupt in interactive mode
> 
>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>  app/test-pmd/testpmd.c           | 11 +++++++++++
>  lib/cmdline/cmdline.h            | 10 ++++++++++
>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>  lib/cmdline/cmdline_private.h    |  2 +-
>  lib/cmdline/version.map          |  3 +++
>  7 files changed, 62 insertions(+), 17 deletions(-)
> 
> --
> 2.39.2

Confirming that this patch fixes Bug: 1180 over Windows.
And CTRL-C works as expected.

Tested-by: Pier Damouny <pdamouny@nvidia.com>

Re: [PATCH v4 0/2] Fix testpmd interrupt regression

[Ferruh Yigit]

On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
> Resolve issues from using control-C in testpmd.
> Fixes regression from recent change to use cmdline_poll().
> 
> v4 - drop sig_atomic_t. Not required requires changes on
>      some platforms.
> 
> Stephen Hemminger (2):
>   testpmd: go back to using cmdline_interact
>   testpmd: enable interrupt in interactive mode
> 
>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>  app/test-pmd/testpmd.c           | 11 +++++++++++
>  lib/cmdline/cmdline.h            | 10 ++++++++++
>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>  lib/cmdline/cmdline_private.h    |  2 +-
>  lib/cmdline/version.map          |  3 +++
>  7 files changed, 62 insertions(+), 17 deletions(-)
> 

This solution is adding new cmdline API for -rc3 and there are some
testpmd changes,
can it be possible to have a simple workaround specific to window for
this release and get this set for next release?

Re: [PATCH v4 0/2] Fix testpmd interrupt regression

[Stephen Hemminger]

On Thu, 16 Mar 2023 12:20:41 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
> > Resolve issues from using control-C in testpmd.
> > Fixes regression from recent change to use cmdline_poll().
> > 
> > v4 - drop sig_atomic_t. Not required requires changes on
> >      some platforms.
> > 
> > Stephen Hemminger (2):
> >   testpmd: go back to using cmdline_interact
> >   testpmd: enable interrupt in interactive mode
> > 
> >  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
> >  app/test-pmd/testpmd.c           | 11 +++++++++++
> >  lib/cmdline/cmdline.h            | 10 ++++++++++
> >  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
> >  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
> >  lib/cmdline/cmdline_private.h    |  2 +-
> >  lib/cmdline/version.map          |  3 +++
> >  7 files changed, 62 insertions(+), 17 deletions(-)
> >   
> 
> This solution is adding new cmdline API for -rc3 and there are some
> testpmd changes,
> can it be possible to have a simple workaround specific to window for
> this release and get this set for next release?


Not really. cmdline_poll() is broken in several ways.
Don't want to fix it or use it.

Re: [PATCH v4 0/2] Fix testpmd interrupt regression

[Ferruh Yigit]

On 3/16/2023 3:31 PM, Stephen Hemminger wrote:
> On Thu, 16 Mar 2023 12:20:41 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
>>> Resolve issues from using control-C in testpmd.
>>> Fixes regression from recent change to use cmdline_poll().
>>>
>>> v4 - drop sig_atomic_t. Not required requires changes on
>>>      some platforms.
>>>
>>> Stephen Hemminger (2):
>>>   testpmd: go back to using cmdline_interact
>>>   testpmd: enable interrupt in interactive mode
>>>
>>>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>>>  app/test-pmd/testpmd.c           | 11 +++++++++++
>>>  lib/cmdline/cmdline.h            | 10 ++++++++++
>>>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>>>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>>>  lib/cmdline/cmdline_private.h    |  2 +-
>>>  lib/cmdline/version.map          |  3 +++
>>>  7 files changed, 62 insertions(+), 17 deletions(-)
>>>   
>>
>> This solution is adding new cmdline API for -rc3 and there are some
>> testpmd changes,
>> can it be possible to have a simple workaround specific to window for
>> this release and get this set for next release?
> 
> 
> Not really. cmdline_poll() is broken in several ways.
> Don't want to fix it or use it.

What about to revert the original fix [^1] in this release and get a new
version of it at early next release?

@Thomas, what do you think? I think better to decide before -rc3.


[^1]
Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")

Re: [PATCH v4 0/2] Fix testpmd interrupt regression

[Thomas Monjalon]

16/03/2023 18:01, Ferruh Yigit:
> On 3/16/2023 3:31 PM, Stephen Hemminger wrote:
> > On Thu, 16 Mar 2023 12:20:41 +0000
> > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> > 
> >> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
> >>> Resolve issues from using control-C in testpmd.
> >>> Fixes regression from recent change to use cmdline_poll().
> >>>
> >>> v4 - drop sig_atomic_t. Not required requires changes on
> >>>      some platforms.
> >>>
> >>> Stephen Hemminger (2):
> >>>   testpmd: go back to using cmdline_interact
> >>>   testpmd: enable interrupt in interactive mode
> >>>
> >>>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
> >>>  app/test-pmd/testpmd.c           | 11 +++++++++++
> >>>  lib/cmdline/cmdline.h            | 10 ++++++++++
> >>>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
> >>>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
> >>>  lib/cmdline/cmdline_private.h    |  2 +-
> >>>  lib/cmdline/version.map          |  3 +++
> >>>  7 files changed, 62 insertions(+), 17 deletions(-)
> >>>   
> >>
> >> This solution is adding new cmdline API for -rc3 and there are some
> >> testpmd changes,
> >> can it be possible to have a simple workaround specific to window for
> >> this release and get this set for next release?
> > 
> > 
> > Not really. cmdline_poll() is broken in several ways.
> > Don't want to fix it or use it.
> 
> What about to revert the original fix [^1] in this release and get a new
> version of it at early next release?
> 
> @Thomas, what do you think? I think better to decide before -rc3.

We should not add risky changes at this stage.
Also I would prefer to see a review from Olivier, the cmdline maintainer.

The reason of this breakage was a cleanup.
I think it is more reasonnable to revert.

[PATCH] app/testpmd: revert cleanup cleanly from signal

[Ferruh Yigit]

This reverts commit 0fd1386c30c3ad9365d7fdd2829bf7cb2e1b9dff.

Because of reported issues on Windows [1] and it is risky to fix it at
this stage, fixed feature will be considered in next version.

[1]
Bugzilla ID: 1180

Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Pier Damouny <pdamouny@nvidia.com>
---
 app/test-pmd/cmdline.c | 29 ++++++++++------
 app/test-pmd/testpmd.c | 77 ++++++++++++++++++++++--------------------
 app/test-pmd/testpmd.h |  1 -
 3 files changed, 59 insertions(+), 48 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc329b..02c72d06b768 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,6 +66,7 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
+static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -13032,22 +13033,28 @@ cmdline_read_from_file(const char *filename)
 void
 prompt(void)
 {
-	struct cmdline *cl;
+	int ret;
 
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL)
 		return;
 
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
+	ret = atexit(prompt_exit);
+	if (ret != 0)
+		fprintf(stderr, "Cannot set exit function for cmdline\n");
 
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
-	}
+	cmdline_interact(testpmd_cl);
+	if (ret != 0)
+		cmdline_stdin_exit(testpmd_cl);
+}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+void
+prompt_exit(void)
+{
+	if (testpmd_cl != NULL) {
+		cmdline_quit(testpmd_cl);
+		cmdline_stdin_exit(testpmd_cl);
+	}
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 2ce19ed47ab4..003269660801 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -11,7 +11,6 @@
 #include <fcntl.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/mman.h>
-#include <sys/select.h>
 #endif
 #include <sys/types.h>
 #include <errno.h>
@@ -232,7 +231,7 @@ unsigned int xstats_display_num; /**< Size of extended statistics to show */
  * In container, it cannot terminate the process which running with 'stats-period'
  * option. Set flag to exit stats period loop after received SIGINT/SIGTERM.
  */
-volatile uint8_t f_quit;
+static volatile uint8_t f_quit;
 uint8_t cl_quit; /* Quit testpmd from cmdline. */
 
 /*
@@ -4448,6 +4447,13 @@ init_port(void)
 	memset(txring_numa, NUMA_NO_CONFIG, RTE_MAX_ETHPORTS);
 }
 
+static void
+force_quit(void)
+{
+	pmd_test_exit();
+	prompt_exit();
+}
+
 static void
 print_stats(void)
 {
@@ -4466,9 +4472,28 @@ print_stats(void)
 }
 
 static void
-signal_handler(int signum __rte_unused)
+signal_handler(int signum)
 {
-	f_quit = 1;
+	if (signum == SIGINT || signum == SIGTERM) {
+		fprintf(stderr, "\nSignal %d received, preparing to exit...\n",
+			signum);
+#ifdef RTE_LIB_PDUMP
+		/* uninitialize packet capture framework */
+		rte_pdump_uninit();
+#endif
+#ifdef RTE_LIB_LATENCYSTATS
+		if (latencystats_enabled != 0)
+			rte_latencystats_uninit();
+#endif
+		force_quit();
+		/* Set flag to indicate the force termination. */
+		f_quit = 1;
+		/* exit with the expected status */
+#ifndef RTE_EXEC_ENV_WINDOWS
+		signal(signum, SIG_DFL);
+		kill(getpid(), signum);
+#endif
+	}
 }
 
 int
@@ -4652,9 +4677,15 @@ main(int argc, char** argv)
 			start_packet_forwarding(0);
 		}
 		prompt();
+		pmd_test_exit();
 	} else
 #endif
 	{
+		char c;
+		int rc;
+
+		f_quit = 0;
+
 		printf("No commandline core given, start packet forwarding\n");
 		start_packet_forwarding(tx_first);
 		if (stats_period != 0) {
@@ -4677,40 +4708,14 @@ main(int argc, char** argv)
 				prev_time = cur_time;
 				rte_delay_us_sleep(US_PER_S);
 			}
-		} else {
-			char c;
-			fd_set fds;
-
-			printf("Press enter to exit\n");
-
-			FD_ZERO(&fds);
-			FD_SET(0, &fds);
-
-			/* wait for signal or enter */
-			ret = select(1, &fds, NULL, NULL, NULL);
-			if (ret < 0 && errno != EINTR)
-				rte_exit(EXIT_FAILURE,
-					 "Select failed: %s\n",
-					 strerror(errno));
-
-			/* if got enter then consume it */
-			if (ret == 1 && read(0, &c, 1) < 0)
-				rte_exit(EXIT_FAILURE,
-					 "Read failed: %s\n",
-					 strerror(errno));
 		}
-	}
 
-	pmd_test_exit();
-
-#ifdef RTE_LIB_PDUMP
-	/* uninitialize packet capture framework */
-	rte_pdump_uninit();
-#endif
-#ifdef RTE_LIB_LATENCYSTATS
-	if (latencystats_enabled != 0)
-		rte_latencystats_uninit();
-#endif
+		printf("Press enter to exit\n");
+		rc = read(0, &c, 1);
+		pmd_test_exit();
+		if (rc < 0)
+			return 1;
+	}
 
 	ret = rte_eal_cleanup();
 	if (ret != 0)
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index bdfbfd36d3c5..b9215720b615 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -34,7 +34,6 @@
 #define RTE_PORT_HANDLING       (uint16_t)3
 
 extern uint8_t cl_quit;
-extern volatile uint8_t f_quit;
 
 /*
  * It is used to allocate the memory for hash key.
-- 
2.34.1

Re: [PATCH v4 0/2] Fix testpmd interrupt regression

[Ferruh Yigit]

On 3/16/2023 5:05 PM, Thomas Monjalon wrote:
> 16/03/2023 18:01, Ferruh Yigit:
>> On 3/16/2023 3:31 PM, Stephen Hemminger wrote:
>>> On Thu, 16 Mar 2023 12:20:41 +0000
>>> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>
>>>> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
>>>>> Resolve issues from using control-C in testpmd.
>>>>> Fixes regression from recent change to use cmdline_poll().
>>>>>
>>>>> v4 - drop sig_atomic_t. Not required requires changes on
>>>>>      some platforms.
>>>>>
>>>>> Stephen Hemminger (2):
>>>>>   testpmd: go back to using cmdline_interact
>>>>>   testpmd: enable interrupt in interactive mode
>>>>>
>>>>>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>>>>>  app/test-pmd/testpmd.c           | 11 +++++++++++
>>>>>  lib/cmdline/cmdline.h            | 10 ++++++++++
>>>>>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>>>>>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>>>>>  lib/cmdline/cmdline_private.h    |  2 +-
>>>>>  lib/cmdline/version.map          |  3 +++
>>>>>  7 files changed, 62 insertions(+), 17 deletions(-)
>>>>>   
>>>>
>>>> This solution is adding new cmdline API for -rc3 and there are some
>>>> testpmd changes,
>>>> can it be possible to have a simple workaround specific to window for
>>>> this release and get this set for next release?
>>>
>>>
>>> Not really. cmdline_poll() is broken in several ways.
>>> Don't want to fix it or use it.
>>
>> What about to revert the original fix [^1] in this release and get a new
>> version of it at early next release?
>>
>> @Thomas, what do you think? I think better to decide before -rc3.
> 
> We should not add risky changes at this stage.
> Also I would prefer to see a review from Olivier, the cmdline maintainer.
> 
> The reason of this breakage was a cleanup.
> I think it is more reasonnable to revert.
> 
>

Sent a revert patch [^1], and assigned to Thomas.

[^1]
https://patches.dpdk.org/project/dpdk/patch/20230316172739.77933-1-ferruh.yigit@amd.com/

Re: [PATCH v4 1/2] testpmd: go back to using cmdline_interact

[Olivier Matz]

Hi Stephen,

Thank you for having a look at this.

On Wed, Mar 15, 2023 at 10:31:31AM -0700, Stephen Hemminger wrote:
> The cmdline library poll function is broken on Windows
> and was never tested, don't use it.
> 
> Instead, use sigaction() to cancel read character on Unix OS's
> and a new helper to cancel I/O on Windows.
> 
> Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
> Bugzilla ID: 1180
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>  app/test-pmd/testpmd.c           | 11 +++++++++++
>  lib/cmdline/cmdline.h            | 10 ++++++++++
>  lib/cmdline/cmdline_os_unix.c    |  5 +++++
>  lib/cmdline/cmdline_os_windows.c | 14 ++++++++++++++
>  lib/cmdline/cmdline_private.h    |  2 +-
>  lib/cmdline/version.map          |  3 +++
>  7 files changed, 58 insertions(+), 14 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6fa870dc329b..072437d9bfcf 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -66,6 +66,7 @@
>  #include "cmdline_tm.h"
>  #include "bpf_cmd.h"
>  
> +static struct cmdline *testpmd_cl;
>  static cmdline_parse_ctx_t *main_ctx;
>  static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
>  	TAILQ_HEAD_INITIALIZER(driver_commands_head);
> @@ -13028,26 +13029,26 @@ cmdline_read_from_file(const char *filename)
>  	printf("Read CLI commands from %s\n", filename);
>  }
>  
> +void
> +prompt_exit(void)
> +{
> +	cmdline_cancel(testpmd_cl);
> +	cmdline_quit(testpmd_cl);
> +}
> +
>  /* prompt function, called from main on MAIN lcore */
>  void
>  prompt(void)
>  {
> -	struct cmdline *cl;
> -
> -	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
> -	if (cl == NULL)
> +	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
> +	if (testpmd_cl == NULL) {
> +		fprintf(stderr,
> +			"Failed to create stdin based cmdline context\n");
>  		return;
> -
> -	/* loop until signal or quit command */
> -	while (f_quit == 0 && cl_quit == 0) {
> -		int status = cmdline_poll(cl);
> -
> -		if (status < 0 || status == RDLINE_EXITED)
> -			break;
>  	}
>  
> -	cmdline_quit(cl);
> -	cmdline_stdin_exit(cl);
> +	cmdline_interact(testpmd_cl);
> +	cmdline_stdin_exit(testpmd_cl);
>  }
>  
>  void
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 2ce19ed47ab4..5cb6f9252395 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -4469,6 +4469,7 @@ static void
>  signal_handler(int signum __rte_unused)
>  {
>  	f_quit = 1;
> +	prompt_exit();
>  }
>  
>  int
> @@ -4479,8 +4480,18 @@ main(int argc, char** argv)
>  	uint16_t count;
>  	int ret;
>  
> +#ifdef RTE_EXEC_ENV_WINDOWS
>  	signal(SIGINT, signal_handler);
>  	signal(SIGTERM, signal_handler);
> +#else
> +	/* Want read() not to be restarted on signal */
> +	struct sigaction action = {
> +		.sa_handler = signal_handler,
> +	};
> +
> +	sigaction(SIGINT, &action, NULL);
> +	sigaction(SIGTERM, &action, NULL);
> +#endif

If we have to change this in testpmd, we'll have to do the same in other
applications.

See my comment in patch 2.

>  
>  	testpmd_logtype = rte_log_register("testpmd");
>  	if (testpmd_logtype < 0)
> diff --git a/lib/cmdline/cmdline.h b/lib/cmdline/cmdline.h
> index b14355ef5121..2a1721cf9712 100644
> --- a/lib/cmdline/cmdline.h
> +++ b/lib/cmdline/cmdline.h
> @@ -60,6 +60,16 @@ int cmdline_poll(struct cmdline *cl);
>  void cmdline_interact(struct cmdline *cl);
>  void cmdline_quit(struct cmdline *cl);
>  
> +
> +/**
> + * This function causes the read() in cmdline_interact to exit.
> + *
> + * @param cl
> + *   The command line object.
> + */
> +__rte_experimental
> +void cmdline_cancel(struct cmdline *cl);
> +

The help says this function causes the read() in cmdline_interact to
exit, but the unix implementation is empty.

Maybe we should instead explain in what condition this function must
called. Also, shouldn't this function call cmdline_quit() too to avoid
the user to do it?

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
> index 64a945a34fb3..5f9839a15f98 100644
> --- a/lib/cmdline/cmdline_os_unix.c
> +++ b/lib/cmdline/cmdline_os_unix.c
> @@ -51,3 +51,8 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
>  {
>  	return vdprintf(fd, format, op);
>  }
> +
> +void
> +cmdline_cancel(__rte_unused struct cmdline *cl)
> +{
> +}
> diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
> index 73ed9ba290b8..80863bfc8a00 100644
> --- a/lib/cmdline/cmdline_os_windows.c
> +++ b/lib/cmdline/cmdline_os_windows.c
> @@ -203,3 +203,17 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
>  
>  	return ret;
>  }
> +
> +void
> +cmdline_cancel(struct cmdline *cl)
> +{
> +	if (!cl)
> +		return;
> +
> +	/* force the outstanding read on console to exit */
> +	if (cl->oldterm.is_console_input) {
> +		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
> +
> +		CancelIoEx(handle, NULL);
> +	}
> +}
> diff --git a/lib/cmdline/cmdline_private.h b/lib/cmdline/cmdline_private.h
> index a3271c76934a..7ec42ac3c78f 100644
> --- a/lib/cmdline/cmdline_private.h
> +++ b/lib/cmdline/cmdline_private.h
> @@ -24,7 +24,7 @@
>  #define RDLINE_HISTORY_MAX_LINE 64
>  
>  struct rdline {
> -	enum rdline_status status;
> +	volatile enum rdline_status status;
>  	/* rdline bufs */
>  	struct cirbuf left;
>  	struct cirbuf right;
> diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
> index e3d59aaf8d61..a8a417cd6ff1 100644
> --- a/lib/cmdline/version.map
> +++ b/lib/cmdline/version.map
> @@ -84,5 +84,8 @@ EXPERIMENTAL {
>  	# added in 22.07
>  	cmdline_parse_check;
>  
> +	# added in 23.03
> +	cmdline_cancel;
> +
>  	local: *;
>  };
> -- 
> 2.39.2
> 

Re: [PATCH v4 2/2] testpmd: enable interrupt in interactive mode

[Olivier Matz]

On Wed, Mar 15, 2023 at 10:31:32AM -0700, Stephen Hemminger wrote:
> The setting in terminal handling for both Unix style and
> Windows was not ensuring that Ctrl-C character would
> cause interrupt.
> 
> This is a first release bug. Testpmd interactive mode has
> always disabled control-c handling on Linux.

This was a design choice, not a bug. This design choice is discussable
today (at that time, dpdk was also running in baremetal without signals
or interrupt). The idea was to behave like a shell, i.e. ctrl-c just
clears the current line.

We may want to change this behavior (I remember an old discussion where
Bruce stated that he would prefer ctrl-c to kill the program), but it
will have an impact on all cmdline users, so to me it has to be
announced.


> 
> Fixes: af75078fece3 ("first public release")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/cmdline/cmdline_os_unix.c    | 3 ++-
>  lib/cmdline/cmdline_os_windows.c | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
> index 5f9839a15f98..356c5b3f3ca2 100644
> --- a/lib/cmdline/cmdline_os_unix.c
> +++ b/lib/cmdline/cmdline_os_unix.c
> @@ -16,7 +16,8 @@ terminal_adjust(struct cmdline *cl)
>  	tcgetattr(0, &cl->oldterm);
>  
>  	memcpy(&term, &cl->oldterm, sizeof(term));
> -	term.c_lflag &= ~(ICANON | ECHO | ISIG);
> +	term.c_lflag &= ~(ICANON | ECHO);
> +	term.c_lflag |= ISIG;
>  	tcsetattr(0, TCSANOW, &term);
>  
>  	setbuf(stdin, NULL);
> diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
> index 80863bfc8a00..8cff3b175747 100644
> --- a/lib/cmdline/cmdline_os_windows.c
> +++ b/lib/cmdline/cmdline_os_windows.c
> @@ -32,10 +32,10 @@ terminal_adjust(struct cmdline *cl)
>  		mode &= ~(
>  			ENABLE_LINE_INPUT |      /* no line buffering */
>  			ENABLE_ECHO_INPUT |      /* no echo */
> -			ENABLE_PROCESSED_INPUT | /* pass Ctrl+C to program */
>  			ENABLE_MOUSE_INPUT |     /* no mouse events */
>  			ENABLE_WINDOW_INPUT);    /* no window resize events */
> -		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT;
> +		mode |= ENABLE_VIRTUAL_TERMINAL_INPUT |
> +			ENABLE_PROCESSED_INPUT; /* Ctrl C processed by the system */
>  		SetConsoleMode(handle, mode);
>  	}
>  
> -- 
> 2.39.2
> 

Re: [PATCH v4 2/2] testpmd: enable interrupt in interactive mode

[Stephen Hemminger]

On Fri, 17 Mar 2023 17:20:59 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:

> On Wed, Mar 15, 2023 at 10:31:32AM -0700, Stephen Hemminger wrote:
> > The setting in terminal handling for both Unix style and
> > Windows was not ensuring that Ctrl-C character would
> > cause interrupt.
> > 
> > This is a first release bug. Testpmd interactive mode has
> > always disabled control-c handling on Linux.  
> 
> This was a design choice, not a bug. This design choice is discussable
> today (at that time, dpdk was also running in baremetal without signals
> or interrupt). The idea was to behave like a shell, i.e. ctrl-c just
> clears the current line.
> 
> We may want to change this behavior (I remember an old discussion where
> Bruce stated that he would prefer ctrl-c to kill the program), but it
> will have an impact on all cmdline users, so to me it has to be
> announced.


Ok, my motivation was to be able to test interrupt in testpmd
interactive mode. Without this change, it requires sending SIGINT
from another process.  Plus almost all programs that have an
interactive mode accept control-c to interrupt.

I split this patch off since it doesn't impact the bugfix
around testpmd and interrupts.

Re: [PATCH v4 1/2] testpmd: go back to using cmdline_interact

[Stephen Hemminger]

On Fri, 17 Mar 2023 17:20:48 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:

> >  
> > +#ifdef RTE_EXEC_ENV_WINDOWS
> >  	signal(SIGINT, signal_handler);
> >  	signal(SIGTERM, signal_handler);
> > +#else
> > +	/* Want read() not to be restarted on signal */
> > +	struct sigaction action = {
> > +		.sa_handler = signal_handler,
> > +	};
> > +
> > +	sigaction(SIGINT, &action, NULL);
> > +	sigaction(SIGTERM, &action, NULL);
> > +#endif  
> 
> If we have to change this in testpmd, we'll have to do the same in other
> applications.

The only a couple other program combining signal() and cmdline_interact().
These programs all exit from signal handler and never return from it.

In examples/ntb, the program is using the signal suicide (lets kill myself)
model, which will work.  Not sure why it bothers just to print a message.

In examples/vdpa, the program is trapping signal and calling close on ports.
This is not signal safe, but that is up to the vdpa maintainers to address.

Also, examples/vm_power_management is calling channel_XXX_exit() routines
which is not signal safe. Ditto, need the maintainers to address that.

> > +__rte_experimental
> > +void cmdline_cancel(struct cmdline *cl);
> > +  
> 
> The help says this function causes the read() in cmdline_interact to
> exit, but the unix implementation is empty.
> 
> Maybe we should instead explain in what condition this function must
> called. Also, shouldn't this function call cmdline_quit() too to avoid
> the user to do it?

Prefer to have function not to do implicit quit.

PS: cmdline functions need better documentation.

[PATCH v5] testpmd: go back to using cmdline_interact

[Stephen Hemminger]

The cmdline_poll() function is broken and was
not fully tested, don't use it.

Instead, use sigaction() to cancel read character on Unix OS's
and a new helper to cancel I/O on Windows.

Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Bugzilla ID: 1180
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v5 - Do not need to expose cmdline_cancel() as user api
     can make cmdline_quit() do it.

     Change to control C handling is separate issue and
     should be discussed more.

 app/test-pmd/cmdline.c           | 26 +++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.c            |  1 +
 lib/cmdline/cmdline_os_unix.c    |  6 ++++++
 lib/cmdline/cmdline_os_windows.c | 14 ++++++++++++++
 lib/cmdline/cmdline_private.h    |  5 ++++-
 6 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6fa870dc329b..7b20bef4e9ba 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -66,6 +66,7 @@
 #include "cmdline_tm.h"
 #include "bpf_cmd.h"
 
+static struct cmdline *testpmd_cl;
 static cmdline_parse_ctx_t *main_ctx;
 static TAILQ_HEAD(, testpmd_driver_commands) driver_commands_head =
 	TAILQ_HEAD_INITIALIZER(driver_commands_head);
@@ -13028,26 +13029,25 @@ cmdline_read_from_file(const char *filename)
 	printf("Read CLI commands from %s\n", filename);
 }
 
+void
+prompt_exit(void)
+{
+	cmdline_quit(testpmd_cl);
+}
+
 /* prompt function, called from main on MAIN lcore */
 void
 prompt(void)
 {
-	struct cmdline *cl;
-
-	cl = cmdline_stdin_new(main_ctx, "testpmd> ");
-	if (cl == NULL)
+	testpmd_cl = cmdline_stdin_new(main_ctx, "testpmd> ");
+	if (testpmd_cl == NULL) {
+		fprintf(stderr,
+			"Failed to create stdin based cmdline context\n");
 		return;
-
-	/* loop until signal or quit command */
-	while (f_quit == 0 && cl_quit == 0) {
-		int status = cmdline_poll(cl);
-
-		if (status < 0 || status == RDLINE_EXITED)
-			break;
 	}
 
-	cmdline_quit(cl);
-	cmdline_stdin_exit(cl);
+	cmdline_interact(testpmd_cl);
+	cmdline_stdin_exit(testpmd_cl);
 }
 
 void
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 2ce19ed47ab4..5cb6f9252395 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -4469,6 +4469,7 @@ static void
 signal_handler(int signum __rte_unused)
 {
 	f_quit = 1;
+	prompt_exit();
 }
 
 int
@@ -4479,8 +4480,18 @@ main(int argc, char** argv)
 	uint16_t count;
 	int ret;
 
+#ifdef RTE_EXEC_ENV_WINDOWS
 	signal(SIGINT, signal_handler);
 	signal(SIGTERM, signal_handler);
+#else
+	/* Want read() not to be restarted on signal */
+	struct sigaction action = {
+		.sa_handler = signal_handler,
+	};
+
+	sigaction(SIGINT, &action, NULL);
+	sigaction(SIGTERM, &action, NULL);
+#endif
 
 	testpmd_logtype = rte_log_register("testpmd");
 	if (testpmd_logtype < 0)
diff --git a/lib/cmdline/cmdline.c b/lib/cmdline/cmdline.c
index 8ad0690d8533..355c7d8ca635 100644
--- a/lib/cmdline/cmdline.c
+++ b/lib/cmdline/cmdline.c
@@ -173,6 +173,7 @@ cmdline_quit(struct cmdline *cl)
 {
 	if (!cl)
 		return;
+	cmdline_cancel(cl);
 	rdline_quit(&cl->rdl);
 }
 
diff --git a/lib/cmdline/cmdline_os_unix.c b/lib/cmdline/cmdline_os_unix.c
index 64a945a34fb3..9a4ec4e33477 100644
--- a/lib/cmdline/cmdline_os_unix.c
+++ b/lib/cmdline/cmdline_os_unix.c
@@ -51,3 +51,9 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 {
 	return vdprintf(fd, format, op);
 }
+
+/* This function is not needed on Linux, instead use sigaction() */
+void
+cmdline_cancel(__rte_unused struct cmdline *cl)
+{
+}
diff --git a/lib/cmdline/cmdline_os_windows.c b/lib/cmdline/cmdline_os_windows.c
index 73ed9ba290b8..80863bfc8a00 100644
--- a/lib/cmdline/cmdline_os_windows.c
+++ b/lib/cmdline/cmdline_os_windows.c
@@ -203,3 +203,17 @@ cmdline_vdprintf(int fd, const char *format, va_list op)
 
 	return ret;
 }
+
+void
+cmdline_cancel(struct cmdline *cl)
+{
+	if (!cl)
+		return;
+
+	/* force the outstanding read on console to exit */
+	if (cl->oldterm.is_console_input) {
+		HANDLE handle = (HANDLE)_get_osfhandle(cl->s_in);
+
+		CancelIoEx(handle, NULL);
+	}
+}
diff --git a/lib/cmdline/cmdline_private.h b/lib/cmdline/cmdline_private.h
index a3271c76934a..86a46cdea61a 100644
--- a/lib/cmdline/cmdline_private.h
+++ b/lib/cmdline/cmdline_private.h
@@ -24,7 +24,7 @@
 #define RDLINE_HISTORY_MAX_LINE 64
 
 struct rdline {
-	enum rdline_status status;
+	volatile enum rdline_status status;
 	/* rdline bufs */
 	struct cirbuf left;
 	struct cirbuf right;
@@ -90,6 +90,9 @@ int cmdline_poll_char(struct cmdline *cl);
 /* Read one character from input. */
 ssize_t cmdline_read_char(struct cmdline *cl, char *c);
 
+/* Force current cmdline read to unblock. */
+void cmdline_cancel(struct cmdline *cl);
+
 /* vdprintf(3) */
 __rte_format_printf(2, 0)
 int cmdline_vdprintf(int fd, const char *format, va_list op);
-- 
2.39.2

Re: [PATCH v5] testpmd: go back to using cmdline_interact

[Olivier Matz]

On Fri, Mar 17, 2023 at 09:59:41AM -0700, Stephen Hemminger wrote:
> The cmdline_poll() function is broken and was
> not fully tested, don't use it.
> 
> Instead, use sigaction() to cancel read character on Unix OS's
> and a new helper to cancel I/O on Windows.
> 
> Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
> Bugzilla ID: 1180
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thank you

Re: [PATCH v5] testpmd: go back to using cmdline_interact

[Thomas Monjalon]

17/03/2023 22:12, Olivier Matz:
> On Fri, Mar 17, 2023 at 09:59:41AM -0700, Stephen Hemminger wrote:
> > The cmdline_poll() function is broken and was
> > not fully tested, don't use it.
> > 
> > Instead, use sigaction() to cancel read character on Unix OS's
> > and a new helper to cancel I/O on Windows.
> > 
> > Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
> > Bugzilla ID: 1180
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Thank you

Applied, thanks.
I hope we won't have new issues with testpmd in -rc3.