From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 804D91B2DF for ; Thu, 16 Nov 2017 17:48:59 +0100 (CET) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Nov 2017 08:48:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,404,1505804400"; d="scan'208";a="150176875" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga004.jf.intel.com with ESMTP; 16 Nov 2017 08:48:58 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 16 Nov 2017 08:48:58 -0800 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.51]) by FMSMSX113.amr.corp.intel.com ([169.254.13.251]) with mapi id 14.03.0319.002; Thu, 16 Nov 2017 08:48:57 -0800 From: "Wiles, Keith" To: Adrien Mazarguil CC: Olivier Matz , "Wu, Jingjing" , "dev@dpdk.org" , Thomas Monjalon , "Yigit, Ferruh" Thread-Topic: [dpdk-dev] [RFC] cmdline: rework as a wrapper to libedit Thread-Index: AQHTWWDZPY7QQo2jgkmExtAq8OdUd6MVYyeAgAHpdACAAHxVAA== Date: Thu, 16 Nov 2017 16:48:56 +0000 Message-ID: <0194E2C3-2A61-4C34-90C8-10D9D797067B@intel.com> References: <1510234868-31053-1-git-send-email-adrien.mazarguil@6wind.com> <48C2CD08-29E1-444C-BF60-B96BE58865A5@intel.com> <20171116092356.GM24849@6wind.com> In-Reply-To: <20171116092356.GM24849@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.241.224.135] Content-Type: text/plain; charset="us-ascii" Content-ID: <0D692D279DB04142BA526C439E494CBE@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 16:49:00 -0000 > On Nov 16, 2017, at 1:23 AM, Adrien Mazarguil wrote: >=20 > Hi Keith, >=20 > On Wed, Nov 15, 2017 at 04:12:07AM +0000, Wiles, Keith wrote: >>=20 >>=20 >>> On Nov 9, 2017, at 5:43 AM, Adrien Mazarguil wrote: >>>=20 >>> This patch removes all code associated with symbols not internally reli= ed >>> on by other DPDK components, makes struct cmdline opaque and then proce= eds >>> to re-implement the remaining functionality as a wrapper to the editlin= e >>> library (also known as libedit) [1]. >>>=20 >>> 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. >>>=20 >>> 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. >>>=20 >>> This is the main motivation behind this rework. Long commands often nee= d 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. >>>=20 >>> Testpmd being one of the main tools used by PMD developers and given fl= ow >>> command lines won't get any shorter, this issue had to be addressed. >>>=20 >>> Three options were considered: >>>=20 >>> - Fixing and enhancing librte_cmdline. >>>=20 >>> The amount of work necessary to add support for edition on multiple lin= es >>> was deemed significant and the result would still have lacked in some >>> areas, such as working backspace/delete keys in all terminals (i.e. ful= l >>> termcap support). >>>=20 >>> - Making testpmd directly rely on a more capable library. >>>=20 >>> 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... >>>=20 >>> - Converting librte_cmdline as a wrapper to a more capable library. >>>=20 >>> Let's be honest, interactive command line handling isn't what makes DPD= K >>> 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. >>>=20 >>> 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. >>>=20 >>> 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]. >>>=20 >>> This rework results in the following changes: >>>=20 >>> - 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-lin= e >>> 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_cmd= line >>> 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. >>>=20 >>> As an added bonus, terminal resizing is now automatically handled. >>>=20 >>> In the future, cmdline_parse could use libedit's advanced tokenizer as >>> well to interpret quoted strings and escape sequences. >>>=20 >>=20 >> I do agree that cmdline is getting pretty old and using libedit is one s= olution around the long commands, but it has a lot more problems IMO. >=20 > Before going further, I'd like to put emphasis on the fact this RFC is no= t > pushing to retain librte_cmdline over your librte_cli proposal. Rather, i= t > removes about 30% of its code and shifts the blame to an external library > without modifying user applications. >=20 > 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 t= he > community. >=20 >> I do not agree it has severed DPDK well, just look at test-pmd and the h= oops people have to jump thru to get a new command or variation of an exist= ing 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. >=20 > Testpmd is indeed messy, but this is not librte_cmdline's fundamental fau= lt > in my opinion, more likely the result of using a copy/paste approach to n= ew > 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 hen= ce > the lack of uniformity in the command hierarchy. >=20 >> I had decided to not use the circular buffer code in cmdline as it did h= ave a few problems for what I wanted and decided to write a standard gap bu= ffer 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 :-) >=20 > 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. >=20 > 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? >=20 > I understand you've invested a lot of effort in this but I think that eve= n > if DPDK moves to librte_cli, the switch to libedit will be unavoidable. >=20 > Keep in mind every time some feature will be requested, someone will rais= e > the question "why not move to libedit instead?" >=20 >> Fixing the long line problem is a very minor issue compared to everythin= g >> else wrong with cmdline. >=20 > 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). >=20 > You should try it. It basically uses dynamic cmdline_parse tokens and hel= p > 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. >=20 > 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). >=20 >> I would suggest we look at CLI and improve it instead. We can add libedi= t to CLI and then finish testing the CLI with test-pmd. The first time I co= nverted test-pmd I did remove and simplify the commands, but I was afraid t= hat would cause a lot of problems for testing and scripts that people have = written, but it is possible to fix these problems too. >>=20 >> I do not think fixing cmdline is the best answer and working to convert = over to CLI is the better answer here. >=20 > 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. >=20 > 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. >=20 > To summarize: >=20 > - You don't like librte_cmdline for various reasons and provide librte_cl= i > as an alternative along a testpmd implementation. I assume the goal is t= o > remove librte_cmdline once every application has switched. >=20 > - I don't mind librte_cmdline, but I don't expect it to grow nor to be us= ed > 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. >=20 > 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 a= gree per-say that libedit is required for CLI as the only feature we need i= s 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 l= ibedit, I will try to put together an example soon. For now we can accept your patch for cmdline as it does add the support wit= hout much effort. >=20 > --=20 > Adrien Mazarguil > 6WIND Regards, Keith