Re: [dpdk-dev] [RFC] cmdline: rework as a wrapper to libedit

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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.mazarguil@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.

-- 
Adrien Mazarguil
6WIND