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

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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.