On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <[email protected]> wrote:
> Flow SET can accept an empty set of actions, with the intended
> semantics of leaving existing actions unmodified. This seems to have
> been brokin after OVS 1.7, as we have assigned the flow's actions
> pointer to NULL in this case, but we never check for the NULL pointer
> later on. This patch restores the intended behavior and documents it
> in the include/linux/openvswitch.h.
>
> Signed-off-by: Jarno Rajahalme <[email protected]>
>
> v4: Maintain OVS 1.7 semantics: Allow missing actions on OVS_FLOW_CMD_SET,
> which cause the existing actions to be retained.
This should not be part of commit msg.
> ---
> datapath/datapath.c | 19 ++++++++++---------
> include/linux/openvswitch.h | 4 +++-
> 2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 1efbb00..918a990 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -811,6 +811,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> goto err_kfree;
> }
> } else if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW) {
> + /* OVS_FLOW_CMD_NEW must have actions. */
> error = -EINVAL;
> goto error;
> }
> @@ -839,19 +840,16 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> flow->key = masked_key;
> flow->unmasked_key = key;
> rcu_assign_pointer(flow->sf_acts, acts);
> + acts = NULL;
>
> /* Put flow in bucket. */
> error = ovs_flow_tbl_insert(&dp->table, flow, &mask);
> - if (error) {
> - acts = NULL;
> + if (error)
> goto err_flow_free;
> - }
Why are you changing this?
>
> reply = ovs_flow_cmd_build_info(flow, dp, info,
> OVS_FLOW_CMD_NEW);
> } 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
> @@ -867,11 +865,14 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> if (!ovs_flow_cmp_unmasked_key(flow, &match))
> goto err_unlock_ovs;
>
> - /* Update actions. */
> - old_acts = ovsl_dereference(flow->sf_acts);
> - rcu_assign_pointer(flow->sf_acts, acts);
> - ovs_nla_free_flow_actions(old_acts);
> + /* 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, dp, info,
> OVS_FLOW_CMD_NEW);
>
> /* Clear stats. */
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index d1ff5ec..e39e437 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -426,7 +426,9 @@ struct ovs_key_nd {
> * @OVS_FLOW_ATTR_ACTIONS: Nested %OVS_ACTION_ATTR_* attributes specifying
> * the actions to take for packets that match the key. Always present in
> * notifications. Required for %OVS_FLOW_CMD_NEW requests, optional for
> - * %OVS_FLOW_CMD_SET requests.
> + * %OVS_FLOW_CMD_SET requests. An %OVS_FLOW_CMD_SET without
> + * %OVS_FLOW_ATTR_ACTIONS will not modify the actions. To clear the actions,
> + * an %OVS_FLOW_ATTR_ACTIONS without any nested attributes must be given.
> * @OVS_FLOW_ATTR_STATS: &struct ovs_flow_stats giving statistics for this
> * flow. Present in notifications if the stats would be nonzero. Ignored in
> * requests.
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
