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.
