[dpdk-dev] [PATCH 1/3] app/testpmd: Moved cmdline_flow to librte_cmdline

[Adrien Mazarguil]

George,

I missed the original RFC thread [1][2] (you should have used it as a cover
letter for this series BTW) please see below.

On Tue, Jan 16, 2018 at 10:24:25AM +0100, Olivier Matz wrote:
> Hi,
> 
> > On Tue, Jan 16, 2018 at 9:40 AM Olivier Matz <olivier.matz at 6wind.com> wrote:
> >
> > On Tue, Jan 16, 2018 at 08:45:32AM +0000, george.dit at gmail.com wrote:
> > > Hi Georgios,
> > >
> > > On Mon, Jan 15, 2018 at 01:30:35AM +0000, Lu, Wenzhuo wrote:
> > > > Hi,
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Georgios Katsikas
> > > > > Sent: Saturday, January 13, 2018 5:01 AM
> > > > > To: olivier.matz at 6wind.com
> > > > > Cc: dev at dpdk.org; Georgios Katsikas <george.dit at gmail.com>
> > > > > Subject: [dpdk-dev] [PATCH 1/3] app/testpmd: Moved cmdline_flow to
> > > > > librte_cmdline
> > > > >
> > > > > Signed-off-by: Georgios Katsikas <george.dit at gmail.com>
> > > > Looks like a good idea to move this code to the lib.
> > > > cc Adrien the author of this file, app/test-pmd/cmdline_flow.c.
> > >
> > > If the command line parsing of rte_flow is something that has some
> > > chances to be shared among multiple applications, I agree it makes sense
> > > to move it in a library.
> > >
> > > However, my opinion is that it would be better to have a specific
> > > library for it, like librte_flow_cmdline, because I'm not sure that
> > > people linking with librte_cmdline always want to pull the rte_flow
> > > parsing code.

In my opinion the entire flow command parser has nothing to do outside
testpmd, it's way too specific, even if another application finds it useful.

Code duplication being a bad thing, your application could as well compile
or even #include app/test-pmd/cmdline_flow.c directly (not pretty, I know)
since it would have the same internal layout as testpmd. Testpmd's Makefile
could be modified to spit it out as a separate library if needed.

What could make more sense would be to extract the parser engine for
librte_cmdline's dynamic tokens (e.g. struct context, struct arg, struct
token) and possibly various generic helpers (e.g. parse_string,
parse_mac_addr), but not enum index nor token_list[] and friends which
define the layout of testpmd's flow command.

This would enable other flow-like commands without duplicating the engine
every time, however I'm still not sure if making it available outside
testpmd is a good idea. Extracting and making it fully generic will require
a considerable amount of work.

> > Hi Lu, Oliver,
> > 
> > Thanks for your feedback!
> > You have a point here, flow commands are only a subset of the parser.
> > Do you want me to create this new library and send another patch?
> 
> Let's first wait for Adrien's feedback, he can have counter-arguments.
> 
> > I guess I have to use librte_cmdline as a template/example for creating the
> > librte_flow_cmdline library.
> 
> It can be used as an example for Makefile and .map file.

I'm not opposed to the idea of exporting the parser engine after making it
properly generic, but please reconsider. Testpmd is an application made to
validate PMD functionality. The flow command implementation, as neat as it
is, remains a complex wrapper on top of the cmdline_parse API which wasn't
originally designed for dynamic tokens. Its syntax may evolve without
warning if deemed necessary. Making it public will subject it to exported
API/ABI constraints and likely impede future evolution.

Assuming your application is not dragging testpmd's legacy, I think it would
be wiser to re-implement a simpler flow command look-alike on top of a more
suitable command-line handling library if you like its syntax.

[1] http://dpdk.org/ml/archives/dev/2018-January/086872.html
[2] Message-ID: CAN9HtFDz+imqbCKfs6a0NE0W7iF8C+-KiNB0nCRywimspjfEDg at mail.gmail.com

-- 
Adrien Mazarguil
6WIND