Re: [dpdk-dev] [PATCH] eal: fix version macro

[Thomas Monjalon <thomas@monjalon.net>]

18/03/2021 13:28, Bruce Richardson:
> On Wed, Mar 17, 2021 at 11:01:25AM +0100, Thomas Monjalon wrote:
> > 17/03/2021 10:48, David Marchand:
> > > On Wed, Mar 17, 2021 at 10:31 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > The macro RTE_VERSION is broken since updated with function calls.
> > > > It is a build-time version number, and must be built with macros.
> > > > For a run-time version number, there is the function rte_version().
> > > >
> > > > Fixes: 5b637a848195 ("eal: fix querying DPDK version at runtime")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Reported-by: David Marchand <david.marchand@redhat.com>
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > ---
> > > >  lib/librte_eal/include/rte_version.h | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/librte_eal/include/rte_version.h b/lib/librte_eal/include/rte_version.h
> > > > index 2f3f727b46..736c5703be 100644
> > > > --- a/lib/librte_eal/include/rte_version.h
> > > > +++ b/lib/librte_eal/include/rte_version.h
> > > > @@ -28,10 +28,10 @@ extern "C" {
> > > >   * All version numbers in one to compare with RTE_VERSION_NUM()
> > > >   */
> > > >  #define RTE_VERSION RTE_VERSION_NUM( \
> > > > -                       rte_version_year(), \
> > > > -                       rte_version_month(), \
> > > > -                       rte_version_minor(), \
> > > > -                       rte_version_release())
> > > > +                       RTE_VER_YEAR, \
> > > > +                       RTE_VER_MONTH, \
> > > > +                       RTE_VER_MINOR, \
> > > > +                       RTE_VER_RELEASE)
> > > >
> > > >  /**
> > > >   * Function to return DPDK version prefix string
> > > 
> > > The original patch wanted to fix rte_version() at runtime.
> > > I don't see the need to keep the rte_version_XXX exports now that
> > > RTE_VERSION is reverted.
> > 
> > I think it may help to query the version numbers at runtime,
> > in "if" condition. Is there another way I'm missing?
> > We may argue that the runtime version number should not be used
> > to decide how to behave in an application.
> > 
> I would also tend toward keeping them, for the same reason that runtime is
> definitely to be preferred over build time, and they are not like to be
> much of a maintenance burden.
> 
> Also, next time we have an ABI break, I wonder if the existing macros
> should be renamed to have an RTE_BUILD_VER_ prefix, to make it clear that
> it's the build version only that is being reported rather than the version
> actually being used. Similarly the functions could be renamed to have
> rte_runtime_ prefix, ensuring that in all cases the user is clear whether
> they are getting the build version or the runtime version.

I am fine with such rename,
but that's already quite clear that a macro is at build time,
and a function is usually evaluated at runtime.