Re: [PATCH v5 1/3] lib: introduce dispatcher library

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2023-10-11 16:57, David Marchand wrote:
> On Mon, Oct 9, 2023 at 6:50 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 
> [snip]
> 
>>>>>> +static int
>>>>>> +evd_set_service_runstate(struct rte_dispatcher *dispatcher, int state)
>>>>>> +{
>>>>>> +       int rc;
>>>>>> +
>>>>>> +       rc = rte_service_component_runstate_set(dispatcher->service_id,
>>>>>> +                                               state);
>>>>>> +
>>>>>> +       if (rc != 0) {
>>>>>> +               RTE_EDEV_LOG_ERR("Unexpected error %d occurred while setting "
>>>>>> +                                "service component run state to %d\n", rc,
>>>>>> +                                state);
>>>>>> +               RTE_ASSERT(0);
>>>>>
>>>>> Why not propagating the error to callers?
>>>>>
>>>>>
>>>>
>>>> The root cause would be a programming error, hence an assertion is more
>>>> appropriate way to deal with the situation.
>>>
>>> Without building RTE_ENABLE_ASSERT (disabled by default), the code
>>> later in this function will still be executed.
>>>
>>
>> If RTE_ASSERT() is not the way to assure a consistent internal library
>> state, what is? RTE_VERIFY()?
> 
> The usual way in DPDK is to use RTE_VERIFY or rte_panic with the error message.
> There is also libc assert().
> 
> RTE_ASSERT is more of a debug macro since it is under a build option.
> 
> 
> But by making the library "panic" on some assertion, I have followup comments:
> - what is the point of returning an int for rte_dispatcher_start() /
> rte_dispatcher_stop()?
> - rte_dispatcher_start() and rte_dispatcher_stop() (doxygen)
> documentation needs updating, as they can't return anything but 0.
> 
> 

Those return vales are purely there for reasons of symmetry, or maybe 
you can call it API consistent, with Eventdev APIs (e.g., the event 
ethernet RX adapter). I guess that's less of an issue now, when it's a 
separate library, but still relevant, since the programmer will be 
familiar with those APIs.

You could also argue that in the future, there may be errors which needs 
to be signaled to the caller. But that's a weak argument, since we don't 
know exactly what those would be (and thus they can't be documented), so 
it's still an API/ABI change, even though the function signature doesn't 
change.

Now I'm leaning toward removing the return value. Please advise. Over.