> On Nov 16, 2017, at 1:23 AM, Adrien Mazarguil <adrien.mazarg...@6wind.com> > wrote: > > Hi Keith, > > On Wed, Nov 15, 2017 at 04:12:07AM +0000, Wiles, Keith wrote: >> >> >>> On Nov 9, 2017, at 5:43 AM, Adrien Mazarguil <adrien.mazarg...@6wind.com> >>> wrote: >>> >>> This patch removes all code associated with symbols not internally relied >>> on by other DPDK components, makes struct cmdline opaque and then proceeds >>> to re-implement the remaining functionality as a wrapper to the editline >>> library (also known as libedit) [1]. >>> >>> Besides adding a new external dependency to its users, its large impact on >>> librte_cmdline's API/ABI also warrants a major version number bump. >>> >>> While librte_cmdline served DPDK well all these years as a small, easy to >>> use and self-sufficient interactive command-line handler, it started to >>> show its limits with testpmd's flow (rte_flow) command, which required >>> support for dynamic tokens and very long commands. >>> >>> This is the main motivation behind this rework. Long commands often need to >>> be displayed on multiple lines, which are not properly supported by >>> librte_cmdline's limited terminal handling capabilities, resulting in a >>> rather frustrating user experience. >>> >>> Testpmd being one of the main tools used by PMD developers and given flow >>> command lines won't get any shorter, this issue had to be addressed. >>> >>> Three options were considered: >>> >>> - Fixing and enhancing librte_cmdline. >>> >>> The amount of work necessary to add support for edition on multiple lines >>> was deemed significant and the result would still have lacked in some >>> areas, such as working backspace/delete keys in all terminals (i.e. full >>> termcap support). >>> >>> - Making testpmd directly rely on a more capable library. >>> >>> All testpmd commands rely on the cmdline_parse interface provided by >>> librte_cmdline. This approach would have required either a complete >>> rewrite or importing the missing bits from librte_cmdline to wrap them >>> around the new library, which naturally led to... >>> >>> - Converting librte_cmdline as a wrapper to a more capable library. >>> >>> Let's be honest, interactive command line handling isn't what makes DPDK >>> shine. It's also far removed from its core functionality, but is still >>> necessary in order to easily implement test and example programs; the >>> cmdline_parse interface is particularly good at this. >>> >>> DPDK actually only relies on cmdline_parse. By removing all the other >>> unused interfaces, implementing what remains on top of a different >>> terminal-handling library would be quick and easy. >>> >>> This last approach was chosen for the stated reasons. Libedit is >>> well-known, BSD-licensed, widely available [2], used by many projects, does >>> everything needed and more [3]. >>> >>> This rework results in the following changes: >>> >>> - Removed circular buffer management interface for command history >>> (cmdline_cirbuf.c), command history being handled by libedit. >>> - Removed raw command-line interpreter (cmdline_rdline.c). >>> - Removed raw terminal handler (cmdline_vt100.c). >>> - Removed all test/example code for the above. >>> - Re-implemented high level interactive and non-interactive command-line >>> handlers (cmdline.c and cmdline_socket.c) on top of libedit using its >>> native interface, not its readline compatibility layer. >>> - Made struct cmdline opaque so that applications relying on librte_cmdline >>> do not need to include any libedit headers. >>> - The only visible change for most applications besides being linked to >>> libedit is they do not have to include cmdline_rdline.h anymore. >>> >>> As an added bonus, terminal resizing is now automatically handled. >>> >>> In the future, cmdline_parse could use libedit's advanced tokenizer as >>> well to interpret quoted strings and escape sequences. >>> >> >> I do agree that cmdline is getting pretty old and using libedit is one >> solution around the long commands, but it has a lot more problems IMO. > > Before going further, I'd like to put emphasis on the fact this RFC is not > pushing to retain librte_cmdline over your librte_cli proposal. Rather, it > removes about 30% of its code and shifts the blame to an external library > without modifying user applications. > > It started some time ago as a quick hack to improve user experience with the > flow command in testpmd using the least amount of time and effort, which I > only recently reformatted as a proper RFC in order to get feedback from the > community. > >> I do not agree it has severed DPDK well, just look at test-pmd and the hoops >> people have to jump thru to get a new command or variation of an existing >> command integrated into test-pmd it is very difficult. Also if you look at >> the command sets in test-pmd they are very odd in that similar commands can >> some times be set up completely different as cmdline is too rigid and >> difficult to use. > > Testpmd is indeed messy, but this is not librte_cmdline's fundamental fault > in my opinion, more likely the result of using a copy/paste approach to new > commands due to lack of time or interest in making things nicer than the > bare minimum to validate features. There's no design direction for it hence > the lack of uniformity in the command hierarchy. > >> I had decided to not use the circular buffer code in cmdline as it did have >> a few problems for what I wanted and decided to write a standard gap buffer >> scheme used in most editors for lines. I had looked at libedit at one point >> decided I did not want another dependence for DPDK. I expect even my version >> does not solve the long line problem, but we can convert to libedit. (and >> toss my pretty code :-) > > I'm not sure adding dependencies to DPDK is an issue anymore. Not in the > sense "there's so many of them already, no one will notice" but more with > the need to focus community efforts a bit more on what DPDK brings that > doesn't exist elsewhere. > > How many DPDK contributors are experts in termcap handling and would care to > invest time in this area, compared to say, squeezing the last drop of > performance out of their employer's HW? > > I understand you've invested a lot of effort in this but I think that even > if DPDK moves to librte_cli, the switch to libedit will be unavoidable. > > Keep in mind every time some feature will be requested, someone will raise > the question "why not move to libedit instead?" > >> Fixing the long line problem is a very minor issue compared to everything >> else wrong with cmdline. > > I beg to differ on this point, however the reason may not be obvious if > you are not familiar with the flow command (the main reason behind this > RFC). > > You should try it. It basically uses dynamic cmdline_parse tokens and help > strings which enables flexible arguments with contextual help (without > printing it for hundreds of unrelated commands) and more or less infinite > command lines. That was the only sane approach to interface rte_flow. > > My point is there's already a case today for long lines support, it's not > minor given rte_flow is bound to replace a lot of the legacy APIs and > associated testpmd commands (flow_director_* to name a few). > >> I would suggest we look at CLI and improve it instead. We can add libedit to >> CLI and then finish testing the CLI with test-pmd. The first time I >> converted test-pmd I did remove and simplify the commands, but I was afraid >> that would cause a lot of problems for testing and scripts that people have >> written, but it is possible to fix these problems too. >> >> I do not think fixing cmdline is the best answer and working to convert over >> to CLI is the better answer here. > > In truth I didn't do my homework. Before your reply I completely forgot > about the librte_cli proposal and related dpdk-draft-cli tree. It didn't > cross my mind to check it out before working on this RFC. > > I'm now aware of how much effort you put in this and what it takes to > reorder and reimplement all testpmd commands. That's huge. It seems like > we're fighting unrelated battles though. > > To summarize: > > - You don't like librte_cmdline for various reasons and provide librte_cli > as an alternative along a testpmd implementation. I assume the goal is to > remove librte_cmdline once every application has switched. > > - I don't mind librte_cmdline, but I don't expect it to grow nor to be used > by applications outside test programs in DPDK itself, hence I choose to > strip its unused parts and make the rest a wrapper to libedit without > modifying applications. > > Both are not incompatible, and since I think libedit will be unavoidable for > librte_cli, my approach can be seen as temporary until something replaces > librte_cmdline. In the meantime, users still benefit from much better > command line handling at no extra cost.
OK, I understand your points and not to state your work was in vain, but it would have been better if we had applied the effort to the CLI. I do not agree per-say that libedit is required for CLI as the only feature we need is to handle long lines and their are easier ways to do then using libedit. I was looking at the problem and I think we can handle long lines without libedit, I will try to put together an example soon. For now we can accept your patch for cmdline as it does add the support without much effort. > > -- > Adrien Mazarguil > 6WIND Regards, Keith
