Re: [dpdk-dev] [PATCH v3 2/2] cmdline: make struct rdline opaque

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

Hi Dmitry,

Few comments below.

On Tue, Oct 05, 2021 at 03:55:16AM +0300, Dmitry Kozlyuk wrote:
> Hide struct rdline definition and some RDLINE_* constants in order
> to be able to change internal buffer sizes transparently to the user.
> Add new functions:
> 
> * rdline_create(): allocate and initialize struct rdline.
>   This function replaces rdline_init() and takes an extra parameter:
>   opaque user data for the callbacks.
> * rdline_free(): deallocate struct rdline.
> * rdline_get_history_buffer_size(): for use in tests.
> * rdline_get_opaque(): to obtain user data in callback functions.
> 
> Remove rdline_init() function from library headers and export list,
> because using it requires the knowledge of sizeof(struct rdline).
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  app/test-cmdline/commands.c            |  2 +-
>  app/test/test_cmdline_lib.c            | 19 ++++--
>  doc/guides/rel_notes/release_21_11.rst |  3 +
>  lib/cmdline/cmdline.c                  |  3 +-
>  lib/cmdline/cmdline_cirbuf.c           |  1 -
>  lib/cmdline/cmdline_private.h          | 49 ++++++++++++++
>  lib/cmdline/cmdline_rdline.c           | 50 ++++++++++++++-
>  lib/cmdline/cmdline_rdline.h           | 88 ++++++++++----------------
>  lib/cmdline/version.map                |  8 ++-
>  9 files changed, 154 insertions(+), 69 deletions(-)
> 
> diff --git a/app/test-cmdline/commands.c b/app/test-cmdline/commands.c
> index d732976f08..a13e1d1afd 100644
> --- a/app/test-cmdline/commands.c
> +++ b/app/test-cmdline/commands.c
> @@ -297,7 +297,7 @@ cmd_get_history_bufsize_parsed(__rte_unused void *parsed_result,
>  	struct rdline *rdl = cmdline_get_rdline(cl);
>  
>  	cmdline_printf(cl, "History buffer size: %zu\n",
> -			sizeof(rdl->history_buf));
> +			rdline_get_history_buffer_size(rdl));

My first impression was that having this function just for the tests was
not really useful.

But thinking a bit more about it, now that the structure is hidden, we
could have an API in the future to change the size of the history
buffer, in which case this function makes sense.

[...]

> diff --git a/lib/cmdline/cmdline_cirbuf.c b/lib/cmdline/cmdline_cirbuf.c
> index 829a8af563..cbb76a7016 100644
> --- a/lib/cmdline/cmdline_cirbuf.c
> +++ b/lib/cmdline/cmdline_cirbuf.c
> @@ -10,7 +10,6 @@
>  
>  #include "cmdline_cirbuf.h"
>  
> -
>  int
>  cirbuf_init(struct cirbuf *cbuf, char *buf, unsigned int start, unsigned int maxlen)
>  {

unexpected change

[...]

> --- a/lib/cmdline/cmdline_rdline.c
> +++ b/lib/cmdline/cmdline_rdline.c
> @@ -13,6 +13,7 @@
>  #include <ctype.h>
>  
>  #include "cmdline_cirbuf.h"
> +#include "cmdline_private.h"
>  #include "cmdline_rdline.h"
>  
>  static void rdline_puts(struct rdline *rdl, const char *buf);
> @@ -37,9 +38,10 @@ isblank2(char c)
>  
>  int
>  rdline_init(struct rdline *rdl,
> -		 rdline_write_char_t *write_char,
> -		 rdline_validate_t *validate,
> -		 rdline_complete_t *complete)
> +	    rdline_write_char_t *write_char,
> +	    rdline_validate_t *validate,
> +	    rdline_complete_t *complete,
> +	    void *opaque)
>  {
>  	if (!rdl || !write_char || !validate || !complete)
>  		return -EINVAL;
> @@ -47,10 +49,40 @@ rdline_init(struct rdline *rdl,
>  	rdl->validate = validate;
>  	rdl->complete = complete;
>  	rdl->write_char = write_char;
> +	rdl->opaque = opaque;
>  	rdl->status = RDLINE_INIT;
>  	return cirbuf_init(&rdl->history, rdl->history_buf, 0, RDLINE_HISTORY_BUF_SIZE);
>  }
>  
> +int
> +rdline_create(struct rdline **out,
> +	      rdline_write_char_t *write_char,
> +	      rdline_validate_t *validate,
> +	      rdline_complete_t *complete,
> +	      void *opaque)
> +{

For consistency, wouldn't it be better to keep the same model than
cmdline_new()? I mean return a pointer and name it rdline_new().


> +	struct rdline *rdl;
> +	int ret;
> +
> +	if (out == NULL)
> +		return -EINVAL;
> +	rdl = malloc(sizeof(*rdl));
> +	if (rdl == NULL)
> +		return -ENOMEM;
> +	ret = rdline_init(rdl, write_char, validate, complete, opaque);
> +	if (ret < 0)
> +		free(rdl);
> +	else
> +		*out = rdl;
> +	return ret;
> +}

[...]

> +size_t
> +rdline_get_history_buffer_size(struct rdline *rdl)
> +{
> +	return sizeof(rdl->history_buf);
> +}
> +
> +void *
> +rdline_get_opaque(struct rdline *rdl)
> +{
> +	return rdl != NULL ? rdl->opaque : NULL;
> +}

rdline_get_opaque() is safe when rdl is NULL, but
rdline_get_history_buffer_size() is not.

To me, both are acceptable but I'd prefer to have the same behavior
for these 2 functions.


Apart from that, looks good to me.

Thanks!
Olivier