From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 068A842A19; Sat, 29 Apr 2023 23:49:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CFB9040E01; Sat, 29 Apr 2023 23:49:58 +0200 (CEST) Received: from mail-io1-f53.google.com (mail-io1-f53.google.com [209.85.166.53]) by mails.dpdk.org (Postfix) with ESMTP id 6944B40A7D; Sat, 29 Apr 2023 23:49:58 +0200 (CEST) Received: by mail-io1-f53.google.com with SMTP id ca18e2360f4ac-763ae18986cso22435839f.3; Sat, 29 Apr 2023 14:49:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682804997; x=1685396997; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=tcdQlktnMNWUcqqOIdQfMSQ/DpcoFTY2UgyC9EuJIWk=; b=mlQGCyVe6DTs2l0HPKOEgXaw5SY+e1g+DgGTzX7qtgfmZzuIR/EOIZz+9CoMoOkhqX /DutRKUBbXdEGyJx0a+UV69WGaQQeXP7d5onYZmvBemYuJUvkFJOsgvNCY1Ae4tTcBh4 /rdwOuXmNWZ7jrRDnGHo0FaicPFoiGMBXFK/l7EfhVnuxzs+iGbCkjrSfpxg41ZZ/7Gq I71xpBrK5vdZL4W63hZvSZByt/aTLV56qxt9xwEW+O9pHP6mM5G7xaTZV3oKwkSP/Xmj VSmWk9vlZEa5C39xLq+2dqdpwJdpVJlCr6iZ7SINzrtACjISNhGJK8vni8xdFvUqebZw mwzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682804997; x=1685396997; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=tcdQlktnMNWUcqqOIdQfMSQ/DpcoFTY2UgyC9EuJIWk=; b=YUOZre+rtYth6Ob/diIf6bm2qj8Voo/6MNvPie8i4t/wONptXyB28QAqAuNiLf7bGI prqjtashVD+/c/fPFSwEFoYnDaK8+XMr+HBxnu1Ja3LGuMNCKmCmbZ2y97B+2pm03A0b S1AhkitrmCGm46nVQElvHfJgiZUXt9v5FNtSwUkcJ1DxKxYdmiq+Z2jGwL+4kB6Ek+is zeV1LCL7EkZIdPqApX6o9l7x3ZouaI13aKCreiJ4hK7IRX9R9vAbxkhfUc9HMvQ0MBqM 3nkfkeFXBujjUSXYnGAlQOnOExOY3hiiYyPj4KkielHaTEDxpUilFj75LDJiyNq6xLu3 POSw== X-Gm-Message-State: AC+VfDxtk1gMsySU9gJ2Ee+lQiNldYjIGRdm7EiNPnzQqs3OwGpTu3T/ QU7EprwkF4yxn07P+1/npRoh54cshJLSlQoVTeE= X-Google-Smtp-Source: ACHHUZ76mL1WrlTdW4Yjmx2/9VPVW78ryVQWWmMJIpmlZf88oqQ0JG5y2eAA5aMvhTjGuVt8b/y1bTeE+uuubvyGKrk= X-Received: by 2002:a05:6e02:104:b0:32a:82e9:23c1 with SMTP id t4-20020a056e02010400b0032a82e923c1mr6885346ilm.21.1682804997578; Sat, 29 Apr 2023 14:49:57 -0700 (PDT) MIME-Version: 1.0 References: <20230428170446.122c8775@hermes.local> <16663033.Ash8RoxBsO@thomas> In-Reply-To: <16663033.Ash8RoxBsO@thomas> From: Cliff Burdick Date: Sat, 29 Apr 2023 14:49:45 -0700 Message-ID: Subject: Re: Generic flow string parser To: Thomas Monjalon Cc: Tom Barbette , Stephen Hemminger , David Marchand , users , Ori Kam , dev@dpdk.org Content-Type: multipart/alternative; boundary="0000000000001b677705fa80920c" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000001b677705fa80920c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > So the only things we need are 2 functions, if I understand well: > > int rte_flow_to_text(const struct rte_flow*); > struct rte_flow *rte_flow_from_text(const char *); > > Here I assume the output of rte_flow_from_text() would be a created flow, > meaning it calls rte_flow_create() under the hood. > Is it what you wish? > Or should it fill port ID, attributes, patterns and actions? I think it should follow closely with what "flow_parse" already does: https://github.com/DPDK/dpdk/blob/d03446724972d2a1bb645ce7f3e64f5ef0203d61/= app/test-pmd/cmdline_flow.c#L11304 Namely, just do the part of populating attributes, patterns, and actions from a string. It's then up to the user if they want to create, validate, or do something else with it (like see how it populated the structures). The flow_parse function appears to take an opaque pointer that's specific to a structure inside of cmdline_flow.c and assign the attributes, actions, and patterns to members of that result struct. I don't know the reason for this, but when calling the function the user doesn't know how many patterns or actions their string will generate. They would either need to pass in structures that are allocated larger than needed, or have a separate API that returns how many actions and patterns are needed for a string, then they need to allocate the correct size themselves. I'm assuming it's not ideal to have the library itself do dynamic memory allocations for the correct size. On Sat, Apr 29, 2023 at 2:40=E2=80=AFPM Thomas Monjalon wrote: > This thread is an API suggestion, it should be discussed in > the developer mailing list (did the Cc here). > > 29/04/2023 16:23, Cliff Burdick: > > > Would rather the flow parser was rewritten as well. Doing open coded > > > parser is much more error prone and hard to extend versus writing the > > > parser in yacc/lex (ie bison/flex). > > > > I agree, and that's kind of why the original suggestion of using testpm= d > > came from. Writing a new parser is obviously the better choice and woul= d > > have been great if testpmd started that way, but a significant amount o= f > > time was invested in that method. Since it works and is tested, it didn= 't > > seem like a bad request to build off that and bring that code into an > rte_ > > API. I'd imagine building a proper parser would not just require the > parser > > piece, but also making sure all the tests now use that, and also the > legacy > > testpmd was converted. It seemed unlikely all of this could be done in = a > > reasonable amount of time and a lot of input from many people. Given th= e > > amount of debugging I (and others) have spent on figuring on why a flow > > spec didn't work properly, this could be a huge timesaver for new > projects > > like Tom mentioned. > > > > On Fri, Apr 28, 2023 at 5:04=E2=80=AFPM Stephen Hemminger < > > stephen@networkplumber.org> wrote: > > > > > On Fri, 28 Apr 2023 12:13:26 -0700 > > > Cliff Burdick wrote: > > > > > > > Hi Stephen, it would definitely not be worthwhile to repeat > everything > > > > that's already tested with testpmd. I was thinking that given that > there > > > > already is a "flow_parse" function that does almost everything > needed, > > > > something like that could be exposed. If we think of the testpmd fl= ow > > > > string as a sort of "IR" for string flow specification, that would > allow > > > > others to implement higher-level transform of a schema like JSON or > YAML > > > > into the testpmd language. Due to the complexity of testpmd and how > it's > > > > the source of true for testing flows, I think it's too great of an > ask to > > > > have testpmd support a new type of parsing. My only suggestion woul= d > be > > > > to take what already exists and expose it in a public API that is > included > > > > in a DPDK install. > > So the only things we need are 2 functions, if I understand well: > > int rte_flow_to_text(const struct rte_flow*); > struct rte_flow *rte_flow_from_text(const char *); > > Here I assume the output of rte_flow_from_text() would be a created flow, > meaning it calls rte_flow_create() under the hood. > Is it what you wish? > Or should it fill port ID, attributes, patterns and actions? > > > --0000000000001b677705fa80920c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
> So the only things we need are 2 functions, if I unde= rstand well:
>
> int rte_flow_to_text(const struct rte_flow*);<= br>> struct rte_flow *rte_flow_from_text(const char *);
>
> = Here I assume the output of rte_flow_from_text() would be a created flow,> meaning it calls rte_flow_create() under the hood.
> Is it wha= t you wish?
> Or should it fill port ID, attributes, patterns and act= ions?

I think it should follow closely with what &qu= ot;flow_parse" already does:=C2=A0https://github.com/DPDK/dpdk/blob/d03446724972d2a1bb645ce7f3e64f= 5ef0203d61/app/test-pmd/cmdline_flow.c#L11304

= Namely, just do the part of populating attributes, patterns, and actions fr= om a string. It's then up to the user if they want to create, validate,= or do something else with it (like see how it populated the structures). T= he flow_parse function appears to take an opaque pointer that's specifi= c to a structure inside of cmdline_flow.c and assign the attributes, action= s, and patterns to members of that result struct. I don't know the reas= on for this, but when calling the function the user doesn't know how ma= ny patterns or actions their string will generate. They would either need t= o pass in structures that are allocated larger than needed, or have a separ= ate API that returns how many actions and patterns are needed for a string,= then they need to allocate the correct size themselves. I'm assuming i= t's not ideal to have the library itself do dynamic memory allocations = for the correct size.

On Sat, Apr 29, 2023 at 2:40=E2=80=AFPM Thomas M= onjalon <thomas@monjalon.net&= gt; wrote:
This = thread is an API suggestion, it should be discussed in
the developer mailing list (did the Cc here).

29/04/2023 16:23, Cliff Burdick:
> > Would rather the flow parser was rewritten as well. Doing open co= ded
> > parser is much more error prone and hard to extend versus writing= the
> > parser in yacc/lex (ie bison/flex).
>
> I agree, and that's kind of why the original suggestion of using t= estpmd
> came from. Writing a new parser is obviously the better choice and wou= ld
> have been great if testpmd started that way, but a significant amount = of
> time was invested in that method. Since it works and is tested, it did= n't
> seem like a bad request to build off that and bring that code into an = rte_
> API. I'd imagine building a proper parser would not just require t= he parser
> piece, but also making sure all the tests now use that, and also the l= egacy
> testpmd was converted. It seemed unlikely all of this could be done in= a
> reasonable amount of time and a lot of input from many people. Given t= he
> amount of debugging I (and others) have spent on figuring on why a flo= w
> spec didn't work properly, this could be a huge timesaver for new = projects
> like Tom mentioned.
>
> On Fri, Apr 28, 2023 at 5:04=E2=80=AFPM Stephen Hemminger <
> stephe= n@networkplumber.org> wrote:
>
> > On Fri, 28 Apr 2023 12:13:26 -0700
> > Cliff Burdick <shaklee3@gmail.com> wrote:
> >
> > > Hi Stephen, it would definitely not be worthwhile to repeat = everything
> > > that's already tested with testpmd. I was thinking that = given that there
> > > already is a "flow_parse" function that does almos= t everything needed,
> > > something like that could be exposed. If we think of the tes= tpmd flow
> > > string as a sort of "IR" for string flow specifica= tion, that would allow
> > > others to implement higher-level transform of a schema like = JSON or YAML
> > > into the testpmd language. Due to the complexity of testpmd = and how it's
> > > the source of true for testing flows, I think it's too g= reat of an ask to
> > > have testpmd support a new type of parsing. My only suggesti= on would be
> > > to take what already exists and expose it in a public API th= at is included
> > > in a DPDK install.

So the only things we need are 2 functions, if I understand well:

int rte_flow_to_text(const struct rte_flow*);
struct rte_flow *rte_flow_from_text(const char *);

Here I assume the output of rte_flow_from_text() would be a created flow, meaning it calls rte_flow_create() under the hood.
Is it what you wish?
Or should it fill port ID, attributes, patterns and actions?


--0000000000001b677705fa80920c--