On 3/17/2023 2:43 AM, fengchengwen wrote: > On 2023/3/17 2:18, Ferruh Yigit wrote: >> On 3/14/2023 12:48 PM, Chengwen Feng wrote: >>> The rte_kvargs_process() was used to parse KV pairs, it also supports >>> to parse 'only keys' (e.g. socket_id) type. And the callback function >>> parameter 'value' is NULL when parsed 'only keys'. >>> >>> It may leads to segment fault when parse args with 'only key', this >>> patchset fixes rest of them. >>> >>> Chengwen Feng (5): >>> app/pdump: fix segment fault when parse args >>> net/memif: fix segment fault when parse devargs >>> net/pcap: fix segment fault when parse devargs >>> net/ring: fix segment fault when parse devargs >>> net/sfc: fix segment fault when parse devargs >> >> Hi Chengwen, >> >> Did you scan all `rte_kvargs_process()` instances? > > No, I was just looking at the modules I was concerned about. > I looked at it briefly, and some modules had the same problem. > >> >> >> And if there would be a way to tell kvargs that a value is expected (or >> not) this checks could be done in kvargs layer, I think this also can be >> to look at. > > Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI. > I also think about just set value = "" when only exist key, It could > perfectly solve the above segment scene. > But it also break the API's behavior. >
What about having a new API, like `rte_kvargs_process_extended()`, That gets an additional flag as parameter, which may have values like following to indicate if key expects a value or not: ARG_MAY_HAVE_VALUE --> "key=value" OR 'key' ARG_WITH_VALUE --> "key=value" ARG_NO_VALUE --> 'key' Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as `rte_kvargs_process()`. This way instead of adding checks, relevant usage can be replaced by `rte_kvargs_process_extended()`, this requires similar amount of change but code will be more clean I think. Do you think does this work? > > Or continue fix the exist code (about 10+ place more), > for new invoking, because the 'arg_handler_t' already well documented > (52ab17efdecf935792ee1d0cb749c0dbd536c083), > they'll take the initiative to prevent this. > > > Hope for more advise for the next. > >> . >>
