DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: dev@dpdk.org, David Marchand <david.marchand@redhat.com>,
	Ali Alnubani <alialnu@nvidia.com>,
	Gregory Etelson <getelson@nvidia.com>,
	Ray Kinsella <mdr@ashroe.eu>
Subject: Re: [dpdk-dev] [PATCH v3 2/2] cmdline: make struct rdline opaque
Date: Tue, 5 Oct 2021 11:11:43 +0200	[thread overview]
Message-ID: <YVwWz4j/bbPS+TWx@platinum> (raw)
In-Reply-To: <20211005120301.3c28e806@sovereign>

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.

  reply	other threads:[~2021-10-05  9:11 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 23:16 [dpdk-dev] [PATCH] cmdline: make cmdline structure opaque Dmitry Kozlyuk
2021-09-10 23:16 ` [dpdk-dev] [PATCH] cmdline: reduce ABI Dmitry Kozlyuk
2021-09-20 11:11   ` David Marchand
2021-09-20 11:21     ` Olivier Matz
2021-09-28 19:47   ` [dpdk-dev] [PATCH v2 0/2] " Dmitry Kozlyuk
2021-09-28 19:47     ` [dpdk-dev] [PATCH v2 1/2] cmdline: make struct cmdline opaque Dmitry Kozlyuk
2021-09-28 19:47     ` [dpdk-dev] [PATCH v2 2/2] cmdline: make struct rdline opaque Dmitry Kozlyuk
2021-10-05  0:55     ` [dpdk-dev] [PATCH v3 0/2] cmdline: reduce ABI Dmitry Kozlyuk
2021-10-05  0:55       ` [dpdk-dev] [PATCH v3 1/2] cmdline: make struct cmdline opaque Dmitry Kozlyuk
2021-10-05  0:55       ` [dpdk-dev] [PATCH v3 2/2] cmdline: make struct rdline opaque Dmitry Kozlyuk
2021-10-05  8:27         ` Olivier Matz
2021-10-05  9:03           ` Dmitry Kozlyuk
2021-10-05  9:11             ` Olivier Matz [this message]
2021-10-05 20:15       ` [dpdk-dev] [PATCH v4 0/2] cmdline: reduce ABI Dmitry Kozlyuk
2021-10-05 20:15         ` [dpdk-dev] [PATCH v4 1/2] cmdline: make struct cmdline opaque Dmitry Kozlyuk
2021-10-05 20:15         ` [dpdk-dev] [PATCH v4 2/2] cmdline: make struct rdline opaque Dmitry Kozlyuk
2021-10-05 20:42           ` Stephen Hemminger
2021-10-06  8:11           ` Olivier Matz
2021-10-07 22:10         ` [dpdk-dev] [PATCH v5 0/2] cmdline: reduce ABI Dmitry Kozlyuk
2021-10-07 22:10           ` [dpdk-dev] [PATCH v5 1/2] cmdline: make struct cmdline opaque Dmitry Kozlyuk
2021-10-08 22:56             ` Narcisa Ana Maria Vasile
2021-10-07 22:10           ` [dpdk-dev] [PATCH v5 2/2] cmdline: make struct rdline opaque Dmitry Kozlyuk
2021-10-08 22:57             ` Narcisa Ana Maria Vasile
2021-10-22 21:24           ` [dpdk-dev] [PATCH v5 0/2] cmdline: reduce ABI Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVwWz4j/bbPS+TWx@platinum \
    --to=olivier.matz@6wind.com \
    --cc=alialnu@nvidia.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=dmitry.kozliuk@gmail.com \
    --cc=getelson@nvidia.com \
    --cc=mdr@ashroe.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).