From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 9EE7E200 for ; Mon, 4 Dec 2017 11:31:28 +0100 (CET) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Dec 2017 02:31:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,358,1508828400"; d="scan'208";a="183739128" Received: from bricha3-mobl3.ger.corp.intel.com ([10.252.17.237]) by fmsmga006.fm.intel.com with SMTP; 04 Dec 2017 02:31:25 -0800 Received: by (sSMTP sendmail emulation); Mon, 04 Dec 2017 10:31:24 +0000 Date: Mon, 4 Dec 2017 10:31:24 +0000 From: Bruce Richardson To: Ferruh Yigit Cc: "Ananyev, Konstantin" , Thomas Monjalon , "dev@dpdk.org" , "vladz@cloudius-systems.com" Message-ID: <20171204103123.GA9832@bricha3-MOBL3.ger.corp.intel.com> References: <20171201022957.64329-1-ferruh.yigit@intel.com> <20171201022957.64329-7-ferruh.yigit@intel.com> <20171201103308.GB10384@bricha3-MOBL3.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772585FAC3718@irsmsx105.ger.corp.intel.com> <20171201131702.GA2552@bricha3-MOBL3.ger.corp.intel.com> <964acb9f-28ee-9c69-fd02-de7d3f09e429@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <964acb9f-28ee-9c69-fd02-de7d3f09e429@intel.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.1 (2017-09-22) Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Dec 2017 10:31:29 -0000 On Fri, Dec 01, 2017 at 03:51:52PM -0800, Ferruh Yigit wrote: > On 12/1/2017 5:17 AM, Bruce Richardson wrote: > > On Fri, Dec 01, 2017 at 11:22:12AM +0000, Ananyev, Konstantin wrote: > >> > >> > >>> -----Original Message----- > >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson > >>> Sent: Friday, December 1, 2017 10:33 AM > >>> To: Yigit, Ferruh > >>> Cc: Thomas Monjalon ; dev@dpdk.org; vladz@cloudius-systems.com > >>> Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object > >>> > >>> On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote: > >>>> "struct rte_eth_rxtx_callback" is defined as internal data structure but > >>>> used in public APIs. > >>>> > >>>> Checking the API documentation shows that intention was using this > >>>> object as opaque object. Data structure only used in delete APIs which > >>>> doesn't require to know the internals of the data structure. > >>>> > >>>> Converting callback parameter in API to void pointer should not require > >>>> any modification in user application because this data structure was > >>>> already marked as internal and only should be used as pointer in > >>>> application. > >>>> > >>>> Signed-off-by: Ferruh Yigit > >>>> --- > >>> > >>> I disagree on this patch. The structure itself is not exposed, only the > >>> name, since it is only passed around as a pointer, so there is no need > >>> to change the parameters to void pointer. It's a named opaque type. > >> > >> Personally I think it would be better to do visa-versa: > >> make rte_eth_add_(rx|tx)_callback() to return struct rte_eth_rxtx_callback * > >> instead of void *. > >> Konstantin > >> > > I didn't realise that it did, so definite +1 to that suggestion. > > No issue on having a named opaque type, but unfortunately struct is exposed > because of inline functions again. > It has been moved into rte_ethdev_core.h but accessible by applications. > > And since intention is an opaque type, because of "void *" return types, I > thought it is better to hide type completely so that application can't access > details. I wouldn't be worried about applications being able to get into the structure. The only compelling reason for me to make the type opaque would be for ABI compatibility, and since that is not a factor here, I don't see the point in changing it to a void *. /Bruce