From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [PATCH v4 2/5] cfgfile: change existing API functions
Date: Wed, 30 Aug 2017 21:07:26 +0100
Message-ID: <20170830200726.GB1656@bricha3-MOBL3.ger.corp.intel.com>
References: <1498560760-104196-2-git-send-email-jacekx.piasecki@intel.com>
 <1499690657-81150-1-git-send-email-jacekx.piasecki@intel.com>
 <1499690657-81150-3-git-send-email-jacekx.piasecki@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: dev@dpdk.org, deepak.k.jain@intel.com, kubax.kozak@intel.com,
 michalx.k.jastrzebski@intel.com
To: Jacek Piasecki <jacekx.piasecki@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by dpdk.org (Postfix) with ESMTP id 350F02BAF
 for <dev@dpdk.org>; Wed, 30 Aug 2017 22:07:32 +0200 (CEST)
Content-Disposition: inline
In-Reply-To: <1499690657-81150-3-git-send-email-jacekx.piasecki@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

I think the commit title needs rewording. This changes the internals not
the API.

On Mon, Jul 10, 2017 at 02:44:14PM +0200, Jacek Piasecki wrote:
> Change to flat arrays in cfgfile struct force slightly
> diffrent data access for most of cfgfile functions.
> This patch provides necessary changes in existing API.
> 
> Signed-off-by: Jacek Piasecki <jacekx.piasecki@intel.com>
> ---

Some comments below. I believe the change in return value from -1 to
-EINVAL, though a more correct error, actually counts as an ABI change,
so I think that should be removed, i.e. keep errors at -1. Once done:

Acked-by: Bruce Richardon <bruce.richardson@intel.com>

>  lib/librte_cfgfile/rte_cfgfile.c | 120 +++++++++++++++++++--------------------
>  lib/librte_cfgfile/rte_cfgfile.h |   6 +-
>  2 files changed, 62 insertions(+), 64 deletions(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index c6ae3e3..50fe37a 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -35,6 +35,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <ctype.h>
> +#include <errno.h>
>  #include <rte_common.h>
>  
>  #include "rte_cfgfile.h"
> @@ -42,13 +43,15 @@
>  struct rte_cfgfile_section {
>  	char name[CFG_NAME_LEN];
>  	int num_entries;
> -	struct rte_cfgfile_entry *entries[0];
> +	int allocated_entries;
> +	struct rte_cfgfile_entry *entries;
>  };
>  
>  struct rte_cfgfile {
>  	int flags;
>  	int num_sections;
> -	struct rte_cfgfile_section *sections[0];
> +	int allocated_sections;
> +	struct rte_cfgfile_section *sections;
>  };
> 

These are good changes, allowing us to have the sections array and
entries arrays separate from the basic data structures.

<snip>
> @@ -409,7 +407,7 @@ rte_cfgfile_section_num_entries(struct rte_cfgfile *cfg,
>  {
>  	const struct rte_cfgfile_section *s = _get_section(cfg, sectionname);
>  	if (s == NULL)
> -		return -1;
> +		return -EINVAL;
>  	return s->num_entries;
>  }

I think this change should be dropped for backward compatibility, or
else put in NEXT_ABI #ifdefs and an ABI notice added to the RN.