[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