On Aug 27, 2013, at 10:43 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Aug 27, 2013 at 04:03:18PM -0700, Jarno Rajahalme wrote:
>>
>> On Aug 27, 2013, at 11:12 AM, Ben Pfaff <b...@nicira.com> wrote:
>>
>>> Suggested-by: pritesh <pritesh.koth...@cisco.com>
>>> Signed-off-by: Ben Pfaff <b...@nicira.com>
>>> ---
>>> ofproto/tunnel.c | 100
>>> +++++++++++++++++++++++++-----------------------------
>>> 1 file changed, 46 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
>>> index 202358b..4729f5f 100644
>>> --- a/ofproto/tunnel.c
>>> +++ b/ofproto/tunnel.c
>> ...
>>> +/* Returns the tnl_port that is the best match for the tunnel data in
>>> 'flow',
>>> + * or NULL if no tnl_port matches 'flow'. */
>>> static struct tnl_port *
>>> -tnl_find(struct tnl_match *match_) OVS_REQ_RDLOCK(rwlock)
>>> +tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
>>> {
>>> - struct tnl_match match = *match_;
>>> - struct tnl_port *tnl_port;
>>> + 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. */
>>> + };
>>> +
>>
>> Sorry to mention this now rather than sooner, but the above would be
>> even more self explanatory if the booleans were replaced with enums,
>> like this:
>>
>> + static const struct tnl_match_pattern patterns[] = {
>> + { KEY_CFG, IP_DST_CFG, IP_SRC_CFG }, /* remote_ip, local_ip,
>> in_key. */
>> + { KEY_CFG, IP_DST_CFG, IP_SRC_ANY }, /* remote_ip, in_key. */
>> + { KEY_FLOW, IP_DST_CFG, IP_SRC_CFG }, /* remote_ip, local_ip. */
>> + { KEY_FLOW, IP_DST_CFG, IP_SRC_ANY }, /* remote_ip. */
>> + { KEY_FLOW, IP_DST_FLOW, IP_SRC_ANY }, /* Flow-based remote. */
>> + { KEY_FLOW, IP_DST_FLOW, IP_SRC_FLOW }, /* Flow-based
>> everything. */
>> + };
>>
>> The key and dst fields could still be used like booleans, so no code
>> needs to change because of this.
>
> Hmm. I liked the idea up to here. I have a feeling that just using the
> enums as booleans would make the code harder to understand, not easier.
>
Then instead of:
match.in_key_flow = p->in_key_flow;
match.in_key = p->in_key_flow ? 0 : flow->tunnel.tun_id;
you could write:
match.in_key_flow = p->in_key_type == KEY_FLOW;
match.in_key = p->in_key_type == KEY_CFG ? flow->tunnel.tun_id : 0;
But then that is still confusing: Why tunnel ID in the 'flow' is only used when
in_key_type
is not KEY_FLOW? The answer here is that the 'flow' represents the incoming
packet (with metadata), and KEY_FLOW means that key matching will be done by
the flow entries later. Maybe rename 'flow' argument as 'packet'...
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
