diff mbox series

ethdev: add comment to warn of ABI breakage

Message ID 20200218104604.30574-1-ciara.power@intel.com (mailing list archive)
State Superseded, archived
Headers
Series ethdev: add comment to warn of ABI breakage |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/travis-robot warning Travis build: failed
ci/Intel-compilation fail apply issues

Commit Message

Power, Ciara Feb. 18, 2020, 10:46 a.m. UTC 
  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(+)

Comments

Ferruh Yigit Feb. 18, 2020, 11:59 a.m. UTC | #1 
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>
Andrew Rybchenko Feb. 18, 2020, 1:13 p.m. UTC | #2 
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>
Thomas Monjalon Feb. 18, 2020, 2:20 p.m. UTC | #3 
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>
diff mbox series

Patch

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. */