| Message ID | 20240711093719.34377-1-martin.kalcok@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] actions: Explicitly finish CT actions. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Thu, Jul 11, 2024 at 11:37 AM Martin Kalcok <martin.kalcok@canonical.com> wrote: > As discussed here [0], a couple of functions that encode > CT-related actions were using older, manual, way of finishing > the action. > > As amusil mentioned here [1], there's a shorter and more explicit > way of doing it. This change replaces manual way with the more > explicit aproach. > > [0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html > [1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html > > Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> > --- > Hi Martin, thank you for the follow up. You have missed encode_CT_NEXT() and add_lb_ct_snat_hairpin_vip_flow() in lflow.c I have also a couple of comments down below. lib/actions.c | 36 +++++++++++++----------------------- > 1 file changed, 13 insertions(+), 23 deletions(-) > > diff --git a/lib/actions.c b/lib/actions.c > index e8cc0994d..51da6210f 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -761,7 +761,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > struct ofpbuf *ofpacts) > { > size_t ct_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, ct_offset); > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->flags = NX_CT_F_COMMIT; > @@ -777,24 +776,19 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > */ > 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; > + ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct); > This is not needed, it can be removed along with the nat_offset. > } > > - size_t set_field_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, set_field_offset); > - > ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); > - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > - ct = ofpacts->header; > - ofpact_finish(ofpacts, &ct->ofpact); > - ofpbuf_push_uninit(ofpacts, ct_offset); > + > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > } > > static void > @@ -1027,7 +1021,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > enum mf_field_id zone_src, struct ofpbuf *ofpacts) > { > const size_t ct_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, ct_offset); > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); > @@ -1040,7 +1033,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > struct ofpact_nat *nat; > size_t nat_offset; > nat_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, nat_offset); > > nat = ofpact_put_NAT(ofpacts); > nat->range_af = cn->family; > @@ -1081,13 +1073,13 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > } > } > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > - ct = ofpacts->header; > + ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct); > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > if (cn->commit) { > ct->flags |= NX_CT_F_COMMIT; > } > - ofpact_finish(ofpacts, &ct->ofpact); > - ofpbuf_push_uninit(ofpacts, ct_offset); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > This can be simplified just by moving the cn->commit if below the other init of ct at the start of the function. > } > > static void > @@ -1383,7 +1375,6 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > /* ct_lb without any destinations means that this is an > established > * connection and we just need to do a NAT. */ > const size_t ct_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, ct_offset); > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > struct ofpact_nat *nat; > @@ -1397,16 +1388,15 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > ct->alg = 0; > > nat_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, nat_offset); > > nat = ofpact_put_NAT(ofpacts); > nat->flags = 0; > nat->range_af = AF_UNSPEC; > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > - ct = ofpacts->header; > - ofpact_finish(ofpacts, &ct->ofpact); > - ofpbuf_push_uninit(ofpacts, ct_offset); > + ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct); > Once again the nat_offset is not needed. The assert with ct_offset is enough. + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > return; > } > > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > We should also add tests that do test the scenario when the encoded action is prefixed by another action for all updated actions e.g. ct_clear; ct_lb; encodes as ct_clear,ct(table=19,zone=NXM_NX_REG13[[0..15]],nat) has prereqs ip ct_clear; ct_lb; reg8[[7]] = 1; encodes as ct_clear,ct(table=19,zone=NXM_NX_REG13[[0..15]],nat),set_field:0x8000000000/0x8000000000->xreg4 has prereqs ip Thanks, Ales
diff --git a/lib/actions.c b/lib/actions.c index e8cc0994d..51da6210f 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -761,7 +761,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, struct ofpbuf *ofpacts) { size_t ct_offset = ofpacts->size; - ofpbuf_pull(ofpacts, ct_offset); struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->flags = NX_CT_F_COMMIT; @@ -777,24 +776,19 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, */ 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; + ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct); } - size_t set_field_offset = ofpacts->size; - ofpbuf_pull(ofpacts, set_field_offset); - ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); - ct = ofpacts->header; - ofpact_finish(ofpacts, &ct->ofpact); - ofpbuf_push_uninit(ofpacts, ct_offset); + + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); } static void @@ -1027,7 +1021,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, enum mf_field_id zone_src, struct ofpbuf *ofpacts) { const size_t ct_offset = ofpacts->size; - ofpbuf_pull(ofpacts, ct_offset); struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); @@ -1040,7 +1033,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, struct ofpact_nat *nat; size_t nat_offset; nat_offset = ofpacts->size; - ofpbuf_pull(ofpacts, nat_offset); nat = ofpact_put_NAT(ofpacts); nat->range_af = cn->family; @@ -1081,13 +1073,13 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, } } - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); - ct = ofpacts->header; + ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct); + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); if (cn->commit) { ct->flags |= NX_CT_F_COMMIT; } - ofpact_finish(ofpacts, &ct->ofpact); - ofpbuf_push_uninit(ofpacts, ct_offset); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); } static void @@ -1383,7 +1375,6 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, /* ct_lb without any destinations means that this is an established * connection and we just need to do a NAT. */ const size_t ct_offset = ofpacts->size; - ofpbuf_pull(ofpacts, ct_offset); struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); struct ofpact_nat *nat; @@ -1397,16 +1388,15 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, ct->alg = 0; nat_offset = ofpacts->size; - ofpbuf_pull(ofpacts, nat_offset); nat = ofpact_put_NAT(ofpacts); nat->flags = 0; nat->range_af = AF_UNSPEC; - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); - ct = ofpacts->header; - ofpact_finish(ofpacts, &ct->ofpact); - ofpbuf_push_uninit(ofpacts, ct_offset); + ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct); + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); return; }
As discussed here [0], a couple of functions that encode CT-related actions were using older, manual, way of finishing the action. As amusil mentioned here [1], there's a shorter and more explicit way of doing it. This change replaces manual way with the more explicit aproach. [0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html [1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> --- lib/actions.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-)