LGTM, thanks for helping with this clean-up. On Wed, Apr 3, 2024 at 7:43 AM Ales Musil <[email protected]> wrote:
> > > On Tue, Apr 2, 2024 at 5:31 PM Dumitru Ceara <[email protected]> wrote: > >> It's not used by any logical flow since v20.12.0. It was kept for >> backwards compatibility reasons in 4799f73fd6ed ("Fix OVN update >> issue when ovn-controller is updated first from 20.06 to 20.09.") but >> now there's no supported release that uses it. >> >> Therefore the action is safe to be removed. Update the tests to reflect >> the fact that the action is not supported anymore. >> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> include/ovn/actions.h | 9 +-- >> lib/actions.c | 141 ------------------------------------------ >> tests/ovn.at | 41 ++++-------- >> utilities/ovn-trace.c | 1 - >> 4 files changed, 12 insertions(+), 180 deletions(-) >> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index dcacbb1fff..8e794450cf 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -66,7 +66,7 @@ struct collector_set_ids; >> OVNACT(EXCHANGE, ovnact_move) \ >> OVNACT(DEC_TTL, ovnact_null) \ >> OVNACT(CT_NEXT, ovnact_ct_next) \ >> - OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ >> + /* CT_COMMIT_V1 is not supported anymore. */ \ >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >> OVNACT(CT_DNAT, ovnact_ct_nat) \ >> OVNACT(CT_SNAT, ovnact_ct_nat) \ >> @@ -260,13 +260,6 @@ struct ovnact_ct_next { >> uint8_t ltable; /* Logical table ID of next table. */ >> }; >> >> -/* OVNACT_CT_COMMIT_V1. */ >> -struct ovnact_ct_commit_v1 { >> - struct ovnact ovnact; >> - uint32_t ct_mark, ct_mark_mask; >> - ovs_be128 ct_label, ct_label_mask; >> -}; >> - >> /* Type of NAT used for the particular action. >> * UNSPEC translates to applying NAT that works for both directions. */ >> enum ovnact_ct_nat_type { >> diff --git a/lib/actions.c b/lib/actions.c >> index 71615fc53c..7c20535717 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -729,71 +729,12 @@ ovnact_ct_next_free(struct ovnact_ct_next *a >> OVS_UNUSED) >> { >> } >> >> -static void >> -parse_ct_commit_v1_arg(struct action_context *ctx, >> - struct ovnact_ct_commit_v1 *cc) >> -{ >> - if (lexer_match_id(ctx->lexer, "ct_mark")) { >> - if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { >> - return; >> - } >> - if (ctx->lexer->token.type == LEX_T_INTEGER) { >> - cc->ct_mark = ntohll(ctx->lexer->token.value.integer); >> - cc->ct_mark_mask = UINT32_MAX; >> - } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { >> - cc->ct_mark = ntohll(ctx->lexer->token.value.integer); >> - cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer); >> - } else { >> - lexer_syntax_error(ctx->lexer, "expecting integer"); >> - return; >> - } >> - lexer_get(ctx->lexer); >> - } else if (lexer_match_id(ctx->lexer, "ct_label")) { >> - if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { >> - return; >> - } >> - if (ctx->lexer->token.type == LEX_T_INTEGER) { >> - cc->ct_label = ctx->lexer->token.value.be128_int; >> - cc->ct_label_mask = OVS_BE128_MAX; >> - } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) { >> - cc->ct_label = ctx->lexer->token.value.be128_int; >> - cc->ct_label_mask = ctx->lexer->token.mask.be128_int; >> - } else { >> - lexer_syntax_error(ctx->lexer, "expecting integer"); >> - return; >> - } >> - lexer_get(ctx->lexer); >> - } else { >> - lexer_syntax_error(ctx->lexer, NULL); >> - } >> -} >> - >> -static void >> -parse_CT_COMMIT_V1(struct action_context *ctx) >> -{ >> - add_prerequisite(ctx, "ip"); >> - >> - struct ovnact_ct_commit_v1 *ct_commit = >> - ovnact_put_CT_COMMIT_V1(ctx->ovnacts); >> - if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { >> - while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { >> - parse_ct_commit_v1_arg(ctx, ct_commit); >> - if (ctx->lexer->error) { >> - return; >> - } >> - lexer_match(ctx->lexer, LEX_T_COMMA); >> - } >> - } >> -} >> - >> static void >> parse_CT_COMMIT(struct action_context *ctx) >> { >> if (ctx->lexer->token.type == LEX_T_LCURLY) { >> parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip", >> WR_CT_COMMIT); >> - } else if (ctx->lexer->token.type == LEX_T_LPAREN) { >> - parse_CT_COMMIT_V1(ctx); >> } else { >> /* Add an empty nested action to allow for "ct_commit;" syntax */ >> add_prerequisite(ctx, "ip"); >> @@ -804,88 +745,6 @@ parse_CT_COMMIT(struct action_context *ctx) >> } >> } >> >> -static void >> -format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, struct ds *s) >> -{ >> - ds_put_cstr(s, "ct_commit("); >> - if (cc->ct_mark_mask) { >> - ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark); >> - if (cc->ct_mark_mask != UINT32_MAX) { >> - ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask); >> - } >> - } >> - if (!ovs_be128_is_zero(cc->ct_label_mask)) { >> - if (ds_last(s) != '(') { >> - ds_put_cstr(s, ", "); >> - } >> - >> - ds_put_format(s, "ct_label="); >> - ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label); >> - if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) { >> - ds_put_char(s, '/'); >> - ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask); >> - } >> - } >> - if (!ds_chomp(s, '(')) { >> - ds_put_char(s, ')'); >> - } >> - ds_put_char(s, ';'); >> -} >> - >> -static void >> -encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, >> - const struct ovnact_encode_params *ep OVS_UNUSED, >> - struct ofpbuf *ofpacts) >> -{ >> - struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >> - ct->flags = NX_CT_F_COMMIT; >> - ct->recirc_table = NX_CT_RECIRC_NONE; >> - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >> - ct->zone_src.ofs = 0; >> - ct->zone_src.n_bits = 16; >> - >> - /* If the datapath supports all-zero SNAT then use it to avoid tuple >> - * collisions at commit time between NATed and firewalled-only >> sessions. >> - */ >> - >> - if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { >> - size_t nat_offset = ofpacts->size; >> - ofpbuf_pull(ofpacts, nat_offset); >> - >> - struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); >> - nat->flags = 0; >> - nat->range_af = AF_UNSPEC; >> - nat->flags |= NX_NAT_F_SRC; >> - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); >> - ct = ofpacts->header; >> - } >> - >> - size_t set_field_offset = ofpacts->size; >> - ofpbuf_pull(ofpacts, set_field_offset); >> - >> - if (cc->ct_mark_mask) { >> - const ovs_be32 value = htonl(cc->ct_mark); >> - const ovs_be32 mask = htonl(cc->ct_mark_mask); >> - ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, >> &mask); >> - } >> - >> - if (!ovs_be128_is_zero(cc->ct_label_mask)) { >> - ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), >> &cc->ct_label, >> - &cc->ct_label_mask); >> - } >> - >> - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >> - ct = ofpacts->header; >> - ofpact_finish(ofpacts, &ct->ofpact); >> -} >> - >> -static void >> -ovnact_ct_commit_v1_free(struct ovnact_ct_commit_v1 *cc OVS_UNUSED) >> -{ >> -} >> - >> - >> - >> static void >> format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s) >> { >> diff --git a/tests/ovn.at b/tests/ovn.at >> index dd2ebce98e..f05437b9cd 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -1318,47 +1318,28 @@ ct_commit { ip4.dst = 192.168.0.1; }; >> >> # Legact ct_commit_v1 action. >> ct_commit(); >> - formats as ct_commit; >> - encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_mark=1); >> - formats as ct_commit(ct_mark=0x1); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_mark)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_mark=1/1); >> - formats as ct_commit(ct_mark=0x1/0x1); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_mark)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_label=1); >> - formats as ct_commit(ct_label=0x1); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_label=1/1); >> - formats as ct_commit(ct_label=0x1/0x1); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_mark=1, ct_label=2); >> - formats as ct_commit(ct_mark=0x1, ct_label=0x2); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> >> ct_commit(ct_label=0x01020304050607080910111213141516); >> - formats as ct_commit(ct_label=0x1020304050607080910111213141516); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1020304050607080910111213141516->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_label=0x181716151413121110090807060504030201); >> - formats as ct_commit(ct_label=0x16151413121110090807060504030201); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x16151413121110090807060504030201->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> >> >> ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_label=18446744073709551615); >> - formats as ct_commit(ct_label=0xffffffffffffffff); >> - encodes as >> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0xffffffffffffffff->ct_label)) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_label=18446744073709551616); >> - Decimal constants must be less than 2**64. >> + Syntax error at `(' expecting `;'. >> >> ct_mark = 12345 >> Field ct_mark is not modifiable. >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >> index e0f1c3ec9a..5e55fbbcc0 100644 >> --- a/utilities/ovn-trace.c >> +++ b/utilities/ovn-trace.c >> @@ -3107,7 +3107,6 @@ trace_actions(const struct ovnact *ovnacts, size_t >> ovnacts_len, >> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, >> super); >> break; >> >> - case OVNACT_CT_COMMIT_V1: >> case OVNACT_CT_COMMIT_V2: >> /* Nothing to do. */ >> break; >> -- >> 2.44.0 >> >> > Makes sense, thanks! > > Acked-by: Ales Musil <[email protected]> > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > [email protected] > <https://red.ht/sig> > -- Best Regards, Martin Kalcok. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
