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

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

On 2023-10-06 11:03, Thomas Monjalon wrote:
> 06/10/2023 10:46, David Marchand:
>> On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>>>> +static int
>>>>> +evd_lookup_handler_idx(struct rte_dispatcher_lcore *lcore,
>>>>> +                      const struct rte_event *event)
>>>>
>>>> Wrt DPDK coding tyle, indent is a single tab.
>>>> Adding an extra tab is recommended when continuing control statements
>>>> like if()/for()/..
>>>>
>>>
>>> Sure, but I don't understand why you mention this.
>>
>> I wanted to remind the DPDK coding style which I try to more closely
>> enforce for new code.
>> indent is off in this file (especially for function prototypes with
>> multiple tabs used).
>>
>>>
>>>> On the other hand, max accepted length for a line is 100 columns.
>>>>
>>>> Wdyt of a single line for this specific case?
>>>
>>>
>>> Are you asking why the evd_lookup_handler_idx() function prototype is
>>> not a single line?
>>>
>>> It would make it long, that's why. Even if 100 wide lines are allowed,
>>> it doesn't means the author is forced to use such long lines?
>>
>> I find it more readable.
>> If you want to stick to 80 columns, please comply with a single tab for indent.
> 
> I think this is a case of continuation line, so it should be 2 tabs.
> We can make it clear in the doc.
> 
> 
>>>>> +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.
> 
> Don't forget you are writing a library, so you shouldn't stop runtime
> because of an error. It is always better to return errors.
> Assert should be used only for debugging, that's why it is disabled by default.
> 
> 
> 

A breach of the API contract should always be met with an assertion - 
library or not. That is the most helpful thing you can do, so that one 
fails early and programmer can go fix the bug.

The use of EINVAL and attempts to return error in the face of API 
violations is a bad practice, in my opinion. What could the program 
possible do, if it learns it's using a function the wrong way? If the 
two memory areas in memmove() overlap, should there be a error code, so 
that caller can retry but "please not with overlapping memory regions 
this time".

I know that the EINVAL pattern is wide-spread practice in DPDK, at the 
cost of performance and complexity, I would argue. I guess this practice 
originates from the kernel/libc tradition of validating system calls (a 
place where this can be actually be done in a reliable manner, unlike in 
normal user code).