From: Olivier Matz <olivier.matz@6wind.com> To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> Cc: dev@dpdk.org, Dmitry Malloy <dmitrym@microsoft.com>, Narcisa Ana Maria Vasile <Narcisa.Vasile@microsoft.com>, Fady Bader <fady@mellanox.com>, Tal Shnaiderman <talshn@mellanox.com>, "Kadam, Pallavi" <pallavi.kadam@intel.com>, Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com> Subject: Re: [dpdk-dev] [PATCH v2 1/7] cmdline: make implementation opaque Date: Thu, 17 Sep 2020 15:34:43 +0200 Message-ID: <20200917133443.GR21395@platinum> (raw) In-Reply-To: <20200730210652.14568-2-dmitry.kozliuk@gmail.com> Hi Dmitry, On Fri, Jul 31, 2020 at 12:06:45AM +0300, Dmitry Kozlyuk wrote: > struct cmdline exposes platform-specific members it contains, most > notably struct termios that is only available on Unix. Make the > structure opaque. > > Remove tests checking struct cmdline content as meaningless. > > Add cmdline_get_rdline() to access history buffer. > The new function is currently used only in tests. > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> First, please forgive me for the very late feedback. It is all the more problematic because I think this patch introduces an ABI breakage, that should have been announced. Making cmdline struct opaque clearly goes in the right direction, as it will prevent future ABI breakage. In my opinion, we could accept the patch for 20.11, knowing it reduce the risk of future ABI breakage, and that cmdline is not a core component of DPDK. However it has to be discussed and accepted by other people. Else, the patch would be delayed for 21.11. From what I see from the other patches, it looks possible to keep cmdline struct public without breaking the existing ABI by adding #ifdef CONFIG_RTE_EXEC_ENV_WINDOWS, is it correct? One minor comment below: > --- a/app/test/test_cmdline_lib.c > +++ b/app/test/test_cmdline_lib.c > @@ -46,22 +46,29 @@ complete_buffer(__rte_unused struct rdline *rdl, > static int > test_cmdline_parse_fns(void) > { > - struct cmdline cl; > + struct cmdline *cl; > + cmdline_parse_ctx_t ctx; > int i = 0; > char dst[CMDLINE_TEST_BUFSIZE]; > > + cl = cmdline_new(&ctx, "prompt", -1, -1); > + if (cl == NULL) { > + printf("Error: cannot create cmdline to test parse fns!\n"); > + return -1; > + } > + > if (cmdline_parse(NULL, "buffer") >= 0) > goto error; > - if (cmdline_parse(&cl, NULL) >= 0) > + if (cmdline_parse(cl, NULL) >= 0) > goto error; > > if (cmdline_complete(NULL, "buffer", &i, dst, sizeof(dst)) >= 0) > goto error; > - if (cmdline_complete(&cl, NULL, &i, dst, sizeof(dst)) >= 0) > + if (cmdline_complete(cl, NULL, &i, dst, sizeof(dst)) >= 0) > goto error; > - if (cmdline_complete(&cl, "buffer", NULL, dst, sizeof(dst)) >= 0) > + if (cmdline_complete(cl, "buffer", NULL, dst, sizeof(dst)) >= 0) > goto error; > - if (cmdline_complete(&cl, "buffer", &i, NULL, sizeof(dst)) >= 0) > + if (cmdline_complete(cl, "buffer", &i, NULL, sizeof(dst)) >= 0) > goto error; > > return 0; > @@ -166,11 +173,11 @@ static int > test_cmdline_fns(void) > { > cmdline_parse_ctx_t ctx; > - struct cmdline cl, *tmp; > + struct cmdline *cl; > > memset(&ctx, 0, sizeof(ctx)); > - tmp = cmdline_new(&ctx, "test", -1, -1); > - if (tmp == NULL) > + cl = cmdline_new(&ctx, "test", -1, -1); > + if (cl == NULL) > goto error; > > if (cmdline_new(NULL, "prompt", 0, 0) != NULL) > @@ -179,7 +186,7 @@ test_cmdline_fns(void) > goto error; > if (cmdline_in(NULL, "buffer", CMDLINE_TEST_BUFSIZE) >= 0) > goto error; > - if (cmdline_in(&cl, NULL, CMDLINE_TEST_BUFSIZE) >= 0) > + if (cmdline_in(cl, NULL, CMDLINE_TEST_BUFSIZE) >= 0) > goto error; > if (cmdline_write_char(NULL, 0) >= 0) > goto error; > @@ -188,31 +195,14 @@ test_cmdline_fns(void) > cmdline_set_prompt(NULL, "prompt"); > cmdline_free(NULL); > cmdline_printf(NULL, "format"); > - /* this should fail as stream handles are invalid */ > - cmdline_printf(tmp, "format"); > cmdline_interact(NULL); > cmdline_quit(NULL); > > - /* check if void calls change anything when they should fail */ > - cl = *tmp; > - > - cmdline_printf(&cl, NULL); > - if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch; > - cmdline_set_prompt(&cl, NULL); > - if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch; > - cmdline_in(&cl, NULL, CMDLINE_TEST_BUFSIZE); > - if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch; > - > - cmdline_free(tmp); > - > return 0; > > error: > printf("Error: function accepted null parameter!\n"); > return -1; > -mismatch: > - printf("Error: data changed!\n"); > - return -1; cmdline_free(cl) is missing. before your patch, cmdline_free(tmp) was already missing in error case by the way. Thanks, Olivier
next prev parent reply other threads:[~2020-09-17 13:34 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-20 21:05 [dpdk-dev] [PATCH 0/7] cmdline: support Windows Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 1/7] cmdline: make implementation opaque Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 2/7] cmdline: add internal wrappers for terminal handling Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 3/7] cmdline: add internal wrappers for character input Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 4/7] cmdline: add internal wrapper for vdprintf Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 5/7] eal/windows: improve compatibility networking headers Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 6/7] cmdline: support Windows Dmitry Kozlyuk 2020-06-28 14:20 ` Fady Bader 2020-06-29 6:23 ` Ranjit Menon 2020-06-29 7:42 ` Dmitry Kozlyuk 2020-06-29 8:12 ` Tal Shnaiderman 2020-06-29 23:56 ` Dmitry Kozlyuk 2020-07-08 1:09 ` Dmitry Kozlyuk 2020-06-20 21:05 ` [dpdk-dev] [PATCH 7/7] examples/cmdline: build on Windows Dmitry Kozlyuk 2020-07-17 22:16 ` [dpdk-dev] [PATCH 0/7] cmdline: support Windows Narcisa Ana Maria Vasile 2020-07-30 18:08 ` Kadam, Pallavi 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 " Dmitry Kozlyuk 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 1/7] cmdline: make implementation opaque Dmitry Kozlyuk 2020-08-05 9:31 ` Kinsella, Ray 2020-08-05 11:17 ` Dmitry Kozlyuk 2020-09-30 8:11 ` Kinsella, Ray 2020-09-30 15:26 ` Dmitry Kozlyuk 2020-09-17 13:34 ` Olivier Matz [this message] 2020-09-17 17:05 ` Stephen Hemminger 2020-09-18 8:33 ` Bruce Richardson 2020-09-18 12:13 ` Ferruh Yigit 2020-09-18 13:23 ` Kinsella, Ray 2020-09-17 23:13 ` Dmitry Kozlyuk 2020-09-18 13:31 ` Kinsella, Ray 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 2/7] cmdline: add internal wrappers for terminal handling Dmitry Kozlyuk 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 3/7] cmdline: add internal wrappers for character input Dmitry Kozlyuk 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 4/7] cmdline: add internal wrapper for vdprintf Dmitry Kozlyuk 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 5/7] eal/windows: improve compatibility networking headers Dmitry Kozlyuk 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 6/7] cmdline: support Windows Dmitry Kozlyuk 2020-07-30 21:06 ` [dpdk-dev] [PATCH v2 7/7] examples/cmdline: build on Windows Dmitry Kozlyuk 2020-09-26 1:33 ` [dpdk-dev] [EXTERNAL] " Narcisa Ana Maria Vasile 2020-09-26 6:03 ` Khoa To 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 0/7] cmdline: support Windows Dmitry Kozlyuk 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 1/7] cmdline: make implementation logically opaque Dmitry Kozlyuk 2020-09-30 8:12 ` Kinsella, Ray 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 2/7] cmdline: add internal wrappers for terminal handling Dmitry Kozlyuk 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 3/7] cmdline: add internal wrappers for character input Dmitry Kozlyuk 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 4/7] cmdline: add internal wrapper for vdprintf Dmitry Kozlyuk 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 5/7] eal/windows: improve compatibility networking headers Dmitry Kozlyuk 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 6/7] cmdline: support Windows Dmitry Kozlyuk 2020-10-14 22:31 ` Thomas Monjalon 2020-09-28 21:50 ` [dpdk-dev] [PATCH v3 7/7] examples/cmdline: build on Windows Dmitry Kozlyuk 2020-10-14 22:33 ` Thomas Monjalon 2020-10-05 15:33 ` [dpdk-dev] [PATCH v3 0/7] cmdline: support Windows Olivier Matz 2020-10-14 22:41 ` Thomas Monjalon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200917133443.GR21395@platinum \ --to=olivier.matz@6wind.com \ --cc=Narcisa.Vasile@microsoft.com \ --cc=dev@dpdk.org \ --cc=dmitry.kozliuk@gmail.com \ --cc=dmitrym@microsoft.com \ --cc=fady@mellanox.com \ --cc=mdr@ashroe.eu \ --cc=nhorman@tuxdriver.com \ --cc=pallavi.kadam@intel.com \ --cc=talshn@mellanox.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git