[ovs-dev] [netlink v3 06/16] datapath: Convert odp_flow_key to use Netlink attributes instead.

Ben Pfaff blp at nicira.com
Fri Jan 7 22:19:00 UTC 2011


On Fri, Jan 07, 2011 at 05:06:31PM -0500, Jesse Gross wrote:
> On Wed, Dec 29, 2010 at 7:56 PM, Ben Pfaff <blp at nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index a3b24e3..3fd7666 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -821,15 +822,20 @@ static int do_put_flow(struct datapath *dp, struct odp_flow_put *uf,
> >                       struct odp_flow_stats *stats)
> >  {
> >        struct tbl_node *flow_node;
> > +       struct sw_flow_key key;
> >        struct sw_flow *flow;
> >        struct tbl *table;
> >        struct sw_flow_actions *acts = NULL;
> >        int error;
> >        u32 hash;
> >
> > -       hash = flow_hash(&uf->flow.key);
> > +       error = flow_copy_from_user(&key, (const struct nlattr __force __user*)uf->flow.key, uf->flow.key_len);
> 
> There's a missing space between __user and the *.  

That's fairly common practice in linux-2.6/net/.  But there are plenty
of counterexamples, so I fixed this one up.

> Also, can we break up this long line?

OK, I broke it just after the final comma.

> > @@ -1581,7 +1609,7 @@ static int compat_del_flow(struct datapath *dp, struct compat_odp_flow __user *u
> >        if (compat_get_flow(&uf, ufp))
> >                return -EFAULT;
> >
> > -       flow = do_del_flow(dp, &uf.key);
> > +       flow = do_del_flow(dp, uf.key, uf.key_len);
> 
> We need a sparse cast for uf.key.

Oops, fixed.  I always forget to recompile on 64-bit.

> >        if (IS_ERR(flow))
> >                return PTR_ERR(flow);
> >
> > @@ -1601,12 +1629,17 @@ static int compat_query_flows(struct datapath *dp,
> >                struct compat_odp_flow __user *ufp = &flows[i];
> >                struct odp_flow uf;
> >                struct tbl_node *flow_node;
> > +               struct sw_flow_key key;
> >                int error;
> >
> >                if (compat_get_flow(&uf, ufp))
> >                        return -EFAULT;
> >
> > -               flow_node = tbl_lookup(table, &uf.key, flow_hash(&uf.key), flow_cmp);
> > +               error = flow_copy_from_user(&key, uf.key, uf.key_len);
> 
> Another cast here as well.

Fixed here too, thanks.

> > +               prev_type = type;
> > +       }
> > +       if (rem)
> > +               return -EINVAL;
> > +
> > +       switch (prev_type) {
> 
> Should this be type (instead of prev_type)?

They're identical at this point because of the "prev_type = type;"
assignment at the end of the loop body.  Also, "type" is out of scope at
this point.

Do you want me to move "type" out of the loop and use it instead?
Wouldn't bother me.

> > +       case ODPKT_UNSPEC:
> > +               return -EINVAL;
> > +
> > +       case ODPKT_TUN_ID:
> > +       case ODPKT_IN_PORT:
> > +               return -EINVAL;
> > +
> > +       case ODPKT_ETHERNET:
> > +       case ODPKT_8021Q:
> > +               return 0;
> > +
> > +       case ODPKT_ETHERTYPE:
> > +               if (swkey->dl_type == htons(ETH_P_IP) ||
> > +                   swkey->dl_type == htons(ETH_P_ARP))
> > +                       return -EINVAL;
> > +               return 0;
> > +
> > +       case ODPKT_IPV4:
> > +               if (swkey->nw_proto == htons(IPPROTO_TCP) ||
> > +                   swkey->nw_proto == htons(IPPROTO_UDP) ||
> > +                   swkey->nw_proto == htons(IPPROTO_ICMP))
> 
> nw_proto is a single byte field, so we don't want the htons.

Oops, thanks.

> Both of these comments also apply the userspace version.

OK, I fixed the nw_proto bit there too.

Here's the incremental:

--8<--------------------------cut here-------------------------->8--

diff --git a/datapath/datapath.c b/datapath/datapath.c
index b862225..8471234 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007, 2008, 2009, 2010 Nicira Networks.
+ * Copyright (c) 2007, 2008, 2009, 2010, 2011 Nicira Networks.
  * Distributed under the terms of the GNU GPL version 2.
  *
  * Significant portions of this file may be copied from parts of the Linux
@@ -834,7 +834,8 @@ static int do_put_flow(struct datapath *dp, struct odp_flow_put *uf,
 	int error;
 	u32 hash;
 
-	error = flow_copy_from_user(&key, (const struct nlattr __force __user*)uf->flow.key, uf->flow.key_len);
+	error = flow_copy_from_user(&key, (const struct nlattr __force __user *)uf->flow.key,
+				    uf->flow.key_len);
 	if (error)
 		return error;
 
@@ -1614,7 +1615,7 @@ static int compat_del_flow(struct datapath *dp, struct compat_odp_flow __user *u
 	if (compat_get_flow(&uf, ufp))
 		return -EFAULT;
 
-	flow = do_del_flow(dp, uf.key, uf.key_len);
+	flow = do_del_flow(dp, (const struct nlattr __force __user *)uf.key, uf.key_len);
 	if (IS_ERR(flow))
 		return PTR_ERR(flow);
 
@@ -1640,7 +1641,7 @@ static int compat_query_flows(struct datapath *dp,
 		if (compat_get_flow(&uf, ufp))
 			return -EFAULT;
 
-		error = flow_copy_from_user(&key, uf.key, uf.key_len);
+		error = flow_copy_from_user(&key, (const struct nlattr __force __user *) uf.key, uf.key_len);
 		if (error)
 			return error;
 
diff --git a/datapath/flow.c b/datapath/flow.c
index ca3ecd6..1349533 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1,6 +1,6 @@
 /*
  * Distributed under the terms of the GNU GPL version 2.
- * Copyright (c) 2007, 2008, 2009, 2010 Nicira Networks.
+ * Copyright (c) 2007, 2008, 2009, 2010, 2011 Nicira Networks.
  *
  * Significant portions of this file may be copied from parts of the Linux
  * kernel, by Linus Torvalds and others.
@@ -557,9 +557,9 @@ static int flow_from_nlattrs(struct sw_flow_key *swkey, const struct nlattr *key
 		return 0;
 
 	case ODPKT_IPV4:
-		if (swkey->nw_proto == htons(IPPROTO_TCP) ||
-		    swkey->nw_proto == htons(IPPROTO_UDP) ||
-		    swkey->nw_proto == htons(IPPROTO_ICMP))
+		if (swkey->nw_proto == IPPROTO_TCP ||
+		    swkey->nw_proto == IPPROTO_UDP ||
+		    swkey->nw_proto == IPPROTO_ICMP)
 			return -EINVAL;
 		return 0;
 
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 7b9258a..eafcd79 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -619,9 +619,9 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
         return 0;
 
     case ODPKT_IPV4:
-        if (flow->nw_proto == htons(IP_TYPE_TCP)
-            || flow->nw_proto == htons(IP_TYPE_UDP)
-            || flow->nw_proto == htons(IP_TYPE_ICMP)) {
+        if (flow->nw_proto == IP_TYPE_TCP
+            || flow->nw_proto == IP_TYPE_UDP
+            || flow->nw_proto == IP_TYPE_ICMP) {
             return EINVAL;
         }
         return 0;




More information about the dev mailing list