diff mbox

[dpdk-dev] maintainers: fix responsibility of flow API bits

Message ID 20180515162211.7030-1-adrien.mazarguil@6wind.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Adrien Mazarguil May 15, 2018, 4:23 p.m. UTC 
  The following commits lack MAINTAINERS entries for this mess.

Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
Fixes: 19c90af6285c ("app/testpmd: add flow command")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: Bernard Iremonger <bernard.iremonger@intel.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

Comments

Iremonger, Bernard May 16, 2018, 9:18 a.m. UTC | #1 
Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, May 15, 2018 5:23 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>
> Subject: [PATCH] maintainers: fix responsibility of flow API bits
> 
> The following commits lack MAINTAINERS entries for this mess.
> 
> Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
> Fixes: 19c90af6285c ("app/testpmd: add flow command")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bd08d4292..187817fff 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -303,6 +303,8 @@ F: devtools/test-null.sh  Flow API
>  M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>  T: git://dpdk.org/next/dpdk-next-net
> +F: app/test-pmd/cmdline_flow.c
> +F: doc/guides/prog_guide/rte_flow.rst

doc/guides/testpmd_app_ug/testpmd_funcs.rst
Should be added to the list of flow related files.

>  F: lib/librte_ethdev/rte_flow*
> 
>  Traffic Management API - EXPERIMENTAL
> --
> 2.11.0

Regards,

Bernard.
Adrien Mazarguil May 16, 2018, 2:35 p.m. UTC | #2 
On Wed, May 16, 2018 at 09:18:01AM +0000, Iremonger, Bernard wrote:
> Hi Adrien,
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Tuesday, May 15, 2018 5:23 PM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Iremonger, Bernard <bernard.iremonger@intel.com>
> > Subject: [PATCH] maintainers: fix responsibility of flow API bits
> > 
> > The following commits lack MAINTAINERS entries for this mess.
> > 
> > Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
> > Fixes: 19c90af6285c ("app/testpmd: add flow command")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  MAINTAINERS | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index bd08d4292..187817fff 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -303,6 +303,8 @@ F: devtools/test-null.sh  Flow API
> >  M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >  T: git://dpdk.org/next/dpdk-next-net
> > +F: app/test-pmd/cmdline_flow.c
> > +F: doc/guides/prog_guide/rte_flow.rst
> 
> doc/guides/testpmd_app_ug/testpmd_funcs.rst
> Should be added to the list of flow related files.

I did not add it because there is no way to split responsibility on a
documentation section basis AFAIK, and only a fraction of this file contains
information about rte_flow-related stuff.
Iremonger, Bernard May 17, 2018, 9:26 a.m. UTC | #3 
Hi Adrien,

<snip>

> > > Subject: [PATCH] maintainers: fix responsibility of flow API bits
> > >
> > > The following commits lack MAINTAINERS entries for this mess.
> > >
> > > Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
> > > Fixes: 19c90af6285c ("app/testpmd: add flow command")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> > > ---
> > >  MAINTAINERS | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS index bd08d4292..187817fff
> > > 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -303,6 +303,8 @@ F: devtools/test-null.sh  Flow API
> > >  M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > >  T: git://dpdk.org/next/dpdk-next-net
> > > +F: app/test-pmd/cmdline_flow.c
> > > +F: doc/guides/prog_guide/rte_flow.rst
> >
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > Should be added to the list of flow related files.
> 
> I did not add it because there is no way to split responsibility on a
> documentation section basis AFAIK, and only a fraction of this file contains
> information about rte_flow-related stuff.
> 
> --
> Adrien Mazarguil
> 6WIND

There are already several maintainers for testpmd, adding another maintainer for the flow parts of this file should be ok (I think).

Regards,

Bernard
Adrien Mazarguil May 17, 2018, 11:20 a.m. UTC | #4 
On Thu, May 17, 2018 at 09:26:15AM +0000, Iremonger, Bernard wrote:
> Hi Adrien,
> 
> <snip>
> 
> > > > Subject: [PATCH] maintainers: fix responsibility of flow API bits
> > > >
> > > > The following commits lack MAINTAINERS entries for this mess.
> > > >
> > > > Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
> > > > Fixes: 19c90af6285c ("app/testpmd: add flow command")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > > Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> > > > ---
> > > >  MAINTAINERS | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS index bd08d4292..187817fff
> > > > 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -303,6 +303,8 @@ F: devtools/test-null.sh  Flow API
> > > >  M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > >  T: git://dpdk.org/next/dpdk-next-net
> > > > +F: app/test-pmd/cmdline_flow.c
> > > > +F: doc/guides/prog_guide/rte_flow.rst
> > >
> > > doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > Should be added to the list of flow related files.
> > 
> > I did not add it because there is no way to split responsibility on a
> > documentation section basis AFAIK, and only a fraction of this file contains
> > information about rte_flow-related stuff.
> > 
> > --
> > Adrien Mazarguil
> > 6WIND
> 
> There are already several maintainers for testpmd, adding another maintainer for the flow parts of this file should be ok (I think).

True, however providing an exact file name instead of a directory or
wildcard makes me the main maintainer/contact for that specific file
(e.g. "app/test-pmd/cmdline_flow.c" vs. "app/test-pmd"), basically I do not
want to be responsible for the entirety of testpmd's documentation.

How about the following instead: another patch that extracts the flow
command documentation in its own file (testpmd_flow.rst?) using the include 
directive (somewhat like cmdline_flow.c). This patch will update MAINTAINERS
accordingly.
Iremonger, Bernard May 17, 2018, 11:26 a.m. UTC | #5 
Hi Adrien,

<snip>


> > > > > Subject: [PATCH] maintainers: fix responsibility of flow API
> > > > > bits
> > > > >
> > > > > The following commits lack MAINTAINERS entries for this mess.
> > > > >
> > > > > Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
> > > > > Fixes: 19c90af6285c ("app/testpmd: add flow command")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > > > Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> > > > > ---
> > > > >  MAINTAINERS | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > > bd08d4292..187817fff
> > > > > 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -303,6 +303,8 @@ F: devtools/test-null.sh  Flow API
> > > > >  M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > > >  T: git://dpdk.org/next/dpdk-next-net
> > > > > +F: app/test-pmd/cmdline_flow.c
> > > > > +F: doc/guides/prog_guide/rte_flow.rst
> > > >
> > > > doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > > > Should be added to the list of flow related files.
> > >
> > > I did not add it because there is no way to split responsibility on
> > > a documentation section basis AFAIK, and only a fraction of this
> > > file contains information about rte_flow-related stuff.
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> >
> > There are already several maintainers for testpmd, adding another maintainer
> for the flow parts of this file should be ok (I think).
> 
> True, however providing an exact file name instead of a directory or wildcard
> makes me the main maintainer/contact for that specific file (e.g. "app/test-
> pmd/cmdline_flow.c" vs. "app/test-pmd"), basically I do not want to be
> responsible for the entirety of testpmd's documentation.
> 
> How about the following instead: another patch that extracts the flow
> command documentation in its own file (testpmd_flow.rst?) using the include
> directive (somewhat like cmdline_flow.c). This patch will update MAINTAINERS
> accordingly.
> 
> --
> Adrien Mazarguil
> 6WIND

Sounds good to me.

Regards,

Bernard.
Ferruh Yigit May 17, 2018, 2:45 p.m. UTC | #6 
On 5/17/2018 12:26 PM, Iremonger, Bernard wrote:
> Hi Adrien,
> 
> <snip>
> 
> 
>>>>>> Subject: [PATCH] maintainers: fix responsibility of flow API
>>>>>> bits
>>>>>>
>>>>>> The following commits lack MAINTAINERS entries for this mess.
>>>>>>
>>>>>> Fixes: 4d73b6fb9907 ("doc: add generic flow API guide")
>>>>>> Fixes: 19c90af6285c ("app/testpmd: add flow command")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>>>> Cc: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>>> ---
>>>>>>  MAINTAINERS | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS index
>>>>>> bd08d4292..187817fff
>>>>>> 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -303,6 +303,8 @@ F: devtools/test-null.sh  Flow API
>>>>>>  M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>>>>  T: git://dpdk.org/next/dpdk-next-net
>>>>>> +F: app/test-pmd/cmdline_flow.c
>>>>>> +F: doc/guides/prog_guide/rte_flow.rst
>>>>>
>>>>> doc/guides/testpmd_app_ug/testpmd_funcs.rst
>>>>> Should be added to the list of flow related files.
>>>>
>>>> I did not add it because there is no way to split responsibility on
>>>> a documentation section basis AFAIK, and only a fraction of this
>>>> file contains information about rte_flow-related stuff.
>>>>
>>>> --
>>>> Adrien Mazarguil
>>>> 6WIND
>>>
>>> There are already several maintainers for testpmd, adding another maintainer
>> for the flow parts of this file should be ok (I think).
>>
>> True, however providing an exact file name instead of a directory or wildcard
>> makes me the main maintainer/contact for that specific file (e.g. "app/test-
>> pmd/cmdline_flow.c" vs. "app/test-pmd"), basically I do not want to be
>> responsible for the entirety of testpmd's documentation.
>>
>> How about the following instead: another patch that extracts the flow
>> command documentation in its own file (testpmd_flow.rst?) using the include
>> directive (somewhat like cmdline_flow.c). This patch will update MAINTAINERS
>> accordingly.
>>
>> --
>> Adrien Mazarguil
>> 6WIND
> 
> Sounds good to me.

Applied to dpdk-next-net/master, thanks.

(Another patch will separate testpmd_flow.rst? and add maintainership for it)
Iremonger, Bernard May 17, 2018, 2:51 p.m. UTC | #7 
Hi Adrien, Ferruh,

<snip>
 
> (Another patch will separate testpmd_flow.rst? and add maintainership for it)


There is also a lot of flow related code in dpdk/app/test-pmd/config.c (lines 985 to 1623), maybe this should be moved to a separate file also, with maintainership added for the new file?

Regards,

Bernard.
Adrien Mazarguil May 17, 2018, 3:50 p.m. UTC | #8 
On Thu, May 17, 2018 at 02:51:43PM +0000, Iremonger, Bernard wrote:
> Hi Adrien, Ferruh,
> 
> <snip>
>  
> > (Another patch will separate testpmd_flow.rst? and add maintainership for it)
> 
> There is also a lot of flow related code in dpdk/app/test-pmd/config.c (lines 985 to 1623), maybe this should be moved to a separate file also, with maintainership added for the new file?

I will consider it also, although the majority of that code is a copy of
rte_flow_copy(), which was added later for failsafe PMD needs.

Testpmd was supposed to use the public version but I wanted to replace
rte_flow_copy() with the more flexible rte_flow_conv() first [1][2].

[1] "[PATCH v1 2/7] ethdev: replace flow API object copy function"
    http://dpdk.org/ml/archives/dev/2017-October/077553.html
[2] "[PATCH v1 4/7] app/testpmd: rely on flow API conversion function"
    http://dpdk.org/ml/archives/dev/2017-October/077555.html
diff mbox

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index bd08d4292..187817fff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -303,6 +303,8 @@  F: devtools/test-null.sh
 Flow API
 M: Adrien Mazarguil <adrien.mazarguil@6wind.com>
 T: git://dpdk.org/next/dpdk-next-net
+F: app/test-pmd/cmdline_flow.c
+F: doc/guides/prog_guide/rte_flow.rst
 F: lib/librte_ethdev/rte_flow*
 
 Traffic Management API - EXPERIMENTAL