[v3,2/2] cmdline: make struct rdline opaque

  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(-)
  

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
  

Hi Olivier,

Thanks for the review, please see below.

2021-10-05 10:27 (UTC+0200), Olivier Matz:
> [...]
> > 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

Will remove in v4.

> [...]
> 
> > --- 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().

If we don't really need to distinguish EINVAL and ENOMEM errors here,
then I agree. Otherwise, do you propose to return the error code via
rte_errno? Currenly no cmdline functions use it. This would also add a
runtime dependency on EAL (currently cmdline only depends on its headers).

> [...]
> > +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.

rdline_get_history_buffer_size() is safe because sizeof() is evaluated at
compile time. There's a unit test checking that all functions are NULL-safe.
  

On Tue, Oct 05, 2021 at 12:03:01PM +0300, Dmitry Kozlyuk wrote:
> Hi Olivier,
> 
> Thanks for the review, please see below.
> 
> 2021-10-05 10:27 (UTC+0200), Olivier Matz:
> > [...]
> > > 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
> 
> Will remove in v4.
> 
> > [...]
> > 
> > > --- 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().
> 
> If we don't really need to distinguish EINVAL and ENOMEM errors here,
> then I agree. Otherwise, do you propose to return the error code via
> rte_errno? Currenly no cmdline functions use it. This would also add a
> runtime dependency on EAL (currently cmdline only depends on its headers).

Good point, I was indeed thinking about NULL + rte_errno, but I did not
anticipate the new dependency to eal.

Given there's no errno in cmdline_new(), which is the main user API, I think
we can do the same for rdline_new().


> > [...]
> > > +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.
> 
> rdline_get_history_buffer_size() is safe because sizeof() is evaluated at
> compile time. There's a unit test checking that all functions are NULL-safe.

Oh yes, of course, thanks.