[v2,01/13] security: fix verification of parameters

  This patch adds verification of the parameters to the ret_security API
functions. All required parameters are checked if they are not NULL.

Checks verify full chain of pointers, e.g. in case of verification of
"instance->ops->session_XXX", they check also "instance"
and "instance->ops".

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Fixes: 1a08c379b9b5 ("security: support user data retrieval")
Cc: anoob.joseph@caviumnetworks.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 config/common_base                 |  1 +
 lib/librte_security/rte_security.c | 59 +++++++++++++++++++++++-------
 2 files changed, 47 insertions(+), 13 deletions(-)
  

08/04/2020 05:13, Lukasz Wojciechowski:
> This patch adds verification of the parameters to the ret_security API
> functions. All required parameters are checked if they are not NULL.
[...]
> --- a/config/common_base
> +++ b/config/common_base
>  CONFIG_RTE_LIBRTE_SECURITY=y
> +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n

Is it a leftover?
  

Hi Thomas,

> 08/04/2020 05:13, Lukasz Wojciechowski:
> > This patch adds verification of the parameters to the ret_security API
> > functions. All required parameters are checked if they are not NULL.
> [...]
> > --- a/config/common_base
> > +++ b/config/common_base
> >  CONFIG_RTE_LIBRTE_SECURITY=y
> > +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
> 
> Is it a leftover?
> 

[Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in datapath. Like in, http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4378

Thanks,
Anoob
  

08/04/2020 15:02, Anoob Joseph:
> Hi Thomas,
> 
> > 08/04/2020 05:13, Lukasz Wojciechowski:
> > > This patch adds verification of the parameters to the ret_security API
> > > functions. All required parameters are checked if they are not NULL.
> > [...]
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > >  CONFIG_RTE_LIBRTE_SECURITY=y
> > > +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
> > 
> > Is it a leftover?
> > 
> 
> [Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in datapath. Like in, http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4378

1/ I don't see it used in this patch
2/ Adding makefile-only option is weird
3/ Adding new compile-time options is discouraged
  

Hi Thomas,

> 
> ----------------------------------------------------------------------
> 08/04/2020 15:02, Anoob Joseph:
> > Hi Thomas,
> >
> > > 08/04/2020 05:13, Lukasz Wojciechowski:
> > > > This patch adds verification of the parameters to the ret_security
> > > > API functions. All required parameters are checked if they are not NULL.
> > > [...]
> > > > --- a/config/common_base
> > > > +++ b/config/common_base
> > > >  CONFIG_RTE_LIBRTE_SECURITY=y
> > > > +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
> > >
> > > Is it a leftover?
> > >
> >
> > [Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in
> > datapath. Like in,
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_dpdk
> > _latest_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4378&d=DwICAg&c=n
> > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=
> >
> STCBgRhcnCb9M6MWQL9CUszLwy2r0NJ_3m93_D5UX3g&s=HVsD0LKZ2Q6UCW
> BSRvbw9beD
> > 7OtuQyWPrRrx9eofnz8&e=
> 
> 1/ I don't see it used in this patch

[Anoob] Following snippet uses.

+#ifdef RTE_LIBRTE_SECURITY_DEBUG
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
+			-ENOTSUP);
+	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
+#endif

> 2/ Adding makefile-only option is weird
> 3/ Adding new compile-time options is discouraged

[Anoob] This is only introduced for data path APIs. And the same approach is followed in eth dev as well.

Thanks,
Anoob
  

Hi guys,

I don't know what is the current status of "legacy" build using 
gnumakes, so I added the new DEBUG flag to config just as it was done in 
other libs like eventdev.
Many guides still point config files as the one that should be changed 
in order to enable some features, so I thought I should add it there.

If I understand well the official build system now is the one based on 
using meson and ninja, however it hasn't got anything similar to the 
gnamakefiles system, e.g.
in the meson.build file for libraries all the libraries have build 
variable set to true and there are few ifs that check it, but as it's 
set to true all libraries build always.
And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
It's kind of weird.

    foreach l:libraries
    *    build = true**
    *    reason = '<unknown reason>' # set if build == false to explain why
         ...
    *    if not build*
             dpdk_libs_disabled += name
             set_variable(name.underscorify() + '_disable_reason', reason)
         else
             enabled_libs += name
    *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
         ...

Have you think about reusing config files in meson configuration and 
have a single point of configuration? Of course all meson flags can 
overwrite the default config.

Best regards
Lukasz


BTW
I got still some warnings from linters when processing Fixes flags. I 
used the fixline alias recommended on dpdk build manual, but the 
automated linter does not find the commits I mention there. What have I 
done wrong?


W dniu 08.04.2020 o 16:44, Anoob Joseph pisze:
> Hi Thomas,
>
>> ----------------------------------------------------------------------
>> 08/04/2020 15:02, Anoob Joseph:
>>> Hi Thomas,
>>>
>>>> 08/04/2020 05:13, Lukasz Wojciechowski:
>>>>> This patch adds verification of the parameters to the ret_security
>>>>> API functions. All required parameters are checked if they are not NULL.
>>>> [...]
>>>>> --- a/config/common_base
>>>>> +++ b/config/common_base
>>>>>   CONFIG_RTE_LIBRTE_SECURITY=y
>>>>> +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
>>>> Is it a leftover?
>>>>
>>> [Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in
>>> datapath. Like in,
>>> https://protect2.fireeye.com/url?k=ebdcc0dd-b6100959-ebdd4b92-0cc47aa8f5ba-33f3beec7b92faf2&q=1&u=https%3A%2F%2Furldefense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttp-3A__code.dpdk.org_dpdk
>>> _latest_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4378&d=DwICAg&c=n
>>> KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
>> WYLn1v9SyTMrT5EQqh2TU&m=
>> STCBgRhcnCb9M6MWQL9CUszLwy2r0NJ_3m93_D5UX3g&s=HVsD0LKZ2Q6UCW
>> BSRvbw9beD
>>> 7OtuQyWPrRrx9eofnz8&e=
>> 1/ I don't see it used in this patch
> [Anoob] Following snippet uses.
>
> +#ifdef RTE_LIBRTE_SECURITY_DEBUG
> +	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
> +			-ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> +#endif
>
>> 2/ Adding makefile-only option is weird
>> 3/ Adding new compile-time options is discouraged
> [Anoob] This is only introduced for data path APIs. And the same approach is followed in eth dev as well.
>
> Thanks,
> Anoob
>
  

08/04/2020 17:49, Lukasz Wojciechowski:
> Hi guys,
> 
> I don't know what is the current status of "legacy" build using 
> gnumakes, so I added the new DEBUG flag to config just as it was done in 
> other libs like eventdev.
> Many guides still point config files as the one that should be changed 
> in order to enable some features, so I thought I should add it there.
> 
> If I understand well the official build system now is the one based on 
> using meson and ninja, however it hasn't got anything similar to the 
> gnamakefiles system, e.g.
> in the meson.build file for libraries all the libraries have build 
> variable set to true and there are few ifs that check it, but as it's 
> set to true all libraries build always.
> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> It's kind of weird.
> 
>     foreach l:libraries
>     *    build = true**
>     *    reason = '<unknown reason>' # set if build == false to explain why
>          ...
>     *    if not build*
>              dpdk_libs_disabled += name
>              set_variable(name.underscorify() + '_disable_reason', reason)
>          else
>              enabled_libs += name
>     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>          ...
> 
> Have you think about reusing config files in meson configuration and 
> have a single point of configuration? Of course all meson flags can 
> overwrite the default config.

This is on purpose.
We are removing most of compile-time options with meson.

I think we can use a global option for debug-specific code.
Bruce, what do you recommend?
  

On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> 08/04/2020 17:49, Lukasz Wojciechowski:
> > Hi guys,
> > 
> > I don't know what is the current status of "legacy" build using 
> > gnumakes, so I added the new DEBUG flag to config just as it was done in 
> > other libs like eventdev.
> > Many guides still point config files as the one that should be changed 
> > in order to enable some features, so I thought I should add it there.
> > 
> > If I understand well the official build system now is the one based on 
> > using meson and ninja, however it hasn't got anything similar to the 
> > gnamakefiles system, e.g.
> > in the meson.build file for libraries all the libraries have build 
> > variable set to true and there are few ifs that check it, but as it's 
> > set to true all libraries build always.
> > And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> > It's kind of weird.
> > 
> >     foreach l:libraries
> >     *    build = true**
> >     *    reason = '<unknown reason>' # set if build == false to explain why
> >          ...
> >     *    if not build*
> >              dpdk_libs_disabled += name
> >              set_variable(name.underscorify() + '_disable_reason', reason)
> >          else
> >              enabled_libs += name
> >     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> >          ...
> > 
> > Have you think about reusing config files in meson configuration and 
> > have a single point of configuration? Of course all meson flags can 
> > overwrite the default config.
> 
> This is on purpose.
> We are removing most of compile-time options with meson.
> 
> I think we can use a global option for debug-specific code.
> Bruce, what do you recommend?
> 
Meson has a built-in global debug setting which could be used. However,
that may be too course-grained. If that is the case there are a couple of
options:

1 Each library can have it's own debug flag defined, which is set on
  the commandline in CFLAGS. Can be done right now - just reuse any of the
  debug variables in the existing make config files (stripping off the
  CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
2 Since that is perhaps not the most usable - though easiest to implement -
  we can look to add a general debug option (or couple of options) in
  meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
  libs or drivers to pass the debug flags to. This will require a little
  work in the meson build infrastructure, but is not that hard. The harder
  part is standardizing the debug flags across all components.

The advantage of #1 is that it works today and just needs some
documentation for each lib/driver what it's debug flags are. The advantage
of #2 is more usability, but it requires a lot more work to standardize
IMHO.

/Bruce
  

09/04/2020 12:14, Bruce Richardson:
> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> > 08/04/2020 17:49, Lukasz Wojciechowski:
> > > Hi guys,
> > > 
> > > I don't know what is the current status of "legacy" build using 
> > > gnumakes, so I added the new DEBUG flag to config just as it was done in 
> > > other libs like eventdev.
> > > Many guides still point config files as the one that should be changed 
> > > in order to enable some features, so I thought I should add it there.
> > > 
> > > If I understand well the official build system now is the one based on 
> > > using meson and ninja, however it hasn't got anything similar to the 
> > > gnamakefiles system, e.g.
> > > in the meson.build file for libraries all the libraries have build 
> > > variable set to true and there are few ifs that check it, but as it's 
> > > set to true all libraries build always.
> > > And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> > > It's kind of weird.
> > > 
> > >     foreach l:libraries
> > >     *    build = true**
> > >     *    reason = '<unknown reason>' # set if build == false to explain why
> > >          ...
> > >     *    if not build*
> > >              dpdk_libs_disabled += name
> > >              set_variable(name.underscorify() + '_disable_reason', reason)
> > >          else
> > >              enabled_libs += name
> > >     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> > >          ...
> > > 
> > > Have you think about reusing config files in meson configuration and 
> > > have a single point of configuration? Of course all meson flags can 
> > > overwrite the default config.
> > 
> > This is on purpose.
> > We are removing most of compile-time options with meson.
> > 
> > I think we can use a global option for debug-specific code.
> > Bruce, what do you recommend?
> > 
> Meson has a built-in global debug setting which could be used. However,
> that may be too course-grained. If that is the case there are a couple of
> options:
> 
> 1 Each library can have it's own debug flag defined, which is set on
>   the commandline in CFLAGS. Can be done right now - just reuse any of the
>   debug variables in the existing make config files (stripping off the
>   CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> 2 Since that is perhaps not the most usable - though easiest to implement -
>   we can look to add a general debug option (or couple of options) in
>   meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
>   libs or drivers to pass the debug flags to. This will require a little
>   work in the meson build infrastructure, but is not that hard. The harder
>   part is standardizing the debug flags across all components.
> 
> The advantage of #1 is that it works today and just needs some
> documentation for each lib/driver what it's debug flags are. The advantage
> of #2 is more usability, but it requires a lot more work to standardize
> IMHO.

In this case, we need a general option as the one already provided by meson.
It means: "I am not in production, I want to see anything behaving wrong
in the datapath."
"Anything" means we don't need a per-library switch.
And for the other needs (out of fast path), we have a new function:
	rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
  

On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
> 09/04/2020 12:14, Bruce Richardson:
> > On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> > > 08/04/2020 17:49, Lukasz Wojciechowski:
> > > > Hi guys,
> > > > 
> > > > I don't know what is the current status of "legacy" build using 
> > > > gnumakes, so I added the new DEBUG flag to config just as it was done in 
> > > > other libs like eventdev.
> > > > Many guides still point config files as the one that should be changed 
> > > > in order to enable some features, so I thought I should add it there.
> > > > 
> > > > If I understand well the official build system now is the one based on 
> > > > using meson and ninja, however it hasn't got anything similar to the 
> > > > gnamakefiles system, e.g.
> > > > in the meson.build file for libraries all the libraries have build 
> > > > variable set to true and there are few ifs that check it, but as it's 
> > > > set to true all libraries build always.
> > > > And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> > > > It's kind of weird.
> > > > 
> > > >     foreach l:libraries
> > > >     *    build = true**
> > > >     *    reason = '<unknown reason>' # set if build == false to explain why
> > > >          ...
> > > >     *    if not build*
> > > >              dpdk_libs_disabled += name
> > > >              set_variable(name.underscorify() + '_disable_reason', reason)
> > > >          else
> > > >              enabled_libs += name
> > > >     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> > > >          ...
> > > > 
> > > > Have you think about reusing config files in meson configuration and 
> > > > have a single point of configuration? Of course all meson flags can 
> > > > overwrite the default config.
> > > 
> > > This is on purpose.
> > > We are removing most of compile-time options with meson.
> > > 
> > > I think we can use a global option for debug-specific code.
> > > Bruce, what do you recommend?
> > > 
> > Meson has a built-in global debug setting which could be used. However,
> > that may be too course-grained. If that is the case there are a couple of
> > options:
> > 
> > 1 Each library can have it's own debug flag defined, which is set on
> >   the commandline in CFLAGS. Can be done right now - just reuse any of the
> >   debug variables in the existing make config files (stripping off the
> >   CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> > 2 Since that is perhaps not the most usable - though easiest to implement -
> >   we can look to add a general debug option (or couple of options) in
> >   meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
> >   libs or drivers to pass the debug flags to. This will require a little
> >   work in the meson build infrastructure, but is not that hard. The harder
> >   part is standardizing the debug flags across all components.
> > 
> > The advantage of #1 is that it works today and just needs some
> > documentation for each lib/driver what it's debug flags are. The advantage
> > of #2 is more usability, but it requires a lot more work to standardize
> > IMHO.
> 
> In this case, we need a general option as the one already provided by meson.
> It means: "I am not in production, I want to see anything behaving wrong
> in the datapath."
> "Anything" means we don't need a per-library switch.
> And for the other needs (out of fast path), we have a new function:
> 	rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
> 
To use the general option in meson something like below is probably all
that is needed to flag the debug build to all components:

diff --git a/config/meson.build b/config/meson.build
index 49482091d..b01cd1251 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -176,6 +176,10 @@ endif
 # add -include rte_config to cflags
 add_project_arguments('-include', 'rte_config.h', language: 'c')

+if get_option('debug')
+       add_project_arguments('-DDEBUG', language: 'c')
+endif
+
 # enable extra warnings and disable any unwanted warnings
 warning_flags = [
        # -Wall is added by meson by default, so add -Wextra only
  

W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
>> 09/04/2020 12:14, Bruce Richardson:
>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
>>>>> Hi guys,
>>>>>
>>>>> I don't know what is the current status of "legacy" build using
>>>>> gnumakes, so I added the new DEBUG flag to config just as it was done in
>>>>> other libs like eventdev.
>>>>> Many guides still point config files as the one that should be changed
>>>>> in order to enable some features, so I thought I should add it there.
>>>>>
>>>>> If I understand well the official build system now is the one based on
>>>>> using meson and ninja, however it hasn't got anything similar to the
>>>>> gnamakefiles system, e.g.
>>>>> in the meson.build file for libraries all the libraries have build
>>>>> variable set to true and there are few ifs that check it, but as it's
>>>>> set to true all libraries build always.
>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
>>>>> It's kind of weird.
>>>>>
>>>>>      foreach l:libraries
>>>>>      *    build = true**
>>>>>      *    reason = '<unknown reason>' # set if build == false to explain why
>>>>>           ...
>>>>>      *    if not build*
>>>>>               dpdk_libs_disabled += name
>>>>>               set_variable(name.underscorify() + '_disable_reason', reason)
>>>>>           else
>>>>>               enabled_libs += name
>>>>>      *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>>>>>           ...
>>>>>
>>>>> Have you think about reusing config files in meson configuration and
>>>>> have a single point of configuration? Of course all meson flags can
>>>>> overwrite the default config.
>>>> This is on purpose.
>>>> We are removing most of compile-time options with meson.
>>>>
>>>> I think we can use a global option for debug-specific code.
>>>> Bruce, what do you recommend?
>>>>
>>> Meson has a built-in global debug setting which could be used. However,
>>> that may be too course-grained. If that is the case there are a couple of
>>> options:
>>>
>>> 1 Each library can have it's own debug flag defined, which is set on
>>>    the commandline in CFLAGS. Can be done right now - just reuse any of the
>>>    debug variables in the existing make config files (stripping off the
>>>    CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
>>> 2 Since that is perhaps not the most usable - though easiest to implement -
>>>    we can look to add a general debug option (or couple of options) in
>>>    meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
>>>    libs or drivers to pass the debug flags to. This will require a little
>>>    work in the meson build infrastructure, but is not that hard. The harder
>>>    part is standardizing the debug flags across all components.
>>>
>>> The advantage of #1 is that it works today and just needs some
>>> documentation for each lib/driver what it's debug flags are. The advantage
>>> of #2 is more usability, but it requires a lot more work to standardize
>>> IMHO.
>> In this case, we need a general option as the one already provided by meson.
>> It means: "I am not in production, I want to see anything behaving wrong
>> in the datapath."
>> "Anything" means we don't need a per-library switch.
>> And for the other needs (out of fast path), we have a new function:
>> 	rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
>>
> To use the general option in meson something like below is probably all
> that is needed to flag the debug build to all components:
>
> diff --git a/config/meson.build b/config/meson.build
> index 49482091d..b01cd1251 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -176,6 +176,10 @@ endif
>   # add -include rte_config to cflags
>   add_project_arguments('-include', 'rte_config.h', language: 'c')
>
> +if get_option('debug')
> +       add_project_arguments('-DDEBUG', language: 'c')
> +endif
> +

This will conflict with DEBUG define for log level.

How about adding similar define in library meson.build file? , e.g

diff --git a/lib/librte_security/meson.build 
b/lib/librte_security/meson.build
index 5679c8b5c..ee92483c5 100644
--- a/lib/librte_security/meson.build
+++ b/lib/librte_security/meson.build
@@ -4,3 +4,7 @@
  sources = files('rte_security.c')
  headers = files('rte_security.h', 'rte_security_driver.h')
  deps += ['mempool', 'cryptodev']
+
+if get_option('debug')
+ add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
+endif


>   # enable extra warnings and disable any unwanted warnings
>   warning_flags = [
>          # -Wall is added by meson by default, so add -Wextra only
>   
>
  

W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
>
> W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
>> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
>>> 09/04/2020 12:14, Bruce Richardson:
>>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
>>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
>>>>>> Hi guys,
>>>>>>
>>>>>> I don't know what is the current status of "legacy" build using
>>>>>> gnumakes, so I added the new DEBUG flag to config just as it was 
>>>>>> done in
>>>>>> other libs like eventdev.
>>>>>> Many guides still point config files as the one that should be 
>>>>>> changed
>>>>>> in order to enable some features, so I thought I should add it 
>>>>>> there.
>>>>>>
>>>>>> If I understand well the official build system now is the one 
>>>>>> based on
>>>>>> using meson and ninja, however it hasn't got anything similar to the
>>>>>> gnamakefiles system, e.g.
>>>>>> in the meson.build file for libraries all the libraries have build
>>>>>> variable set to true and there are few ifs that check it, but as 
>>>>>> it's
>>>>>> set to true all libraries build always.
>>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
>>>>>> It's kind of weird.
>>>>>>
>>>>>>      foreach l:libraries
>>>>>>      *    build = true**
>>>>>>      *    reason = '<unknown reason>' # set if build == false to 
>>>>>> explain why
>>>>>>           ...
>>>>>>      *    if not build*
>>>>>>               dpdk_libs_disabled += name
>>>>>>               set_variable(name.underscorify() + 
>>>>>> '_disable_reason', reason)
>>>>>>           else
>>>>>>               enabled_libs += name
>>>>>>      *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>>>>>>           ...
>>>>>>
>>>>>> Have you think about reusing config files in meson configuration and
>>>>>> have a single point of configuration? Of course all meson flags can
>>>>>> overwrite the default config.
>>>>> This is on purpose.
>>>>> We are removing most of compile-time options with meson.
>>>>>
>>>>> I think we can use a global option for debug-specific code.
>>>>> Bruce, what do you recommend?
>>>>>
>>>> Meson has a built-in global debug setting which could be used. 
>>>> However,
>>>> that may be too course-grained. If that is the case there are a 
>>>> couple of
>>>> options:
>>>>
>>>> 1 Each library can have it's own debug flag defined, which is set on
>>>>    the commandline in CFLAGS. Can be done right now - just reuse 
>>>> any of the
>>>>    debug variables in the existing make config files (stripping off 
>>>> the
>>>>    CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
>>>> 2 Since that is perhaps not the most usable - though easiest to 
>>>> implement -
>>>>    we can look to add a general debug option (or couple of options) in
>>>>    meson, e.g. debug_libs=, debug_drivers=, where each option takes 
>>>> a list of
>>>>    libs or drivers to pass the debug flags to. This will require a 
>>>> little
>>>>    work in the meson build infrastructure, but is not that hard. 
>>>> The harder
>>>>    part is standardizing the debug flags across all components.
>>>>
>>>> The advantage of #1 is that it works today and just needs some
>>>> documentation for each lib/driver what it's debug flags are. The 
>>>> advantage
>>>> of #2 is more usability, but it requires a lot more work to 
>>>> standardize
>>>> IMHO.
>>> In this case, we need a general option as the one already provided 
>>> by meson.
>>> It means: "I am not in production, I want to see anything behaving 
>>> wrong
>>> in the datapath."
>>> "Anything" means we don't need a per-library switch.
>>> And for the other needs (out of fast path), we have a new function:
>>>     rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
>>>
>> To use the general option in meson something like below is probably all
>> that is needed to flag the debug build to all components:
>>
>> diff --git a/config/meson.build b/config/meson.build
>> index 49482091d..b01cd1251 100644
>> --- a/config/meson.build
>> +++ b/config/meson.build
>> @@ -176,6 +176,10 @@ endif
>>   # add -include rte_config to cflags
>>   add_project_arguments('-include', 'rte_config.h', language: 'c')
>>
>> +if get_option('debug')
>> +       add_project_arguments('-DDEBUG', language: 'c')
>> +endif
>> +
>
> This will conflict with DEBUG define for log level.
Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but 
in few places you can find something like:
#define NTB_LOG(level, fmt, args...) \
     rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \

         __func__, ##args)

and usage like this:
NTB_LOG(DEBUG, "Link is not up.");

>
> How about adding similar define in library meson.build file? , e.g
>
> diff --git a/lib/librte_security/meson.build 
> b/lib/librte_security/meson.build
> index 5679c8b5c..ee92483c5 100644
> --- a/lib/librte_security/meson.build
> +++ b/lib/librte_security/meson.build
> @@ -4,3 +4,7 @@
>  sources = files('rte_security.c')
>  headers = files('rte_security.h', 'rte_security_driver.h')
>  deps += ['mempool', 'cryptodev']
> +
> +if get_option('debug')
> + add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
> +endif
>
>
>>   # enable extra warnings and disable any unwanted warnings
>>   warning_flags = [
>>          # -Wall is added by meson by default, so add -Wextra only
>>
  

09/04/2020 16:21, Lukasz Wojciechowski:
> W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
> > W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
> >> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
> >>> 09/04/2020 12:14, Bruce Richardson:
> >>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> >>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
> >>>>>> Hi guys,
> >>>>>>
> >>>>>> I don't know what is the current status of "legacy" build using
> >>>>>> gnumakes, so I added the new DEBUG flag to config just as it was 
> >>>>>> done in
> >>>>>> other libs like eventdev.
> >>>>>> Many guides still point config files as the one that should be 
> >>>>>> changed
> >>>>>> in order to enable some features, so I thought I should add it 
> >>>>>> there.
> >>>>>>
> >>>>>> If I understand well the official build system now is the one 
> >>>>>> based on
> >>>>>> using meson and ninja, however it hasn't got anything similar to the
> >>>>>> gnamakefiles system, e.g.
> >>>>>> in the meson.build file for libraries all the libraries have build
> >>>>>> variable set to true and there are few ifs that check it, but as 
> >>>>>> it's
> >>>>>> set to true all libraries build always.
> >>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
> >>>>>> It's kind of weird.
> >>>>>>
> >>>>>>      foreach l:libraries
> >>>>>>      *    build = true**
> >>>>>>      *    reason = '<unknown reason>' # set if build == false to 
> >>>>>> explain why
> >>>>>>           ...
> >>>>>>      *    if not build*
> >>>>>>               dpdk_libs_disabled += name
> >>>>>>               set_variable(name.underscorify() + 
> >>>>>> '_disable_reason', reason)
> >>>>>>           else
> >>>>>>               enabled_libs += name
> >>>>>>      *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> >>>>>>           ...
> >>>>>>
> >>>>>> Have you think about reusing config files in meson configuration and
> >>>>>> have a single point of configuration? Of course all meson flags can
> >>>>>> overwrite the default config.
> >>>>> This is on purpose.
> >>>>> We are removing most of compile-time options with meson.
> >>>>>
> >>>>> I think we can use a global option for debug-specific code.
> >>>>> Bruce, what do you recommend?
> >>>>>
> >>>> Meson has a built-in global debug setting which could be used. 
> >>>> However,
> >>>> that may be too course-grained. If that is the case there are a 
> >>>> couple of
> >>>> options:
> >>>>
> >>>> 1 Each library can have it's own debug flag defined, which is set on
> >>>>    the commandline in CFLAGS. Can be done right now - just reuse 
> >>>> any of the
> >>>>    debug variables in the existing make config files (stripping off 
> >>>> the
> >>>>    CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> >>>> 2 Since that is perhaps not the most usable - though easiest to 
> >>>> implement -
> >>>>    we can look to add a general debug option (or couple of options) in
> >>>>    meson, e.g. debug_libs=, debug_drivers=, where each option takes 
> >>>> a list of
> >>>>    libs or drivers to pass the debug flags to. This will require a 
> >>>> little
> >>>>    work in the meson build infrastructure, but is not that hard. 
> >>>> The harder
> >>>>    part is standardizing the debug flags across all components.
> >>>>
> >>>> The advantage of #1 is that it works today and just needs some
> >>>> documentation for each lib/driver what it's debug flags are. The 
> >>>> advantage
> >>>> of #2 is more usability, but it requires a lot more work to 
> >>>> standardize
> >>>> IMHO.
> >>> In this case, we need a general option as the one already provided 
> >>> by meson.
> >>> It means: "I am not in production, I want to see anything behaving 
> >>> wrong
> >>> in the datapath."
> >>> "Anything" means we don't need a per-library switch.
> >>> And for the other needs (out of fast path), we have a new function:
> >>>     rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
> >>>
> >> To use the general option in meson something like below is probably all
> >> that is needed to flag the debug build to all components:
> >>
> >> diff --git a/config/meson.build b/config/meson.build
> >> index 49482091d..b01cd1251 100644
> >> --- a/config/meson.build
> >> +++ b/config/meson.build
> >> @@ -176,6 +176,10 @@ endif
> >>   # add -include rte_config to cflags
> >>   add_project_arguments('-include', 'rte_config.h', language: 'c')
> >>
> >> +if get_option('debug')
> >> +       add_project_arguments('-DDEBUG', language: 'c')
> >> +endif
> >> +
> >
> > This will conflict with DEBUG define for log level.
> Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but 
> in few places you can find something like:
> #define NTB_LOG(level, fmt, args...) \
>      rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \
> 
>          __func__, ##args)
> 
> and usage like this:
> NTB_LOG(DEBUG, "Link is not up.");

This is not a conflict.
The compiler sees only RTE_LOG_DEBUG.

Anyway the right name for the general flag should be RTE_DEBUG.


> > How about adding similar define in library meson.build file? , e.g
> >
> > diff --git a/lib/librte_security/meson.build 
> > b/lib/librte_security/meson.build
> > index 5679c8b5c..ee92483c5 100644
> > --- a/lib/librte_security/meson.build
> > +++ b/lib/librte_security/meson.build
> > @@ -4,3 +4,7 @@
> >  sources = files('rte_security.c')
> >  headers = files('rte_security.h', 'rte_security_driver.h')
> >  deps += ['mempool', 'cryptodev']
> > +
> > +if get_option('debug')
> > + add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
> > +endif

It can work yes.
But it would be simpler to align on single debug flag.
  

W dniu 09.04.2020 o 17:22, Thomas Monjalon pisze:
> 09/04/2020 16:21, Lukasz Wojciechowski:
>> W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
>>> W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
>>>> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
>>>>> 09/04/2020 12:14, Bruce Richardson:
>>>>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
>>>>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
>>>>>>>> Hi guys,
>>>>>>>>
>>>>>>>> I don't know what is the current status of "legacy" build using
>>>>>>>> gnumakes, so I added the new DEBUG flag to config just as it was
>>>>>>>> done in
>>>>>>>> other libs like eventdev.
>>>>>>>> Many guides still point config files as the one that should be
>>>>>>>> changed
>>>>>>>> in order to enable some features, so I thought I should add it
>>>>>>>> there.
>>>>>>>>
>>>>>>>> If I understand well the official build system now is the one
>>>>>>>> based on
>>>>>>>> using meson and ninja, however it hasn't got anything similar to the
>>>>>>>> gnamakefiles system, e.g.
>>>>>>>> in the meson.build file for libraries all the libraries have build
>>>>>>>> variable set to true and there are few ifs that check it, but as
>>>>>>>> it's
>>>>>>>> set to true all libraries build always.
>>>>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
>>>>>>>> It's kind of weird.
>>>>>>>>
>>>>>>>>       foreach l:libraries
>>>>>>>>       *    build = true**
>>>>>>>>       *    reason = '<unknown reason>' # set if build == false to
>>>>>>>> explain why
>>>>>>>>            ...
>>>>>>>>       *    if not build*
>>>>>>>>                dpdk_libs_disabled += name
>>>>>>>>                set_variable(name.underscorify() +
>>>>>>>> '_disable_reason', reason)
>>>>>>>>            else
>>>>>>>>                enabled_libs += name
>>>>>>>>       *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>>>>>>>>            ...
>>>>>>>>
>>>>>>>> Have you think about reusing config files in meson configuration and
>>>>>>>> have a single point of configuration? Of course all meson flags can
>>>>>>>> overwrite the default config.
>>>>>>> This is on purpose.
>>>>>>> We are removing most of compile-time options with meson.
>>>>>>>
>>>>>>> I think we can use a global option for debug-specific code.
>>>>>>> Bruce, what do you recommend?
>>>>>>>
>>>>>> Meson has a built-in global debug setting which could be used.
>>>>>> However,
>>>>>> that may be too course-grained. If that is the case there are a
>>>>>> couple of
>>>>>> options:
>>>>>>
>>>>>> 1 Each library can have it's own debug flag defined, which is set on
>>>>>>     the commandline in CFLAGS. Can be done right now - just reuse
>>>>>> any of the
>>>>>>     debug variables in the existing make config files (stripping off
>>>>>> the
>>>>>>     CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
>>>>>> 2 Since that is perhaps not the most usable - though easiest to
>>>>>> implement -
>>>>>>     we can look to add a general debug option (or couple of options) in
>>>>>>     meson, e.g. debug_libs=, debug_drivers=, where each option takes
>>>>>> a list of
>>>>>>     libs or drivers to pass the debug flags to. This will require a
>>>>>> little
>>>>>>     work in the meson build infrastructure, but is not that hard.
>>>>>> The harder
>>>>>>     part is standardizing the debug flags across all components.
>>>>>>
>>>>>> The advantage of #1 is that it works today and just needs some
>>>>>> documentation for each lib/driver what it's debug flags are. The
>>>>>> advantage
>>>>>> of #2 is more usability, but it requires a lot more work to
>>>>>> standardize
>>>>>> IMHO.
>>>>> In this case, we need a general option as the one already provided
>>>>> by meson.
>>>>> It means: "I am not in production, I want to see anything behaving
>>>>> wrong
>>>>> in the datapath."
>>>>> "Anything" means we don't need a per-library switch.
>>>>> And for the other needs (out of fast path), we have a new function:
>>>>>      rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
>>>>>
>>>> To use the general option in meson something like below is probably all
>>>> that is needed to flag the debug build to all components:
>>>>
>>>> diff --git a/config/meson.build b/config/meson.build
>>>> index 49482091d..b01cd1251 100644
>>>> --- a/config/meson.build
>>>> +++ b/config/meson.build
>>>> @@ -176,6 +176,10 @@ endif
>>>>    # add -include rte_config to cflags
>>>>    add_project_arguments('-include', 'rte_config.h', language: 'c')
>>>>
>>>> +if get_option('debug')
>>>> +       add_project_arguments('-DDEBUG', language: 'c')
>>>> +endif
>>>> +
>>> This will conflict with DEBUG define for log level.
>> Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but
>> in few places you can find something like:
>> #define NTB_LOG(level, fmt, args...) \
>>       rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \
>>
>>           __func__, ##args)
>>
>> and usage like this:
>> NTB_LOG(DEBUG, "Link is not up.");
> This is not a conflict.
> The compiler sees only RTE_LOG_DEBUG.
>
> Anyway the right name for the general flag should be RTE_DEBUG.
>
Ok, I'll use RTE_DEBUG.

@Bruce Is it ok, so that I would create a patch for meson.build?
>>> How about adding similar define in library meson.build file? , e.g
>>>
>>> diff --git a/lib/librte_security/meson.build
>>> b/lib/librte_security/meson.build
>>> index 5679c8b5c..ee92483c5 100644
>>> --- a/lib/librte_security/meson.build
>>> +++ b/lib/librte_security/meson.build
>>> @@ -4,3 +4,7 @@
>>>   sources = files('rte_security.c')
>>>   headers = files('rte_security.h', 'rte_security_driver.h')
>>>   deps += ['mempool', 'cryptodev']
>>> +
>>> +if get_option('debug')
>>> + add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
>>> +endif
> It can work yes.
> But it would be simpler to align on single debug flag.
>
In this set of patches I would only align the security and tests code to 
RTE_DEBUG.
Changes in global configuration to use a single debug flag should be 
done in separate set of patches.
>
  

On Thu, Apr 09, 2020 at 06:10:11PM +0200, Lukasz Wojciechowski wrote:
> 
> W dniu 09.04.2020 o 17:22, Thomas Monjalon pisze:
> > 09/04/2020 16:21, Lukasz Wojciechowski:
> >> W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
> >>> W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
> >>>> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
> >>>>> 09/04/2020 12:14, Bruce Richardson:
> >>>>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> >>>>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
> >>>>>>>> Hi guys,
> >>>>>>>>
> >>>>>>>> I don't know what is the current status of "legacy" build using
> >>>>>>>> gnumakes, so I added the new DEBUG flag to config just as it was
> >>>>>>>> done in
> >>>>>>>> other libs like eventdev.
> >>>>>>>> Many guides still point config files as the one that should be
> >>>>>>>> changed
> >>>>>>>> in order to enable some features, so I thought I should add it
> >>>>>>>> there.
> >>>>>>>>
> >>>>>>>> If I understand well the official build system now is the one
> >>>>>>>> based on
> >>>>>>>> using meson and ninja, however it hasn't got anything similar to the
> >>>>>>>> gnamakefiles system, e.g.
> >>>>>>>> in the meson.build file for libraries all the libraries have build
> >>>>>>>> variable set to true and there are few ifs that check it, but as
> >>>>>>>> it's
> >>>>>>>> set to true all libraries build always.
> >>>>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
> >>>>>>>> It's kind of weird.
> >>>>>>>>
> >>>>>>>>       foreach l:libraries
> >>>>>>>>       *    build = true**
> >>>>>>>>       *    reason = '<unknown reason>' # set if build == false to
> >>>>>>>> explain why
> >>>>>>>>            ...
> >>>>>>>>       *    if not build*
> >>>>>>>>                dpdk_libs_disabled += name
> >>>>>>>>                set_variable(name.underscorify() +
> >>>>>>>> '_disable_reason', reason)
> >>>>>>>>            else
> >>>>>>>>                enabled_libs += name
> >>>>>>>>       *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> >>>>>>>>            ...
> >>>>>>>>
> >>>>>>>> Have you think about reusing config files in meson configuration and
> >>>>>>>> have a single point of configuration? Of course all meson flags can
> >>>>>>>> overwrite the default config.
> >>>>>>> This is on purpose.
> >>>>>>> We are removing most of compile-time options with meson.
> >>>>>>>
> >>>>>>> I think we can use a global option for debug-specific code.
> >>>>>>> Bruce, what do you recommend?
> >>>>>>>
> >>>>>> Meson has a built-in global debug setting which could be used.
> >>>>>> However,
> >>>>>> that may be too course-grained. If that is the case there are a
> >>>>>> couple of
> >>>>>> options:
> >>>>>>
> >>>>>> 1 Each library can have it's own debug flag defined, which is set on
> >>>>>>     the commandline in CFLAGS. Can be done right now - just reuse
> >>>>>> any of the
> >>>>>>     debug variables in the existing make config files (stripping off
> >>>>>> the
> >>>>>>     CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> >>>>>> 2 Since that is perhaps not the most usable - though easiest to
> >>>>>> implement -
> >>>>>>     we can look to add a general debug option (or couple of options) in
> >>>>>>     meson, e.g. debug_libs=, debug_drivers=, where each option takes
> >>>>>> a list of
> >>>>>>     libs or drivers to pass the debug flags to. This will require a
> >>>>>> little
> >>>>>>     work in the meson build infrastructure, but is not that hard.
> >>>>>> The harder
> >>>>>>     part is standardizing the debug flags across all components.
> >>>>>>
> >>>>>> The advantage of #1 is that it works today and just needs some
> >>>>>> documentation for each lib/driver what it's debug flags are. The
> >>>>>> advantage
> >>>>>> of #2 is more usability, but it requires a lot more work to
> >>>>>> standardize
> >>>>>> IMHO.
> >>>>> In this case, we need a general option as the one already provided
> >>>>> by meson.
> >>>>> It means: "I am not in production, I want to see anything behaving
> >>>>> wrong
> >>>>> in the datapath."
> >>>>> "Anything" means we don't need a per-library switch.
> >>>>> And for the other needs (out of fast path), we have a new function:
> >>>>>      rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
> >>>>>
> >>>> To use the general option in meson something like below is probably all
> >>>> that is needed to flag the debug build to all components:
> >>>>
> >>>> diff --git a/config/meson.build b/config/meson.build
> >>>> index 49482091d..b01cd1251 100644
> >>>> --- a/config/meson.build
> >>>> +++ b/config/meson.build
> >>>> @@ -176,6 +176,10 @@ endif
> >>>>    # add -include rte_config to cflags
> >>>>    add_project_arguments('-include', 'rte_config.h', language: 'c')
> >>>>
> >>>> +if get_option('debug')
> >>>> +       add_project_arguments('-DDEBUG', language: 'c')
> >>>> +endif
> >>>> +
> >>> This will conflict with DEBUG define for log level.
> >> Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but
> >> in few places you can find something like:
> >> #define NTB_LOG(level, fmt, args...) \
> >>       rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \
> >>
> >>           __func__, ##args)
> >>
> >> and usage like this:
> >> NTB_LOG(DEBUG, "Link is not up.");
> > This is not a conflict.
> > The compiler sees only RTE_LOG_DEBUG.
> >
> > Anyway the right name for the general flag should be RTE_DEBUG.
> >
> Ok, I'll use RTE_DEBUG.
> 
> @Bruce Is it ok, so that I would create a patch for meson.build?

Sure. Setting RTE_DEBUG at top level when a debug build is set is probably
a good idea.

/Bruce