From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH] Add external parser support for unknown commands. Date: Tue, 4 Nov 2014 14:29:04 -0500 Message-ID: <20141104192904.GD9995@hmsreliant.think-freely.org> References: <307F2C60-7638-40C8-A9BC-DC3EE3D59F8C@windriver.com> <20141103141658.GA6964@bricha3-MOBL3> <7D39110F-D305-4C48-8BD6-37F6DCD2E434@windriver.com> <20141103160602.GA6625@localhost.localdomain> <6F166021-567F-4B30-8EAE-EDB68A86C6CC@windriver.com> <20141103152650.262e4da3@urahara> <20141103234230.GA2660@localhost.localdomain> <35090085-66D7-4F23-9867-A0DCC09E7DDD@windriver.com> <20141104112742.GB9995@hmsreliant.think-freely.org> <6F5C8CE3-5882-4222-BF8F-B8A29A3EB1BD@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" To: "Wiles, Roger Keith" Return-path: Content-Disposition: inline In-Reply-To: <6F5C8CE3-5882-4222-BF8F-B8A29A3EB1BD-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Tue, Nov 04, 2014 at 02:44:39PM +0000, Wiles, Roger Keith wrote: >=20 > > On Nov 4, 2014, at 5:27 AM, Neil Horman wrote= : > >=20 > > On Tue, Nov 04, 2014 at 04:52:48AM +0000, Wiles, Roger Keith wrote: > >>=20 > >>> On Nov 3, 2014, at 5:42 PM, Neil Horman wro= te: > >>>=20 > >>> On Mon, Nov 03, 2014 at 03:26:50PM -0800, Stephen Hemminger wrote: > >>>> On Mon, 3 Nov 2014 16:50:15 +0000 > >>>> "Wiles, Roger Keith" wrote: > >>>>=20 > >>>>>=20 > >>>>>> On Nov 3, 2014, at 10:06 AM, Neil Horman = wrote: > >>>>>>=20 > >>>>>> On Mon, Nov 03, 2014 at 02:25:51PM +0000, Wiles, Roger Keith wro= te: > >>>>>>>=20 > >>>>>>>> On Nov 3, 2014, at 8:16 AM, Bruce Richardson wrote: > >>>>>>>>=20 > >>>>>>>> On Mon, Nov 03, 2014 at 02:08:46PM +0000, Wiles, Roger Keith w= rote: > >>>>>>>>>=20 > >>>>>>>>>> On Nov 3, 2014, at 4:41 AM, Bruce Richardson wrote: > >>>>>>>>>>=20 > >>>>>>>>>> On Sun, Nov 02, 2014 at 04:28:28PM -0600, Keith Wiles wrote: > >>>>>>>>>>> Allow for a external parser to handle the command line if t= he > >>>>>>>>>>> command is not found and the developer has called the routi= ne > >>>>>>>>>>> int cmdline_set_external_parser(struct cmdline * cl, > >>>>>>>>>>> cmdline_external_parser_t parser= ); > >>>>>>>>>>> function to set the function pointer. > >>>>>>>>>>>=20 > >>>>>>>>>>> The function for the external parser function should return= CMDLINE_PARSE_NOMATCH > >>>>>>>>>>> if not able to match the command requested or zero is handl= ed. > >>>>>>>>>>>=20 > >>>>>>>>>>> Prototype of external routine: > >>>>>>>>>>> int (*cmdline_external_parser_t)(struct cmdline * cl, const= char * buy); > >>>>>>>>>>>=20 > >>>>>>>>>>> Signed-off-by: Keith Wiles > >>>>>>>>>>=20 > >>>>>>>>>> Hi Keith, > >>>>>>>>>>=20 > >>>>>>>>>> what is the expected use case for this? Is it for embedding = other programming languages alongside the existing DPDK command-line or s= ome other purpose? [Perhaps the use case could be called out in the patch= description] > >>>>>>>>>=20 > >>>>>>>>> Hi Bruce, > >>>>>>>>>=20 > >>>>>>>>> I guess the external parser could be used for other programmi= ng languages, but the case I was looking at was to provide a default esca= pe from the command line parser to allow my application to handle the com= mands not understood by the parser. Now that you point it out I could use= something like =E2=80=98%=E2=80=99 to execute a sin= gle line of script code, which is a good idea (thanks). > >>>>>>>>>=20 > >>>>>>>>> One case I am looking at is when you want to execute a comman= d and do not want to add the support into the commands.c file for every p= ossible command. Take the case where you have a bunch of scripts (Lua) in= a directory much like a bin directory. Then you could type foo.lua or fo= o on the command line and execute the foo.lua having the application dete= ct you want to load and run a Lua script after it has finished parsing fo= r the builtin commands. > >>>>>>>>>=20 > >>>>>>>>> For Pktgen I had to add a command called =E2=80=98run =E2=80=99 to support running a script with arguments. = I also needed to add a argvlist type to cmdline to not error out on that = command and split up the args into a argv list like format. (Maybe I need= to submit that code??) It seemed more straight forward to just pass the = command line to the application to run the command. I understand that see= ms like a minor point, but it does make it easier to use and to support t= he features I want to support in my PoC. > >>>>>>>>>=20 > >>>>>>>>> Using this method you can just type the name instead of somet= hing like =E2=80=98run foo.lua=E2=80=99 or just =E2=80=98run foo=E2=80=99= and let the code figure out what to run. I have more plans for this feat= ures as well and have not finished the basic PoC yet. If you want a peek = I can show you what I am working on currently. > >>>>>>>>>=20 > >>>>>>>>> Does this help and do I really need to add all of this to the= commit message :-) > >>>>>>>>>=20 > >>>>>>>> Thanks for the explanation. However, if you are looking to hav= e the application handle a bunch of commands itself, why does it need to = use the commandline library at all? Why not just have the app handle all = the commands instead of some of them? > >>>>>>>=20 > >>>>>>> I guess that would be reasonable, but then I would have to add = support for all of the command line parsing being done in the cmdline cod= e. Think of this as a default case for the parser and to me that makes mo= re sense then just doing my own command line design. In the cmdline code = you guys provided is a lot of features like history, control key support,= arg parsing (IP, MAC) and many others. I would rather not have to write = that code myself. > >>>>>>>=20 > >>>>>>> The default case is the same behavior today, with giving a no m= atch error unless they add the external parser. > >>>>>>=20 > >>>>>> It seems alot simpler than that to me. Looking at the test appl= ications, the > >>>>>> command line parser expects the application to create an array o= f > >>>>>> cmdline_parse_ctx_t structures to support new option parsing. I= f your goal is > >>>>>> to support other languages, it seems to make more sense to just = use foreign > >>>>>> language bindings to merge your coding language support with the= DPDK > >>>>>> (ostensibly you will already have to do that if you want to use = other parts of > >>>>>> the DPDK). > >>>>> Hi Neil, > >>>>>=20 > >>>>> A true language binding like Lua or one of those other languages = :-) you are correct to believe binding directly using =E2=80=98C=E2=80=99= code is the right solution . In Pktgen I use Lua as the direct language = binding and extend Lua with specific Pktgen functions. > >>>>>=20 > >>>>> What I am doing here is to add a default case to cmdline code, wh= ich just happens to allow me to parse the cmdline in the application. Bei= ng able to execute say a line of script code is not really the requiremen= t IMO. Being able to extend the cmdline code with a default case is a goo= d feature and allows the developer to extend cmdline for some simple case= s. The cmdline code is kind of simple, but does require a fair amount of = structures, code and understanding to write a complex extendable command = line interface. It does seem hard to find a clean, simple and usable embe= dded command line code base is not very easy to locate.=20 > >>>>>=20 > >>>>> Adding a true language binding really requires using code to exte= nd the language as I did with Lua and Pktgen. It could have been done wit= h any language I just picked Lua, but the patch does not really add suppo= rt for a language other then giving some support for someone to handle th= e no_match case. > >>>>>=20 > >>>>> The use case for this feature is not just for Pktgen, but another= solution I hope everyone will find useful when I get it more complete. > >>>>>=20 > >>>>> Thanks > >>>>> ++Keith > >>>>>=20 > >>>>> PS. on a different topic I was thinking about suggesting and writ= ing a patch to add Lua with DPDK specific binding and extensions. (also a= llowing those `other` languages too :-) Being able to use a scripting lan= guage and be able to call DPDK API=E2=80=99s could be useful. How useful = not sure at this time. (If you want to talk about this topic please start= a new thread). > >>>>>>=20 > >>>>>> Am I missing something? > >>>>>> Neil > >>>>>>=20 > >>>>>>=20 > >>>>>>>>=20 > >>>>>>>> /Bruce > >>>>>>>=20 > >>>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River= mobile 972-213-5533 > >>>>>=20 > >>>>> Keith Wiles, Principal Technologist with CTO office, Wind River m= obile 972-213-5533 > >>>>=20 > >>>> I wouldn't invest a lot of sweat in the command line parser. > >>>> The one in the DPDK is "good enough" for what it needs to do, but = really isn't > >>>> very complete and flexible. Seems like the kind of thing that does= n't really even > >>>> need to be in DPDK. Better off being part of some other library. > >>>>=20 > >>> Well, something needs to be there to parse the libraries' common op= tions, though > >>> I agree, making eal_cmdline just a registration frontend to getopt = or > >>> getopt_long would be sufficient. > >>=20 > >> Until we have a better command line solution, which I think would be= great, but in the mean time I would like to see this patch applied if no= one has a technical reason or better suggestion. > >>=20 > >> I think this patch is fairly simple and I think we need a way to han= dle the default case. If someone could please review the patch, that woul= d be great. > >>=20 > > I have an objection, specifically, that its not necessecary. You can= already > > accomplish what you want to do by adding structures to the context ar= ray in the > > cmdline structure. I realize its not as easy as just adding an exter= nal parser > > function, but its the designed way to add options. This does little = more than > > add addition API surface without any real need. > > Neil >=20 > Neil >=20 > I do not agree with your comments as I see it to be a small extension t= o cmdline to handle the case where I will have to possibly add a huge num= ber of commands/code to the cmdline structures. Using this method I am ab= le to add these very simple commands without having to add more code for = this use case. >=20 Can you provide a real example here? usnig vague terms like "huge" reall= y makes more of an emotional argument than a factual one. To cite an example the cmdline_test program adds a command line paramter that just lets you pars= e a number out of the command line. Silly, granted, but it serves the purpos= e. Its called cmd_num, and with functions and data all told, it looks like it ta= kes about 17 lines of code. Now thats more than what you're adding with you = patch, I grant you, but I assert that the "potentially huge" argument you're mak= ing above is false, especially whan you consider that some reasonably clever = coding can likely allow you to reuse function parsing fairly easily. > Lets say you have a directory on the disk that has possibly a 100 littl= e commands, without this minor change I would have to write 100 little st= ructures/code for cmdline to handle each case. Another option is to write= a single command to handle these commands. I used this method in Pktgen = and could do =E2=80=98run foo =E2=80=99 style commands, but it woul= d be much simpler for the user to just type =E2=80=98foo =E2=80=99 = instead. >=20 Yes, but thats honestly true of every command line parser, something need= s to define the tokens that identify the option, the function to handle its interpretation and the structures to glue it all together. That can be i= n the DPDK, or in something else. To use your example above, one presumes that= your 100 little commands all have some sort of parsing structure coded elsewhe= re, along with the code to do that parsing, right? You don't seem to take tha= t additional code into account here. If you can remove that parsing code f= rom your application, then it seems to me the additional 17 lines above reall= y isn't that big a deal. It seems to me that, what this boils down to is the fact that you have an application that needs to parse a single command line with two different = parser, whcih is just kind of a lousy situation, because parsers all assume that = they are the only thing parsing the command line. Honestly, I really question= the need for a command line parser so tightly integrated with DPDK at all. rte_eal_init really shouldn't be acepting a straight command line buffer.= As a library, it should more likely accept a configuration structure which it = just doles out to individual components during initalization. That can leave applications like yours to handle command line parsing 100% as you see fi= t, and then build the DPDK configuration from that as you like. > Having a default handler for commands just makes a lot of sense to me a= nd I do not buy the 'added API surface without any real need' statement. I'm not sure whats not to buy there. I see the above as facts: 1) You're adding API surface (you added a function that is exported from = the library) 2) You don't need to do (1) (theres an already existing alternate method = to do what you want) You can argue all day that your method is better, but it doesn't change t= he fact that (1) and (2) are true. And I don't want to go adding additional metho= ds to the existing ones as there will be a need to support them in the future, = and as such, If we're going to start deprecating API's in favor of superior desi= gns, I'd like to do that as infrequently as possible. Neil =20 >=20 > Thanks > ++Keith > >=20 > >=20 > >> ++Keith=20 > >>=20 > >>>=20 > >>> Neil > >>>=20 > >>=20 > >> Keith Wiles, Principal Technologist with CTO office, Wind River mobi= le 972-213-5533 >=20 > Keith Wiles, Principal Technologist with CTO office, Wind River mobile = 972-213-5533 >=20