[PATCH v6 08/27] eal: deprecate RTE_FUNC_PTR_* macros

[Morten Brørup]

> From: David Marchand [mailto:david.marchand at redhat.com]
> Sent: Wednesday, 26 October 2022 11.21
> 
> On Wed, Oct 26, 2022 at 11:05 AM Morten Brørup
> <mb at smartsharesystems.com> wrote:
> >
> > > From: David Marchand [mailto:david.marchand at redhat.com]
> > > Sent: Wednesday, 14 September 2022 09.58
> > >
> > > Those macros have no real value and are easily replaced with a
> simple
> > > if() block.
> > >
> > > Existing users have been converted using a new cocci script.
> > > Deprecate them.
> > >
> > > Signed-off-by: David Marchand <david.marchand at redhat.com>
> > > ---
> >
> > [...]
> >
> > >  /* Macros to check for invalid function pointers */
> > > -#define RTE_FUNC_PTR_OR_ERR_RET(func, retval) do { \
> > > +#define RTE_FUNC_PTR_OR_ERR_RET(func, retval)
> > > RTE_DEPRECATED(RTE_FUNC_PTR_OR_ERR_RET) \
> > > +do { \
> > >       if ((func) == NULL) \
> > >               return retval; \
> > >  } while (0)
> > >
> > > -#define RTE_FUNC_PTR_OR_RET(func) do { \
> > > +#define RTE_FUNC_PTR_OR_RET(func)
> RTE_DEPRECATED(RTE_FUNC_PTR_OR_RET)
> > > \
> > > +do { \
> > >       if ((func) == NULL) \
> > >               return; \
> > >  } while (0)
> >
> > rte_ethdev.h has somewhat similar macros [1]:
> >
> > /* Macros to check for valid port */
> > #define RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, retval) do { \
> >         if (!rte_eth_dev_is_valid_port(port_id)) { \
> >                 RTE_ETHDEV_LOG(ERR, "Invalid port_id=%u\n", port_id);
> \
> >                 return retval; \
> >         } \
> > } while (0)
> >
> > #define RTE_ETH_VALID_PORTID_OR_RET(port_id) do { \
> >         if (!rte_eth_dev_is_valid_port(port_id)) { \
> >                 RTE_ETHDEV_LOG(ERR, "Invalid port_id=%u\n", port_id);
> \
> >                 return; \
> >         } \
> > } while (0)
> >
> > However, these log an error message. It makes me wonder about
> consistency...
> >
> > Are the RTE_FUNC_PTR_OR_* macros a special case, or should similar
> macros, such as RTE_ETH_VALID_PORTID_OR_*, also be deprecated?
> >
> > Or should the RTE_FUNC_PTR_OR_* macros be modified to log an error
> message instead of being deprecated?
> 
> The difference is that those ethdev macros validate something
> expressed in their name "is this portid valid".
> 
> The RTE_FUNC_PTR_OR_* macros were really just a if() + return block.
> I don't see what message to log for them.

The RTE_ETH_VALID_PORTID_OR_* macros seem to serve the same purpose as the RTE_FUNC_PTR_OR_* macros.

Is it just a coincidence that the RTE_FUNC_PTR_OR_* macros don't log an error message, while similar macros do?

Cleaning up is certainly good, so I don't object to this patch!

I'm only asking if a broader scope applies:

Are the RTE_FUNC_PTR_OR_* macros a unique case (so they can be removed as suggested), or should we add an error message to the macros instead of removing them?