Re: [dpdk-dev] [PATCH v3 7/7] ethdev: deprecate rte_flow_copy function

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 8/31/2018 10:01 AM, Adrien Mazarguil wrote:
> No users left for this function, time to deprecate it.
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> --
> v3 changes:
> 
> - Removed deprecation notice (finally got Ferruh's point), made patch last
>   in series.
> 
> v2 changes:
> 
> - Patch was not present in original series.
> ---
>  doc/guides/rel_notes/deprecation.rst | 7 -------
>  lib/librte_ethdev/rte_flow.h         | 7 ++++++-
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index e2dbee317..48cfb266b 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -88,10 +88,3 @@ Deprecation Notices
>    - ``rte_pdump_set_socket_dir`` will be removed;
>    - The parameter, ``path``, of ``rte_pdump_init`` will be removed;
>    - The enum ``rte_pdump_socktype`` will be removed.
> -
> -* ethdev: flow API function ``rte_flow_copy()`` will be deprecated in v18.11
> -  in favor of ``rte_flow_conv()`` (which will appear in that version) and
> -  subsequently removed for v19.02.
> -
> -  This is due to a lack of flexibility and reliance on a type unusable with
> -  C++ programs (struct rte_flow_desc).
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 052ceefb6..f062ffead 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2332,6 +2332,7 @@ rte_flow_error_set(struct rte_flow_error *error,
>  		   const char *message);
>  
>  /**
> + * @deprecated
>   * @see rte_flow_copy()
>   */
>  struct rte_flow_desc {
> @@ -2343,10 +2344,13 @@ struct rte_flow_desc {
>  };
>  
>  /**
> + * @deprecated
>   * Copy an rte_flow rule description.
>   *
>   * This interface is kept for compatibility with older applications but is
> - * implemented as a wrapper to rte_flow_conv().
> + * implemented as a wrapper to rte_flow_conv(). It is deprecated due to its
> + * lack of flexibility and reliance on a type unusable with C++ programs
> + * (struct rte_flow_desc).
>   *
>   * @param[in] fd
>   *   Flow rule description.
> @@ -2365,6 +2369,7 @@ struct rte_flow_desc {
>   *   If len is lower than the size of the flow, the number of bytes that would
>   *   have been written to desc had it been sufficient. Nothing is written.
>   */
> +__rte_deprecated
>  size_t
>  rte_flow_copy(struct rte_flow_desc *fd, size_t len,
>  	      const struct rte_flow_attr *attr,
> 


Not exactly related to this patch but I have a deprecation process question,
what we are mostly doing is:

1) Release N, document in deprecation.rst the API that will be changed
2) Release N + 1, change the API and remove the note from the deprecation.rst

But using __rte_deprecated makes sense, how can we include this into process?

It looks like we can use __rte_deprecated only when an API replaced (add a new
one & deprecate old one), but if an API changed switch needs to be atomic.


Options I can think of:

For replacing an API,

A)
a1) Release N, add note to deprecation.rst and add __rte_deprecated to old API
a2) Release N + 1, add new API, remove note from deprecation.rst and remove old API

B)
b1) Release N, add note to deprecation.rst
b2) Release N + 1, remove note from deprecation.rst add __rte_deprecated to old
API and add new API (as non experimental [1])
b3) Release N + 1 + M, remove old API (perhaps cleanup on each new LTS)



For changing exiting API,

C)
c1) Release N, add note to deprecation.rst
c2) Release N + 1, remove note from deprecation.rst and switch to new API

The problem with C) is, even developer sees the deprecation note, there is
nothing she can do or prepare, only in "Release N + 1" she will hit to the
change and will update her code. Not very useful for developer, so what about:

D)
d1) Release N, add note to deprecation.rst implement new API within RTE_NEXT_ABI
#ifdef
d2) Release N + 1, remove note from deprecation.rst and switch to new API

Switching to D means one can't send deprecation notice without doing the actual
implementation, so there is a big difference between C).

I am for B & D, what do you think?





[1]
This is happening again with rte_flow_conv(), old API is deprecated, new API is
experimental, from user point of view there is no stable API.
I am not happy with "an API should be experimental for first release it has been
introduced" policy, is it really helping, can we re-visit this again?