[v4,1/2] testpmd: go back to using cmdline_interact
Checks
Commit Message
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(-)
Comments
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
>
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.
@@ -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
@@ -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)
@@ -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
@@ -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)
+{
+}
@@ -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);
+ }
+}
@@ -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;
@@ -84,5 +84,8 @@ EXPERIMENTAL {
# added in 22.07
cmdline_parse_check;
+ # added in 23.03
+ cmdline_cancel;
+
local: *;
};