[ovs-dev] ip_src_flow

Jarno Rajahalme jrajahalme at nicira.com
Fri Aug 23 23:06:13 UTC 2013


On Aug 23, 2013, at 1:33 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Fri, Aug 23, 2013 at 12:46:47PM -0700, Jarno Rajahalme wrote:
>> On Aug 23, 2013, at 11:26 AM, Ben Pfaff <blp at nicira.com> wrote:
>>> Looking at tnl_find() in ofproto/tunnel.c, it maintains the invariant
>>> "set <field> to 0 if and only if we set <field>_flow to true", except
>>> for <field> = ip_src.  Is that a bug?
>>> 
>> 
>> No, it is not a bug. Tunnel config allows you to leave local address
>> unset (=0), allowing the selected route to determine the source
>> address to use. Even more, it looks like find_route() in kernel
>> might override the source address, even if it is given (in the
>> config, or in the flow with _flow set to true).
> 
> I see, thanks.
> 
> The code might be more obviously correct like this.  If anyone agrees
> then I'll check it over and write it up formally.

There is now maybe some duplicate initialization of different tnl_match
instances between this function and it's sole caller. Maybe pass in a
const flow * and pick up the pieces for each round from there instead?

  Jarno

> 
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 202358b..1559b16 100644
> *** a/ofproto/tunnel.c
> --- b/ofproto/tunnel.c
> ***************
> *** 416,466 ****
>  static struct tnl_port *
>  tnl_find(struct tnl_match *match_) OVS_REQ_RDLOCK(rwlock)
>  {
> !     struct tnl_match match = *match_;
> !     struct tnl_port *tnl_port;
> 
> !     /* remote_ip, local_ip, in_key */
> !     tnl_port = tnl_find_exact(&match);
> !     if (tnl_port) {
> !         return tnl_port;
> !     }
> 
> !     /* remote_ip, in_key */
> !     match.ip_src = 0;
> !     tnl_port = tnl_find_exact(&match);
> !     if (tnl_port) {
> !         return tnl_port;
> !     }
> !     match.ip_src = match_->ip_src;
> 
> !     /* remote_ip, local_ip */
> !     match.in_key = 0;
> !     match.in_key_flow = true;
> !     tnl_port = tnl_find_exact(&match);
> !     if (tnl_port) {
> !         return tnl_port;
> !     }
> 
> !     /* remote_ip */
> !     match.ip_src = 0;
> !     tnl_port = tnl_find_exact(&match);
> !     if (tnl_port) {
> !         return tnl_port;
> !     }
> 
> !     /* Flow-based remote */
> !     match.ip_dst = 0;
> !     match.ip_dst_flow = true;
> !     tnl_port = tnl_find_exact(&match);
> !     if (tnl_port) {
> !         return tnl_port;
> !     }
> 
> !     /* Flow-based everything */
> !     match.ip_src_flow = true;
> !     tnl_port = tnl_find_exact(&match);
> !     if (tnl_port) {
> !         return tnl_port;
>      }
> 
>      return NULL;
> --- 416,461 ----
>  static struct tnl_port *
>  tnl_find(struct tnl_match *match_) OVS_REQ_RDLOCK(rwlock)
>  {
> !     enum ip_src_type {
> !         IP_SRC_EXACT,           /* ip_src must match exactly. */
> !         IP_SRC_ANY,             /* Any ip_src is acceptable. */
> !         IP_SRC_FLOW             /* ip_src is handled in flow table. */
> !     };
> ! 
> !     struct tnl_match_pattern {
> !         bool in_key_flow;
> !         bool ip_dst_flow;
> !         enum ip_src_type ip_src;
> !     };
> ! 
> !     static const struct tnl_match_pattern patterns[] = {
> !         { false, false, IP_SRC_EXACT }, /* remote_ip, local_ip, in_key. */
> !         { false, false, IP_SRC_ANY },   /* remote_ip, in_key. */
> !         { true,  false, IP_SRC_EXACT }, /* remote_ip, local_ip. */
> !         { true,  false, IP_SRC_ANY },   /* remote_ip. */
> !         { true,  true,  IP_SRC_ANY },   /* Flow-based remote. */
> !         { true,  true,  IP_SRC_FLOW },  /* Flow-based everything. */
> !     };
> 
> !     struct tnl_match match = *match_;
> !     const struct tnl_match_pattern *p;
> 
> !     for (p = patterns; p < &patterns[ARRAY_SIZE(patterns)]; p++) {
> !         struct tnl_port *tnl_port;
> 
> !         match.in_key_flow = p->in_key_flow;
> !         match.in_key = p->in_key_flow ? 0 : match_->in_key;
> 
> !         match.ip_dst_flow = p->ip_dst_flow;
> !         match.ip_dst = p->ip_dst_flow ? 0 : match_->ip_dst_flow;
> 
> !         match.ip_src_flow = p->ip_src == IP_SRC_FLOW;
> !         match.ip_src = p->ip_src == IP_SRC_EXACT ? match_->ip_src : 0;
> 
> !         tnl_port = tnl_find_exact(&match);
> !         if (tnl_port) {
> !             return tnl_port;
> !         }
>      }
> 
>      return NULL;




More information about the dev mailing list