[ovs-dev,4/9] ofproto/trace: Query ct_state for conntrack recirc from DP

Instead of using fixed default conntrack state 'trk|new' in
ofproto/trace for conntrack recirculation, this patch queries the
conntrack state from datapath using ct_dpif_get_info().

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 lib/ct-dpif.c                | 42 ++++++++++++++++++++++++++++++++++++++
 lib/ct-dpif.h                |  2 ++
 ofproto/ofproto-dpif-trace.c | 48 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 81 insertions(+), 11 deletions(-)

On 08/25/2017 03:51 PM, Yi-Hung Wei wrote:
> Instead of using fixed default conntrack state 'trk|new' in
> ofproto/trace for conntrack recirculation, this patch queries the
> conntrack state from datapath using ct_dpif_get_info().
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
>   lib/ct-dpif.c                | 42 ++++++++++++++++++++++++++++++++++++++
>   lib/ct-dpif.h                |  2 ++
>   ofproto/ofproto-dpif-trace.c | 48 ++++++++++++++++++++++++++++++++++----------
>   3 files changed, 81 insertions(+), 11 deletions(-)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 3e6f4cbe9be2..91388dd6681b 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -442,3 +442,45 @@ ct_dpif_format_tcp_stat(struct ds * ds, int tcp_state, int conn_per_state)
>       ds_put_cstr(ds, "]");
>       ds_put_format(ds, "=%u", conn_per_state);
>   }
> +
> +/* Converts a given 'flow' to conntrack 'tuple'. Returns true if the
> + * conversion is valid. Returns false and reports error in 'ds', if
> + * the type of the connection is not supported. */
> +bool
> +ct_dpif_flow_to_tuple(struct flow *flow, struct ct_dpif_tuple *tuple,
> +                      struct ds *ds)
> +{
> +    memset(tuple, 0, sizeof *tuple);

Are you sure this is necessary?  It looks to me like all the fields will be set in this function if it returns true.  If it returns false then presumably tuple would be ignored by the caller anyway.

Perhaps I'm missing something?

> +
> +    if (flow->dl_type == htons(ETH_TYPE_IP)) {
> +        tuple->l3_type = AF_INET;
> +        memcpy(&tuple->src.in, &flow->nw_src, sizeof tuple->src.in);
> +        memcpy(&tuple->dst.in, &flow->nw_dst, sizeof tuple->dst.in);
> +    } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
> +        tuple->l3_type = AF_INET6;
> +        memcpy(&tuple->src.in6, &flow->ipv6_src, sizeof tuple->src.in6);
> +        memcpy(&tuple->dst.in6, &flow->ipv6_dst, sizeof tuple->dst.in6);
> +    } else {
> +        ds_put_format(ds,
> +                      "Failed to convert flow to conntrack tuple "
> +                      "(Unsupported dl_type: %"PRIu16").",
> +                      ntohs(flow->dl_type));
> +        return false;
> +    }
> +
> +    tuple->ip_proto = flow->nw_proto;
> +
> +    /* Conntrack does support ICMP, however, we can not get ICMP id
> +     * from 'flow'. */
> +    if (flow->nw_proto == IPPROTO_TCP || flow->nw_proto == IPPROTO_UDP) {
> +        tuple->src_port = flow->tp_src;
> +        tuple->dst_port = flow->tp_dst;
> +    } else {
> +        ds_put_format(ds,
> +                      "Failed to convert flow to conntrack tuple "
> +                      "(Unsupported ip_proto: %"PRIu8").", flow->nw_proto);
> +        return false;
> +    }
> +
> +    return true;
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 0c82fb022f2b..f4ca07b5e776 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -17,6 +17,7 @@
>   #ifndef CT_DPIF_H
>   #define CT_DPIF_H
>
> +#include "flow.h"
>   #include "openvswitch/types.h"
>   #include "packets.h"
>
> @@ -209,5 +210,6 @@ void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
>   void ct_dpif_format_tuple(struct ds *, const struct ct_dpif_tuple *);
>   uint8_t ct_dpif_coalesce_tcp_state(uint8_t state);
>   void ct_dpif_format_tcp_stat(struct ds *, int, int);
> +bool ct_dpif_flow_to_tuple(struct flow *, struct ct_dpif_tuple *, struct ds *);
>
>   #endif /* CT_DPIF_H */
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index c3c929520a2d..a86cf211803e 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -18,7 +18,9 @@
>
>   #include "ofproto-dpif-trace.h"
>
> +#include <errno.h>
>   #include "conntrack.h"
> +#include "ct-dpif.h"
>   #include "dpif.h"
>   #include "ofproto-dpif-xlate.h"
>   #include "openvswitch/ofp-parse.h"
> @@ -645,20 +647,44 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
>                         recirc_node->recirc_id);
>
>           if (recirc_node->type == OFT_RECIRC_CONNTRACK) {
> -            uint32_t ct_state;
> +            struct ct_dpif_info ct_info;
> +            memset(&ct_info, 0, sizeof(ct_info));
> +

This memset doesn't seem necessary either so far as I can tell - but again, perhaps I'm missing something.

>               if (ovs_list_is_empty(next_ct_states)) {
> -                ct_state = CS_TRACKED | CS_NEW;
> -                ds_put_cstr(output, " - resume conntrack with default "
> -                            "ct_state=trk|new (use --ct-next to customize)");
> +                struct ds errs = DS_EMPTY_INITIALIZER;
> +                struct ct_dpif_tuple tuple;
> +                int err, indent_size;

The use of 'err' and 'errs' here is a little confusing to me.  Could the ds errs variable name be changed to something a bit
different?

> +
> +                indent_size = 14 + recirc_node->recirc_id / 16;
Why magic numbers 14 and 16?  What do they do?

> +                ds_put_cstr(output, " - resume conntrack with conntrack info"
> +                            "from datapath\n");
> +                ds_put_char_multiple(output, ' ', indent_size);
> +
> +                if (ct_dpif_flow_to_tuple(&recirc_node->flow, &tuple, &errs)) {
> +                    err = ct_dpif_get_info(ofproto->backer->dpif, &tuple,
> +                                            recirc_node->flow.ct_zone,
> +                                            &ct_info);
> +                    if (err) {
> +                        ds_put_format(&errs, "%s", ovs_strerror(err));
> +                    }
> +                }
> +
> +                if (errs.length) {

Why not just 'if (err)'  ?

> +                    ct_info.ct_state = CS_TRACKED | CS_NEW;
> +                    ds_put_format(output, "Failed to query ct_state from "
> +                                  "datapath (%s).\n", ds_cstr(&errs));
> +                    ds_put_char_multiple(output, ' ', indent_size);
> +                    ds_put_format(output, "Use default ct_state = trk|new.");
> +                } else {
> +                    ct_dpif_format_info(&ct_info, output);
> +                }
> +                ds_put_format(output, " (use --ct-next to customize)");
> +                ds_destroy(&errs);
>               } else {
> -                oftrace_pop_ct_state(next_ct_states, &ct_state);
> -                struct ds s = DS_EMPTY_INITIALIZER;
> -                format_flags(&s, ct_state_to_string, ct_state, '|');
> -                ds_put_format(output, " - resume conntrack with ct_state=%s",
> -                              ds_cstr(&s));
> -                ds_destroy(&s);
> +                oftrace_pop_ct_state(next_ct_states, &ct_info.ct_state);
> +                ct_dpif_format_info(&ct_info, output);
>               }
> -            recirc_node->flow.ct_state = ct_state;
> +            recirc_node->flow.ct_state = ct_info.ct_state;
>           }
>           ds_put_char(output, '\n');
>           ds_put_char_multiple(output, '=', 79);
>

On Fri, Aug 25, 2017 at 03:51:14PM -0700, Yi-Hung Wei wrote:
> Instead of using fixed default conntrack state 'trk|new' in
> ofproto/trace for conntrack recirculation, this patch queries the
> conntrack state from datapath using ct_dpif_get_info().
> 
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

I'm getting a patch reject trying to apply this.  Will you rebase and
re-post the series?

Thanks,

Ben.