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

[Thomas Monjalon <thomas@monjalon.net>]

This patch is abandoned. Current behaviour is kept.


15/07/2021 11:06, 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@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Thomas Monjalon <thomas@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.