[PATCH net-next] ovs: allow nl 'flow set' to use ufid without flow key

[PATCH net-next] ovs: allow nl 'flow set' to use ufid without flow key

[Samuel Gauthier]

When we want to change a flow using netlink, we have to identify it to
be able to perform a lookup. Both the flow key and unique flow ID
(ufid) are valid identifiers, but we always have to specify the flow
key in the netlink message. When both attributes are there, the ufid
is used.

This commit allows to use the ufid without having to provide the flow
key, as it is already done in the netlink 'flow get' and 'flow del'
path.

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
 net/openvswitch/datapath.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index deadfdab1bc3..06f15143cf2a 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1100,21 +1100,20 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
 	struct sw_flow_match match;
 	struct sw_flow_id sfid;
 	u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
-	int error;
+	int error = 0;
 	bool log = !a[OVS_FLOW_ATTR_PROBE];
 	bool ufid_present;
 
-	/* Extract key. */
-	error = -EINVAL;
-	if (!a[OVS_FLOW_ATTR_KEY]) {
-		OVS_NLERR(log, "Flow key attribute not present in set flow.");
-		goto error;
-	}
-
 	ufid_present = ovs_nla_get_ufid(&sfid, a[OVS_FLOW_ATTR_UFID], log);
-	ovs_match_init(&match, &key, &mask);
-	error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
-				  a[OVS_FLOW_ATTR_MASK], log);
+	if (a[OVS_FLOW_ATTR_KEY]) {
+		ovs_match_init(&match, &key, NULL);
+		error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
+					  NULL, log);
+	} else if (!ufid_present) {
+		OVS_NLERR(log,
+			  "Flow set message rejected, Key attribute missing.");
+		error = -EINVAL;
+	}
 	if (error)
 		goto error;
 
-- 
2.2.1.62.g3f15098

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net-next] ovs: allow nl 'flow set' to use ufid without flow key

[pravin shelar]

On Wed, Mar 9, 2016 at 9:05 AM, Samuel Gauthier
<samuel.gauthier@6wind.com> wrote:
> When we want to change a flow using netlink, we have to identify it to
> be able to perform a lookup. Both the flow key and unique flow ID
> (ufid) are valid identifiers, but we always have to specify the flow
> key in the netlink message. When both attributes are there, the ufid
> is used.
>
> This commit allows to use the ufid without having to provide the flow
> key, as it is already done in the netlink 'flow get' and 'flow del'
> path.
>
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> ---
>  net/openvswitch/datapath.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index deadfdab1bc3..06f15143cf2a 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1100,21 +1100,20 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
>         struct sw_flow_match match;
>         struct sw_flow_id sfid;
>         u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
> -       int error;
> +       int error = 0;
>         bool log = !a[OVS_FLOW_ATTR_PROBE];
>         bool ufid_present;
>
> -       /* Extract key. */
> -       error = -EINVAL;
> -       if (!a[OVS_FLOW_ATTR_KEY]) {
> -               OVS_NLERR(log, "Flow key attribute not present in set flow.");
> -               goto error;
> -       }
> -
>         ufid_present = ovs_nla_get_ufid(&sfid, a[OVS_FLOW_ATTR_UFID], log);
> -       ovs_match_init(&match, &key, &mask);
> -       error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
> -                                 a[OVS_FLOW_ATTR_MASK], log);
> +       if (a[OVS_FLOW_ATTR_KEY]) {
> +               ovs_match_init(&match, &key, NULL);
> +               error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
> +                                         NULL, log);
> +       } else if (!ufid_present) {
> +               OVS_NLERR(log,
> +                         "Flow set message rejected, Key attribute missing.");
> +               error = -EINVAL;
> +       }

Set command sets new action for given flow. The action validation
depends on the flow key and mask. so userspace needs to provide the
key and mask.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net-next] ovs: allow nl 'flow set' to use ufid without flow key

[Samuel Gauthier]

Sorry, I missed that. Thank you for pointing it out.

Although, set command is also used to reset the flow statistics, and the
action attribute seems optional. Would you find acceptable to make the key
attribute mandatory only if the action attribute is provided?

The ovs-dpctl command could then be simplified to support ovs-dpctl --clear
mod-flow ufid:<ufid> instead of having to specify the flow and action when
you just want to reset the statistics of a flow.
Le 9 mars 2016 10:35 PM, "pravin shelar" <pshelar@ovn.org> a écrit :

> On Wed, Mar 9, 2016 at 9:05 AM, Samuel Gauthier
> <samuel.gauthier@6wind.com> wrote:
> > When we want to change a flow using netlink, we have to identify it to
> > be able to perform a lookup. Both the flow key and unique flow ID
> > (ufid) are valid identifiers, but we always have to specify the flow
> > key in the netlink message. When both attributes are there, the ufid
> > is used.
> >
> > This commit allows to use the ufid without having to provide the flow
> > key, as it is already done in the netlink 'flow get' and 'flow del'
> > path.
> >
> > Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> > ---
> >  net/openvswitch/datapath.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> > index deadfdab1bc3..06f15143cf2a 100644
> > --- a/net/openvswitch/datapath.c
> > +++ b/net/openvswitch/datapath.c
> > @@ -1100,21 +1100,20 @@ static int ovs_flow_cmd_set(struct sk_buff *skb,
> struct genl_info *info)
> >         struct sw_flow_match match;
> >         struct sw_flow_id sfid;
> >         u32 ufid_flags =
> ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
> > -       int error;
> > +       int error = 0;
> >         bool log = !a[OVS_FLOW_ATTR_PROBE];
> >         bool ufid_present;
> >
> > -       /* Extract key. */
> > -       error = -EINVAL;
> > -       if (!a[OVS_FLOW_ATTR_KEY]) {
> > -               OVS_NLERR(log, "Flow key attribute not present in set
> flow.");
> > -               goto error;
> > -       }
> > -
> >         ufid_present = ovs_nla_get_ufid(&sfid, a[OVS_FLOW_ATTR_UFID],
> log);
> > -       ovs_match_init(&match, &key, &mask);
> > -       error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
> > -                                 a[OVS_FLOW_ATTR_MASK], log);
> > +       if (a[OVS_FLOW_ATTR_KEY]) {
> > +               ovs_match_init(&match, &key, NULL);
> > +               error = ovs_nla_get_match(net, &match,
> a[OVS_FLOW_ATTR_KEY],
> > +                                         NULL, log);
> > +       } else if (!ufid_present) {
> > +               OVS_NLERR(log,
> > +                         "Flow set message rejected, Key attribute
> missing.");
> > +               error = -EINVAL;
> > +       }
>
> Set command sets new action for given flow. The action validation
> depends on the flow key and mask. so userspace needs to provide the
> key and mask.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net-next] ovs: allow nl 'flow set' to use ufid without flow key

[pravin shelar]

On Wed, Mar 9, 2016 at 4:25 PM, Samuel Gauthier
<samuel.gauthier@6wind.com> wrote:
> Sorry, I missed that. Thank you for pointing it out.
>
> Although, set command is also used to reset the flow statistics, and the
> action attribute seems optional. Would you find acceptable to make the key
> attribute mandatory only if the action attribute is provided?
>
> The ovs-dpctl command could then be simplified to support ovs-dpctl --clear
> mod-flow ufid:<ufid> instead of having to specify the flow and action when
> you just want to reset the statistics of a flow.
>

That is fine with me.

Thanks,
Pravin.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[PATCH v2 net-next] ovs: allow nl 'flow set' to use ufid without flow key

[Samuel Gauthier]

When we want to change a flow using netlink, we have to identify it to
be able to perform a lookup. Both the flow key and unique flow ID
(ufid) are valid identifiers, but we always have to specify the flow
key in the netlink message. When both attributes are there, the ufid
is used. The flow key is used to validate the actions provided by
the userland.

This commit allows to use the ufid without having to provide the flow
key, as it is already done in the netlink 'flow get' and 'flow del'
path. The flow key remains mandatory when an action is provided.

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
v2:
 - Restore mask init and parsing
 - Keep the flow key mandatory when an action is provided

 net/openvswitch/datapath.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index deadfdab1bc3..db6858f2f67a 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1100,26 +1100,32 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
 	struct sw_flow_match match;
 	struct sw_flow_id sfid;
 	u32 ufid_flags = ovs_nla_get_ufid_flags(a[OVS_FLOW_ATTR_UFID_FLAGS]);
-	int error;
+	int error = 0;
 	bool log = !a[OVS_FLOW_ATTR_PROBE];
 	bool ufid_present;
 
-	/* Extract key. */
-	error = -EINVAL;
-	if (!a[OVS_FLOW_ATTR_KEY]) {
-		OVS_NLERR(log, "Flow key attribute not present in set flow.");
-		goto error;
-	}
-
 	ufid_present = ovs_nla_get_ufid(&sfid, a[OVS_FLOW_ATTR_UFID], log);
-	ovs_match_init(&match, &key, &mask);
-	error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
-				  a[OVS_FLOW_ATTR_MASK], log);
+	if (a[OVS_FLOW_ATTR_KEY]) {
+		ovs_match_init(&match, &key, &mask);
+		error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
+					  a[OVS_FLOW_ATTR_MASK], log);
+	} else if (!ufid_present) {
+		OVS_NLERR(log,
+			  "Flow set message rejected, Key attribute missing.");
+		error = -EINVAL;
+	}
 	if (error)
 		goto error;
 
 	/* Validate actions. */
 	if (a[OVS_FLOW_ATTR_ACTIONS]) {
+		if (!a[OVS_FLOW_ATTR_KEY]) {
+			OVS_NLERR(log,
+				  "Flow key attribute not present in set flow.");
+			error = -EINVAL;
+			goto error;
+		}
+
 		acts = get_flow_actions(net, a[OVS_FLOW_ATTR_ACTIONS], &key,
 					&mask, log);
 		if (IS_ERR(acts)) {
-- 
2.2.1.62.g3f15098

Re: [ovs-dev] [PATCH v2 net-next] ovs: allow nl 'flow set' to use ufid without flow key

[pravin shelar]

On Thu, Mar 10, 2016 at 8:14 AM, Samuel Gauthier
<samuel.gauthier@6wind.com> wrote:
> When we want to change a flow using netlink, we have to identify it to
> be able to perform a lookup. Both the flow key and unique flow ID
> (ufid) are valid identifiers, but we always have to specify the flow
> key in the netlink message. When both attributes are there, the ufid
> is used. The flow key is used to validate the actions provided by
> the userland.
>
> This commit allows to use the ufid without having to provide the flow
> key, as it is already done in the netlink 'flow get' and 'flow del'
> path. The flow key remains mandatory when an action is provided.
>
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> ---
> v2:
>  - Restore mask init and parsing
>  - Keep the flow key mandatory when an action is provided
>
Looks good.

Acked-by: Pravin B Shelar <pshelar@ovn.org>

Re: [PATCH v2 net-next] ovs: allow nl 'flow set' to use ufid without flow key

[Simon Horman]

On Thu, Mar 10, 2016 at 05:14:59PM +0100, Samuel Gauthier wrote:
> When we want to change a flow using netlink, we have to identify it to
> be able to perform a lookup. Both the flow key and unique flow ID
> (ufid) are valid identifiers, but we always have to specify the flow
> key in the netlink message. When both attributes are there, the ufid
> is used. The flow key is used to validate the actions provided by
> the userland.
> 
> This commit allows to use the ufid without having to provide the flow
> key, as it is already done in the netlink 'flow get' and 'flow del'
> path. The flow key remains mandatory when an action is provided.
> 
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>

Reviewed-by: Simon Horman <simon.horman@netronome.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH v2 net-next] ovs: allow nl 'flow set' to use ufid without flow key

[David Miller]

From: Samuel Gauthier <samuel.gauthier@6wind.com>
Date: Thu, 10 Mar 2016 17:14:59 +0100

> When we want to change a flow using netlink, we have to identify it to
> be able to perform a lookup. Both the flow key and unique flow ID
> (ufid) are valid identifiers, but we always have to specify the flow
> key in the netlink message. When both attributes are there, the ufid
> is used. The flow key is used to validate the actions provided by
> the userland.
> 
> This commit allows to use the ufid without having to provide the flow
> key, as it is already done in the netlink 'flow get' and 'flow del'
> path. The flow key remains mandatory when an action is provided.
> 
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>

Applied.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev