From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id 155661B2BF for ; Thu, 16 Nov 2017 10:24:09 +0100 (CET) Received: by mail-wm0-f41.google.com with SMTP id 70so1098114wmf.1 for ; Thu, 16 Nov 2017 01:24:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=jteDq4voyEPJsjZskoy431dv6qRCtOxHjFtag0sBHCA=; b=unhZEkgOo+3ecaacMcNlbLz6S4gL2EXS2cTl0uwHmXJTOrrDPkLRGo/j1iY3TJqPVq ygBZTtOT3iW25ZLcccikYmdr+1xb5VPvN4zCGPeaByzzycZK9EWKUbP9gBC/h5optcuu yPZe6zGe9tGUZafIWTMx1LlWDl9X3TDJCQZQOsz3jM7+csotZGqgjRe1v5JqCTdtGeSb w+IegJhnJkdfWKVXe8bZzWUooDkJP0MLq1jUxOVi3HLL1UI7uoIzT6Jd9Dgtvz5nCex8 r3jccKVUZu1EdwxbW3wfOG8kY7iBoeDN44mikqEDu9eVUseJ5mwHN5RCFTszZ7//EsAR Q4Yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=jteDq4voyEPJsjZskoy431dv6qRCtOxHjFtag0sBHCA=; b=XaA4F+wWIs9Vl+90pNoeSeyrvgHKaQIE1jCdIY7yfl5SuhPwSCLKBfze82G8yUpz06 mdsF+duX7aEo5z9VXk5wUsSrCiwMf/cJB65iKvmujtCN/s4lDlbrm+rrvGNTryck4BFE iq5iFx085iK9gEmnJnQ5sV3wZhef+8j9gRaSFEWtjOPkECLPAUg7m1FX1jAO0MM7CJuc Cmhl7EFPCW5hVbLcvRVkcFCDLvM+NBdA0T7zFahq2+zxk1gWzpQvE1kobPpBsaZ48LFh mox3AfgtrHwx90ksoA/LGEnQESUVtJD+01JRtN8uoMyH+8WgYityw8oKWknY+PJi0UcT Te1w== X-Gm-Message-State: AJaThX4c+qgW21nAgFoFd/SPKW4q7xinKFqxzoxEHCedCrmUSHRlz9Kd PzvAP4T3952FjSh+iGZ3o6TlvA== X-Google-Smtp-Source: AGs4zMbGI0eDQs9LW03YVnnKgXyZUolh/uAlu+2fUKGBzFI3baXJNhSRQHoQls4VCiMFQNHkfP7YpQ== X-Received: by 10.80.165.139 with SMTP id a11mr1851492edc.95.1510824248597; Thu, 16 Nov 2017 01:24:08 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id h16sm644962edj.33.2017.11.16.01.24.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 16 Nov 2017 01:24:07 -0800 (PST) Date: Thu, 16 Nov 2017 10:23:56 +0100 From: Adrien Mazarguil To: "Wiles, Keith" Cc: Olivier Matz , "Wu, Jingjing" , "dev@dpdk.org" , Thomas Monjalon , "Yigit, Ferruh" Message-ID: <20171116092356.GM24849@6wind.com> References: <1510234868-31053-1-git-send-email-adrien.mazarguil@6wind.com> <48C2CD08-29E1-444C-BF60-B96BE58865A5@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <48C2CD08-29E1-444C-BF60-B96BE58865A5@intel.com> Subject: Re: [dpdk-dev] [RFC] cmdline: rework as a wrapper to libedit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Nov 2017 09:24:09 -0000 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 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