Hello Maxime,
On 08/06/20 17:53 +0200, Maxime Coquelin wrote:
> This patch makes rte_dev_probe() to return the
> rte_device pointer on success and NULL on error
> instead of returning 0 on success and negative
> value on error.
>
> The goal is to avoid that the calling application
> iterates the devices list afterwards to retrieve
> the pointer. Retrieving the pointer is required
> for calling rte_dev_remove() later.
>
I agree with the idea. I recall starting to do it on the legacy functions
(rte_eal_hotplug_*), but it was scrapped for API compat.
> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
> ---
> app/test-pmd/testpmd.c | 2 +-
> drivers/net/failsafe/failsafe.c | 5 +++--
> lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++--------
> lib/librte_eal/include/rte_dev.h | 4 ++--
> 4 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4989d22ca8..f777f449a8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2764,7 +2764,7 @@ attach_port(char *identifier)
> return;
> }
>
> - if (rte_dev_probe(identifier) < 0) {
> + if (rte_dev_probe(identifier) == NULL) {
> TESTPMD_LOG(ERR, "Failed to attach port %s\n", identifier);
> return;
> }
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index 72362f35de..e32effdef2 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -341,6 +341,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
> struct rte_eth_dev *eth_dev;
> struct sub_device *sdev;
> struct rte_devargs devargs;
> + struct rte_device *dev;
> uint8_t i;
> int ret;
>
> @@ -378,8 +379,8 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
> continue;
> }
> if (!devargs_already_listed(&devargs)) {
> - ret = rte_dev_probe(devargs.name);
> - if (ret < 0) {
> + dev = rte_dev_probe(devargs.name);
> + if (dev == NULL) {
> ERROR("Failed to probe devargs %s",
> devargs.name);
> continue;
> diff --git a/lib/librte_eal/common/eal_common_dev.c
> b/lib/librte_eal/common/eal_common_dev.c
> index 9e4f09d83e..72baae2e48 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -120,7 +120,9 @@ rte_eal_hotplug_add(const char *busname, const char
> *devname,
> if (ret != 0)
> return ret;
>
> - ret = rte_dev_probe(devargs);
> + if (rte_dev_probe(devargs) == NULL)
> + ret = -1;
> +
> free(devargs);
>
> return ret;
> @@ -192,7 +194,7 @@ local_dev_probe(const char *devargs, struct rte_device
> **new_dev)
> return ret;
> }
>
> -int
> +struct rte_device *
> rte_dev_probe(const char *devargs)
> {
> struct eal_dev_mp_req req;
> @@ -212,12 +214,12 @@ rte_dev_probe(const char *devargs)
> if (ret != 0) {
> RTE_LOG(ERR, EAL,
> "Failed to send hotplug request to primary\n");
> - return -ENOMSG;
> + return NULL;
Is is a problem to keep the ENOMSG through rte_errno?
> }
> if (req.result != 0)
> RTE_LOG(ERR, EAL,
> "Failed to hotplug add device\n");
> - return req.result;
> + return NULL;
> }
>
> /* attach a shared device from primary start from here: */
> @@ -236,7 +238,7 @@ rte_dev_probe(const char *devargs)
> * process.
> */
> if (ret != -EEXIST)
> - return ret;
> + return dev;
> }
>
> /* primary send attach sync request to secondary. */
> @@ -261,11 +263,11 @@ rte_dev_probe(const char *devargs)
>
> /* for -EEXIST, we don't need to rollback. */
> if (ret == -EEXIST)
> - return ret;
> + return dev;
> goto rollback;
> }
>
> - return 0;
> + return dev;
>
> rollback:
> req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK;
> @@ -282,7 +284,7 @@ rte_dev_probe(const char *devargs)
> "Failed to rollback device attach on primary."
> "Devices in secondary may not sync with primary\n");
>
> - return ret;
> + return NULL;
> }
>
> int
> diff --git a/lib/librte_eal/include/rte_dev.h
> b/lib/librte_eal/include/rte_dev.h
> index c8d985fb5c..9cf7c7fd71 100644
> --- a/lib/librte_eal/include/rte_dev.h
> +++ b/lib/librte_eal/include/rte_dev.h
> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char
> *devname,
> * @param devargs
> * Device arguments including bus, class and driver properties.
> * @return
> - * 0 on success, negative on error.
> + * Generic device pointer on success, NULL on error.
If rte_errno is set, mapping codes to meanings would be helpful here.
Acked-by: Gaetan Rivet <gr...@u256.net>
--
Gaëtan
