[dpdk-dev] [PATCH v2 04/18] eal: add lightweight kvarg parsing utility

[Gaëtan Rivet]

On Fri, Mar 23, 2018 at 07:54:11AM -0400, Neil Horman wrote:
> On Fri, Mar 23, 2018 at 10:31:22AM +0100, Gaëtan Rivet wrote:
> > On Thu, Mar 22, 2018 at 08:53:49PM -0400, Neil Horman wrote:
> > > On Thu, Mar 22, 2018 at 05:27:51PM +0100, Gaëtan Rivet wrote:
> > > > On Thu, Mar 22, 2018 at 10:10:37AM -0400, Neil Horman wrote:
> > > > > On Wed, Mar 21, 2018 at 05:32:24PM +0000, Wiles, Keith wrote:
> > > > > > 
> > > > > > 
> > > > > > > On Mar 21, 2018, at 12:15 PM, Gaetan Rivet <gaetan.rivet at 6wind.com> wrote:
> > > > > > > 
> > > > > > > This library offers a quick way to parse parameters passed with a
> > > > > > > key=value syntax.
> > > > > > > 
> > > > > > > A single function is needed and finds the relevant element within the
> > > > > > > text. No dynamic allocation is performed. It is possible to chain the
> > > > > > > parsing of each pairs for quickly scanning a list.
> > > > > > > 
> > > > > > > This utility is private to the EAL and should allow avoiding having to
> > > > > > > move around the more complete librte_kvargs.
> > > > > > 
> > > > > > What is the big advantage with this code and the librte_kvargs code. Is it just no allocation, rte_kvargs needs to be build before parts of EAL or what?
> > > > > > 
> > > > > > My concern is we have now two flavors one in EAL and one in librte_kvargs, would it not be more reasonable to improve rte_kvargs to remove your objections? I am all for fast, better, stronger code :-)
> > > > > > 
> > > > > +1, this really doesn't make much sense to me.  Two parsing routines seems like
> > > > > its just asking for us to have to fix parsing bugs in two places.  If allocation
> > > > > is a concern, I don't see why you can't just change the malloc in
> > > > > rte_kvargs_parse to an automatic allocation on the stack, or a preallocation set
> > > > > of kvargs that can be shared from init time.
> > > > 
> > > > I think the existing allocation scheme is fine for other usages (in
> > > > drivers and so on). Not for what I wanted to do.
> > > > 
> > > Ok, but thats an adressable issue.  you can bifurcate the parse function to an
> > > internal function that accepts any preallocated kvargs struct, and export two
> > > wrapper functions, one which allocates the struct from the heap, another which
> > > allocated automatically on the stack.
> > > 
> > 
> > Sure, everything is possible.
> > 
> Ok.
> 
> > > > >                                               librte_kvargs isn't necessecarily
> > > > > the best parsing library ever, but its not bad, and it just seems wrong to go
> > > > > re-inventing the wheel.
> > > > > 
> > > > 
> > > > It serves a different purpose than the one I'm pursuing.
> > > > 
> > > > This helper is lightweight and private. If I wanted to integrate my
> > > > needs with librte_kvargs, I would be adding new functionalities, making
> > > > it more complex, and for a use-case that is useless for the vast
> > > > majority of users of the lib.
> > > > 
> > > Ok, to that end:
> > > 
> > > 1) Privacy is not an issue (at least from my understanding of what your doing).
> > > If we start with the assumption that librte_kvargs is capable of satisfying your
> > > needs (even if its not done in an optimal way), the fact that your version of
> > > the function is internal to the library doesn't seem overly relevant, unless
> > > theres something critical to that privacy that I'm missing.
> > > 
> > 
> > Privacy is only a point I brought up to say that the impact of this
> > function is minimal. People looking to parse their kvargs should not
> > have any ambiguity regarding how they should do so. Only librte_kvargs
> > is available.
> > 
> Ok, would you also council others developing dpdk apps to write their own
> parsing routines when what they needed was trivial for the existing library?
> You are people too :)
> 
> > > 2) Lightweight function  seems like something that can be integrated with
> > > librte_kvargs.  Looking at it, what may I ask in librte_kvargs is insufficiently
> > > non-performant for your needs, specifically?  We talked about the heap
> > > allocation above, is there something else? The string duplication perhaps?
> > > 
> > > 
> > 
> > Mostly the way to use it.
> > The filter strings are
> > bus=value,.../class=value,...
> > 
> > where either bus= list or class= list can be omitted, but at least one
> > must appear.
> > 
> Ok, so whats the problem with using librte_kvargs for that?  Is it that the list
> that acts as the value to the key isn't parsed out into its own set of tokens?
> That seems entirely addressable.
> 
> > I want to read a single kvarg. I do not want to parse the whole string.
> > the '/' signifies the end of the current layer.
> > 
> This makes it seem like librte_kvargs can handle this as a trivial case of its
> functionality.
> 
> > librte_kvargs does not care about those points. I cannot ask it to only
> > read either bus or class, as it would then throw an error for all the
> > other keys (which the EAL has necessarily no knowledge of).
> > 
> But you can ask it to read both, and within your libraries logic make the
> determination as to the validitiy of receiving both.  Alternatively you can
> modify the valid_keys check in kvargs to be a regex that matches on either bus
> or class, or accept an ignore parameter for keys that may appear but should be
> ignored in the light of other keys.  Theres lots of options here.
> 

No, I will not be adding regex parsing to express a set of acceptable
token :)

> > So I would need to:
> > 
> >   * Add a custom storage scheme
> >   * Add a custom parsing mode stopping at the first kvarg
> >   * Add an edge-case to ignore the '/', so as not to throw off the rest
> >     of the parsing (least it be considered part of the previous kvarg
> >     value field).
> > 
> > Seeing this, does adding those really specifics functionality help
> > librte_kvargs to be more useful and usable? I do not think so.
> > 
> I think you're overcomplicating this.
> How about enhancing librte_kvargs to make parsing configurable
> such that invalid keys get ignored rather than generate errors?
> 

Invalid keys can already be ignored, it's not an issue.

> > It would only serve to disrupt the library for a marginal use-case, with
> > the introduction of edge-cases that will blur the specs of the lib's
> > API, making it harder to avoid subtle bugs.
> > 
> What do you mean "disrupt the library"?  What is its purpose of a library if not
> to do the jobs we want it to?  If everyone created their own routine to do
> something that a library could do with some modifications, we'd be left with
> 1000 versions of the same routine.  If the existing library does 99% of what you
> want it to, lets ennumerate what the missing %1 is and make the change, not
> throw the baby out with the bathwater.
> 
> > Only way to do so sanely would be to add rte_parse_kv as part of
> > librte_kvargs, as is. But then the whole thing does not make sense IMO:
> > no one would care to use it, the maintainance effort is the same, the
> > likelyhood of bugs as well (but in the process we would disrupt the
> > distribution of librte_kvargs by moving it within the EAL).
> > 
> > I see no benefit to either solution.
> > 
> Again, thats an overcomplication.  As I read it, you have a need to interrogate
> a key/value string, whos contents may contain invalid keys (for your parsing
> purposes), and whos values may themselves be lists, correct?  If so, I don't see
> the problem in enhancing libkvargs to:
> 
> 1) Allow for the parsing routine to ignore invalid keys (or even ignore specific
> invalid keys, and trigger on any unknown keys)
> 
> 2) Allows for the subparsing of lists into their own set of tokens.
> 

What I would do if I wanted to use librte_kvargs, would be to duplicate
the input text, mangle it to respect the intended format, and feed it to
librte_kvargs for parsing. Then I would iterate over the pairs, and stop
on the two I'm concerned about.

What I dislike here:

  * I actually do the bulk of the parsing by hand (recognizing first a
    valid input, modifying it to respect the lib, and after iterating on
    the list of pairs and strcmp-ing the ones I want). This is approximately
    as complicated as the current helper function.

  * I have to move librte_kvargs in the EAL.

> > > > If that's really an issue, I'm better off simply removing rte_parse_kv
> > > > and writing the parsing by hand within my function. This would be ugly
> > > > and tedious, but less than moving librte_kvargs within EAL and changing
> > > > it to my needs.
> > > I don't think thats necessecary, I just think if you can ennumerate the items
> > > that are non-performant for your needs we can make some changes to librte_kvargs
> > > to optimize around them, or offer parsing options to avoid them, and in the
> > > process avoid some code duplication
> > > 
> > 
> > I think it makes sense to have specialized functions for specialized
> > use-cases, and forcing the code to be generic and work with all of them
> > will make it more complicated.
> > 
> This isn't specialized, its trivial.  Its just a trivial case that libkvargs
> isn't built to handle at the moment.  Lets get it there.
> 

My use-case is trivial. Putting it within the librte_kvargs makes it
more complicated to write. I hate this.

> > The genericity would only be worth it if people actually needed to parse
> > the device strings the same way I do. No one has any business doing so.
> > This genericity adds complexity and issues, without even being useful in
> > the first place.
> > 
> I really think you're trying to take a short cut here where none is needed, and
> I'm sorry, but I can't support that.

There are countably infinite solutions, we will probably reach one (and
it might even do what we want).

-- 
Gaëtan Rivet
6WIND