[v1,1/2] ring: fix the misdescription of the param

Message ID 20200729063105.11299-2-feifei.wang2@arm.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series wrong pointer passed of ring |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Feifei Wang July 29, 2020, 6:31 a.m. UTC
  When enqueue one element to the ring, the param "obj" should be the
object to be added into the ring. The object is of type void*.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/librte_ring/rte_ring.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Comments

David Marchand July 29, 2020, 3:59 p.m. UTC | #1
On Wed, Jul 29, 2020 at 8:31 AM Feifei Wang <feifei.wang2@arm.com> wrote:
>
> When enqueue one element to the ring, the param "obj" should be the
> object to be added into the ring. The object is of type void*.

I understand void * as a pointer to an object you don't know the type of.
I would keep the current description.

Honnappa, Konstantin, Olivier ?

>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  lib/librte_ring/rte_ring.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> index da17ed6d7..418536b61 100644
> --- a/lib/librte_ring/rte_ring.h
> +++ b/lib/librte_ring/rte_ring.h
> @@ -276,7 +276,7 @@ rte_ring_enqueue_bulk(struct rte_ring *r, void * const *obj_table,
>   * @param r
>   *   A pointer to the ring structure.
>   * @param obj
> - *   A pointer to the object to be added.
> + *   A pointer (object) to be added.
  
Ananyev, Konstantin July 29, 2020, 4:24 p.m. UTC | #2
> >
> > When enqueue one element to the ring, the param "obj" should be the
> > object to be added into the ring. The object is of type void*.
> 
> I understand void * as a pointer to an object you don't know the type of.
> I would keep the current description.
> 
> Honnappa, Konstantin, Olivier ?

I think current one is a bit cleaner.
Konstantin


> 
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  lib/librte_ring/rte_ring.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > index da17ed6d7..418536b61 100644
> > --- a/lib/librte_ring/rte_ring.h
> > +++ b/lib/librte_ring/rte_ring.h
> > @@ -276,7 +276,7 @@ rte_ring_enqueue_bulk(struct rte_ring *r, void * const *obj_table,
> >   * @param r
> >   *   A pointer to the ring structure.
> >   * @param obj
> > - *   A pointer to the object to be added.
> > + *   A pointer (object) to be added.
> 
> 
> --
> David Marchand
  
Honnappa Nagarahalli July 29, 2020, 7:34 p.m. UTC | #3
<snip>

> Subject: RE: [dpdk-dev] [PATCH v1 1/2] ring: fix the misdescription of the
> param
> 
> > >
> > > When enqueue one element to the ring, the param "obj" should be the
> > > object to be added into the ring. The object is of type void*.
> >
> > I understand void * as a pointer to an object you don't know the type of.
> > I would keep the current description.
> >
> > Honnappa, Konstantin, Olivier ?
> 
> I think current one is a bit cleaner.
+1, the existing documentation is fine.

> Konstantin
> 
> 
> >
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >  lib/librte_ring/rte_ring.h | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > > index da17ed6d7..418536b61 100644
> > > --- a/lib/librte_ring/rte_ring.h
> > > +++ b/lib/librte_ring/rte_ring.h
> > > @@ -276,7 +276,7 @@ rte_ring_enqueue_bulk(struct rte_ring *r, void *
> const *obj_table,
> > >   * @param r
> > >   *   A pointer to the ring structure.
> > >   * @param obj
> > > - *   A pointer to the object to be added.
> > > + *   A pointer (object) to be added.
> >
> >
> > --
> > David Marchand
  
Feifei Wang July 30, 2020, 10:16 a.m. UTC | #4
Hi, David, Konstantin and Honnappa

> -----邮件原件-----
> 发件人: David Marchand <david.marchand@redhat.com>
> 发送时间: 2020年7月30日 0:00
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Olivier Matz <olivier.matz@6wind.com>
> 抄送: dev <dev@dpdk.org>; nd <nd@arm.com>; dpdk stable
> <stable@dpdk.org>; thomas@monjalon.net
> 主题: Re: [dpdk-dev] [PATCH v1 1/2] ring: fix the misdescription of the param
> 
> On Wed, Jul 29, 2020 at 8:31 AM Feifei Wang <feifei.wang2@arm.com>
> wrote:
> >
> > When enqueue one element to the ring, the param "obj" should be the
> > object to be added into the ring. The object is of type void*.
> 
> I understand void * as a pointer to an object you don't know the type of.
> I would keep the current description.
> 
Sorry for my commit message cannot express my view clearly. 
Following is my supplementary explanation of this:

First,  the APIs to be changed are:
1. rte_ring_mp_enqueue
2. rte_ring_sp_enqueue
3. rte_ring_enqueue

Second, Let's use an example to explain this:
1. We call API in form: rte_ring_enqueue(r, obj).
2. Current function header indicates that we will have ring[idx] = *obj.
3. However, this is not the case, what we eventually have is: ring[idx] = obj.

So I think the current function header is misleading or
maybe the implementation of above three functions need to be changed.

> Honnappa, Konstantin, Olivier ?
> 
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  lib/librte_ring/rte_ring.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > index da17ed6d7..418536b61 100644
> > --- a/lib/librte_ring/rte_ring.h
> > +++ b/lib/librte_ring/rte_ring.h
> > @@ -276,7 +276,7 @@ rte_ring_enqueue_bulk(struct rte_ring *r, void *
> const *obj_table,
> >   * @param r
> >   *   A pointer to the ring structure.
> >   * @param obj
> > - *   A pointer to the object to be added.
> > + *   A pointer (object) to be added.
> 
> 
> --
> David Marchand
  
Honnappa Nagarahalli July 31, 2020, 5:26 a.m. UTC | #5
<snip>

> > 主题: Re: [dpdk-dev] [PATCH v1 1/2] ring: fix the misdescription of the
> > param
> >
> > On Wed, Jul 29, 2020 at 8:31 AM Feifei Wang <feifei.wang2@arm.com>
> > wrote:
> > >
> > > When enqueue one element to the ring, the param "obj" should be the
> > > object to be added into the ring. The object is of type void*.
> >
> > I understand void * as a pointer to an object you don't know the type of.
> > I would keep the current description.
> >
> Sorry for my commit message cannot express my view clearly.
> Following is my supplementary explanation of this:
> 
> First,  the APIs to be changed are:
> 1. rte_ring_mp_enqueue
> 2. rte_ring_sp_enqueue
> 3. rte_ring_enqueue
> 
> Second, Let's use an example to explain this:
> 1. We call API in form: rte_ring_enqueue(r, obj).
> 2. Current function header indicates that we will have ring[idx] = *obj.
When you say function header, I am assuming you are talking about the documentation. IMO, the current documentation does not convey any details of the implementation. It is talking about what is expected from the user.

> 3. However, this is not the case, what we eventually have is: ring[idx] = obj.
> 
> So I think the current function header is misleading or maybe the
> implementation of above three functions need to be changed.
> 
> > Honnappa, Konstantin, Olivier ?
> >
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >  lib/librte_ring/rte_ring.h | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > > index da17ed6d7..418536b61 100644
> > > --- a/lib/librte_ring/rte_ring.h
> > > +++ b/lib/librte_ring/rte_ring.h
> > > @@ -276,7 +276,7 @@ rte_ring_enqueue_bulk(struct rte_ring *r, void *
> > const *obj_table,
> > >   * @param r
> > >   *   A pointer to the ring structure.
> > >   * @param obj
> > > - *   A pointer to the object to be added.
> > > + *   A pointer (object) to be added.
> >
> >
> > --
> > David Marchand
>
  

Patch

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index da17ed6d7..418536b61 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -276,7 +276,7 @@  rte_ring_enqueue_bulk(struct rte_ring *r, void * const *obj_table,
  * @param r
  *   A pointer to the ring structure.
  * @param obj
- *   A pointer to the object to be added.
+ *   A pointer (object) to be added.
  * @return
  *   - 0: Success; objects enqueued.
  *   - -ENOBUFS: Not enough room in the ring to enqueue; no object is enqueued.
@@ -293,7 +293,7 @@  rte_ring_mp_enqueue(struct rte_ring *r, void *obj)
  * @param r
  *   A pointer to the ring structure.
  * @param obj
- *   A pointer to the object to be added.
+ *   A pointer (object) to be added.
  * @return
  *   - 0: Success; objects enqueued.
  *   - -ENOBUFS: Not enough room in the ring to enqueue; no object is enqueued.
@@ -314,7 +314,7 @@  rte_ring_sp_enqueue(struct rte_ring *r, void *obj)
  * @param r
  *   A pointer to the ring structure.
  * @param obj
- *   A pointer to the object to be added.
+ *   A pointer (object) to be added.
  * @return
  *   - 0: Success; objects enqueued.
  *   - -ENOBUFS: Not enough room in the ring to enqueue; no object is enqueued.