Re: [PATCH 0/5] fix segment fault when parse args

[Thomas Monjalon <thomas@monjalon.net>]

23/03/2023 12:58, fengchengwen:
> On 2023/3/22 21:49, Thomas Monjalon wrote:
> > 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.
> > "
> 
> I think it's okay to add extra parameter for rte_kvargs_process. But it will
> break ABI.
> Also I notice patchset was deferred in patchwork.
> 
> Does it mean that the new version can't accept until the 23.11 release cycle ?

It is a bit too late to take a decision in 23.03 cycle.
Let's continue this discussion.
We can either have some fixes in 23.07 or have an ABI breaking change in 23.11.