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

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Mar 18, 2021 at 03:41:35PM +0100, Thomas Monjalon wrote:
> 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.
> 

If we take the existing rte_version function, without checking the source
code, one has no way of checking if that is resolved at runtime (as it is
now) or at compile-time (as it was). However, if we assume that that is a
bug and that all such functions should be run-time operations, then
there is no difficulty.

/Bruce