On 6 March 2017 at 16:22, Jarno Rajahalme <[email protected]> wrote: > Userspace support for datapath original direction conntrack tuple. > > Signed-off-by: Jarno Rajahalme <[email protected]>
Minor comments below, but otherwise: Acked-by: Joe Stringer <[email protected]> > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 677c0d2..8ae501b 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -159,12 +159,44 @@ static unsigned hash_to_bucket(uint32_t hash) > > static void > write_ct_md(struct dp_packet *pkt, uint16_t state, uint16_t zone, > - uint32_t mark, ovs_u128 label) > + const struct conn *conn, const struct conn_key *key) > { > pkt->md.ct_state = state | CS_TRACKED; > pkt->md.ct_zone = zone; > - pkt->md.ct_mark = mark; > - pkt->md.ct_label = label; > + pkt->md.ct_mark = conn ? conn->mark : 0; > + pkt->md.ct_label = conn ? conn->label : OVS_U128_ZERO; > + > + /* Use the original direction tuple if we have it. */ > + if (conn) { > + key = &conn->key; > + } > + pkt->md.ct_orig_tuple_ipv6 = false; > + if (key) { > + if (key->dl_type == htons(ETH_TYPE_IP)) { > + pkt->md.ct_orig_tuple.ipv4 = (struct ovs_key_ct_tuple_ipv4) { > + key->src.addr.ipv4_aligned, > + key->dst.addr.ipv4_aligned, > + key->nw_proto != IPPROTO_ICMP > + ? key->src.port : htons(key->src.icmp_type), I think the style usually lines up '?' and ':' so the two alternatives can be clearly seen. IMHO this is easier to read (yeah, I guess it doesn't come out right in email but it should be right in the code): key->nw_proto != IPPROTO_ICMP ? key->src.port : htons(key->src.icmp_type), > + key->nw_proto != IPPROTO_ICMP > + ? key->dst.port : htons(key->src.icmp_code), > + key->nw_proto, > + }; > + } else if (key->dl_type == htons(ETH_TYPE_IPV6)) { > + pkt->md.ct_orig_tuple_ipv6 = true; > + pkt->md.ct_orig_tuple.ipv6 = (struct ovs_key_ct_tuple_ipv6) { > + key->src.addr.ipv6_aligned, > + key->dst.addr.ipv6_aligned, > + key->nw_proto != IPPROTO_ICMPV6 > + ? key->src.port : htons(key->src.icmp_type), > + key->nw_proto != IPPROTO_ICMPV6 > + ? key->dst.port : htons(key->src.icmp_code), > + key->nw_proto, > + }; > + } > + } else { > + memset(&pkt->md.ct_orig_tuple, 0, sizeof pkt->md.ct_orig_tuple); > + } > } I think I missed this before, but it seems like 'key' is never used in this function. I think it could be simplified a bit. There are two callers: write_ct_md(pkt, state, zone, conn, &ctx->key); write_ct_md(pkts[i], CS_INVALID, zone, NULL, NULL); In the former case, 'conn' is specified, so within the function 'key' is set to &conn->key, then in the latter case both 'conn' and 'key' are NULL, so we only need to zero the whole orig_tuple. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
