Re: [dpdk-dev] [EXT] [RFC PATCH 0/2] introduce __rte_internal tag

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jun 07, 2019 at 09:41:07AM +0000, Jerin Jacob Kollanukkaran wrote:
> > -----Original Message-----
> > From: Neil Horman <nhorman@tuxdriver.com>
> > Sent: Thursday, June 6, 2019 10:26 PM
> > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> > Cc: Bruce Richardson <bruce.richardson@intel.com>; dev@dpdk.org;
> > Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag
> > 
> > On Thu, Jun 06, 2019 at 04:23:26PM +0000, Jerin Jacob Kollanukkaran wrote:
> > > > -----Original Message-----
> > > > From: Neil Horman <nhorman@tuxdriver.com>
> > > > Sent: Thursday, June 6, 2019 8:57 PM
> > > > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> > > > Cc: Bruce Richardson <bruce.richardson@intel.com>; dev@dpdk.org;
> > > > Thomas Monjalon <thomas@monjalon.net>
> > > > Subject: Re: [EXT] [RFC PATCH 0/2] introduce __rte_internal tag
> > > >
> > > > On Thu, Jun 06, 2019 at 03:14:42PM +0000, Jerin Jacob Kollanukkaran
> > wrote:
> > > > ><snip as this is getting long>
> > > > >
> > > > > I don't have any strong opinion on name prefix vs marking as
> > > > __rte_internal.
> > > > > Or combination of both. I am fine any approach.
> > > > >
> > > > > I have only strong option on not to  induce objdump dependency for
> > > > checkpatch.
> > > > > For the reason mentioned in
> > > > > http://mails.dpdk.org/archives/dev/2019-
> > > > June/134160.html.
> > > > >
> > > >
> > > > Sorry, in my haste I didn't fully adress this in your previous email
> > > >
> > > > I'm really uncertain what you mean by introducing a checkpatch
> > > > dependency on objdump here.  Theres nothing preventing you from
> > > > running checkpatch before you build the library.  The only thing
> > > > checkpatch does in dpdk is scan the patches for sytle violations,
> > > > and for changes in the map file for movement to and from the
> > EXPERIMENTAL section (i.e. no use of objdump).
> > > >
> > > > My patch modifies check-experimental-syms.sh (adding an objdump scan
> > > > for INTERNAL symbols, and renaming the script to
> > > > check-special-syms.sh to be more meaningful).  That script however,
> > > > is not run by checkpatch, its run during compilation of the library
> > > > to ensure that any symbol in a map file is also tagged with
> > > > __rte_internal in the corresponding object).  Theres no path from
> > > > checkpatches to check-experimental-syms.sh
> > > >
> > > > What I meant in my last comment was that any dependency on objdump
> > > > in check-[experimental|special]-syms.sh already existed prior to this
> > patch.
> > >
> > > I see. I thought your patches addressing issue related to
> > > http://patches.dpdk.org/patch/53590/
> > >
> > It does, it just does it in a different way than you do it.
> > 
> > > Where checkpatch.sh complaints when we add new internal driver API
> > > with out rte_experimental? That's where all the discussion started.
> > > The reason why I was saying the API name prefix to mark as internal
> > > API is that checkpatch can detect that case.
> > >
> > Ah, thats a fair point, with my approach we need to add some code to check-
> > symbol-change.sh such that any symbols listed in an INTERNAL section were
> > ignored.  That would provide behavioral compatibility to what you were
> > doing in your patch.
> 
> OK.
> 
> > But regardless, there is no dependency on objdump that wasn't there
> > before, are we in agreement on that?
> 
> Yes.
> 
ok, let me revise this patch with that change in place.  I'll also complete
tagging of the affected internal functions so we have a better idea of what this
change entails

Neil