On Mon, Mar 24, 2014 at 9:50 AM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
> 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:
>
right, there is not documentation.
> * @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."
>
Sounds good.
Thanks,
Pravin.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
