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

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

On Tue, Jan 16, 2018 at 03:54:57PM +0100, george.dit@gmail.com wrote:
> Hi Adrien,
> 
> Thanks for your insights and sorry for omitting the cover letter (this is
> my first patch in DPDK).

No problem, don't worry about that. Remember to put as much context as close
as possible to the related changes. The commit log of a patch is in fact a
more appropriate place since a cover letter is simply a summary of a
subsequent series and is discarded once applied.

> I understand your concerns. The reason I proposed this patch is to
> facilitate a more high level vendor agnostic API for configuring and
> monitoring DPDK-based NICs.
> 
> To avoid just copying thousands of lines of code into my application, do
> you think it is feasible to move at least the components (struct context,
> struct arg, struct token and the parse_* helpers) you mentioned and
> restructure testpmd in a way that allows applications to re-use all of its
> parsing features?

Yes I think it's feasible, although at the cost of great effort because
you'd need to untangle engine code from parser code to expose the former,
the flow command being a mix of both. Proper APIs must be devised for that,
hence my question: is it really worth the trouble?

Other contributors already asked me how they could reuse the flow command
parser to implement similar testpmd commands without copy/pasting the entire
file, so I'm already convinced separating at least the engine part makes
sense at the testpmd level. Moving it to librte_cmdline for external
applications seems more complex though.

> I could give it a try and come back with a new patch, otherwise I am
> perfectly ok if you want to do it instead.

While I'd certainly like to do it (at least at the testpmd level), it's
unlikely to happen anytime soon due to other priorities.

Feel free to take care of it if you're motivated enough, just keep in mind
right now I don't think this should be exposed as a public API. I can change
my mind if you manage to make it generic enough.

> On Tue, Jan 16, 2018 at 3:31 PM, Adrien Mazarguil <
> adrien.mazarguil@6wind.com> wrote:
> 
> > 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@6wind.com>
> > wrote:
> > > >
> > > > On Tue, Jan 16, 2018 at 08:45:32AM +0000, george.dit@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@dpdk.org] On Behalf Of Georgios
> > Katsikas
> > > > > > > Sent: Saturday, January 13, 2018 5:01 AM
> > > > > > > To: olivier.matz@6wind.com
> > > > > > > Cc: dev@dpdk.org; Georgios Katsikas <george.dit@gmail.com>
> > > > > > > Subject: [dpdk-dev] [PATCH 1/3] app/testpmd: Moved cmdline_flow
> > to
> > > > > > > librte_cmdline
> > > > > > >
> > > > > > > Signed-off-by: Georgios Katsikas <george.dit@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@mail.
> > gmail.com
> >
> > --
> > Adrien Mazarguil
> > 6WIND
> >
> 
> 
> 
> -- 
> Georgios Katsikas
> Industrial Ph.D. Student
> Network Intelligence Group
> Decision, Networks, and Analytics (DNA) Lab
> RISE SICS
> E-Mail:  georgios.katsikas@ri.se

-- 
Adrien Mazarguil
6WIND