ethdev: avoid usage of uninit device info in bad port case
diff mbox series

Message ID 1563873208-5096-1-git-send-email-arybchenko@solarflare.com
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • ethdev: avoid usage of uninit device info in bad port case
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-Compile-Testing success Compile Testing PASS
ci/Intel-compilation fail Compilation issues
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Andrew Rybchenko July 23, 2019, 9:13 a.m. UTC
rte_eth_dev_info_get() returns void and caller does know if the function
does its job or not. Changing of the return value to int would be
API/ABI breakage which requires deprecation process and cannot be
backported to stable branches. For now, make sure that device info is
initialized even in the case of invalid port ID.

Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
Cc: stable@dpdk.org

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit July 23, 2019, 10:40 a.m. UTC | #1
On 7/23/2019 10:13 AM, Andrew Rybchenko wrote:
> rte_eth_dev_info_get() returns void and caller does know if the function
> does its job or not. Changing of the return value to int would be
> API/ABI breakage which requires deprecation process and cannot be
> backported to stable branches. For now, make sure that device info is
> initialized even in the case of invalid port ID.

+1 to return a status from function for long term.

But someone looks below code can think we are doing an unnecessary memset for
the error case, and will fix it :)
What do you think adding a comment to prevent this?

> 
> Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 64191737c..97b093edc 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -2552,10 +2552,11 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>  		.nb_mtu_seg_max = UINT16_MAX,
>  	};
>  
> +	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
> +
>  	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>  	dev = &rte_eth_devices[port_id];
>  
> -	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>  	dev_info->rx_desc_lim = lim;
>  	dev_info->tx_desc_lim = lim;
>  	dev_info->device = dev->device;
>
Andrew Rybchenko July 23, 2019, 12:16 p.m. UTC | #2
On 7/23/19 1:40 PM, Ferruh Yigit wrote:
> On 7/23/2019 10:13 AM, Andrew Rybchenko wrote:
>> rte_eth_dev_info_get() returns void and caller does know if the function
>> does its job or not. Changing of the return value to int would be
>> API/ABI breakage which requires deprecation process and cannot be
>> backported to stable branches. For now, make sure that device info is
>> initialized even in the case of invalid port ID.
> +1 to return a status from function for long term.

Thomas, what do you think? Should we finally fix it?
I think it is almost harmless API/ABI breakage.
If yes, I'll send deprecation notice to do it in v19.11.

> But someone looks below code can think we are doing an unnecessary memset for
> the error case, and will fix it :)
> What do you think adding a comment to prevent this?

See v2.
Thomas Monjalon July 23, 2019, 12:50 p.m. UTC | #3
23/07/2019 14:16, Andrew Rybchenko:
> On 7/23/19 1:40 PM, Ferruh Yigit wrote:
> > On 7/23/2019 10:13 AM, Andrew Rybchenko wrote:
> >> rte_eth_dev_info_get() returns void and caller does know if the function
> >> does its job or not. Changing of the return value to int would be
> >> API/ABI breakage which requires deprecation process and cannot be
> >> backported to stable branches. For now, make sure that device info is
> >> initialized even in the case of invalid port ID.
> > +1 to return a status from function for long term.
> 
> Thomas, what do you think? Should we finally fix it?
> I think it is almost harmless API/ABI breakage.
> If yes, I'll send deprecation notice to do it in v19.11.

Yes, generally speaking we should not have any void return in API.

Patch
diff mbox series

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 64191737c..97b093edc 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -2552,10 +2552,11 @@  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 		.nb_mtu_seg_max = UINT16_MAX,
 	};
 
+	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
+
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
 
-	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
 	dev_info->rx_desc_lim = lim;
 	dev_info->tx_desc_lim = lim;
 	dev_info->device = dev->device;