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>
> ---
> datapath/datapath.c | 160
> +++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 116 insertions(+), 44 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index a13208c..2901d69 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -805,16 +805,16 @@ static struct sk_buff *ovs_flow_cmd_build_info(const
> struct sw_flow *flow,
> return skb;
> }
>
> -static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info
> *info)
> +static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> {
> struct nlattr **a = info->attrs;
> struct ovs_header *ovs_header = info->userhdr;
> struct sw_flow_key key, masked_key;
> - struct sw_flow *flow = NULL;
> + struct sw_flow *flow;
> struct sw_flow_mask mask;
> struct sk_buff *reply;
> struct datapath *dp;
> - struct sw_flow_actions *acts = NULL;
> + struct sw_flow_actions *acts;
> struct sw_flow_match match;
> int error;
>
> @@ -830,23 +830,21 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> goto error;
>
> /* Validate actions. */
> - if (a[OVS_FLOW_ATTR_ACTIONS]) {
> - acts =
> ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
> - error = PTR_ERR(acts);
> - if (IS_ERR(acts))
> - goto error;
> + error = -EINVAL;
> + if (!a[OVS_FLOW_ATTR_ACTIONS])
> + goto error;
>
> - ovs_flow_mask_key(&masked_key, &key, &mask);
> - error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
> - &masked_key, 0, &acts);
> - if (error) {
> - OVS_NLERR("Flow actions may not be safe on all
> matching packets.\n");
> - goto err_kfree;
> - }
> - } else if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW) {
> - /* OVS_FLOW_CMD_NEW must have actions. */
> - error = -EINVAL;
> + acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
> + error = PTR_ERR(acts);
> + if (IS_ERR(acts))
> goto error;
> +
> + ovs_flow_mask_key(&masked_key, &key, &mask);
> + error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
> + &masked_key, 0, &acts);
> + if (error) {
> + OVS_NLERR("Flow actions may not be safe on all matching
> packets.\n");
> + goto err_kfree;
> }
>
> ovs_lock();
> @@ -858,11 +856,6 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> /* Check if this is a duplicate flow */
> flow = ovs_flow_tbl_lookup(&dp->table, &key);
> if (!flow) {
> - /* Bail out if we're not allowed to create a new flow. */
> - error = -ENOENT;
> - if (info->genlhdr->cmd == OVS_FLOW_CMD_SET)
> - goto err_unlock_ovs;
> -
> /* Allocate flow. */
> flow = ovs_flow_alloc();
> if (IS_ERR(flow)) {
> @@ -880,11 +873,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> acts = NULL;
> goto err_flow_free;
> }
> -
> - reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> - info, OVS_FLOW_CMD_NEW,
> false);
> } else {
> - /* We found a matching flow. */
> + struct sw_flow_actions *old_acts;
> +
> /* Bail out if we're not allowed to modify an existing flow.
> * We accept NLM_F_CREATE in place of the intended NLM_F_EXCL
> * because Generic Netlink treats the latter as a dump
> @@ -892,30 +883,113 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
> *skb, struct genl_info *info)
> * gets fixed.
> */
> error = -EEXIST;
> - if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW &&
> - info->nlhdr->nlmsg_flags & (NLM_F_CREATE | NLM_F_EXCL))
> + if (info->nlhdr->nlmsg_flags & (NLM_F_CREATE | NLM_F_EXCL))
> 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;
> + /* Update actions. */
> + old_acts = ovsl_dereference(flow->sf_acts);
> + rcu_assign_pointer(flow->sf_acts, acts);
> + ovs_nla_free_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);
> + ovs_unlock();
> +
> + if (reply) {
> + if (!IS_ERR(reply))
> + ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
> + else
> + netlink_set_err(sock_net(skb->sk)->genl_sock, 0,
> + ovs_dp_flow_multicast_group.id,
> + PTR_ERR(reply));
> + }
> + return 0;
> +
> +err_flow_free:
> + ovs_flow_free(flow, false);
> +err_unlock_ovs:
> + ovs_unlock();
> +err_kfree:
> + kfree(acts);
> +error:
> + return error;
> +}
> +
> +static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct nlattr **a = info->attrs;
> + struct ovs_header *ovs_header = info->userhdr;
> + struct sw_flow_key key, masked_key;
> + struct sw_flow *flow;
> + struct sw_flow_mask mask;
> + struct sk_buff *reply = NULL;
> + struct datapath *dp;
> + struct sw_flow_actions *acts = NULL;
> + struct sw_flow_match match;
> + int error;
> +
> + /* Extract key. */
> + error = -EINVAL;
> + if (!a[OVS_FLOW_ATTR_KEY])
> + goto error;
> +
> + ovs_match_init(&match, &key, &mask);
> + error = ovs_nla_get_match(&match,
> + a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK]);
> + if (error)
> + goto error;
> +
> + /* Validate actions. */
> + if (a[OVS_FLOW_ATTR_ACTIONS]) {
> + acts =
> ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
> + error = PTR_ERR(acts);
> + if (IS_ERR(acts))
> + goto error;
> +
> + ovs_flow_mask_key(&masked_key, &key, &mask);
> + error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
> + &masked_key, 0, &acts);
> + if (error) {
> + OVS_NLERR("Flow actions may not be safe on all
> matching packets.\n");
> + goto err_kfree;
> }
> + }
>
> - 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.
> + 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?
> + /* 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
