DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: David Marchand <david.marchand@redhat.com>, dev <dev@dpdk.org>,
	dpdk stable <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] eal: fix version macro
Date: Thu, 18 Mar 2021 15:45:06 +0000	[thread overview]
Message-ID: <20210318154506.GD1633@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <2329735.2CMtTcrZrs@thomas>

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

  reply	other threads:[~2021-03-18 15:45 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-17  9:31 Thomas Monjalon
2021-03-17  9:48 ` David Marchand
2021-03-17 10:01   ` Thomas Monjalon
2021-03-18 12:28     ` Bruce Richardson
2021-03-18 14:41       ` Thomas Monjalon
2021-03-18 15:45         ` Bruce Richardson [this message]
2021-03-17 15:36 ` David Marchand
2021-03-17 15:39   ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210318154506.GD1633@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).