Re: [dpdk-dev] [PATCH v20.11 2/2] eal: improve device probing API

["Gaëtan Rivet" <grive@u256.net>]

On 10/06/20 19:08 +0200, Maxime Coquelin wrote:
> Hi Gaëtan,
> 
> On 6/10/20 2:06 PM, Gaëtan Rivet wrote:
> > 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.coquelin@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(-)
> >>
> ...
> >>  
> >>  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.
> 
> Actually David made me the same comment before I post the patch.
> I am fine with setting rte_errno. If we do that, I think we should have
> fixed error code in rte_dev_probe() and not propagate error codes from
> functions it calls. Otherwise it's likely the API doc will be outdated
> quite rapidly.
> 
> What do you think?
> 

Well we're stuck with the classic errno limitations.

I agree with you, if we consider possible errors as part of a function
API, then we cannot recursively inherit this API from callees.

That being said, masking errors is not ok. If an error cannot be handled
by rte_dev_probe(), it should log an appropriate message and set
rte_errno to a value that is part of its API. If the error can be
handled, then errno should be reset to its original value preceding
rte_dev_probe() call.

Of course the EAL rarely does it, and even myself I probably rarely
respected this behavior, but it would be nice if we could all define a
common agreed-upon discipline in the EAL and stick to it. I think the
current state of error reporting in EAL is terrible for users downstream.

> > Acked-by: Gaetan Rivet <grive@u256.net>
> > 
> 
> Great, could you also ack the deprecation notice, as this is the part
> that needs to be merged into v20.08?
> 

I wanted to refresh myself with the latest rules about API breakage
before doing so but got context switched away :) . I will get back to it.

> Thanks!
> Maxime
> 

-- 
Gaëtan