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: sta...@dpdk.org
>>>>>
>>>>> Signed-off-by: Thomas Monjalon <tho...@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(ð_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.
