22/03/2023 09:53, Ferruh Yigit: > On 3/22/2023 1:15 AM, fengchengwen wrote: > > On 2023/3/21 21:50, Ferruh Yigit wrote: > >> 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? > > > > Yes, it can work. > > > > But I think the introduction of new API adds some complexity. > > And a good API definition could more simpler. > > > > Other option is changing existing API, but that may be widely used and > changing it impacts applications, I don't think it worth.
I've planned a change in kvargs API 5 years ago and never did it: >From doc/guides/rel_notes/deprecation.rst: " * kvargs: The function ``rte_kvargs_process`` will get a new parameter for returning key match count. It will ease handling of no-match case. " > Of course we can live with as it is and add checks to the callback > functions, although I still believe a new 'process()' API is better idea.
