[ovs-dev] [PATCH v2 08/15] conntrack: Implement dumping to ct_entry.

[Daniele Di Proietto]

On 19/04/2016 14:49, "Joe Stringer" <joe at ovn.org> wrote:

>On 15 April 2016 at 17:02, Daniele Di Proietto <diproiettod at vmware.com> wrote:
>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>
><snip>
>
>> +static void
>> +conn_key_to_tuple(const struct conn_key *key, struct ct_dpif_tuple *tuple)
>> +{
>> +    if (key->dl_type == htons(ETH_TYPE_IP)) {
>> +        tuple->l3_type = AF_INET;
>> +    } else if (key->dl_type == htons(ETH_TYPE_IPV6)) {
>> +        tuple->l3_type = AF_INET6;
>> +    }
>> +    tuple->ip_proto = key->nw_proto;
>> +    ct_endpoint_to_ct_dpif_inet_addr(&key->src.addr, &tuple->src,
>> +                                     key->dl_type);
>> +    ct_endpoint_to_ct_dpif_inet_addr(&key->dst.addr, &tuple->dst,
>> +                                     key->dl_type);
>> +
>> +    if (key->nw_proto == IPPROTO_ICMP) {
>> +        tuple->icmp_id = key->src.port;
>> +        /* ICMP type and code are not tracked */
>> +        tuple->icmp_type = 0;
>> +        tuple->icmp_code = 0;
>> +    } else {
>> +        tuple->src_port = key->src.port;
>> +        tuple->dst_port = key->dst.port;
>> +    }
>
>I think in the Linux implementation, the original ICMP message's icmp
>type/code are stored/provided here. Might be a behaviour parity
>question.

Yes, but they're not part of the key.  I guess they could be tracked separately
with a conntrack-icmp module.

>
>> +}
>> +
>> +static void
>> +conn_to_ct_dpif_entry(const struct conn *conn, struct ct_dpif_entry *entry,
>> +                      long long now)
>> +{
>> +    struct ct_l4_proto *class;
>> +    long long expiration;
>> +    memset(entry, 0, sizeof *entry);
>> +    conn_key_to_tuple(&conn->key, &entry->tuple_orig);
>> +    conn_key_to_tuple(&conn->rev_key, &entry->tuple_reply);
>> +
>> +    entry->zone = conn->key.zone;
>> +    entry->mark = conn->mark;
>> +
>> +    memcpy(&entry->labels, &conn->label, sizeof(entry->labels));
>> +    /* Not implemented yet */
>> +    entry->timestamp.start = 0;
>> +    entry->timestamp.stop = 0;
>
>Is it better to reflect that timestamps are unsupported up to the user
>or simply report 0 when it's unsupported? (I don't have a particular
>preference, just asking the question)

I was leaning towards setting them to 0.  It is a special value which
will not be printed in ct_dpif_format_timestamp().