[v2,1/7] cmdline: make implementation opaque
Checks
Commit Message
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>
---
app/test-cmdline/commands.c | 8 +++--
app/test/test_cmdline_lib.c | 42 +++++++++-------------
lib/librte_cmdline/cmdline.c | 10 ++++--
lib/librte_cmdline/cmdline.h | 15 ++++----
lib/librte_cmdline/cmdline_parse.c | 4 +--
lib/librte_cmdline/cmdline_private.h | 22 ++++++++++++
lib/librte_cmdline/cmdline_socket.c | 6 ++--
lib/librte_cmdline/rte_cmdline_version.map | 8 +++++
8 files changed, 68 insertions(+), 47 deletions(-)
create mode 100644 lib/librte_cmdline/cmdline_private.h
Comments
On 30/07/2020 22:06, 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.
Should it be INTERNAL then? Is it useful outside of the test cases?
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> app/test-cmdline/commands.c | 8 +++--
> app/test/test_cmdline_lib.c | 42 +++++++++-------------
> lib/librte_cmdline/cmdline.c | 10 ++++--
> lib/librte_cmdline/cmdline.h | 15 ++++----
> lib/librte_cmdline/cmdline_parse.c | 4 +--
> lib/librte_cmdline/cmdline_private.h | 22 ++++++++++++
> lib/librte_cmdline/cmdline_socket.c | 6 ++--
> lib/librte_cmdline/rte_cmdline_version.map | 8 +++++
> 8 files changed, 68 insertions(+), 47 deletions(-)
> create mode 100644 lib/librte_cmdline/cmdline_private.h
>
> diff --git a/app/test-cmdline/commands.c b/app/test-cmdline/commands.c
> index d67c0ca6a..ff7ac973e 100644
> --- a/app/test-cmdline/commands.c
> +++ b/app/test-cmdline/commands.c
> @@ -294,8 +294,10 @@ cmd_get_history_bufsize_parsed(__rte_unused void *parsed_result,
> struct cmdline *cl,
> __rte_unused void *data)
> {
> + struct rdline *rdl = cmdline_get_rdline(cl);
> +
> cmdline_printf(cl, "History buffer size: %zu\n",
> - sizeof(cl->rdl.history_buf));
> + sizeof(rdl->history_buf));
> }
>
> cmdline_parse_token_string_t cmd_get_history_bufsize_tok =
> @@ -326,7 +328,9 @@ cmd_clear_history_parsed(__rte_unused void *parsed_result,
> struct cmdline *cl,
> __rte_unused void *data)
> {
> - rdline_clear_history(&cl->rdl);
> + struct rdline *rdl = cmdline_get_rdline(cl);
> +
> + rdline_clear_history(rdl);
> }
>
> cmdline_parse_token_string_t cmd_clear_history_tok =
> diff --git a/app/test/test_cmdline_lib.c b/app/test/test_cmdline_lib.c
> index dec465da5..a7f5a7f7f 100644
> --- 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;
> }
>
> /* test library functions. the point of these tests is not so much to test
> diff --git a/lib/librte_cmdline/cmdline.c b/lib/librte_cmdline/cmdline.c
> index cfd703e5b..6f3fdd598 100644
> --- a/lib/librte_cmdline/cmdline.c
> +++ b/lib/librte_cmdline/cmdline.c
> @@ -13,14 +13,12 @@
> #include <fcntl.h>
> #include <poll.h>
> #include <errno.h>
> -#include <termios.h>
> #include <netinet/in.h>
>
> #include <rte_string_fns.h>
>
> -#include "cmdline_parse.h"
> -#include "cmdline_rdline.h"
> #include "cmdline.h"
> +#include "cmdline_private.h"
>
> static void
> cmdline_valid_buffer(struct rdline *rdl, const char *buf,
> @@ -103,6 +101,12 @@ cmdline_new(cmdline_parse_ctx_t *ctx, const char *prompt, int s_in, int s_out)
> return cl;
> }
>
> +struct rdline*
> +cmdline_get_rdline(struct cmdline *cl)
> +{
> + return &cl->rdl;
> +}
> +
> void
> cmdline_free(struct cmdline *cl)
> {
> diff --git a/lib/librte_cmdline/cmdline.h b/lib/librte_cmdline/cmdline.h
> index 243f99d20..96674dfda 100644
> --- a/lib/librte_cmdline/cmdline.h
> +++ b/lib/librte_cmdline/cmdline.h
> @@ -8,8 +8,8 @@
> #define _CMDLINE_H_
>
> #include <rte_common.h>
> +#include <rte_compat.h>
>
> -#include <termios.h>
> #include <cmdline_rdline.h>
> #include <cmdline_parse.h>
>
> @@ -23,14 +23,7 @@
> extern "C" {
> #endif
>
> -struct cmdline {
> - int s_in;
> - int s_out;
> - cmdline_parse_ctx_t *ctx;
> - struct rdline rdl;
> - char prompt[RDLINE_PROMPT_SIZE];
> - struct termios oldterm;
> -};
> +struct cmdline;
>
> struct cmdline *cmdline_new(cmdline_parse_ctx_t *ctx, const char *prompt, int s_in, int s_out);
> void cmdline_set_prompt(struct cmdline *cl, const char *prompt);
> @@ -40,6 +33,10 @@ void cmdline_printf(const struct cmdline *cl, const char *fmt, ...)
> int cmdline_in(struct cmdline *cl, const char *buf, int size);
> int cmdline_write_char(struct rdline *rdl, char c);
>
> +__rte_experimental
> +struct rdline *
> +cmdline_get_rdline(struct cmdline *cl);
> +
> /**
> * This function is nonblocking equivalent of ``cmdline_interact()``. It polls
> * *cl* for one character and interpret it. If return value is *RDLINE_EXITED*
> diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
> index b57b30e8f..ea0979158 100644
> --- a/lib/librte_cmdline/cmdline_parse.c
> +++ b/lib/librte_cmdline/cmdline_parse.c
> @@ -10,15 +10,13 @@
> #include <string.h>
> #include <inttypes.h>
> #include <ctype.h>
> -#include <termios.h>
>
> #include <netinet/in.h>
>
> #include <rte_string_fns.h>
>
> -#include "cmdline_rdline.h"
> -#include "cmdline_parse.h"
> #include "cmdline.h"
> +#include "cmdline_private.h"
>
> #ifdef RTE_LIBRTE_CMDLINE_DEBUG
> #define debug_printf printf
> diff --git a/lib/librte_cmdline/cmdline_private.h b/lib/librte_cmdline/cmdline_private.h
> new file mode 100644
> index 000000000..3b1c70e9f
> --- /dev/null
> +++ b/lib/librte_cmdline/cmdline_private.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2020 Dmitry Kozlyuk
> + */
> +
> +#ifndef _CMDLINE_PRIVATE_H_
> +#define _CMDLINE_PRIVATE_H_
> +
> +#include <termios.h>
> +
> +#include <cmdline_rdline.h>
> +#include <cmdline_parse.h>
> +
> +struct cmdline {
> + int s_in;
> + int s_out;
> + cmdline_parse_ctx_t *ctx;
> + struct rdline rdl;
> + char prompt[RDLINE_PROMPT_SIZE];
> + struct termios oldterm;
> +};
> +
> +#endif
> diff --git a/lib/librte_cmdline/cmdline_socket.c b/lib/librte_cmdline/cmdline_socket.c
> index ecb3d82b6..5e4b734d6 100644
> --- a/lib/librte_cmdline/cmdline_socket.c
> +++ b/lib/librte_cmdline/cmdline_socket.c
> @@ -11,12 +11,10 @@
> #include <stdarg.h>
> #include <inttypes.h>
> #include <fcntl.h>
> -#include <termios.h>
>
> -#include "cmdline_parse.h"
> -#include "cmdline_rdline.h"
> -#include "cmdline_socket.h"
> #include "cmdline.h"
> +#include "cmdline_private.h"
> +#include "cmdline_socket.h"
>
> struct cmdline *
> cmdline_file_new(cmdline_parse_ctx_t *ctx, const char *prompt, const char *path)
> diff --git a/lib/librte_cmdline/rte_cmdline_version.map b/lib/librte_cmdline/rte_cmdline_version.map
> index 95fce812f..135ecc71f 100644
> --- a/lib/librte_cmdline/rte_cmdline_version.map
> +++ b/lib/librte_cmdline/rte_cmdline_version.map
> @@ -69,3 +69,11 @@ DPDK_20.0 {
>
> local: *;
> };
> +
> +EXPERIMENTAL {
> + global:
> +
> + cmdline_get_rdline;
> +
> + local: *;
> +};
>
On Wed, 5 Aug 2020 10:31:31 +0100, Kinsella, Ray wrote:
> On 30/07/2020 22:06, 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.
>
> Should it be INTERNAL then? Is it useful outside of the test cases?
There are already exposed rdline_*() functions that require struct rdline
pointer, which is now only accessible via this function for struct cmdline
instances. Thus, public API would be broken with INTERNAL for such use cases.
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
On Thu, 17 Sep 2020 15:34:43 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:
> 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.
Since 20.11 is a API/ABI breaking release, I think breaking ABI
is okay without announcement. What matters more is if that API would
need to be impacted. API changes need some announcement.
Hi Olivier, thanks for the review.
> 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?
Windows team is now working on getting testpmd, tests, and examples
functional. librte_cmdline is essential for that, so delaying to 21.11 is the
worst option. If unannounced ABI breakage is deemed intolerable, I'd certainly
go with #ifdef.
On Thu, Sep 17, 2020 at 10:05:48AM -0700, Stephen Hemminger wrote:
> On Thu, 17 Sep 2020 15:34:43 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
>
> > 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.
>
> Since 20.11 is a API/ABI breaking release, I think breaking ABI
> is okay without announcement. What matters more is if that API would
> need to be impacted. API changes need some announcement.
This is something that we need to get a clear decision from technical board
on, I think, since there are some other proposed ABI changes in patches
that were not pre-announced, e.g. changing the lpm structure.
On 9/18/2020 9:33 AM, Bruce Richardson wrote:
> On Thu, Sep 17, 2020 at 10:05:48AM -0700, Stephen Hemminger wrote:
>> On Thu, 17 Sep 2020 15:34:43 +0200
>> Olivier Matz <olivier.matz@6wind.com> wrote:
>>
>>> 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.
>>
>> Since 20.11 is a API/ABI breaking release, I think breaking ABI
>> is okay without announcement. What matters more is if that API would
>> need to be impacted. API changes need some announcement.
>
> This is something that we need to get a clear decision from technical board
> on, I think, since there are some other proposed ABI changes in patches
> that were not pre-announced, e.g. changing the lpm structure.
>
And we accepted another in ethdev one, but that library already has
bunch of deprecation notices on it and one more change won't has much
affect.
Overall looks OK to me to accept minor changes without deprecation
notices, big ones can be investigated case by case, and +1 to have some
guidance from the techboard.
On 17/09/2020 18:05, Stephen Hemminger wrote:
> On Thu, 17 Sep 2020 15:34:43 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
>
>> 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.
>
> Since 20.11 is a API/ABI breaking release, I think breaking ABI
> is okay without announcement. What matters more is if that API would
> need to be impacted. API changes need some announcement.
Right but 20.11 being an ABI breaking release,
still doesn't waive the need to observe the deprecation notice, 3-acks from Maintainers etc.
On 17/09/2020 14:34, Olivier Matz wrote:
> 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.
I think this is more a theoretical ABI breakage, than a real ABI breakage.
As the consuming application is never likely to access the structure,
As in most cases, I doubt it ever has any knowledge of the structure.
My 2c, is that I think it is fine not consider this as ABI breakage.
>
> 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
>
On 05/08/2020 12:17, Dmitry Kozlyuk wrote:
> On Wed, 5 Aug 2020 10:31:31 +0100, Kinsella, Ray wrote:
>> On 30/07/2020 22:06, 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.
>>
>> Should it be INTERNAL then? Is it useful outside of the test cases?
>
> There are already exposed rdline_*() functions that require struct rdline
> pointer, which is now only accessible via this function for struct cmdline
> instances. Thus, public API would be broken with INTERNAL for such use cases.
>
Right but that runs a bit contrary to what you said in the commit log.
"Add cmdline_get_rdline() to access history buffer.
The new function is currently used only in tests."
In anycase, given the elapse in time since my feedback.
I will ACK the changes to MAP file.
Ray K
On Wed, 30 Sep 2020 09:11:41 +0100, Kinsella, Ray wrote:
> On 05/08/2020 12:17, Dmitry Kozlyuk wrote:
> > On Wed, 5 Aug 2020 10:31:31 +0100, Kinsella, Ray wrote:
> >> On 30/07/2020 22:06, 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.
> >>
> >> Should it be INTERNAL then? Is it useful outside of the test cases?
> >
> > There are already exposed rdline_*() functions that require struct rdline
> > pointer, which is now only accessible via this function for struct cmdline
> > instances. Thus, public API would be broken with INTERNAL for such use cases.
> >
>
> Right but that runs a bit contrary to what you said in the commit log.
>
> "Add cmdline_get_rdline() to access history buffer.
> The new function is currently used only in tests."
>
> In anycase, given the elapse in time since my feedback.
> I will ACK the changes to MAP file.
I rather meant logical breakage that technical one: no functions are removed,
yes, but if existing applications use cmdline::rdline, they won't have a
legitimate way of getting it anymore. The comment you're quoting was before
the workaround, but the WA exists only to keep ABI and we'd prefer the users
to remove direct accesses of the structure fields by 21.11. To do that, we
give them cmdline_get_rdline() and make it public, not internal.
@@ -294,8 +294,10 @@ cmd_get_history_bufsize_parsed(__rte_unused void *parsed_result,
struct cmdline *cl,
__rte_unused void *data)
{
+ struct rdline *rdl = cmdline_get_rdline(cl);
+
cmdline_printf(cl, "History buffer size: %zu\n",
- sizeof(cl->rdl.history_buf));
+ sizeof(rdl->history_buf));
}
cmdline_parse_token_string_t cmd_get_history_bufsize_tok =
@@ -326,7 +328,9 @@ cmd_clear_history_parsed(__rte_unused void *parsed_result,
struct cmdline *cl,
__rte_unused void *data)
{
- rdline_clear_history(&cl->rdl);
+ struct rdline *rdl = cmdline_get_rdline(cl);
+
+ rdline_clear_history(rdl);
}
cmdline_parse_token_string_t cmd_clear_history_tok =
@@ -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;
}
/* test library functions. the point of these tests is not so much to test
@@ -13,14 +13,12 @@
#include <fcntl.h>
#include <poll.h>
#include <errno.h>
-#include <termios.h>
#include <netinet/in.h>
#include <rte_string_fns.h>
-#include "cmdline_parse.h"
-#include "cmdline_rdline.h"
#include "cmdline.h"
+#include "cmdline_private.h"
static void
cmdline_valid_buffer(struct rdline *rdl, const char *buf,
@@ -103,6 +101,12 @@ cmdline_new(cmdline_parse_ctx_t *ctx, const char *prompt, int s_in, int s_out)
return cl;
}
+struct rdline*
+cmdline_get_rdline(struct cmdline *cl)
+{
+ return &cl->rdl;
+}
+
void
cmdline_free(struct cmdline *cl)
{
@@ -8,8 +8,8 @@
#define _CMDLINE_H_
#include <rte_common.h>
+#include <rte_compat.h>
-#include <termios.h>
#include <cmdline_rdline.h>
#include <cmdline_parse.h>
@@ -23,14 +23,7 @@
extern "C" {
#endif
-struct cmdline {
- int s_in;
- int s_out;
- cmdline_parse_ctx_t *ctx;
- struct rdline rdl;
- char prompt[RDLINE_PROMPT_SIZE];
- struct termios oldterm;
-};
+struct cmdline;
struct cmdline *cmdline_new(cmdline_parse_ctx_t *ctx, const char *prompt, int s_in, int s_out);
void cmdline_set_prompt(struct cmdline *cl, const char *prompt);
@@ -40,6 +33,10 @@ void cmdline_printf(const struct cmdline *cl, const char *fmt, ...)
int cmdline_in(struct cmdline *cl, const char *buf, int size);
int cmdline_write_char(struct rdline *rdl, char c);
+__rte_experimental
+struct rdline *
+cmdline_get_rdline(struct cmdline *cl);
+
/**
* This function is nonblocking equivalent of ``cmdline_interact()``. It polls
* *cl* for one character and interpret it. If return value is *RDLINE_EXITED*
@@ -10,15 +10,13 @@
#include <string.h>
#include <inttypes.h>
#include <ctype.h>
-#include <termios.h>
#include <netinet/in.h>
#include <rte_string_fns.h>
-#include "cmdline_rdline.h"
-#include "cmdline_parse.h"
#include "cmdline.h"
+#include "cmdline_private.h"
#ifdef RTE_LIBRTE_CMDLINE_DEBUG
#define debug_printf printf
new file mode 100644
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Dmitry Kozlyuk
+ */
+
+#ifndef _CMDLINE_PRIVATE_H_
+#define _CMDLINE_PRIVATE_H_
+
+#include <termios.h>
+
+#include <cmdline_rdline.h>
+#include <cmdline_parse.h>
+
+struct cmdline {
+ int s_in;
+ int s_out;
+ cmdline_parse_ctx_t *ctx;
+ struct rdline rdl;
+ char prompt[RDLINE_PROMPT_SIZE];
+ struct termios oldterm;
+};
+
+#endif
@@ -11,12 +11,10 @@
#include <stdarg.h>
#include <inttypes.h>
#include <fcntl.h>
-#include <termios.h>
-#include "cmdline_parse.h"
-#include "cmdline_rdline.h"
-#include "cmdline_socket.h"
#include "cmdline.h"
+#include "cmdline_private.h"
+#include "cmdline_socket.h"
struct cmdline *
cmdline_file_new(cmdline_parse_ctx_t *ctx, const char *prompt, const char *path)
@@ -69,3 +69,11 @@ DPDK_20.0 {
local: *;
};
+
+EXPERIMENTAL {
+ global:
+
+ cmdline_get_rdline;
+
+ local: *;
+};