[v4,1/5] kvargs: add one new process API
Checks
Commit Message
The rte_kvargs_process() was used to handle key=value (e.g.
socket_id=0), it also supports to handle 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 handle both key=value and
only-key cases.
2) Constraint the rte_kvargs_process() can only handle key=value cases,
it will return -1 when handle only-key case (that is the matched key's
value is NULL).
This patch also make sure the rte_kvargs_process_opt() and
rte_kvargs_process() API both return -1 when the kvlist parameter is
NULL.
[1] https://patches.dpdk.org/project/dpdk/patch/20230320092110.37295-1-fengchengwen@huawei.com/
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
doc/guides/rel_notes/release_23_11.rst | 13 ++++++++
lib/kvargs/rte_kvargs.c | 43 ++++++++++++++++++++------
lib/kvargs/rte_kvargs.h | 37 ++++++++++++++++++++--
lib/kvargs/version.map | 3 ++
4 files changed, 84 insertions(+), 12 deletions(-)
Comments
On Sun, 5 Nov 2023 05:45:35 +0000
Chengwen Feng <fengchengwen@huawei.com> wrote:
> +* **Updated kvargs process API.**
> +
> + * Introduced rte_kvargs_process_opt() API, which inherits the function
> + of rte_kvargs_process() and could handle both key=value and only-key
> + cases.
> +
> + * Constraint rte_kvargs_process() API can only handle key=value cases,
> + it will return -1 when handle only-key case (that is the matched key's
> + value is NULL).
> +
Looks good but may I suggest some alternatives.
Since this is an API and ABI change as was not announced, maybe a little late
in the process for this release. And since unlikely to go in 23.11 need to do something
better in 24.03.
What about changing the args to rte_kvargs_process() to add an additional default
value. Most callers don't have a default (use key-value) but the ones that take only-key
would pass the default value.
Then use ABI versioning to add the new arg and you would not have to introduce
a new function. And add a legacy API version that does what current code does.
All the drivers in DPDK would get the new API/ABI but any customer apps using rte_kvargs_process()
would get the old version.
Hi Stephen,
On 2023/11/6 11:18, Stephen Hemminger wrote:
> On Sun, 5 Nov 2023 05:45:35 +0000
> Chengwen Feng <fengchengwen@huawei.com> wrote:
>
>> +* **Updated kvargs process API.**
>> +
>> + * Introduced rte_kvargs_process_opt() API, which inherits the function
>> + of rte_kvargs_process() and could handle both key=value and only-key
>> + cases.
>> +
>> + * Constraint rte_kvargs_process() API can only handle key=value cases,
>> + it will return -1 when handle only-key case (that is the matched key's
>> + value is NULL).
>> +
>
> Looks good but may I suggest some alternatives.
>
> Since this is an API and ABI change as was not announced, maybe a little late
> in the process for this release. And since unlikely to go in 23.11 need to do something
> better in 24.03.
>
> What about changing the args to rte_kvargs_process() to add an additional default
> value. Most callers don't have a default (use key-value) but the ones that take only-key
> would pass the default value.
The API definition changed, it may need modify most drivers.
Although it's a little late, better continue current.
Thanks
Chengwen
>
> Then use ABI versioning to add the new arg and you would not have to introduce
> a new function. And add a legacy API version that does what current code does.
>
> All the drivers in DPDK would get the new API/ABI but any customer apps using rte_kvargs_process()
> would get the old version.
>
>
> .
>
On Mon, 6 Nov 2023 15:13:35 +0800
fengchengwen <fengchengwen@huawei.com> wrote:
> >> +
> >
> > Looks good but may I suggest some alternatives.
> >
> > Since this is an API and ABI change as was not announced, maybe a little late
> > in the process for this release. And since unlikely to go in 23.11 need to do something
> > better in 24.03.
> >
> > What about changing the args to rte_kvargs_process() to add an additional default
> > value. Most callers don't have a default (use key-value) but the ones that take only-key
> > would pass the default value.
>
> The API definition changed, it may need modify most drivers.
>
> Although it's a little late, better continue current.
>
> Thanks
> Chengwen
Looking ahead, I would like to replace all of EAL args and KVargs processing
with something more like the python parseargs library. The API is cleaner and
incorporating the help with arg parsing is a real benefit. Thomas also suggested
integrating help in the arg parsing.
Something like: https://github.com/cofyc/argparse
Hi Stephen,
On 2023/11/7 0:19, Stephen Hemminger wrote:
> On Mon, 6 Nov 2023 15:13:35 +0800
> fengchengwen <fengchengwen@huawei.com> wrote:
>
>>>> +
>>>
>>> Looks good but may I suggest some alternatives.
>>>
>>> Since this is an API and ABI change as was not announced, maybe a little late
>>> in the process for this release. And since unlikely to go in 23.11 need to do something
>>> better in 24.03.
>>>
>>> What about changing the args to rte_kvargs_process() to add an additional default
>>> value. Most callers don't have a default (use key-value) but the ones that take only-key
>>> would pass the default value.
>>
>> The API definition changed, it may need modify most drivers.
>>
>> Although it's a little late, better continue current.
>>
>> Thanks
>> Chengwen
>
> Looking ahead, I would like to replace all of EAL args and KVargs processing
> with something more like the python parseargs library. The API is cleaner and
> incorporating the help with arg parsing is a real benefit. Thomas also suggested
> integrating help in the arg parsing.
>
> Something like: https://github.com/cofyc/argparse
The argparse is great, especial it integrate help in the arg parsing, which could help
for parse args.
I will propose a argparse library RFC later, and we could both work on it.
As for this commit, I think it has difference: the KV pair was not separated by spaces,
So for: dpdk-testpmd -a 0000:7d:00.0,a=1,b=2,c=3 --file-prefix=feng
We could parse 'a' has value: 0000:7d:00.0,a=1,b=2,c=3
'file-prefix' has value: feng
by argparse.
and then use kvargs to parse a=1,b=2,c=3.
Thanks
Chengwen
>
> .
>
@@ -137,6 +137,19 @@ New Features
a group's miss actions, which are the actions to be performed on packets
that didn't match any of the flow rules in the group.
+* **Updated kvargs process API.**
+
+ * Introduced rte_kvargs_process_opt() API, which inherits the function
+ of rte_kvargs_process() and could handle both key=value and only-key
+ cases.
+
+ * Constraint rte_kvargs_process() API can only handle key=value cases,
+ it will return -1 when handle only-key case (that is the matched key's
+ value is NULL).
+
+ * Make sure rte_kvargs_process_opt() and rte_kvargs_process() API both
+ return -1 when the kvlist parameter is NULL.
+
* **Updated Amazon ena (Elastic Network Adapter) net driver.**
* Upgraded ENA HAL to latest version.
@@ -167,31 +167,56 @@ rte_kvargs_count(const struct rte_kvargs *kvlist, const char *key_match)
return ret;
}
-/*
- * For each matching key, call the given handler function.
- */
-int
-rte_kvargs_process(const struct rte_kvargs *kvlist,
- const char *key_match,
- arg_handler_t handler,
- void *opaque_arg)
+static int
+kvargs_process_common(const struct rte_kvargs *kvlist,
+ const char *key_match,
+ arg_handler_t handler,
+ void *opaque_arg,
+ bool support_only_key)
{
const struct rte_kvargs_pair *pair;
unsigned i;
if (kvlist == NULL)
- return 0;
+ return -1;
for (i = 0; i < kvlist->count; i++) {
pair = &kvlist->pairs[i];
if (key_match == NULL || strcmp(pair->key, key_match) == 0) {
+ if (!support_only_key && pair->value == NULL)
+ return -1;
if ((*handler)(pair->key, pair->value, opaque_arg) < 0)
return -1;
}
}
+
return 0;
}
+/*
+ * For each matching key in key/value, call the given handler function.
+ */
+int
+rte_kvargs_process(const struct rte_kvargs *kvlist,
+ const char *key_match,
+ arg_handler_t handler,
+ void *opaque_arg)
+{
+ return kvargs_process_common(kvlist, key_match, handler, opaque_arg, false);
+}
+
+/*
+ * 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)
+{
+ return kvargs_process_common(kvlist, key_match, handler, opaque_arg, true);
+}
+
/* free the rte_kvargs structure */
void
rte_kvargs_free(struct rte_kvargs *kvlist)
@@ -172,14 +172,17 @@ const char *rte_kvargs_get_with_value(const struct rte_kvargs *kvlist,
const char *key, const char *value);
/**
- * Call a handler function for each key/value matching the key
+ * Call a handler function for each key=value matching the key
*
- * For each key/value association that matches the given key, calls the
+ * For each key=value 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.
*
+ * @note Compared to @see rte_kvargs_process_opt, this API will return -1
+ * when handle only-key case (that is the matched key's value is NULL).
+ *
* @param kvlist
- * The rte_kvargs structure. No error if NULL.
+ * The rte_kvargs structure.
* @param key_match
* The key on which the handler should be called, or NULL to process handler
* on all associations
@@ -195,6 +198,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.
+ * @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;
};