Re: [dpdk-dev] [PATCH] Add external parser support for unknown commands.

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Nov 04, 2014 at 04:52:48AM +0000, Wiles, Roger Keith wrote:
> 
> > On Nov 3, 2014, at 5:42 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > 
> > 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" <keith.wiles@windriver.com> wrote:
> >> 
> >>> 
> >>>> On Nov 3, 2014, at 10:06 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >>>> 
> >>>> On Mon, Nov 03, 2014 at 02:25:51PM +0000, Wiles, Roger Keith wrote:
> >>>>> 
> >>>>>> On Nov 3, 2014, at 8:16 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> >>>>>> 
> >>>>>> On Mon, Nov 03, 2014 at 02:08:46PM +0000, Wiles, Roger Keith wrote:
> >>>>>>> 
> >>>>>>>> On Nov 3, 2014, at 4:41 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> >>>>>>>> 
> >>>>>>>> On Sun, Nov 02, 2014 at 04:28:28PM -0600, Keith Wiles wrote:
> >>>>>>>>> Allow for a external parser to handle the command line if the
> >>>>>>>>> command is not found and the developer has called the routine
> >>>>>>>>> int cmdline_set_external_parser(struct cmdline * cl,
> >>>>>>>>>                             cmdline_external_parser_t parser);
> >>>>>>>>> function to set the function pointer.
> >>>>>>>>> 
> >>>>>>>>> The function for the external parser function should return CMDLINE_PARSE_NOMATCH
> >>>>>>>>> if not able to match the command requested or zero is handled.
> >>>>>>>>> 
> >>>>>>>>> Prototype of external routine:
> >>>>>>>>> int (*cmdline_external_parser_t)(struct cmdline * cl, const char * buy);
> >>>>>>>>> 
> >>>>>>>>> Signed-off-by: Keith Wiles <keith.wiles@windriver.com>
> >>>>>>>> 
> >>>>>>>> Hi Keith,
> >>>>>>>> 
> >>>>>>>> what is the expected use case for this? Is it for embedding other programming languages alongside the existing DPDK command-line or some other purpose? [Perhaps the use case could be called out in the patch description]
> >>>>>>> 
> >>>>>>> Hi Bruce,
> >>>>>>> 
> >>>>>>> I guess the external parser could be used for other programming languages, but the case I was looking at was to provide a default escape from the command line parser to allow my application to handle the commands not understood by the parser. Now that you point it out I could use something like ‘%<line-of-script-code>’ to execute a single line of script code, which is a good idea (thanks).
> >>>>>>> 
> >>>>>>> One case I am looking at is when you want to execute a command and do not want to add the support into the commands.c file for every possible 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 foo on the command line and execute the foo.lua having the application detect you want to load and run a Lua script after it has finished parsing for the builtin commands.
> >>>>>>> 
> >>>>>>> For Pktgen I had to add a command called ‘run <filename> <args…>’ 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 seems like a minor point, but it does make it easier to use and to support the features I want to support in my PoC.
> >>>>>>> 
> >>>>>>> Using this method you can just type the name instead of something like ‘run foo.lua’ or just ‘run foo’ and let the code figure out what to run. I have more plans for this features 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.
> >>>>>>> 
> >>>>>>> Does this help and do I really need to add all of this to the commit message :-)
> >>>>>>> 
> >>>>>> Thanks for the explanation. However, if you are looking to have 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?
> >>>>> 
> >>>>> 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 code. Think of this as a default case for the parser and to me that makes more 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.
> >>>>> 
> >>>>> The default case is the same behavior today, with giving a no match error unless they add the external parser.
> >>>> 
> >>>> It seems alot simpler than that to me.  Looking at the test applications, the
> >>>> command line parser expects the application to create an array of
> >>>> cmdline_parse_ctx_t structures to support new option parsing.  If 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,
> >>> 
> >>> A true language binding like Lua or one of those other languages :-) you are correct to believe binding directly using ‘C’ code is the right solution . In Pktgen I use Lua as the direct language binding and extend Lua with specific Pktgen functions.
> >>> 
> >>> What I am doing here is to add a default case to cmdline code, which just happens to allow me to parse the cmdline in the application. Being able to execute say a line of script code is not really the requirement IMO. Being able to extend the cmdline code with a default case is a good feature and allows the developer to extend cmdline for some simple cases. 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 embedded command line code base is not very easy to locate. 
> >>> 
> >>> Adding a true language binding really requires using code to extend the language as I did with Lua and Pktgen. It could have been done with any language I just picked Lua, but the patch does not really add support for a language other then giving some support for someone to handle the no_match case.
> >>> 
> >>> 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.
> >>> 
> >>> Thanks
> >>> ++Keith
> >>> 
> >>> PS. on a different topic I was thinking about suggesting and writing a patch to add Lua with DPDK specific binding and extensions. (also allowing those `other` languages too :-) Being able to use a scripting language and be able to call DPDK API’s could be useful. How useful not sure at this time. (If you want to talk about this topic please start a new thread).
> >>>> 
> >>>> Am I missing something?
> >>>> Neil
> >>>> 
> >>>> 
> >>>>>> 
> >>>>>> /Bruce
> >>>>> 
> >>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >>> 
> >>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> >> 
> >> 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 doesn't really even
> >> need to be in DPDK. Better off being part of some other library.
> >> 
> > Well, something needs to be there to parse the libraries' common options, though
> > I agree, making eal_cmdline just a registration frontend to getopt or
> > getopt_long would be sufficient.
> 
> 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.
> 
> I think this patch is fairly simple and I think we need a way to handle the default case. If someone could please review the patch, that would be great.
> 
I have an objection, specifically, that its not necessecary.  You can already
accomplish what you want to do by adding structures to the context array in the
cmdline structure.  I realize its not as easy as just adding an external parser
function, but its the designed way to add options.  This does little more than
add addition API surface without any real need.
Neil


> ++Keith 
> 
> > 
> > Neil
> > 
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>