diff mbox series

[3/3] lib/librte_meter: update abi to include new rfc4115 function

Message ID 154333210155.44971.11196185167374567770.stgit@dbuild (mailing list archive)
State Superseded, archived
Delegated to: Cristian Dumitrescu
Headers
Series lib/librte_meter: add RFC4115 trTCM meter support |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Eelco Chaudron Nov. 27, 2018, 3:21 p.m. UTC 
  Update the ABI to include the new RFC4115 meter functions
---
 lib/librte_meter/Makefile              |    2 +-
 lib/librte_meter/meson.build           |    2 +-
 lib/librte_meter/rte_meter_version.map |    9 +++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

Comments

Eelco Chaudron Nov. 28, 2018, 9:27 a.m. UTC | #1 
On 28 Nov 2018, at 9:38, David Marchand wrote:

> Hello Eelco,
>
> On Tue, Nov 27, 2018 at 4:22 PM Eelco Chaudron <echaudro@redhat.com> 
> wrote:
>
>> Update the ABI to include the new RFC4115 meter functions
>> ---
>>  lib/librte_meter/Makefile              |    2 +-
>>  lib/librte_meter/meson.build           |    2 +-
>>  lib/librte_meter/rte_meter_version.map |    9 +++++++++
>>  3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_meter/Makefile b/lib/librte_meter/Makefile
>> index 2dc071e8e..79ad79744 100644
>> --- a/lib/librte_meter/Makefile
>> +++ b/lib/librte_meter/Makefile
>> @@ -16,7 +16,7 @@ LDLIBS += -lrte_eal
>>
>>  EXPORT_MAP := rte_meter_version.map
>>
>> -LIBABIVER := 2
>> +LIBABIVER := 3
>>
>>  #
>>  # all source are stored in SRCS-y
>>
>
> As far as I understood the policy around the EXPERIMENTAL section, you
> don't need to bump the library version.

Thought I would add the new function as none experimental, i.e. next 
version, but checkpatch did not allow me to do this.

Tried to find info on what the right process was, as these functions are 
just another meter implementation using similar existing APIs. If anyone 
has any background on this please point me to it.

I changed the library version as an existing data structure changed 
(which in theory should not change the location of the data), but the 
ABI check popped warnings so I decided to increase the version.

>
> + you should squash this into the first patch.

I’ll do this on the next version.

Thanks,

Eelco
Thomas Monjalon Nov. 28, 2018, 10:09 a.m. UTC | #2 
28/11/2018 10:27, Eelco Chaudron:
> On 28 Nov 2018, at 9:38, David Marchand wrote:
> > On Tue, Nov 27, 2018 at 4:22 PM Eelco Chaudron <echaudro@redhat.com> 
> > wrote:
> >> --- a/lib/librte_meter/Makefile
> >> +++ b/lib/librte_meter/Makefile
> >> -LIBABIVER := 2
> >> +LIBABIVER := 3
> >
> > As far as I understood the policy around the EXPERIMENTAL section, you
> > don't need to bump the library version.
> 
> Thought I would add the new function as none experimental, i.e. next 
> version, but checkpatch did not allow me to do this.
> 
> Tried to find info on what the right process was, as these functions are 
> just another meter implementation using similar existing APIs. If anyone 
> has any background on this please point me to it.

It is documented here:
	http://doc.dpdk.org/guides/contributing/versioning.html

The case for "similar API" is not handled specifically so far.
So you need to introduce it as experimental.

> I changed the library version as an existing data structure changed 
> (which in theory should not change the location of the data), but the 
> ABI check popped warnings so I decided to increase the version.

It deserves to analyze why the ABI check raises a warning.
If it really needs to bump the ABI version, you should justify it
in the commit message, and explain what changed in the ABI section
of the release notes, plus update the version in the release notes.
Eelco Chaudron Nov. 28, 2018, 12:40 p.m. UTC | #3 
On 28 Nov 2018, at 11:09, Thomas Monjalon wrote:

> 28/11/2018 10:27, Eelco Chaudron:
>> On 28 Nov 2018, at 9:38, David Marchand wrote:
>>> On Tue, Nov 27, 2018 at 4:22 PM Eelco Chaudron <echaudro@redhat.com>
>>> wrote:
>>>> --- a/lib/librte_meter/Makefile
>>>> +++ b/lib/librte_meter/Makefile
>>>> -LIBABIVER := 2
>>>> +LIBABIVER := 3
>>>
>>> As far as I understood the policy around the EXPERIMENTAL section, 
>>> you
>>> don't need to bump the library version.
>>
>> Thought I would add the new function as none experimental, i.e. next
>> version, but checkpatch did not allow me to do this.
>>
>> Tried to find info on what the right process was, as these functions 
>> are
>> just another meter implementation using similar existing APIs. If 
>> anyone
>> has any background on this please point me to it.
>
> It is documented here:
> 	http://doc.dpdk.org/guides/contributing/versioning.html
>
> The case for "similar API" is not handled specifically so far.
> So you need to introduce it as experimental.

Thanks for the clarification, will update the APIs with 
__rte_experimental in the next iteration.

>> I changed the library version as an existing data structure changed
>> (which in theory should not change the location of the data), but the
>> ABI check popped warnings so I decided to increase the version.
>
> It deserves to analyze why the ABI check raises a warning.
> If it really needs to bump the ABI version, you should justify it
> in the commit message, and explain what changed in the ABI section
> of the release notes, plus update the version in the release notes.

Will look at it more closely and update it for the next iteration.
Thomas Monjalon Nov. 28, 2018, 12:51 p.m. UTC | #4 
28/11/2018 13:40, Eelco Chaudron:
> 
> On 28 Nov 2018, at 11:09, Thomas Monjalon wrote:
> 
> > 28/11/2018 10:27, Eelco Chaudron:
> >> On 28 Nov 2018, at 9:38, David Marchand wrote:
> >>> On Tue, Nov 27, 2018 at 4:22 PM Eelco Chaudron <echaudro@redhat.com>
> >>> wrote:
> >>>> --- a/lib/librte_meter/Makefile
> >>>> +++ b/lib/librte_meter/Makefile
> >>>> -LIBABIVER := 2
> >>>> +LIBABIVER := 3
> >>>
> >>> As far as I understood the policy around the EXPERIMENTAL section, 
> >>> you
> >>> don't need to bump the library version.
> >>
> >> Thought I would add the new function as none experimental, i.e. next
> >> version, but checkpatch did not allow me to do this.
> >>
> >> Tried to find info on what the right process was, as these functions 
> >> are
> >> just another meter implementation using similar existing APIs. If 
> >> anyone
> >> has any background on this please point me to it.
> >
> > It is documented here:
> > 	http://doc.dpdk.org/guides/contributing/versioning.html
> >
> > The case for "similar API" is not handled specifically so far.
> > So you need to introduce it as experimental.
> 
> Thanks for the clarification, will update the APIs with 
> __rte_experimental in the next iteration.

You should also add this on top of the doxygen comment:

 * @warning
 * @b EXPERIMENTAL: this API may change without prior notice
Eelco Chaudron Nov. 28, 2018, 1:17 p.m. UTC | #5 
On 28 Nov 2018, at 13:51, Thomas Monjalon wrote:

> 28/11/2018 13:40, Eelco Chaudron:
>>
>> On 28 Nov 2018, at 11:09, Thomas Monjalon wrote:
>>
>>> 28/11/2018 10:27, Eelco Chaudron:
>>>> On 28 Nov 2018, at 9:38, David Marchand wrote:
>>>>> On Tue, Nov 27, 2018 at 4:22 PM Eelco Chaudron <echaudro@redhat.com>
>>>>> wrote:
>>>>>> --- a/lib/librte_meter/Makefile
>>>>>> +++ b/lib/librte_meter/Makefile
>>>>>> -LIBABIVER := 2
>>>>>> +LIBABIVER := 3
>>>>>
>>>>> As far as I understood the policy around the EXPERIMENTAL section,
>>>>> you
>>>>> don't need to bump the library version.
>>>>
>>>> Thought I would add the new function as none experimental, i.e. next
>>>> version, but checkpatch did not allow me to do this.
>>>>
>>>> Tried to find info on what the right process was, as these functions
>>>> are
>>>> just another meter implementation using similar existing APIs. If
>>>> anyone
>>>> has any background on this please point me to it.
>>>
>>> It is documented here:
>>> 	http://doc.dpdk.org/guides/contributing/versioning.html
>>>
>>> The case for "similar API" is not handled specifically so far.
>>> So you need to introduce it as experimental.
>>
>> Thanks for the clarification, will update the APIs with
>> __rte_experimental in the next iteration.
>
> You should also add this on top of the doxygen comment:
>
>  * @warning
>  * @b EXPERIMENTAL: this API may change without prior notice

Thanks, done!
diff mbox series

Patch

diff --git a/lib/librte_meter/Makefile b/lib/librte_meter/Makefile
index 2dc071e8e..79ad79744 100644
--- a/lib/librte_meter/Makefile
+++ b/lib/librte_meter/Makefile
@@ -16,7 +16,7 @@  LDLIBS += -lrte_eal
 
 EXPORT_MAP := rte_meter_version.map
 
-LIBABIVER := 2
+LIBABIVER := 3
 
 #
 # all source are stored in SRCS-y
diff --git a/lib/librte_meter/meson.build b/lib/librte_meter/meson.build
index 947bc19e2..422123e20 100644
--- a/lib/librte_meter/meson.build
+++ b/lib/librte_meter/meson.build
@@ -1,6 +1,6 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2017 Intel Corporation
 
-version = 2
+version = 3
 sources = files('rte_meter.c')
 headers = files('rte_meter.h')
diff --git a/lib/librte_meter/rte_meter_version.map b/lib/librte_meter/rte_meter_version.map
index cb79f0c2b..4b460d580 100644
--- a/lib/librte_meter/rte_meter_version.map
+++ b/lib/librte_meter/rte_meter_version.map
@@ -17,3 +17,12 @@  DPDK_18.08 {
 	rte_meter_srtcm_profile_config;
 	rte_meter_trtcm_profile_config;
 };
+
+EXPERIMENTAL {
+	global:
+
+	rte_meter_trtcm_rfc4115_color_aware_check;
+	rte_meter_trtcm_rfc4115_color_blind_check;
+	rte_meter_trtcm_rfc4115_config;
+	rte_meter_trtcm_rfc4115_profile_config;
+};