On Mar 21, 2014, at 1:52 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Fri, Feb 21, 2014 at 11:41 AM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
>> Flow SET can set an empty set of actions by not passing any actions.
>> Previously, we assigned the flow's actions pointer to NULL in this
>> case, but we never check for the NULL pointer later on. This patch
>> modifies this case to allocate a valid but empty set of actions
>> instead.
>>
> Good catch.
>
>> Note: If might be prudent to allow missing actions also in
>> OVS_FLOW_CMD_NEW.
>>
> dpif-linux can avoid sending dummy action if we allow that. But for
> compatibility we need to keep it anyways. So lets keep it as it is.
>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> ---
>> datapath/datapath.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index f7c3391..130300f 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -810,7 +810,14 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
>> struct genl_info *info)
>> 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) {
>> + } else if (info->genlhdr->cmd == OVS_FLOW_CMD_SET) {
>> + /* Need empty actions. */
>> + acts = ovs_nla_alloc_flow_actions(0);
>> + error = PTR_ERR(acts);
>> + if (IS_ERR(acts))
>> + goto error;
>> + } else {
>> + /* OVS_FLOW_CMD_NEW must have actions. */
>> error = -EINVAL;
>> goto error;
>> }
>
> Before this bug (OVS 1.7) set action use to ignore action if they were
> NULL. In that case set-action was pure stats-clear call. Can we use
> same semantics now? I mean ignore NULL actions for set-actions.
Do we have this behavior documented somewhere? In include/linux/openvswitch.h
we currently have:
* @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.
How about adding:
“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.”
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
