Re: [dpdk-dev] [PATCH v4 02/18] telemetry: move code to metrics for later reuse

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

On Fri, Apr 24, 2020 at 08:29:53AM -0700, Stephen Hemminger wrote:
> On Fri, 24 Apr 2020 13:41:43 +0100
> Ciara Power <ciara.power@intel.com> wrote:
> 
> > This commit moves some of the telemetry library code to a new file in
> > the metrics library. No modifications are made to the moved code,
> > except what is needed to allow it to compile and run. The additional
> > code in metrics is built only when the Jansson library is  present.
> > Telemetry functions as normal, using the functions from the
> > metrics_telemetry file. This move will enable code be reused by the new
> > version of telemetry in a later commit, to support backward
> > compatibility with the existing telemetry usage.
> > 
> > Signed-off-by: Ciara Power <ciara.power@intel.com>
> 
> 
> Minor comments, none of these are show stoppers.
> 
> 
> > diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
> > index df5e32c59f..9b38d7787c 100644
> > --- a/lib/librte_metrics/rte_metrics.c
> > +++ b/lib/librte_metrics/rte_metrics.c
> > @@ -13,7 +13,6 @@
> >  #include <rte_memzone.h>
> >  #include <rte_spinlock.h>
> >  
> > -#define RTE_METRICS_MAX_METRICS 256
> >  #define RTE_METRICS_MEMZONE_NAME "RTE_METRICS"
> >  
> >  /**
> > diff --git a/lib/librte_metrics/rte_metrics.h b/lib/librte_metrics/rte_metrics.h
> > index 77bffe08e4..466ca98c31 100644
> > --- a/lib/librte_metrics/rte_metrics.h
> > +++ b/lib/librte_metrics/rte_metrics.h
> > @@ -32,6 +32,7 @@ extern "C" {
> >  
> >  /** Maximum length of metric name (including null-terminator) */
> >  #define RTE_METRICS_MAX_NAME_LEN 64
> > +#define RTE_METRICS_MAX_METRICS 256
> 
> Exposing max metrics to API/ABI does limit you in the future.
> 
I'm not sure moving the definition limits us any more than before. So long
as the structures which are defined based on this definition are internal
and only accessed via API functions, there should be no problems with
changing this and using function versioning for compatibility.

/Bruce