[ovs-dev] [PATCH v4 1/7] datapath: Avoid assigning a NULL pointer to flow actions.

Pravin Shelar pshelar at nicira.com
Mon Mar 24 20:41:17 UTC 2014


On Mon, Mar 24, 2014 at 1:24 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>
> On Mar 24, 2014, at 12:24 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>
> On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <jrajahalme at nicira.com>
> 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 <jrajahalme at nicira.com>
>
> 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.
>
>
> So I left that on the wrong side of '--' line, sorry.
>
> ---
> 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?
>
>
> I found it easier to think and reason about freeing 'acts' by changing it to
> NULL as soon as the actions are assigned to the flow. I can easily revert
> this though.
>

Lets drop it. This not related to this patch anyways.

For the patch:
Acked-by: Pravin B Shelar <pshelar at nicira.com>



More information about the dev mailing list