DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	Bruce Richardson <bruce.richardson@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] devtools: skip the symbol check when map file under drivers
Date: Thu, 23 May 2019 16:17:09 -0400	[thread overview]
Message-ID: <20190523201709.GB18326@hmswarspite.think-freely.org> (raw)
In-Reply-To: <9743169.QmDmBymccE@xps>

On Thu, May 23, 2019 at 08:59:07PM +0200, Thomas Monjalon wrote:
> 23/05/2019 19:57, Neil Horman:
> > On Thu, May 23, 2019 at 02:21:29PM +0000, Jerin Jacob Kollanukkaran wrote:
> > > From: Neil Horman <nhorman@tuxdriver.com>
> > > > > > > > > IMO, The name prefix matters. The rte_* should denote it a
> > > > > > > > > DPDK API and application suppose to use it.
> > > > > > > > >
> > > > > > > > It doesn't, its just a convention.  We have no documentation
> > > > > > > > that indicates what the meaning of an rte_* prefix is
> > > > > > > > specficially, above and beyond the fact thats how we name
> > > > > > > > functions in the DPDK.  If you want to submit a patch to
> > > > > > > > formalize the meaning of function prefixes, you're welcome too
> > > > > > > > (though I won't support it, perhaps others will).  But even if
> > > > > > > > you do, it doesn't address the underlying problem, which is that
> > > > applications still have access to those symbols.
> > > > > > > > Maintaining an ABI by assertion of prefix is really a lousy way
> > > > > > > > to communicate what functions should be accessed by an
> > > > > > > > application and which shouldn't.  If a function is exported, and
> > > > > > > > included in the header file, people will try to use
> > > > > > >
> > > > > > > The current scheme in the driver/common is that, the header files
> > > > > > > are NOT made It as public ie not installed make install.
> > > > > > > The consumer driver includes that using relative path wrt DPDK
> > > > > > > source
> > > > > > directory.
> > > > > > >
> > > > > > Well, thats a step in the right direction.  I'd still like to see
> > > > > > some enforcement to prevent the inadvertent use of those APIs though
> > > > >
> > > > > Yes header file  is  not exported. Not sure how a client can use those.
> > > > > Other than doing some hacking.
> > > > >
> > > > Yes, self prototyping the exported functions would be a way around that.
> > > > > >
> > > > > > > Anyway I will add experimental section to make tool happy.
> > > > > > >
> > > > > > That really not the right solution.  Marking them as experimental is
> > > > > > just papering over the problem, and suggests to users that they will
> > > > > > one day be stable.
> > > > >
> > > > > That what my original concern.
> > > > >
> > > > > > What you want is to explicitly mark those symbols as internal only,
> > > > > > so that any inadvertent use gets flagged.
> > > > >
> > > > > What is your final thought? I can assume the following for my patch
> > > > > generation
> > > > >
> > > > > # No need to mark as experimental
> > > > > # Add @internal to denote it is a internal function like followed some places
> > > > in EAL.
> > > > >
> > > > These are both correct, yes.
> > > > 
> > > > In addition, I would like to see some mechanism that explicitly marks the
> > > > function as exported only for the purposes of internal use.  I understand that
> > > > yours is a case in which this is not expressly needed because you don't
> > > > prototype those functions, but what I'd like to see is a macro in rte_compat.h
> > > > somewhere like this:
> > > > 
> > > > #define INTERNAL_USE_ONLY do {static_assert(0, "Function is only available
> > > > for internal DPDK usage");} while(0)
> > > > 
> > > > so that, in your exported header file (of which I'm sure you have one, even if
> > > > it doesn't contain your private functions, you can do something like this:
> > > > 
> > > > #ifdef BUILDING_RTE_SDK
> > > > void somefunc(int val);
> > > > #else
> > > > #define somefunc(x) INTERNAL_USE_ONLY
> > > > #endif
> > > 
> > > I think, We have two cases
> > > 1) Internal functions are NOT available via  DPDK SDK exported header files
> > > 2) Internal functions are available via DPDK SDK exported header files
> > > 
> > > I think, you are trying to address case 2( as case 1 is not applicable in this context due lack of header file)
> > > For case 2, IMO, the above scheme will not be enough as 
> > > The consumer entity can simply add the exact C flags to skip that check in this case, -DBUILDING_RTE_SDK.
> > > IMO, it would be correct remove private functions from public header files. No strong options on this.
> > >  
> > 
> > I'm thinking about it a bit differently.  Internal functions should never be
> > available to user, weather they are prototyped in DPDK header files or not.
> > Unfortunately, because of how library symbol exports work, there is no way to
> > differentiate between which exported functions are internal or external, they
> > are only exported or not, and as such, they are always resolveable by someone
> > linking against them (regardless of which hackery is used to achieve that
> > result).  I'd like a way to prevent users who are only using the SDK (not
> > building it) from accessing those symbols, and the above is the best solution I
> > can come up with.  I admit its not great, but it does place a roadblock in the
> > way of users who attempt to use symbols we don't want to give them access to.
> > And yes its circumventable by defining BUILDING_RTE_SDK, but I would think its
> > clear that they are not building the SDK, and so they should not be doing that.
> > 
> > Just not exporting the requisite header files is an easier solution, so if thats
> > the consensus I can be ok with that, but I would really love to have a way to
> > document in the code those functions which are not meant for external
> > consumption.
> 
> I think there are good ideas here.
> Please come with a patch and we'll try to apply the chosen policy
> to the existing code.
> 
Ok, I'll give it a shot
Neil

> 
> 

  reply	other threads:[~2019-05-23 20:17 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-23 14:21 Jerin Jacob Kollanukkaran
2019-05-23 17:57 ` Neil Horman
2019-05-23 18:59   ` Thomas Monjalon
2019-05-23 20:17     ` Neil Horman [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-05-22 13:41 Jerin Jacob Kollanukkaran
2019-05-22 14:11 ` Neil Horman
2019-05-22 13:12 Jerin Jacob Kollanukkaran
2019-05-22 13:40 ` Neil Horman
2019-05-22 14:12   ` Thomas Monjalon
2019-05-22 14:33     ` Neil Horman
2019-05-22 11:54 Jerin Jacob Kollanukkaran
2019-05-22 13:13 ` Neil Horman
2019-05-21 19:56 jerinj
2019-05-21 20:27 ` Neil Horman

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=20190523201709.GB18326@hmswarspite.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    --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).