[dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback

[Ferruh Yigit]

On 7/14/2021 4:42 PM, Thomas Monjalon wrote:
> 14/07/2021 16:16, Matan Azrad:
>> From: Thomas Monjalon
>>> 13/07/2021 15:42, Matan Azrad:
>>>> From: Thomas Monjalon
>>>>> When registering a new event callback, if allocation fails, there is
>>>>> no need for unregistering the callback, because it is not registered.
>>>>>
>>>>> Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all
>>>>> ports")
>>>>> Cc: stable at dpdk.org
>>>>>
>>>>> Signed-off-by: Thomas Monjalon <thomas at monjalon.net>
>>>>> ---
>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>> @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t
>>>>>                         } else {
>>>>>                                 rte_spinlock_unlock(&eth_dev_cb_lock);
>>>>> -                               rte_eth_dev_callback_unregister(port_id, event,
>>>>> -                                                               cb_fn, cb_arg);
>>>>
>>>> Please pay attention to the case of port_id=RTE_ETH_ALL where the user
>>> wants to register the event for all the ports.
>>>>
>>>> In this case, when a failure happens for one of the ports, this unregister call
>>> cleans the callback from all the ports.
>>>
>>> Yes I missed it. Now I better understand the intent, thanks.
>>>
>>> Next question: do we really want to rollback already registered ports?
>>> Anyway, if we are out of memory, I think it is better not doing more
>>> operations.
>>> There can be various opinions on this topic, please give yours.
>>
>> Sure,
>> I understand that memory error is serious,
>> Do you think it is a fatal error? If so, maybe we should use rte_exit?
> 
> We don't call rte_exit in the lib, so the app can do whatever it wants.
> 

+1

>> That way or others, I think the behavior should be a convention for all the file functions(at least).
> 
> What do you mean "all the file functions"?
> 
>> I tend to do cleanup on any error.
> 
> I would like to hear opinions from others as well.
> 

I also tend to do the cleanup, since API returns error I think application will
be right to think that no callback registered, partially registered callbacks on
error can be confusing.