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.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(-) > >> > ... > >> > >> 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 <gr...@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
