From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH v2 04/18] eal: add lightweight kvarg parsing utility Date: Fri, 23 Mar 2018 14:12:36 +0100 Message-ID: <20180323131236.yagasxv2nxorfjl5@bidouze.vm.6wind.com> References: <20180322141037.GC6272@hmswarspite.think-freely.org> <20180322162751.z2kjkpjcg6aw76kk@bidouze.vm.6wind.com> <20180323005349.GC16562@neilslaptop.think-freely.org> <20180323093122.erjd7fxfj4locvt2@bidouze.vm.6wind.com> <20180323115411.GA14308@hmswarspite.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: "Wiles, Keith" , "dev@dpdk.org" To: Neil Horman Return-path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id CE7324D3A for ; Fri, 23 Mar 2018 14:12:53 +0100 (CET) Received: by mail-wm0-f46.google.com with SMTP id i75so3502421wmf.0 for ; Fri, 23 Mar 2018 06:12:53 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20180323115411.GA14308@hmswarspite.think-freely.org> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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