On 6/11/24 00:07, Dumitru Ceara wrote: > On 6/10/24 18:05, [email protected] wrote: >> On Mon, 2024-06-10 at 12:54 +0000, Naveen Yerramneni wrote: >>> Action length is getting set incorrectly during ct_commit encode >>> due to which ct action is getting skipped during phsycial flows >>> creation. This issue is noticed only if ct_commit is prefixed >>> with other actions. >>> >>> logical flow: reg8[17] = 1; ct_commit { ct_mark.blocked = 1; }; >>> without fix: encodes as set_field:0x2000000000000/0x2000000000000- >>>> xreg4 >>> with fix: encodes as set_field:0x2000000000000/0x2000000000000- >>>> xreg4,ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1- >>>> ct_mark)) >>> >>> Signed-off-by: Naveen Yerramneni <[email protected]> >>> --- > > Thanks Naveen, for the catch! > >>> v1: > > Well, this should actually be v2 but that's a technicality. > >>> - Addressed review comments from Ales. >>> --- >>> lib/actions.c | 3 +++ >>> tests/ovn.at | 3 +++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 361d55009..794d2e916 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -760,6 +760,8 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >>> const struct ovnact_encode_params *ep >>> OVS_UNUSED, >>> struct ofpbuf *ofpacts) >>> { >>> + size_t ct_offset = ofpacts->size; >>> + ofpbuf_pull(ofpacts, ct_offset); > > Nit: missing new line (but I can fix that up at apply time). > >>> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >>> ct->flags = NX_CT_F_COMMIT; >>> ct->recirc_table = NX_CT_RECIRC_NONE; >>> @@ -791,6 +793,7 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, >>> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); >>> ct = ofpacts->header; >>> ofpact_finish(ofpacts, &ct->ofpact); >>> + ofpbuf_push_uninit(ofpacts, ct_offset); >> >> Just a thought here. I was just wondering if this wouldn't be a good >> opportunity to also replace the "manual" way of finishing the action >> encoding (as @amusil referred to it in this review [0]) with the more >> explicit approach (example included in [0]). >> > > I agree that we should do that but we should do it in multiple > functions, e.g.: > > encode_CT_NEXT() > encode_ct_nat() > encode_ct_lb() > > I think it's fine to do that in a follow up patch. Martin, do you have > time to post one? Otherwise I'll put it on my list of things to do in > the future. > > Regarding the current bug fix I'm planning to apply and backport this > patch, pending ovsrobot re-run results.
Tests passed. I applied this to main and all supported stable branches. Best regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
