Thread (132 messages) 132 messages, 13 authors, 2021-04-29

Re: [dpdk-dev] [PATCH v2 1/7] eal: add wrappers for POSIX string functions

From: Thomas Monjalon <hidden>
Date: 2021-03-16 09:51:25

27/02/2021 21:23, Dmitry Kozlyuk:
2021-02-23 09:45, Bruce Richardson:
quoted
On Tue, Feb 23, 2021 at 01:57:50AM +0300, Dmitry Kozlyuk wrote:
quoted
2021-02-22 14:26, Bruce Richardson:  
quoted
As you say, though, the main issue will be whether we have instances in
public header files or not. I would hope that no static inline functions in
DPDK use any of the functions in question, but I'm not sure. Perhaps if
there are instances in public headers those could be reworked to not use
the problematic functions.  
No instances of strdup(), strncasecmp(), or strtok_r() in any DPDK headers.
  
quoted
For any functions, such as strdup, which are not in a public header I would
suggest the following as a possible start point, based off what was done
for strlcpy.

* In DPDK (probably EAL), define an rte_strdup function for use as a
  fallback.
* Inside the meson build scripts, use "cc.has_function()" to check if the
  regular strdup function is available. If not, then add "-DRTE_NO_STRDUP"
  to the c_args for DPDK building
* Inside our DPDK header (rte_string_fns.h in the strdup case), we can add
  a conditional define such as:
   #ifdef RTE_NO_STRDUP
   #define strdup(s) rte_strdup(s)
   #endif

Thoughts on this?  
Looks good to me, I can rework the patchset like so.

Policy considerations:
1. The approach only applies to platform-agnostic functions, like str*().
   Functions like sleep() still belong to librte_eal.
2. Deprecated functions, like index(3p), should be replaced
   with alternatives suggested by the standard.
3. If a standard C11 alternative is available, it should be used.
   This mostly applies to types, like u_int32 -> uint32_t
   (it's even in DPDK coding style already, isn't it?).

A nit: RTE_NO_XXX -> RTE_HAS_XXX (for consistency with existing macros)?  
Sure, thanks.
There's a meson issue with `cc.has_function()`:
https://github.com/mesonbuild/meson/issues/5628
The meson issue can be fixed or workarounded probably.
Is it the reason for the RTE_INTERNAL proposal below?
What if we just define RTE_INTERNAL for librte_eal/windows/include/rte_os.h
(and other public headers if need be) to distinguish the case when it's used
from within DPDK?
I'm not sure to follow the need for RTE_INTERNAL.

In general, 3 guidelines:
	- avoid inline functions in public headers
	- mark exported internal functions with __rte_internal and in version.map
	- export internal functions in a separate file

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help