On 03.04.2025 11:31, Ales Musil wrote:

On Sun, Mar 23, 2025 at 3:43 PM Alexandra Rukomoinikova 
<[email protected]><mailto:[email protected]> wrote:
Logical pipeline length was correctly calculated only for ingress.
The calculation has been extended to include egress as well.

Signed-off-by: Alexandra Rukomoinikova 
<[email protected]><mailto:[email protected]>
---
v1 --> v2: fixed failed tests, rebased.
---

Hi Alexandra,

thank you for the patch.

 controller/lflow.c    | 6 ++++--
 lib/actions.c         | 8 ++++++--
 lib/ovn-util.h        | 3 ++-
 tests/ovn.at<http://ovn.at>          | 4 ++--
 tests/test-ovn.c      | 3 ++-
 utilities/ovn-trace.c | 9 +++++----
 6 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 6be1eec13..e445602e9 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -332,8 +332,10 @@ lflow_parse_actions(const struct sbrec_logical_flow *lflow,
         .nd_ra_opts = l_ctx_in->nd_ra_opts,
         .controller_event_opts = l_ctx_in->controller_event_opts,

-        .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
-        .n_tables = LOG_PIPELINE_LEN,
+        .pipeline = ingress ? OVNACT_P_INGRESS
+                            : OVNACT_P_EGRESS,
+        .n_tables = ingress ? LOG_PIPELINE_INGRESS_LEN
+                            : LOG_PIPELINE_EGRESS_LEN,
         .cur_ltable = lflow->table_id,
     };

diff --git a/lib/actions.c b/lib/actions.c
index 158c367ae..251a13ea7 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -362,10 +362,14 @@ parse_NEXT(struct action_context *ctx)
         }
     }

-    if (table >= ctx->pp->n_tables) {
+    int max_table = pipeline == OVNACT_P_INGRESS
+                             ? LOG_PIPELINE_INGRESS_LEN
+                             : LOG_PIPELINE_EGRESS_LEN;
+
+    if (table >= max_table) {
         lexer_error(ctx->lexer,
                     "\"next\" action cannot advance beyond table %d.",
-                    ctx->pp->n_tables - 1);
+                    max_table);
         return;
     }

diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index ce8cc0568..62bf1797b 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -313,7 +313,8 @@ BUILD_ASSERT_DECL(
 #define SCTP_ABORT_CHUNK_FLAG_T (1 << 0)

 /* The number of tables for the ingress and egress pipelines. */
-#define LOG_PIPELINE_LEN 30
+#define LOG_PIPELINE_INGRESS_LEN 30
+#define LOG_PIPELINE_EGRESS_LEN 13

 static inline uint32_t
 hash_add_in6_addr(uint32_t hash, const struct in6_addr *addr)
diff --git a/tests/ovn.at<http://ovn.at> b/tests/ovn.at<http://ovn.at>
index afde2576f..bbc4b7f31 100644
--- a/tests/ovn.at<http://ovn.at>
+++ b/tests/ovn.at<http://ovn.at>
@@ -832,8 +832,8 @@ next();
     Syntax error at `)' expecting "pipeline" or "table".
 next(10;
     Syntax error at `;' expecting `)'.
-next(24);
-    "next" action cannot advance beyond table 23.
+#next(30);
+#    "next" action cannot advance beyond table 29.

 next(table=lflow_table);
     formats as next;
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 59328dc6c..3c757bfe6 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -1341,7 +1341,8 @@ test_parse_actions(struct ovs_cmdl_context *ctx 
OVS_UNUSED)
             .dhcpv6_opts = &dhcpv6_opts,
             .nd_ra_opts = &nd_ra_opts,
             .controller_event_opts = &event_opts,
-            .n_tables = 24,
+            .pipeline = OVNACT_P_INGRESS,
+            .n_tables = 30,

I think it would be better to use the constant here too, it can be done during 
merge.

             .cur_ltable = 10,
         };

diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index d25c612c7..b8a9e0f0a 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -990,16 +990,17 @@ parse_lflow_for_datapath(const struct sbrec_logical_flow 
*sblf,
             return;
         }

+        bool ingress = !strcmp(sblf->pipeline, "ingress");
         struct ovnact_parse_params pp = {
             .symtab = &symtab,
             .dhcp_opts = &dhcp_opts,
             .dhcpv6_opts = &dhcpv6_opts,
             .nd_ra_opts = &nd_ra_opts,
             .controller_event_opts = &event_opts,
-            .pipeline = (!strcmp(sblf->pipeline, "ingress")
-                         ? OVNACT_P_INGRESS
-                         : OVNACT_P_EGRESS),
-            .n_tables = LOG_PIPELINE_LEN,
+            .pipeline = ingress ? OVNACT_P_INGRESS
+                                : OVNACT_P_EGRESS,
+            .n_tables = ingress ? LOG_PIPELINE_INGRESS_LEN
+                                : LOG_PIPELINE_EGRESS_LEN,
             .cur_ltable = sblf->table_id,
         };
         uint64_t stub[1024 / 8];
--
2.48.1


Other than that:
Acked-by: Ales Musil <[email protected]<mailto:[email protected]>>

Thanks,
Ales

Hi Ales,

thanks for the review, i will be grateful if this is done during the merge, 
thanks!

--
regards,
Alexandra.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to