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

[Olivier Matz <olivier.matz@6wind.com>]

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
>