On Mar 28, 2014, at 1:26 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Mar 25, 2014 at 2:35 PM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
>> Following patch will be easier to reason about with separate
>> ovs_flow_cmd_new() and ovs_flow_cmd_set() functions.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> —
...
>> - reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
>> - info, OVS_FLOW_CMD_NEW,
>> false);
>> + ovs_lock();
>> + dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + error = -ENODEV;
>> + if (!dp)
>> + goto err_unlock_ovs;
>>
>> - /* Clear stats. */
>> - if (a[OVS_FLOW_ATTR_CLEAR])
>> - ovs_flow_stats_clear(flow);
>> + /* Check if this is a duplicate flow */
> This is not clear comment for cmd_set.
>
Will change to “ Check that the flow exists."
>> + flow = ovs_flow_tbl_lookup(&dp->table, &key);
>> + error = -ENOENT;
>> + if (!flow)
>> + goto err_unlock_ovs;
>> +
>> + /* The unmasked key has to be the same for flow updates. */
>> + if (!ovs_flow_cmp_unmasked_key(flow, &match))
>> + goto err_unlock_ovs;
>> +
>> + /* Update actions, if present. */
>> + if (acts) {
>> + struct sw_flow_actions *old_acts;
>> +
>> + old_acts = ovsl_dereference(flow->sf_acts);
>> + rcu_assign_pointer(flow->sf_acts, acts);
>> + ovs_nla_free_flow_actions(old_acts);
>> }
>> +
>> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
>> + info, OVS_FLOW_CMD_NEW, false);
> hy not use OVS_FLOW_CMD_SET, to build reply?
>
That could break backwards compatibility, as we have not done that in the past
either.
>> + /* Clear stats. */
>> + if (a[OVS_FLOW_ATTR_CLEAR])
>> + ovs_flow_stats_clear(flow);
>> ovs_unlock();
>>
>> if (reply) {
>> @@ -928,8 +1002,6 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
>> struct genl_info *info)
>> }
>> return 0;
>>
>> -err_flow_free:
>> - ovs_flow_free(flow, false);
>> err_unlock_ovs:
>> ovs_unlock();
>> err_kfree:
>> @@ -1081,7 +1153,7 @@ static struct genl_ops dp_flow_genl_ops[] = {
>> { .cmd = OVS_FLOW_CMD_NEW,
>> .flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. */
>> .policy = flow_policy,
>> - .doit = ovs_flow_cmd_new_or_set
>> + .doit = ovs_flow_cmd_new
>> },
>> { .cmd = OVS_FLOW_CMD_DEL,
>> .flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. */
>> @@ -1097,7 +1169,7 @@ static struct genl_ops dp_flow_genl_ops[] = {
>> { .cmd = OVS_FLOW_CMD_SET,
>> .flags = GENL_ADMIN_PERM, /* Requires CAP_NET_ADMIN privilege. */
>> .policy = flow_policy,
>> - .doit = ovs_flow_cmd_new_or_set,
>> + .doit = ovs_flow_cmd_set,
>> },
>> };
>>
> otherwise looks good.
> Acked-by: Pravin B Shelar <pshe...@nicira.com>
>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
