On Wed, Apr 27, 2016 at 06:36:20AM +0000, Daniele Di Proietto wrote: > Thank you for your comments, Flavio!
Thank you for the patchset :-) > > On 26/04/2016 16:35, "Flavio Leitner" <f...@sysclose.org> wrote: > > >On Fri, Apr 15, 2016 at 05:02:36PM -0700, Daniele Di Proietto wrote: [...] > >> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h > >> new file mode 100644 > >> index 0000000..e668c44 > >> --- /dev/null > >> +++ b/lib/conntrack-private.h > >> @@ -0,0 +1,77 @@ > >> +/* > >> + * Copyright (c) 2015, 2016 Nicira, Inc. > >> + * > >> + * Licensed under the Apache License, Version 2.0 (the "License"); > >> + * you may not use this file except in compliance with the License. > >> + * You may obtain a copy of the License at: > >> + * > >> + * http://www.apache.org/licenses/LICENSE-2.0 > >> + * > >> + * Unless required by applicable law or agreed to in writing, software > >> + * distributed under the License is distributed on an "AS IS" BASIS, > >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > >> implied. > >> + * See the License for the specific language governing permissions and > >> + * limitations under the License. > >> + */ > >> + > >> +#ifndef CONNTRACK_PRIVATE_H > >> +#define CONNTRACK_PRIVATE_H 1 > >> + > >> +#include <sys/types.h> > >> +#include <netinet/in.h> > >> +#include <netinet/ip6.h> > >> + > >> +#include "hmap.h" > >> +#include "openvswitch/types.h" > >> +#include "packets.h" > >> +#include "unaligned.h" > >> + > >> +struct ct_addr { > >> + union { > >> + ovs_16aligned_be32 ipv4; > >> + union ovs_16aligned_in6_addr ipv6; > >> + ovs_be32 ipv4_aligned; > >> + struct in6_addr ipv6_aligned; > >> + }; > >> +}; > >> + > >> +struct ct_endpoint { > >> + struct ct_addr addr; > >> + ovs_be16 port; > >> +}; > >> + > >> +struct conn_key { > >> + struct ct_endpoint src; > >> + struct ct_endpoint dst; > >> + > >> + ovs_be16 dl_type; > >> + uint8_t nw_proto; > >> + uint16_t zone; > >> +}; > > > >Based on the above I presume we consider all IPs in the same space > >regardless of the vlan, correct? > > Yes, I think this is what the kernel connection tracker does. That's right. > >> + > >> +struct conn { > >> + struct conn_key key; > >> + struct conn_key rev_key; > >> + long long expiration; > >> + struct hmap_node node; > >> + uint32_t mark; > >> + ovs_u128 label; > >> +}; > >> + > >> +enum ct_update_res { > >> + CT_UPDATE_INVALID, > >> + CT_UPDATE_VALID, > >> + CT_UPDATE_NEW, > >> +}; > >> + > >> +struct ct_l4_proto { > >> + struct conn *(*new_conn)(struct dp_packet *pkt, long long now); > >> + bool (*valid_new)(struct dp_packet *pkt); > >> + enum ct_update_res (*conn_update)(struct conn *conn, struct dp_packet > >> *pkt, > >> + bool reply, long long now); > >> +}; > >> + > >> +extern struct ct_l4_proto ct_proto_tcp; > >> +extern struct ct_l4_proto ct_proto_other; > >> + > >> +#endif /* conntrack-private.h */ > >> diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c > >> new file mode 100644 > >> index 0000000..4d80038 > >> --- /dev/null > >> +++ b/lib/conntrack-tcp.c > >> @@ -0,0 +1,476 @@ > >> +/*- > >> + * Copyright (c) 2001 Daniel Hartmeier > >> + * Copyright (c) 2002 - 2008 Henning Brauer > >> + * Copyright (c) 2012 Gleb Smirnoff <gleb...@freebsd.org> > >> + * Copyright (c) 2015, 2016 Nicira, Inc. > >> + * All rights reserved. > >> + * > >> + * Redistribution and use in source and binary forms, with or without > >> + * modification, are permitted provided that the following conditions > >> + * are met: > >> + * > >> + * - Redistributions of source code must retain the above copyright > >> + * notice, this list of conditions and the following disclaimer. > >> + * - Redistributions in binary form must reproduce the above > >> + * copyright notice, this list of conditions and the following > >> + * disclaimer in the documentation and/or other materials provided > >> + * with the distribution. > >> + * > >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > >> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > >> + * COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, > >> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, > >> + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > >> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > >> + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > >> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN > >> + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > >> + * POSSIBILITY OF SUCH DAMAGE. > >> + * > >> + * Effort sponsored in part by the Defense Advanced Research Projects > >> + * Agency (DARPA) and Air Force Research Laboratory, Air Force > >> + * Materiel Command, USAF, under agreement number F30602-01-2-0537. > >> + * > >> + * $OpenBSD: pf.c,v 1.634 2009/02/27 12:37:45 henning Exp $ > >> + */ > >> + > >> +#include <config.h> > >> + > >> +#include "conntrack-private.h" > >> +#include "ct-dpif.h" > >> +#include "dp-packet.h" > >> +#include "util.h" > >> + > >> +struct tcp_peer { > >> + enum ct_dpif_tcp_state state; > >> + uint32_t seqlo; /* Max sequence number sent > >> */ > >> + uint32_t seqhi; /* Max the other end ACKd + > >> win */ > >> + uint16_t max_win; /* largest window (pre > >> scaling) */ > >> + uint8_t wscale; /* window scaling factor > >> */ > >> +}; > >> + > >> +struct conn_tcp { > >> + struct conn up; > >> + struct tcp_peer peer[2]; > >> +}; > >> + > >> +enum { > >> + TCPOPT_EOL, > >> + TCPOPT_NOP, > >> + TCPOPT_WINDOW = 3, > >> +}; > >> + > >> +/* TCP sequence numbers are 32 bit integers operated > >> + * on with modular arithmetic. These macros can be > >> + * used to compare such integers. */ > >> +#define SEQ_LT(a,b) ((int)((a)-(b)) < 0) > >> +#define SEQ_LEQ(a,b) ((int)((a)-(b)) <= 0) > >> +#define SEQ_GT(a,b) ((int)((a)-(b)) > 0) > >> +#define SEQ_GEQ(a,b) ((int)((a)-(b)) >= 0) > >> + > >> +#define SEQ_MIN(a, b) ((SEQ_LT(a, b)) ? (a) : (b)) > >> +#define SEQ_MAX(a, b) ((SEQ_GT(a, b)) ? (a) : (b)) > > > >I am okay to leave those here, but since they are generic > >operations, maybe we could define somewhere else and just > >redefine here as SEQ_ operations. > > I am fine both ways. Do you have a suggestion for the name > of the generic macro? s/SEQ/INT/ ? Then put somewhere common and just redefine here SEQ_LT() to be INT_LT()? I am not good with names :-) Just that it seems to be generic enough to be useful in other places. [...] > >> +static uint8_t > >> +tcp_get_wscale(const struct tcp_header *tcp) > >> +{ > >> + int len = TCP_OFFSET(tcp->tcp_ctl) * 4 - sizeof *tcp; > >> + const uint8_t *opt = (const uint8_t *)(tcp + 1); > >> + uint8_t wscale = 0; > >> + uint8_t optlen; > >> + > >> + while (len >= 3) { > >> + if (*opt == TCPOPT_EOL) { > >> + break; > >> + } > >> + switch (*opt) { > > > >Maybe we could do: > > > > case TCPOPT_EOL: > > break; > > I think > > The goal was to exit the while loop. I guess we could do > > case TCPOPT_EOL: > return wscale; That looks better to me. Thanks! fbl _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
