Re: [PATCH v3 1/5] kvargs: add one new process API

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/3/2023 7:38 AM, Chengwen Feng wrote:
> The rte_kvargs_process() was used to parse key-value (e.g. socket_id=0),
> it also supports to parse only-key (e.g. socket_id). But many drivers's
> callback can only handle key-value, it will segment fault if handles
> only-key. so the patchset [1] was introduced.
> 
> Because the patchset [1] modified too much drivers, therefore:
> 1) A new API rte_kvargs_process_opt() was introduced, it inherits the
> function of rte_kvargs_process() which could parse both key-value and
> only-key.
> 2) Constraint the rte_kvargs_process() can only parse key-value.
> 

Hi Chengwen,

This works, but behavior change in 'rte_kvargs_process()' can hit some
exiting users who handles both "key=value" and "key" cases.

Other option is to keep 'rte_kvargs_process()' behavior same but add a
new API like 'rte_kvargs_process_safe()' that checks "value == NULL"
case, but this requires more existing code to change and in this option
existing users doesn't get the benefit of the NULL check by default.


Assuming number of the users that use 'rte_kvargs_process()' for
"key=value" is majority, OK to continue with your change, but please
document this in the release notes to highlight.



> [1] https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengchengwen@huawei.com/
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/kvargs/rte_kvargs.c | 29 ++++++++++++++++++++++++++++-
>  lib/kvargs/rte_kvargs.h | 28 ++++++++++++++++++++++++++++
>  lib/kvargs/version.map  |  3 +++
>  3 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c
> index c77bb82feb..5ce8664238 100644
> --- a/lib/kvargs/rte_kvargs.c
> +++ b/lib/kvargs/rte_kvargs.c
> @@ -168,7 +168,7 @@ rte_kvargs_count(const struct rte_kvargs *kvlist, const char *key_match)
>  }
>  
>  /*
> - * For each matching key, call the given handler function.
> + * For each matching key in key/value, call the given handler function.
>   */
>  int
>  rte_kvargs_process(const struct rte_kvargs *kvlist,
> @@ -179,6 +179,33 @@ rte_kvargs_process(const struct rte_kvargs *kvlist,
>  	const struct rte_kvargs_pair *pair;
>  	unsigned i;
>  
> +	if (kvlist == NULL)
> +		return 0;
> +

I think it should return error here, ignoring arg silently with success
can cause trouble in the application. If error returned, at least
application can know argument not provided as it should be (value is
missing).


> +	for (i = 0; i < kvlist->count; i++) {
> +		pair = &kvlist->pairs[i];
> +		if (pair->value == NULL)
> +			continue;
> +		if (key_match == NULL || strcmp(pair->key, key_match) == 0) {
> +			if ((*handler)(pair->key, pair->value, opaque_arg) < 0)
> +				return -1;
> +		}
> +	}
> +	return 0;
> +}
> +

'rte_kvargs_process()' & 'rte_kvargs_process_opt()' implementations are
very similar, to reduce duplication what about create
'rte_kvargs_process_common()' static function and both use this common
with different parameter?


> +/*
> + * For each matching key in key/value or only-key, call the given handler function.
> + */
> +int
> +rte_kvargs_process_opt(const struct rte_kvargs *kvlist,
> +		       const char *key_match,
> +		       arg_handler_t handler,
> +		       void *opaque_arg)
> +{
> +	const struct rte_kvargs_pair *pair;
> +	unsigned int i;
> +
>  	if (kvlist == NULL)
>  		return 0;
>  
> diff --git a/lib/kvargs/rte_kvargs.h b/lib/kvargs/rte_kvargs.h
> index 4900b750bc..522e83f757 100644
> --- a/lib/kvargs/rte_kvargs.h
> +++ b/lib/kvargs/rte_kvargs.h
> @@ -195,6 +195,34 @@ const char *rte_kvargs_get_with_value(const struct rte_kvargs *kvlist,
>  int rte_kvargs_process(const struct rte_kvargs *kvlist,
>  	const char *key_match, arg_handler_t handler, void *opaque_arg);
>  
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Call a handler function for each key/value or only-key matching the key
> + *
>

In API documentation, the difference between  'rte_kvargs_process_opt()'
& 'rte_kvargs_process()' is, 'rte_kvargs_process()' doesn't have "or
only-key", this is easy to miss.
Can you please add more not to 'rte_kvargs_process()' saying on
"only-key" case it returns error?


> + * For each key/value or only-key association that matches the given key, calls
> + * the handler function with the for a given arg_name passing the value on the
> + * dictionary for that key and a given extra argument.
> + *
> + * @param kvlist
> + *   The rte_kvargs structure. No error if NULL.
> + * @param key_match
> + *   The key on which the handler should be called, or NULL to process handler
> + *   on all associations
> + * @param handler
> + *   The function to call for each matching key
> + * @param opaque_arg
> + *   A pointer passed unchanged to the handler
> + *
> + * @return
> + *   - 0 on success
> + *   - Negative on error
> + */
> +__rte_experimental
> +int rte_kvargs_process_opt(const struct rte_kvargs *kvlist,
> +	const char *key_match, arg_handler_t handler, void *opaque_arg);
> +
>

I am not sure about the API name, 'rte_kvargs_process_opt()', what is
'opt' here, "optional"?
What about 'rte_kvargs_process_unsafe()', not quite strong on this too
but not able to come with better one, please feel free to try other
alternatives.


>  /**
>   * Count the number of associations matching the given key
>   *
> diff --git a/lib/kvargs/version.map b/lib/kvargs/version.map
> index 387a94e725..15d482e9b3 100644
> --- a/lib/kvargs/version.map
> +++ b/lib/kvargs/version.map
> @@ -16,4 +16,7 @@ EXPERIMENTAL {
>  
>  	# added in 21.11
>  	rte_kvargs_get_with_value;
> +
> +	# added in 23.11
> +	rte_kvargs_process_opt;
>  };