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

[george.dit@gmail.com]

Hi Adrien,

Thanks for your insights and sorry for omitting the cover letter (this is
my first patch in DPDK).
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?
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.

Best regards,
Georgios

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