On 25/05/2016 07:19, "Fischetti, Antonio" <antonio.fische...@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Joe Stringer [mailto:j...@ovn.org]
>> Sent: Tuesday, May 24, 2016 7:26 PM
>> To: Fischetti, Antonio <antonio.fische...@intel.com>
>> Cc: Daniele Di Proietto <diproiet...@vmware.com>; dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v3 04/16] conntrack: New userspace
>> connection tracker.
>>
>> On 24 May 2016 at 07:19, Fischetti, Antonio
>> <antonio.fische...@intel.com> wrote:
>> > Hi Daniele, just a comment below.
>> > Apart from that, it looks good to me, thanks.
>> >
>> > Acked-by: Antonio Fischetti <antonio.fische...@intel.com>
>> >
>> >> -----Original Message-----
>> >> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of
>> Daniele
>> >> Di Proietto
>> >> Sent: Tuesday, May 17, 2016 1:56 AM
>> >> To: dev@openvswitch.org
>> >> Subject: [ovs-dev] [PATCH v3 04/16] conntrack: New userspace
>> >> connection tracker.
>> >>
>> >> This commit adds the conntrack module.
>> >>
>> >> It is a connection tracker that resides entirely in userspace.
>> Its
>> >> primary user will be the dpif-netdev datapath.
>> >>
>> >> The module main goal is to provide conntrack_execute(), which
>> offers
>> >> a
>> >> convenient interface to implement the datapath ct() action.
>> >>
>> >> The conntrack module uses two submodules to deal with the l4
>> protocol
>> >> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
>> >>
>> >> The conntrack-tcp submodule implementation is adapted from
>> FreeBSD's
>> >> pf
>> >> subsystem, therefore it's BSD licensed. It has been slightly
>> altered
>> >> to
>> >> match the OVS coding style and to allow the pickup of already
>> >> established connections.
>> >>
>> >> Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com>
>> >> ---
>> >> COPYING | 1 +
>> >> debian/copyright.in | 4 +
>> >> include/openvswitch/types.h | 4 +
>> >> lib/automake.mk | 5 +
>> >> lib/conntrack-other.c | 85 +++++
>> >> lib/conntrack-private.h | 88 +++++
>> >> lib/conntrack-tcp.c | 463 +++++++++++++++++++++++
>> >> lib/conntrack.c | 883
>> >> ++++++++++++++++++++++++++++++++++++++++++++
>> >> lib/conntrack.h | 151 ++++++++
>> >> lib/util.h | 9 +
>> >> 10 files changed, 1693 insertions(+)
>> >> create mode 100644 lib/conntrack-other.c
>> >> create mode 100644 lib/conntrack-private.h
>> >> create mode 100644 lib/conntrack-tcp.c
>> >> create mode 100644 lib/conntrack.c
>> >> create mode 100644 lib/conntrack.h
>> >>
[...]
>> >> +static struct conn *
>> >> +process_one(struct conntrack *ct, struct dp_packet *pkt,
>> >> + struct conn_lookup_ctx *ctx, uint16_t zone,
>> >> + bool commit, long long now)
>> >> +{
>> >> + unsigned bucket = hash_to_bucket(ctx->hash);
>> >> + struct conn *conn = ctx->conn;
>> >> + uint16_t state = 0;
>> >> +
>> >> + if (conn) {
>> >> + if (ctx->related) {
>> >> + state |= CS_RELATED;
>> >> + if (ctx->reply) {
>> >> + state |= CS_REPLY_DIR;
>> >> + }
>> >> + } else {
>> >> + enum ct_update_res res;
>> >> +
>> >> + res = conn_update(conn, pkt, ctx->reply, now);
>> >> +
>> >> + switch (res) {
>> >> + case CT_UPDATE_VALID:
>> >> + state |= CS_ESTABLISHED;
>> >> + if (ctx->reply) {
>> >> + state |= CS_REPLY_DIR;
>> >> + }
>> >> + break;
>> >> + case CT_UPDATE_INVALID:
>> >> + state |= CS_INVALID;
>> >> + break;
>> >> + case CT_UPDATE_NEW:
>> >> + hmap_remove(&ct->buckets[bucket].connections,
>> &conn-
>> >> >node);
>> >> + atomic_count_dec(&ct->n_conn);
>> >> + delete_conn(conn);
>> >> + conn = conn_not_found(ct, pkt, ctx, &state,
>> commit,
>> >> now);
>> >> + break;
>> >> + }
>> >
>> > [Antonio F] Sorry to repeat, but I'd prefer to add the 'default'
>> > case, here. I mean something like
>> >
>> > default:
>> > state |= CS_INVALID;
>> > break;
>> >
>> > I know if we add new items to enum ct_update_res we can get a
>> > warning from the compiler, but I wouldn't rely on that.
>>
>> If we're going to rely upon a default case that we don't expect to
>> hit, we should consider calling OVS_NOT_REACHED() rather than
>> treating the
>> traffic as invalid; this would be easier to track down than quietly
>> marking some traffic as invalid.
>
>[Antonio F] Both options looks fine to me, I just wanted to point out
>that it should be better to add a default case to cover any possible value
Ok, I will add a default label with OVS_NOT_REACHED().
Thanks for your review!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
