diff mbox

[dpdk-dev,1/2] ethdev: remove useless null checks

Message ID 1453377431-25850-2-git-send-email-david.marchand@6wind.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

David Marchand Jan. 21, 2016, 11:57 a.m. UTC 
We are in static functions and those passed arguments can't be NULL.

Signed-off-by: David Marchand <david.marchand@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 15 ---------------
 1 file changed, 15 deletions(-)

Comments

Jan Viktorin Jan. 21, 2016, 7:02 p.m. UTC | #1 
On Thu, 21 Jan 2016 12:57:10 +0100
David Marchand <david.marchand@6wind.com> wrote:

> We are in static functions and those passed arguments can't be NULL.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> 
> ---
> lib/librte_ether/rte_ethdev.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index af990e2..951fb1c 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -220,9 +220,6 @@ rte_eth_dev_create_unique_device_name(char *name, size_t size,
>  {
>  	int ret;
>  
> -	if ((name == NULL) || (pci_dev == NULL))
> -		return -EINVAL;

Do you use a kind of assert in DPDK? The patch looks OK, however, I
would prefer something like

	assert_not_null(name);
	assert_not_null(pci_dev);

Usually, if some outer code is broken by mistake, the assert catches
such an issue. At the same time, it documents the code by telling
"this must never be NULL here". I agree, that returning -EINVAL for
this kind of check is incorrect.

Same for other changes...

> [snip]
Thomas Monjalon Jan. 22, 2016, 9:11 a.m. UTC | #2 
2016-01-21 20:02, Jan Viktorin:
> On Thu, 21 Jan 2016 12:57:10 +0100
> David Marchand <david.marchand@6wind.com> wrote:
> > -	if ((name == NULL) || (pci_dev == NULL))
> > -		return -EINVAL;
> 
> Do you use a kind of assert in DPDK? The patch looks OK, however, I
> would prefer something like
> 
> 	assert_not_null(name);
> 	assert_not_null(pci_dev);
> 
> Usually, if some outer code is broken by mistake, the assert catches
> such an issue. At the same time, it documents the code by telling
> "this must never be NULL here". I agree, that returning -EINVAL for
> this kind of check is incorrect.
> 
> Same for other changes...

For this patch, I agree to remove useless checks.
For the other checks, EINVAL seems to be the right error code.
And yes you are right, we could replace most of EINVAL returns by a kind
of assert. RTE_VERIFY would be appropriate.
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index af990e2..951fb1c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -220,9 +220,6 @@  rte_eth_dev_create_unique_device_name(char *name, size_t size,
 {
 	int ret;
 
-	if ((name == NULL) || (pci_dev == NULL))
-		return -EINVAL;
-
 	ret = snprintf(name, size, "%d:%d.%d",
 			pci_dev->addr.bus, pci_dev->addr.devid,
 			pci_dev->addr.function);
@@ -505,9 +502,6 @@  rte_eth_dev_is_detachable(uint8_t port_id)
 static int
 rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id)
 {
-	if ((addr == NULL) || (port_id == NULL))
-		goto err;
-
 	/* re-construct pci_device_list */
 	if (rte_eal_pci_scan())
 		goto err;
@@ -531,9 +525,6 @@  rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr)
 	struct rte_pci_addr freed_addr;
 	struct rte_pci_addr vp;
 
-	if (addr == NULL)
-		goto err;
-
 	/* check whether the driver supports detach feature, or not */
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;
@@ -566,9 +557,6 @@  rte_eth_dev_attach_vdev(const char *vdevargs, uint8_t *port_id)
 	char *name = NULL, *args = NULL;
 	int ret = -1;
 
-	if ((vdevargs == NULL) || (port_id == NULL))
-		goto end;
-
 	/* parse vdevargs, then retrieve device name and args */
 	if (rte_eal_parse_devargs_str(vdevargs, &name, &args))
 		goto end;
@@ -600,9 +588,6 @@  rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname)
 {
 	char name[RTE_ETH_NAME_MAX_LEN];
 
-	if (vdevname == NULL)
-		goto err;
-
 	/* check whether the driver supports detach feature, or not */
 	if (rte_eth_dev_is_detachable(port_id))
 		goto err;