* [PATCH 0/2] minor cleanup in ethdev hotplug
@ 2016-01-21 11:57 David Marchand
2016-01-21 11:57 ` [PATCH 1/2] ethdev: remove useless null checks David Marchand
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: David Marchand @ 2016-01-21 11:57 UTC (permalink / raw)
To: dev
It was first a preparation step for future patchsets, but I am not sure what
will become of them, so sending this anyway since it does not hurt to clean
this now.
--
David Marchand
David Marchand (2):
ethdev: remove useless checks
ethdev: move code to common place in hotplug
lib/librte_ether/rte_ethdev.c | 84 +++++++++++++++++++++----------------------
1 file changed, 40 insertions(+), 44 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/2] ethdev: remove useless null checks 2016-01-21 11:57 [PATCH 0/2] minor cleanup in ethdev hotplug David Marchand @ 2016-01-21 11:57 ` David Marchand 2016-01-21 19:02 ` [dpdk-dev,1/2] " Jan Viktorin 2016-01-21 11:57 ` [PATCH 2/2] ethdev: move code to common place in hotplug David Marchand 2016-01-22 14:06 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug David Marchand 2 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2016-01-21 11:57 UTC (permalink / raw) To: dev 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; - 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; -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [dpdk-dev,1/2] ethdev: remove useless null checks 2016-01-21 11:57 ` [PATCH 1/2] ethdev: remove useless null checks David Marchand @ 2016-01-21 19:02 ` Jan Viktorin 2016-01-22 9:11 ` Thomas Monjalon 0 siblings, 1 reply; 16+ messages in thread From: Jan Viktorin @ 2016-01-21 19:02 UTC (permalink / raw) To: David Marchand; +Cc: dev 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] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev,1/2] ethdev: remove useless null checks 2016-01-21 19:02 ` [dpdk-dev,1/2] " Jan Viktorin @ 2016-01-22 9:11 ` Thomas Monjalon 0 siblings, 0 replies; 16+ messages in thread From: Thomas Monjalon @ 2016-01-22 9:11 UTC (permalink / raw) To: Jan Viktorin; +Cc: dev 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] ethdev: move code to common place in hotplug 2016-01-21 11:57 [PATCH 0/2] minor cleanup in ethdev hotplug David Marchand 2016-01-21 11:57 ` [PATCH 1/2] ethdev: remove useless null checks David Marchand @ 2016-01-21 11:57 ` David Marchand 2016-01-21 15:38 ` [dpdk-dev, " Jan Viktorin 2016-01-22 14:06 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug David Marchand 2 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2016-01-21 11:57 UTC (permalink / raw) To: dev Move these error logs and checks on detach capabilities in a common place. Signed-off-by: David Marchand <david.marchand@6wind.com> --- lib/librte_ether/rte_ethdev.c | 69 +++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index 951fb1c..9083783 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -514,7 +514,6 @@ rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id) return 0; err: - RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); return -1; } @@ -525,10 +524,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; - /* check whether the driver supports detach feature, or not */ - if (rte_eth_dev_is_detachable(port_id)) - goto err; - /* get pci address by port id */ if (rte_eth_dev_get_addr_by_port(port_id, &freed_addr)) goto err; @@ -546,7 +541,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr) *addr = freed_addr; return 0; err: - RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); return -1; } @@ -577,8 +571,6 @@ end: free(name); free(args); - if (ret < 0) - RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); return ret; } @@ -588,10 +580,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname) { char name[RTE_ETH_NAME_MAX_LEN]; - /* check whether the driver supports detach feature, or not */ - if (rte_eth_dev_is_detachable(port_id)) - goto err; - /* get device name by port id */ if (rte_eth_dev_get_name_by_port(port_id, name)) goto err; @@ -603,7 +591,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname) strncpy(vdevname, name, sizeof(name)); return 0; err: - RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); return -1; } @@ -612,14 +599,25 @@ int rte_eth_dev_attach(const char *devargs, uint8_t *port_id) { struct rte_pci_addr addr; + int ret = -1; if ((devargs == NULL) || (port_id == NULL)) - return -EINVAL; + goto err; - if (eal_parse_pci_DomBDF(devargs, &addr) == 0) - return rte_eth_dev_attach_pdev(&addr, port_id); - else - return rte_eth_dev_attach_vdev(devargs, port_id); + if (eal_parse_pci_DomBDF(devargs, &addr) == 0) { + ret = rte_eth_dev_attach_pdev(&addr, port_id); + if (ret < 0) + goto err; + } else { + ret = rte_eth_dev_attach_vdev(devargs, port_id); + if (ret < 0) + goto err; + } + + return 0; +err: + RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); + return ret; } /* detach the device, then store the name of the device */ @@ -627,26 +625,39 @@ int rte_eth_dev_detach(uint8_t port_id, char *name) { struct rte_pci_addr addr; - int ret; + int ret = -1; if (name == NULL) - return -EINVAL; + goto err; + + /* check whether the driver supports detach feature, or not */ + if (rte_eth_dev_is_detachable(port_id)) + goto err; if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) { ret = rte_eth_dev_get_addr_by_port(port_id, &addr); if (ret < 0) - return ret; + goto err; ret = rte_eth_dev_detach_pdev(port_id, &addr); - if (ret == 0) - snprintf(name, RTE_ETH_NAME_MAX_LEN, - "%04x:%02x:%02x.%d", - addr.domain, addr.bus, - addr.devid, addr.function); + if (ret < 0) + goto err; - return ret; - } else - return rte_eth_dev_detach_vdev(port_id, name); + snprintf(name, RTE_ETH_NAME_MAX_LEN, + "%04x:%02x:%02x.%d", + addr.domain, addr.bus, + addr.devid, addr.function); + } else { + ret = rte_eth_dev_detach_vdev(port_id, name); + if (ret < 0) + goto err; + } + + return 0; + +err: + RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); + return ret; } static int -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [dpdk-dev, 2/2] ethdev: move code to common place in hotplug 2016-01-21 11:57 ` [PATCH 2/2] ethdev: move code to common place in hotplug David Marchand @ 2016-01-21 15:38 ` Jan Viktorin 2016-01-21 18:06 ` David Marchand 0 siblings, 1 reply; 16+ messages in thread From: Jan Viktorin @ 2016-01-21 15:38 UTC (permalink / raw) To: David Marchand; +Cc: dev On Thu, 21 Jan 2016 12:57:11 +0100 David Marchand <david.marchand@6wind.com> wrote: > Move these error logs and checks on detach capabilities in a common place. > > Signed-off-by: David Marchand <david.marchand@6wind.com> > > --- > lib/librte_ether/rte_ethdev.c | 69 +++++++++++++++++++++++++------------------ > 1 file changed, 40 insertions(+), 29 deletions(-) > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index 951fb1c..9083783 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -514,7 +514,6 @@ rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id) > > return 0; > [snip] > > @@ -612,14 +599,25 @@ int > rte_eth_dev_attach(const char *devargs, uint8_t *port_id) > { > struct rte_pci_addr addr; > + int ret = -1; > > if ((devargs == NULL) || (port_id == NULL)) > - return -EINVAL; > + goto err; This change modifies the return value from -EINVAL to -1. I don't know whether is this an issue but it looks suspicious. > > - if (eal_parse_pci_DomBDF(devargs, &addr) == 0) > - return rte_eth_dev_attach_pdev(&addr, port_id); > - else > - return rte_eth_dev_attach_vdev(devargs, port_id); > + if (eal_parse_pci_DomBDF(devargs, &addr) == 0) { > + ret = rte_eth_dev_attach_pdev(&addr, port_id); > + if (ret < 0) > + goto err; > + } else { > + ret = rte_eth_dev_attach_vdev(devargs, port_id); > + if (ret < 0) > + goto err; > + } > + > + return 0; > +err: > + RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); > + return ret; > } > > /* detach the device, then store the name of the device */ > @@ -627,26 +625,39 @@ int > rte_eth_dev_detach(uint8_t port_id, char *name) > { > struct rte_pci_addr addr; > - int ret; > + int ret = -1; > > if (name == NULL) > - return -EINVAL; > + goto err; Same here... > + > + /* check whether the driver supports detach feature, or not */ > + if (rte_eth_dev_is_detachable(port_id)) > + goto err; > > [snip] -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev, 2/2] ethdev: move code to common place in hotplug 2016-01-21 15:38 ` [dpdk-dev, " Jan Viktorin @ 2016-01-21 18:06 ` David Marchand 2016-01-21 18:42 ` Thomas Monjalon 0 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2016-01-21 18:06 UTC (permalink / raw) To: Jan Viktorin, Thomas Monjalon; +Cc: dev On Thu, Jan 21, 2016 at 4:38 PM, Jan Viktorin <viktorin@rehivetech.com> wrote: > On Thu, 21 Jan 2016 12:57:11 +0100 > David Marchand <david.marchand@6wind.com> wrote: > [snip] >> @@ -612,14 +599,25 @@ int >> rte_eth_dev_attach(const char *devargs, uint8_t *port_id) >> { >> struct rte_pci_addr addr; >> + int ret = -1; >> >> if ((devargs == NULL) || (port_id == NULL)) >> - return -EINVAL; >> + goto err; > > This change modifies the return value from -EINVAL to -1. I don't know > whether is this an issue but it looks suspicious. Should not be an issue, as the api does not give details on expected negative return values. Just noticed, this also introduces a new log message that was not displayed before. To be safe, I suppose I should restore this. Thomas, opinion ? Thanks. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev, 2/2] ethdev: move code to common place in hotplug 2016-01-21 18:06 ` David Marchand @ 2016-01-21 18:42 ` Thomas Monjalon 2016-01-22 7:15 ` David Marchand 0 siblings, 1 reply; 16+ messages in thread From: Thomas Monjalon @ 2016-01-21 18:42 UTC (permalink / raw) To: David Marchand; +Cc: dev, Jan Viktorin 2016-01-21 19:06, David Marchand: > On Thu, Jan 21, 2016 at 4:38 PM, Jan Viktorin <viktorin@rehivetech.com> wrote: > > On Thu, 21 Jan 2016 12:57:11 +0100 > > David Marchand <david.marchand@6wind.com> wrote: > > > [snip] > >> @@ -612,14 +599,25 @@ int > >> rte_eth_dev_attach(const char *devargs, uint8_t *port_id) > >> { > >> struct rte_pci_addr addr; > >> + int ret = -1; > >> > >> if ((devargs == NULL) || (port_id == NULL)) > >> - return -EINVAL; > >> + goto err; > > > > This change modifies the return value from -EINVAL to -1. I don't know > > whether is this an issue but it looks suspicious. > > Should not be an issue, as the api does not give details on expected > negative return values. > Just noticed, this also introduces a new log message that was not > displayed before. > > To be safe, I suppose I should restore this. > > Thomas, opinion ? I'm OK with the log message added for this error case. I would just keep the -EINVAL return value. Other nit: you are allowed to fix the (moved) log message. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev, 2/2] ethdev: move code to common place in hotplug 2016-01-21 18:42 ` Thomas Monjalon @ 2016-01-22 7:15 ` David Marchand 0 siblings, 0 replies; 16+ messages in thread From: David Marchand @ 2016-01-22 7:15 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev, Jan Viktorin Hello, On Thu, Jan 21, 2016 at 7:42 PM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > 2016-01-21 19:06, David Marchand: >> On Thu, Jan 21, 2016 at 4:38 PM, Jan Viktorin <viktorin@rehivetech.com> wrote: >> > This change modifies the return value from -EINVAL to -1. I don't know >> > whether is this an issue but it looks suspicious. >> >> Should not be an issue, as the api does not give details on expected >> negative return values. >> Just noticed, this also introduces a new log message that was not >> displayed before. >> >> To be safe, I suppose I should restore this. >> >> Thomas, opinion ? > > I'm OK with the log message added for this error case. > I would just keep the -EINVAL return value. Ok will do. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/2] minor cleanup in ethdev hotplug 2016-01-21 11:57 [PATCH 0/2] minor cleanup in ethdev hotplug David Marchand 2016-01-21 11:57 ` [PATCH 1/2] ethdev: remove useless null checks David Marchand 2016-01-21 11:57 ` [PATCH 2/2] ethdev: move code to common place in hotplug David Marchand @ 2016-01-22 14:06 ` David Marchand 2016-01-22 14:06 ` [PATCH v2 1/2] ethdev: remove useless null checks David Marchand ` (2 more replies) 2 siblings, 3 replies; 16+ messages in thread From: David Marchand @ 2016-01-22 14:06 UTC (permalink / raw) To: dev; +Cc: viktorin It was first a preparation step for future patchsets, but I am not sure what will become of them, so sending this anyway since it does not hurt to clean this now. Changes since v1: - rebased on HEAD (previous patchset was based on another patch I sent separately) - restored EINVAL error code for rte_eth_dev_(at|de)tach (thanks Jan) -- David Marchand David Marchand (2): ethdev: remove useless null checks ethdev: move code to common place in hotplug lib/librte_ether/rte_ethdev.c | 92 +++++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 46 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] ethdev: remove useless null checks 2016-01-22 14:06 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug David Marchand @ 2016-01-22 14:06 ` David Marchand 2016-01-26 15:50 ` Jan Viktorin 2016-01-22 14:06 ` [PATCH v2 2/2] ethdev: move code to common place in hotplug David Marchand 2016-01-27 15:10 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug Thomas Monjalon 2 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2016-01-22 14:06 UTC (permalink / raw) To: dev; +Cc: viktorin 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 ed971b4..cab74e0 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; @@ -602,9 +590,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; -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ethdev: remove useless null checks 2016-01-22 14:06 ` [PATCH v2 1/2] ethdev: remove useless null checks David Marchand @ 2016-01-26 15:50 ` Jan Viktorin 2016-01-27 9:40 ` David Marchand 0 siblings, 1 reply; 16+ messages in thread From: Jan Viktorin @ 2016-01-26 15:50 UTC (permalink / raw) To: David Marchand; +Cc: dev What about the RTE_VERIFY? I think, it's more appropriate here. Otherwise, feel free to add: Reviewed-by: Jan Viktorin <viktorin@rehivetech.com> On Fri, 22 Jan 2016 15:06:57 +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 ed971b4..cab74e0 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; > @@ -602,9 +590,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; -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ethdev: remove useless null checks 2016-01-26 15:50 ` Jan Viktorin @ 2016-01-27 9:40 ` David Marchand 0 siblings, 0 replies; 16+ messages in thread From: David Marchand @ 2016-01-27 9:40 UTC (permalink / raw) To: Jan Viktorin; +Cc: dev On Tue, Jan 26, 2016 at 4:50 PM, Jan Viktorin <viktorin@rehivetech.com> wrote: > What about the RTE_VERIFY? I think, it's more appropriate here. Well, here, I am removing useless checks in static functions. But for the rest of ethdev api, I agree we could add some RTE_VERIFY. > Otherwise, feel free to add: > > Reviewed-by: Jan Viktorin <viktorin@rehivetech.com> Thanks. -- David Marchand ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] ethdev: move code to common place in hotplug 2016-01-22 14:06 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug David Marchand 2016-01-22 14:06 ` [PATCH v2 1/2] ethdev: remove useless null checks David Marchand @ 2016-01-22 14:06 ` David Marchand 2016-01-26 15:48 ` Jan Viktorin 2016-01-27 15:10 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug Thomas Monjalon 2 siblings, 1 reply; 16+ messages in thread From: David Marchand @ 2016-01-22 14:06 UTC (permalink / raw) To: dev; +Cc: viktorin Move these error logs and checks on detach capabilities in a common place. Signed-off-by: David Marchand <david.marchand@6wind.com> --- Changes since v1: - restore EINVAL error code for rte_eth_dev_(at|de)tach lib/librte_ether/rte_ethdev.c | 77 ++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 31 deletions(-) diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index cab74e0..a45563d 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -514,7 +514,6 @@ rte_eth_dev_attach_pdev(struct rte_pci_addr *addr, uint8_t *port_id) return 0; err: - RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); return -1; } @@ -525,10 +524,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; - /* check whether the driver supports detach feature, or not */ - if (rte_eth_dev_is_detachable(port_id)) - goto err; - /* get pci address by port id */ if (rte_eth_dev_get_addr_by_port(port_id, &freed_addr)) goto err; @@ -546,7 +541,6 @@ rte_eth_dev_detach_pdev(uint8_t port_id, struct rte_pci_addr *addr) *addr = freed_addr; return 0; err: - RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); return -1; } @@ -579,8 +573,6 @@ end: if (args) free(args); - if (ret < 0) - RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); return ret; } @@ -590,10 +582,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname) { char name[RTE_ETH_NAME_MAX_LEN]; - /* check whether the driver supports detach feature, or not */ - if (rte_eth_dev_is_detachable(port_id)) - goto err; - /* get device name by port id */ if (rte_eth_dev_get_name_by_port(port_id, name)) goto err; @@ -605,7 +593,6 @@ rte_eth_dev_detach_vdev(uint8_t port_id, char *vdevname) strncpy(vdevname, name, sizeof(name)); return 0; err: - RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); return -1; } @@ -614,14 +601,27 @@ int rte_eth_dev_attach(const char *devargs, uint8_t *port_id) { struct rte_pci_addr addr; + int ret = -1; - if ((devargs == NULL) || (port_id == NULL)) - return -EINVAL; + if ((devargs == NULL) || (port_id == NULL)) { + ret = -EINVAL; + goto err; + } - if (eal_parse_pci_DomBDF(devargs, &addr) == 0) - return rte_eth_dev_attach_pdev(&addr, port_id); - else - return rte_eth_dev_attach_vdev(devargs, port_id); + if (eal_parse_pci_DomBDF(devargs, &addr) == 0) { + ret = rte_eth_dev_attach_pdev(&addr, port_id); + if (ret < 0) + goto err; + } else { + ret = rte_eth_dev_attach_vdev(devargs, port_id); + if (ret < 0) + goto err; + } + + return 0; +err: + RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n"); + return ret; } /* detach the device, then store the name of the device */ @@ -629,26 +629,41 @@ int rte_eth_dev_detach(uint8_t port_id, char *name) { struct rte_pci_addr addr; - int ret; + int ret = -1; - if (name == NULL) - return -EINVAL; + if (name == NULL) { + ret = -EINVAL; + goto err; + } + + /* check whether the driver supports detach feature, or not */ + if (rte_eth_dev_is_detachable(port_id)) + goto err; if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) { ret = rte_eth_dev_get_addr_by_port(port_id, &addr); if (ret < 0) - return ret; + goto err; ret = rte_eth_dev_detach_pdev(port_id, &addr); - if (ret == 0) - snprintf(name, RTE_ETH_NAME_MAX_LEN, - "%04x:%02x:%02x.%d", - addr.domain, addr.bus, - addr.devid, addr.function); + if (ret < 0) + goto err; - return ret; - } else - return rte_eth_dev_detach_vdev(port_id, name); + snprintf(name, RTE_ETH_NAME_MAX_LEN, + "%04x:%02x:%02x.%d", + addr.domain, addr.bus, + addr.devid, addr.function); + } else { + ret = rte_eth_dev_detach_vdev(port_id, name); + if (ret < 0) + goto err; + } + + return 0; + +err: + RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n"); + return ret; } static int -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] ethdev: move code to common place in hotplug 2016-01-22 14:06 ` [PATCH v2 2/2] ethdev: move code to common place in hotplug David Marchand @ 2016-01-26 15:48 ` Jan Viktorin 0 siblings, 0 replies; 16+ messages in thread From: Jan Viktorin @ 2016-01-26 15:48 UTC (permalink / raw) To: David Marchand; +Cc: dev On Fri, 22 Jan 2016 15:06:58 +0100 David Marchand <david.marchand@6wind.com> wrote: > Move these error logs and checks on detach capabilities in a common place. > > Signed-off-by: David Marchand <david.marchand@6wind.com> Reviewed-by: Jan Viktorin <viktorin@rehivetech.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/2] minor cleanup in ethdev hotplug 2016-01-22 14:06 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug David Marchand 2016-01-22 14:06 ` [PATCH v2 1/2] ethdev: remove useless null checks David Marchand 2016-01-22 14:06 ` [PATCH v2 2/2] ethdev: move code to common place in hotplug David Marchand @ 2016-01-27 15:10 ` Thomas Monjalon 2 siblings, 0 replies; 16+ messages in thread From: Thomas Monjalon @ 2016-01-27 15:10 UTC (permalink / raw) To: David Marchand; +Cc: dev, viktorin 2016-01-22 15:06, David Marchand: > It was first a preparation step for future patchsets, but I am not sure what > will become of them, so sending this anyway since it does not hurt to clean > this now. > > Changes since v1: > - rebased on HEAD (previous patchset was based on another patch I sent > separately) > - restored EINVAL error code for rte_eth_dev_(at|de)tach (thanks Jan) Applied, thanks ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-01-27 15:11 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-21 11:57 [PATCH 0/2] minor cleanup in ethdev hotplug David Marchand 2016-01-21 11:57 ` [PATCH 1/2] ethdev: remove useless null checks David Marchand 2016-01-21 19:02 ` [dpdk-dev,1/2] " Jan Viktorin 2016-01-22 9:11 ` Thomas Monjalon 2016-01-21 11:57 ` [PATCH 2/2] ethdev: move code to common place in hotplug David Marchand 2016-01-21 15:38 ` [dpdk-dev, " Jan Viktorin 2016-01-21 18:06 ` David Marchand 2016-01-21 18:42 ` Thomas Monjalon 2016-01-22 7:15 ` David Marchand 2016-01-22 14:06 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug David Marchand 2016-01-22 14:06 ` [PATCH v2 1/2] ethdev: remove useless null checks David Marchand 2016-01-26 15:50 ` Jan Viktorin 2016-01-27 9:40 ` David Marchand 2016-01-22 14:06 ` [PATCH v2 2/2] ethdev: move code to common place in hotplug David Marchand 2016-01-26 15:48 ` Jan Viktorin 2016-01-27 15:10 ` [PATCH v2 0/2] minor cleanup in ethdev hotplug Thomas Monjalon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.