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 <step...@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
>
