Re: [dpdk-dev] [EXT] Re: [PATCH v2 01/13] security: fix verification of parameters

[Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>]

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
>
-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com