From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 11FE141EBE; Fri, 17 Mar 2023 17:20:52 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E60C842F98; Fri, 17 Mar 2023 17:20:51 +0100 (CET) Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by mails.dpdk.org (Postfix) with ESMTP id 8530840395 for ; Fri, 17 Mar 2023 17:20:50 +0100 (CET) Received: by mail-wm1-f46.google.com with SMTP id az3-20020a05600c600300b003ed2920d585so5466289wmb.2 for ; Fri, 17 Mar 2023 09:20:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; t=1679070050; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=lKteHLeO8aFLUMHlyp1sgE3ugbx5cMlvtXsQzTdoJcM=; b=Zc73aU6jU5NWSpKtZB0sHtKjZ5Zca7xLluA62v3XuW8zeFegEu20Udn36p7ZFgTxxa 9S7Li8NBismYqXaY0fDcBIQZvuL9wEL+HjoIEvU7CscwdqY0DjR5ZlGXwndOrH+fGxE/ e4scdlYBYQcUo2icRmtvczEwI3BiuU8rGEin8Ogi0lOyRj+XAnSLclTkHicGJiznrxRr EHsGGxTHCKWLYYAz9WzNxPFbvGGmjnVBGkB1D1PO+J/aufXrZSH0Wx4RZxMt3d/IU21P uk7/wnEm1rdhQdsj5uGv6jaKY96DZy6iEikAXZkK7R1rbgeZ6DdE56P9awG2s4h5kFod K1mg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679070050; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=lKteHLeO8aFLUMHlyp1sgE3ugbx5cMlvtXsQzTdoJcM=; b=zZKfZjH9ibU7mwxopPIDPM5PS1ScqcljJ0cRHC7SyCt+f3UoMusm2rqMENZPPZww0N E3IyQn0d5t5SCYYBeE8Ly2AJ5WNRiYdSPpPRwzh2KnFTlXJKzUwk9taOIuA/jiQOfS1i tf+Gk5Ziqo+4XIta81gIRWFu652VVfA0wnD3JDtpbKf/VRnps45gsSPHt2yLh/GbHb/E Lw/Xe02585ulVyBMK0KRsXux4yFvvz1FCYEk8QXxG00J4KLNNkvO7v6x4Hv3PNjIcRkA V1da3/sSW6xSiFakbEXF05W/+6qfhOGtJ8q4o0g+7J3vhejSvH60Cdvg/8gXJH7IN7uY b84A== X-Gm-Message-State: AO0yUKUU04IKVH/BjCgR3OibZV4b9BASaQZhFro4XAiYon7bbYj8i9ba ykG3Y2JmS4YyzvqbOp0d0qtdAw== X-Google-Smtp-Source: AK7set9vyUF1gl4+9mtfMJ3v4RO7g1P+IqllEw7oEeYv1yQx4sszNCGmPnVP58BahSXMwXAfbwYYQA== X-Received: by 2002:a05:600c:3ca2:b0:3eb:3104:efec with SMTP id bg34-20020a05600c3ca200b003eb3104efecmr25985647wmb.16.1679070050203; Fri, 17 Mar 2023 09:20:50 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id g24-20020a7bc4d8000000b003dc47d458cdsm2418408wmk.15.2023.03.17.09.20.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Mar 2023 09:20:49 -0700 (PDT) Date: Fri, 17 Mar 2023 17:20:48 +0100 From: Olivier Matz To: Stephen Hemminger Cc: dev@dpdk.org, Aman Singh , Yuying Zhang , Ferruh Yigit Subject: Re: [PATCH v4 1/2] testpmd: go back to using cmdline_interact Message-ID: References: <20230313171905.67720-1-stephen@networkplumber.org> <20230315173132.4044-1-stephen@networkplumber.org> <20230315173132.4044-2-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230315173132.4044-2-stephen@networkplumber.org> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > --- > 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 >