Pravin,
I’ll need to send a new version of the rest of the patches due to this change.
You may want to hold your review for a while.
Jarno
On Mar 24, 2014, at 9:53 AM, Pravin Shelar <pshe...@nicira.com> wrote:
> 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
