[v3,1/5] kvargs: add one new process API
Checks
Commit Message
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.
[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(-)
Comments
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;
> };
Hi Ferruh,
On 2023/11/3 21:09, Ferruh Yigit wrote:
> 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.
Thanks, already fix in v4
>
>
>
>> [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).
fix in v4.
>
>
>> + 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?
nice catch, fix in v4.
>
>
>> +/*
>> + * 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?
add one @note in v4.
>
>
>> + * 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"?
Yes, it mean optional of value is valid or NULL.
> 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.
also have no better name, so keep the same with v3.
Thanks
Chengwen
>
>
>> /**
>> * 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;
>> };
>
> .
>
@@ -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;
+
+ 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;
+}
+
+/*
+ * 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;
@@ -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
+ *
+ * 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);
+
/**
* Count the number of associations matching the given key
*
@@ -16,4 +16,7 @@ EXPERIMENTAL {
# added in 21.11
rte_kvargs_get_with_value;
+
+ # added in 23.11
+ rte_kvargs_process_opt;
};