On Tue, May 20, 2025 at 11:01 AM Dumitru Ceara <dce...@redhat.com> wrote:
> On 4/2/25 8:54 AM, Ales Musil via dev wrote:
> > Populate CT fields during CT action with original fields. This
> > is needed for tracking actions that use those fields.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
>
> Hi Ales,
>
> Sorry for the delay in reviewing this.
>
Hi Dumitru,
thank you for the review.
>
> > v2: Rebase on top of current main.
> > ---
> > utilities/ovn-trace.c | 24 +++++++++++++++++++++++-
> > 1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > index d25c612c7..18fd022c2 100644
> > --- a/utilities/ovn-trace.c
> > +++ b/utilities/ovn-trace.c
> > @@ -2433,6 +2433,19 @@ execute_dns_lookup(const struct ovnact_result
> *dl, struct flow *uflow,
> > "*** dns_lookup action not implemented");
> > }
> >
> > +/* Populate CT fields from the flow corresponding counterpart */
>
> Nit: missing dot at the end of the sentence.
>
> > +static void
> > +populate_ct_fields(struct flow *ct_flow, struct flow *original_flow)
> > +{
> > + ct_flow->ct_nw_proto = original_flow->nw_proto;
> > + /* L3 */
> > + ct_flow->ct_nw_src = original_flow->nw_src;
> > + ct_flow->ct_nw_dst = original_flow->nw_dst;
>
> What if the flow is for IPv6 packets? I guess we need a check on the
> eth type, similar to what we do in flow_clear_conntrack().
>
Good point, added in v3.
> > + /* L4 */
> > + ct_flow->ct_tp_src = original_flow->tp_src;
> > + ct_flow->ct_tp_dst = original_flow->tp_dst;
> > +}
> > +
> > static void
> > execute_ct_next(const struct ovnact_ct_next *ct_next,
> > const struct ovntrace_datapath *dp, struct flow *uflow,
> > @@ -2454,6 +2467,8 @@ execute_ct_next(const struct ovnact_ct_next
> *ct_next,
> > /* Trace the actions in the next table. */
> > struct flow ct_flow = *uflow;
> > ct_flow.ct_state = state;
> > + populate_ct_fields(&ct_flow, uflow);
> > +
> > trace__(dp, &ct_flow, ct_next->ltable, pipeline, &node->subs);
> >
> > /* Upon return, we will trace the actions following the ct action
> in the
> > @@ -2518,6 +2533,8 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
> > super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(&s));
> > ds_destroy(&s);
> >
> > + populate_ct_fields(&ct_flow, uflow);
> > +
> > /* Trace the actions in the next table. */
> > trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs);
> >
> > @@ -2542,6 +2559,8 @@ ct_commit_to_zone__(const struct
> ovnact_ct_commit_to_zone *ct_nat,
> > struct ovntrace_node *node = ovntrace_node_append(
> > super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(action));
> >
> > + populate_ct_fields(&ct_flow, uflow);
> > +
> > /* Trace the actions in the next table. */
> > trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs);
> >
> > @@ -2645,6 +2664,9 @@ execute_ct_lb(const struct ovnact_ct_lb *ct_lb,
> > ct_lb->ovnact.type == OVNACT_CT_LB_MARK ? "ct_lb_mark" :
> "ct_lb",
> > ds_cstr_ro(&comment));
> > ds_destroy(&comment);
> > +
> > + populate_ct_fields(&ct_lb_flow, uflow);
> > +
> > trace__(dp, &ct_lb_flow, ct_lb->ltable, pipeline, &node->subs);
> > }
> >
> > @@ -3710,7 +3732,7 @@ trace(const char *dp_s, const char *flow_s)
> >
> > return ds_steal_cstr(&output);
> > }
> > -
> > +
> > static void
> > ovntrace_exit(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > const char *argv[] OVS_UNUSED, void *exiting_)
>
> While at it, I think the execute_ct_snat_to_vip() might need some
> attention too. That's essentially a ct(commit, nat, src=...).
>
>
That function doesn't seem to reflect what is happening accurately.
I think we should fix this as a follow up.
> Regards,
> Dumitru
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
