Re: [dpdk-dev] [PATCH] ethdev: add comment to warn of ABI breakage

[Thomas Monjalon <thomas@monjalon.net>]

18/02/2020 14:13, Andrew Rybchenko:
> On 2/18/20 2:59 PM, Ferruh Yigit wrote:
> > On 2/18/2020 10:46 AM, Ciara Power wrote:
> >> If a function is added to the eth_dev_ops struct before
> >> tx_descriptor_status function, this will cause ABI breakage. This is due
> >> to static inline functions using this function, and some other functions
> >> above it in the struct, so they cannot change position. A comment is
> >> added to inform developers of this possible breakage.
> >>
> >> Signed-off-by: Ciara Power <ciara.power@intel.com>
> >> ---
> >>  lib/librte_ethdev/rte_ethdev_core.h | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
> >> index 7bf97e24e..1ad04a129 100644
> >> --- a/lib/librte_ethdev/rte_ethdev_core.h
> >> +++ b/lib/librte_ethdev/rte_ethdev_core.h
> >> @@ -667,6 +667,9 @@ struct eth_dev_ops {
> >>  	/**< Check the status of a Rx descriptor. */
> >>  	eth_tx_descriptor_status_t tx_descriptor_status;
> >>  	/**< Check the status of a Tx descriptor. */
> >> +	/* Static inline functions use functions above this comment.
> >> +	 * New dev_ops functions should be added below to avoid breaking ABI.
> >> +	 */
> >>  	eth_rx_enable_intr_t       rx_queue_intr_enable;  /**< Enable Rx queue interrupt. */
> >>  	eth_rx_disable_intr_t      rx_queue_intr_disable; /**< Disable Rx queue interrupt. */
> >>  	eth_tx_queue_setup_t       tx_queue_setup;/**< Set up device TX queue. */
> >>
> > 
> > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> LGTM, but may I suggest to make the first line in the comment
> empty to make it easier to notice. I.e.
>   /*
>    * Static inline functions use functions above this comment.
>    * ...
>    */
> 
> Or even add empty lines before and after the comment.
> 
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

In the same idea as Andrew, I would suggest some uppercases,
maybe "ABOVE" and "BELOW".

Acked-by: Thomas Monjalon <thomas@monjalon.net>