Re: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist
> -Original Message- > From: Wiles, Keith > Sent: Wednesday, August 9, 2017 5:36 PM > To: Ananyev, Konstantin > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist > > > > On Aug 9, 2017, at 8:53 AM, Ananyev, Konstantin > > wrote: > > > > > > > >> -Original Message- > >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Keith Wiles > >> Sent: Wednesday, August 9, 2017 4:30 PM > >> To: dev@dpdk.org > >> Subject: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist > >> > >> rte_kvargs_process() should return error if the key > >> is not found in the kvlist or kvlist is NULL. > > > > Looks like an API breakage for me… > > To me it is not an ABI breakage only fixing the routine to return the correct > status. > > > Could you probably explain why do you think it is necessary? > > For the no key found surely seems like an error to me, Not necessary, it could be an optional one. > but I guess you can always require someone to call the rte_kvargs_count() API > first Yep. > which is not stated any place. > > As for the kvlist being null that is an error IMO and returning success is > not valid. > > But to save a huge discussion I rejected the patch in patchwork for now. Ok. Konstantin > > > Konstantin > > > >> > >> Minor documentation changes and update for when an > >> error is returned. > >> > >> Signed-off-by: Keith Wiles > >> --- > >> lib/librte_kvargs/rte_kvargs.c | 7 --- > >> lib/librte_kvargs/rte_kvargs.h | 7 --- > >> 2 files changed, 8 insertions(+), 6 deletions(-) > >> > >> diff --git a/lib/librte_kvargs/rte_kvargs.c > >> b/lib/librte_kvargs/rte_kvargs.c > >> index 854ac83f5..c8e8f4b28 100755 > >> --- a/lib/librte_kvargs/rte_kvargs.c > >> +++ b/lib/librte_kvargs/rte_kvargs.c > >> @@ -158,16 +158,17 @@ rte_kvargs_process(const struct rte_kvargs *kvlist, > >>void *opaque_arg) > >> { > >>const struct rte_kvargs_pair *pair; > >> - unsigned i; > >> + unsigned int i, found = 0; > >> > >>for (i = 0; i < kvlist->count; i++) { > >>pair = &kvlist->pairs[i]; > >> - if (key_match == NULL || strcmp(pair->key, key_match) == 0) { > >> + if (!key_match || strcmp(pair->key, key_match) == 0) { > >> + found++; > >>if ((*handler)(pair->key, pair->value, opaque_arg) < 0) > >>return -1; > >>} > >>} > >> - return 0; > >> + return (!found) ? -1 : 0; > >> } > >> > >> /* free the rte_kvargs structure */ > >> diff --git a/lib/librte_kvargs/rte_kvargs.h > >> b/lib/librte_kvargs/rte_kvargs.h > >> index 5821c726a..260d4db5b 100755 > >> --- a/lib/librte_kvargs/rte_kvargs.h > >> +++ b/lib/librte_kvargs/rte_kvargs.h > >> @@ -115,9 +115,9 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); > >> * Call a handler function for each key/value matching the key > >> * > >> * 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 > >> + * handler function with the given arg_name passing the value in the > >> * dictionary for that key and a given extra argument. If *kvlist* is NULL > >> - * function does nothing. > >> + * function does nothing and returns error. > >> * > >> * @param kvlist > >> * The rte_kvargs structure > >> @@ -131,7 +131,8 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); > >> * > >> * @return > >> * - 0 on success > >> - * - Negative on error > >> + * - Negative on error or > >> + * if *key_match* does not match an entry in *kvlist* > >> */ > >> int rte_kvargs_process(const struct rte_kvargs *kvlist, > >>const char *key_match, arg_handler_t handler, void *opaque_arg); > >> -- > >> 2.11.0 > > > > Regards, > Keith
Re: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist
> On Aug 9, 2017, at 8:53 AM, Ananyev, Konstantin > wrote: > > > >> -Original Message- >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Keith Wiles >> Sent: Wednesday, August 9, 2017 4:30 PM >> To: dev@dpdk.org >> Subject: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist >> >> rte_kvargs_process() should return error if the key >> is not found in the kvlist or kvlist is NULL. > > Looks like an API breakage for me… To me it is not an ABI breakage only fixing the routine to return the correct status. > Could you probably explain why do you think it is necessary? For the no key found surely seems like an error to me, but I guess you can always require someone to call the rte_kvargs_count() API first which is not stated any place. As for the kvlist being null that is an error IMO and returning success is not valid. But to save a huge discussion I rejected the patch in patchwork for now. > Konstantin > >> >> Minor documentation changes and update for when an >> error is returned. >> >> Signed-off-by: Keith Wiles >> --- >> lib/librte_kvargs/rte_kvargs.c | 7 --- >> lib/librte_kvargs/rte_kvargs.h | 7 --- >> 2 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c >> index 854ac83f5..c8e8f4b28 100755 >> --- a/lib/librte_kvargs/rte_kvargs.c >> +++ b/lib/librte_kvargs/rte_kvargs.c >> @@ -158,16 +158,17 @@ rte_kvargs_process(const struct rte_kvargs *kvlist, >> void *opaque_arg) >> { >> const struct rte_kvargs_pair *pair; >> -unsigned i; >> +unsigned int i, found = 0; >> >> for (i = 0; i < kvlist->count; i++) { >> pair = &kvlist->pairs[i]; >> -if (key_match == NULL || strcmp(pair->key, key_match) == 0) { >> +if (!key_match || strcmp(pair->key, key_match) == 0) { >> +found++; >> if ((*handler)(pair->key, pair->value, opaque_arg) < 0) >> return -1; >> } >> } >> -return 0; >> +return (!found) ? -1 : 0; >> } >> >> /* free the rte_kvargs structure */ >> diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h >> index 5821c726a..260d4db5b 100755 >> --- a/lib/librte_kvargs/rte_kvargs.h >> +++ b/lib/librte_kvargs/rte_kvargs.h >> @@ -115,9 +115,9 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); >> * Call a handler function for each key/value matching the key >> * >> * 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 >> + * handler function with the given arg_name passing the value in the >> * dictionary for that key and a given extra argument. If *kvlist* is NULL >> - * function does nothing. >> + * function does nothing and returns error. >> * >> * @param kvlist >> * The rte_kvargs structure >> @@ -131,7 +131,8 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); >> * >> * @return >> * - 0 on success >> - * - Negative on error >> + * - Negative on error or >> + * if *key_match* does not match an entry in *kvlist* >> */ >> int rte_kvargs_process(const struct rte_kvargs *kvlist, >> const char *key_match, arg_handler_t handler, void *opaque_arg); >> -- >> 2.11.0 > Regards, Keith
Re: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist
> -Original Message- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Keith Wiles > Sent: Wednesday, August 9, 2017 4:30 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist > > rte_kvargs_process() should return error if the key > is not found in the kvlist or kvlist is NULL. Looks like an API breakage for me... Could you probably explain why do you think it is necessary? Konstantin > > Minor documentation changes and update for when an > error is returned. > > Signed-off-by: Keith Wiles > --- > lib/librte_kvargs/rte_kvargs.c | 7 --- > lib/librte_kvargs/rte_kvargs.h | 7 --- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c > index 854ac83f5..c8e8f4b28 100755 > --- a/lib/librte_kvargs/rte_kvargs.c > +++ b/lib/librte_kvargs/rte_kvargs.c > @@ -158,16 +158,17 @@ rte_kvargs_process(const struct rte_kvargs *kvlist, > void *opaque_arg) > { > const struct rte_kvargs_pair *pair; > - unsigned i; > + unsigned int i, found = 0; > > for (i = 0; i < kvlist->count; i++) { > pair = &kvlist->pairs[i]; > - if (key_match == NULL || strcmp(pair->key, key_match) == 0) { > + if (!key_match || strcmp(pair->key, key_match) == 0) { > + found++; > if ((*handler)(pair->key, pair->value, opaque_arg) < 0) > return -1; > } > } > - return 0; > + return (!found) ? -1 : 0; > } > > /* free the rte_kvargs structure */ > diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h > index 5821c726a..260d4db5b 100755 > --- a/lib/librte_kvargs/rte_kvargs.h > +++ b/lib/librte_kvargs/rte_kvargs.h > @@ -115,9 +115,9 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); > * Call a handler function for each key/value matching the key > * > * 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 > + * handler function with the given arg_name passing the value in the > * dictionary for that key and a given extra argument. If *kvlist* is NULL > - * function does nothing. > + * function does nothing and returns error. > * > * @param kvlist > * The rte_kvargs structure > @@ -131,7 +131,8 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); > * > * @return > * - 0 on success > - * - Negative on error > + * - Negative on error or > + * if *key_match* does not match an entry in *kvlist* > */ > int rte_kvargs_process(const struct rte_kvargs *kvlist, > const char *key_match, arg_handler_t handler, void *opaque_arg); > -- > 2.11.0
[dpdk-dev] [PATCH] kvargs: return error if key not find in kvlist
rte_kvargs_process() should return error if the key is not found in the kvlist or kvlist is NULL. Minor documentation changes and update for when an error is returned. Signed-off-by: Keith Wiles --- lib/librte_kvargs/rte_kvargs.c | 7 --- lib/librte_kvargs/rte_kvargs.h | 7 --- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c index 854ac83f5..c8e8f4b28 100755 --- a/lib/librte_kvargs/rte_kvargs.c +++ b/lib/librte_kvargs/rte_kvargs.c @@ -158,16 +158,17 @@ rte_kvargs_process(const struct rte_kvargs *kvlist, void *opaque_arg) { const struct rte_kvargs_pair *pair; - unsigned i; + unsigned int i, found = 0; for (i = 0; i < kvlist->count; i++) { pair = &kvlist->pairs[i]; - if (key_match == NULL || strcmp(pair->key, key_match) == 0) { + if (!key_match || strcmp(pair->key, key_match) == 0) { + found++; if ((*handler)(pair->key, pair->value, opaque_arg) < 0) return -1; } } - return 0; + return (!found) ? -1 : 0; } /* free the rte_kvargs structure */ diff --git a/lib/librte_kvargs/rte_kvargs.h b/lib/librte_kvargs/rte_kvargs.h index 5821c726a..260d4db5b 100755 --- a/lib/librte_kvargs/rte_kvargs.h +++ b/lib/librte_kvargs/rte_kvargs.h @@ -115,9 +115,9 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); * Call a handler function for each key/value matching the key * * 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 + * handler function with the given arg_name passing the value in the * dictionary for that key and a given extra argument. If *kvlist* is NULL - * function does nothing. + * function does nothing and returns error. * * @param kvlist * The rte_kvargs structure @@ -131,7 +131,8 @@ void rte_kvargs_free(struct rte_kvargs *kvlist); * * @return * - 0 on success - * - Negative on error + * - Negative on error or + * if *key_match* does not match an entry in *kvlist* */ int rte_kvargs_process(const struct rte_kvargs *kvlist, const char *key_match, arg_handler_t handler, void *opaque_arg); -- 2.11.0