Re: [dpdk-dev] [PATCH v4 1/4] regexdev: introduce regexdev subsystem

[Thomas Monjalon <thomas@monjalon.net>]

02/07/2020 09:46, Ori Kam:
> From: Jerin Jacob <jerinj@marvell.com>
> --- a/config/common_base
> +++ b/config/common_base
>  #
> +# Compile regex device support
> +#
> +CONFIG_RTE_LIBRTE_REGEXDEV=y
> +
> +#
>  # Compile librte_ring
>  #
>  CONFIG_RTE_LIBRTE_RING=y
> @@ -1141,3 +1146,4 @@ CONFIG_RTE_APP_CRYPTO_PERF=y
>  # Compile the eventdev application
>  #
>  CONFIG_RTE_APP_EVENTDEV=y
> +

Why this empty line?

> --- a/doc/api/doxy-api-index.md
> +++ b/doc/api/doxy-api-index.md
> @@ -26,6 +26,7 @@ The public API headers are grouped by topics:
>    [event_timer_adapter]    (@ref rte_event_timer_adapter.h),
>    [event_crypto_adapter]   (@ref rte_event_crypto_adapter.h),
>    [rawdev]             (@ref rte_rawdev.h),
> +  [regexdev]           (@ref rte_regexdev.h),
>    [metrics]            (@ref rte_metrics.h),

Please move regexdev after [compress].

> --- a/doc/api/doxy-api.conf.in
> +++ b/doc/api/doxy-api.conf.in
> @@ -61,6 +61,7 @@ INPUT                   = @TOPDIR@/doc/api/doxy-api-index.md \
>                            @TOPDIR@/lib/librte_rcu \
>                            @TOPDIR@/lib/librte_reorder \
>                            @TOPDIR@/lib/librte_rib \
> +                          @TOPDIR@/lib/librte_regexdev \
>                            @TOPDIR@/lib/librte_ring \

What is the ordering here?

> --- a/doc/guides/prog_guide/index.rst
> +++ b/doc/guides/prog_guide/index.rst
> @@ -72,3 +72,4 @@ Programmer's Guide
>      lto
>      profile_app
>      glossary
> +    regexdev_lib

Why adding it at the end?
I would suggest adding regexdev after compressdev.

> --- /dev/null
> +++ b/doc/guides/prog_guide/regexdev_lib.rst

Please remove the _lib suffix in this filename.

[...]
> +The dequeue API uses the same format as the enqueue API of processed but
> +the ``nb_ops`` and ``ops`` parameters are now used to specify the max processed
> +operations the user wishes to retrieve and the location in which to store them.
> +The API call returns the actual number of processed operations returned, this
> +can never be larger than ``nb_ops``.
> +

Please avoid empty line at end of files.

> --- /dev/null
> +++ b/lib/librte_regexdev/Makefile
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(C) 2019 Marvell International Ltd.
> +# Copyright(C) 2020 Mellanox International Ltd.

Mellanox copyright is wrong. It should be:
Copyright 2020 Mellanox Technologies, Ltd

> --- /dev/null
> +++ b/lib/librte_regexdev/meson.build
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2020 Mellanox Corporation

Wrong copyright. Please check them all.

> +
> +allow_experimental_apis = true

Internal libraries do not need such flag anymore.

> +sources = files('rte_regexdev.c')
> +headers = files('rte_regexdev.h')
> +deps += ['mbuf']
[...]
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -25,7 +25,7 @@ libraries = [
>  	'gro', 'gso', 'ip_frag', 'jobstats',
>  	'kni', 'latencystats', 'lpm', 'member',
>  	'power', 'pdump', 'rawdev',
> -	'rib', 'reorder', 'sched', 'security', 'stack', 'vhost',
> +	'regexdev', 'rib', 'reorder', 'sched', 'security', 'stack', 'vhost',

strange choice :)
I would have added regex at the end of the previous line which is shorter.