Re: [ovs-dev] [PATCH] Do not perform validation of learn actions during parsing

2013-05-03 Thread Ben Pfaff 
On Thu, May 02, 2013 at 06:06:35PM +0900, Simon Horman wrote:
> Do not perform validation of learn actions during parsing.
> I believe this is consistent with the handling of all other actions.
> 
> Verification of all actions, including learn actions, occurs separately
> in ofpact_check__().
> 
> This the code portion of this patch is larger than might otherwise be
> expected as the flow argument of learn_parse() is now unused and thus
> removed.  This propagates up the call-chain some way.
> 
> This was suggested by Jesse Gross in response to an enhancement
> I made to the validation performed during parsing learn actions
> to allow it to correctly account for changes to the dl_type
> due to MPLS push and pop actions.
> 
> Tests have also been updated to use ovs-ofctl add-flow* instead
> of ovs-ofctl parse-flow to to allow verification occur using
> ofpact_check__().
> 
> Cc: Jesse Gross 
> Signed-off-by: Simon Horman 

The problem with this change is that it makes the error messages
impossible to read.  Just look at the learn.at update from your diff.
It changes this error message:

  ovs-ofctl: load:5->NXM_OF_IP_DST[]: cannot specify destination field
  ip_dst because prerequisites are not satisfied

into this one:

  OFPT_ERROR (xid=0x2): OFPBAC_BAD_ARGUMENT
  OFPT_FLOW_MOD (xid=0x2):
  (***truncated to 64 bytes from 120***)
    01 0e 00 78 00 00 00 02-00 38 20 ff 00 00 00 00 |...x.8 .|
  0010  00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ||
  0020  00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ||
  0030  00 00 00 00 00 00 00 00-00 00 00 00 00 00 80 00 ||

I know we'll get complaints about that.

Here's a change to ofp-parse.c that changes the error message back
into:

  2013-05-03T16:34:21Z|1|meta_flow|WARN|destination field ip_dst lacks 
correct prerequisites

which is less specific than before but at least hints at the problem.

Can you please work that in?

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index fce0765..c55bf5f 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1009,6 +1009,8 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, 
const char *str_,
 str_to_inst_ofpacts(act_str, &ofpacts);
 fm->ofpacts_len = ofpacts.size;
 fm->ofpacts = ofpbuf_steal_data(&ofpacts);
+
+ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow, OFPP_MAX);
 } else {
 fm->ofpacts_len = 0;
 fm->ofpacts = NULL;

Thanks,

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

Re: [ovs-dev] [PATCH 1/1] [branch-1.4] [ofproto-dpif] Memory leak at specific PACKET_INs

2013-05-03 Thread Zoltan Kiss 
Now I've tried with the latest 1.4 code, but it still produces these 
message intermittently. Here are some examples:


May  3 16:32:27 localhost ovs-vswitchd: 
00093|ofproto_dpif|WARN|unexpected flow from datapath 
in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
May  3 16:32:28 localhost ovs-vswitchd: 
00094|ofproto_dpif|WARN|unexpected flow from datapath 
in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
May  3 16:32:29 localhost ovs-vswitchd: 
00095|ofproto_dpif|WARN|unexpected flow from datapath 
in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
May  3 16:32:30 localhost ovs-vswitchd: 
00096|ofproto_dpif|WARN|unexpected flow from datapath 
in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)


Regards,

Zoli

On 30/04/13 23:16, Ben Pfaff wrote:

Commit fdd80c39b1822db6c8b5c (datapath: Check gso_type for correct
sk_buff in queue_gso_packets().) could be related, but I am not
certain.  It's the mostly likely known-fixed problem I see in a quick
scan of branch-1.4 commits following cc8ccb5.

Could you provide a log excerpt that includes some of these messages?

On Tue, Apr 30, 2013 at 10:08:20AM +0100, Zoltan Kiss wrote:

Hi,

The latest commit we included in XS 6.1 is cc8ccb5 (26 Jun 2012
14:43:54: lib: Do not assume sig_atomic_t is int)

Regards,

Zoli

On 25/04/13 19:25, Ben Pfaff wrote:

It's not "normal".  It indicates a bug, although if they are only seen
occasionally it is not a big deal.  What commit is the original XS 6.1
OVS based on?

On Wed, Apr 24, 2013 at 09:48:32PM +0100, Zoltan Kiss wrote:

Hi,

Thanks, I've made my comments on that thread. Another related
question came to my mind: I've seen from time to time some
"ofproto_dpif|WARN|unexpected flow from datapath" messages, not just
with these patches but with the original XS 6.1 ovs. I couldn't see
a pattern in the type of flows. Is this normal?




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

Re: [ovs-dev] [PATCH] openvswitch.h: Introduce new fields for mega flow.

2013-05-03 Thread Jesse Gross 
On Thu, May 2, 2013 at 9:34 PM, Ben Pfaff  wrote:
> On Thu, May 02, 2013 at 08:20:52PM -0700, Jesse Gross wrote:
>> On Thu, May 2, 2013 at 1:30 PM, Andy Zhou  wrote:
>> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> > index e890fd8..6048e9b 100644
>> > --- a/include/linux/openvswitch.h
>> > +++ b/include/linux/openvswitch.h
>> > @@ -287,6 +313,8 @@ enum ovs_key_attr {
>> > OVS_KEY_ATTR_IPV4_TUNNEL,  /* struct ovs_key_ipv4_tunnel */
>> >  #endif
>> >
>> > +   OVS_KEY_ATTR_MEGA = 60, /* Indicate a mega flow key */
>> > +   OVS_KEY_ATTR_MASK = 61, /* Nested OVS_MASK_ATTR_* attributes */
>>
>> I'm not really a big fan of having OVS_KEY_ATTR_MASK in the
>> ovs_key_attr namespace for two reasons:
>>  * It's not really semantically equivalent and putting it at the end
>> to try to differentiate it seems somewhat ugly.
>
> Regarding the latter, I suggested putting it at the end only because of
> the precedent that this is where attributes go that are not yet
> upstream.
>
>>  * New userspaces will not be able to communicate with old kernels
>> since they will reject an unknown flow attribute. This isn't
>> inherently a dealbreaker since it's OK for new programs to require new
>> features but it seems like it will result in a lot of extra detection
>> and compatibility logic in various places.
>
> Userspace needs to be able to figure out what kinds of megaflows are
> acceptable to the kernel.  My favorite possibility is that the kernel
> promises that any field that it understands can have bitwise wildcard
> matches, that is, every field is maskable.
>
> This policy makes life easy for userspace, because any flow it wants
> to set up in response to a packet arrival will ordinarily be a subset
> of the packet's microflow.  For example, if a TCP packet arrives, then
> userspace will might want to set up a flow that matches on the entire
> microflow, or on the microflow except one or both of the TCP ports, or
> on just the destination MAC address from the microflow.  In each of
> those cases, userspace would know that the kernel would understand the
> flow match.
>
> This policy is not hard to implement in the kernel.  If in the future
> we add a new field that for some reason we do not want to make
> maskable, we could still do that without breaking backward
> compatibility, because userspace would already need changes to add
> support for that new field.  On the other hand, *removing* maskability
> for a field would break compatibility, so if that is something we may
> want to do, then we should choose a different policy.
>
> When userspace cannot set up the exact kind of megaflow that it wants
> to, it has to be able to fall back on setting up some other flow.  If
> we can use the "every field is maskable" policy, then this will never
> happen, so it is not a major concern.
>
>> From a compatibility perspective, it seems like the ideal thing to do
>> is to continue using the OVS_FLOW_ATTR_KEY for exact matches and then
>> have an additional flow attr that specifies wildcards using the same
>> format as the values. Old kernels would just ignore the wildcards and
>> install the exact match, in effect looking very similar to subfacets.
>> In fact, this would also allow the kernel to impose arbitrary
>> restrictions on the types of wildcards that it supports since it could
>> choose to ignore some wildcard fields but not others.
>>
>> As far as the format of the wildcards, I think we could just list the
>> values that have some form of mask with missing values being
>> implicitly completely wildcarded. (note: I think it's actually
>> possible to require all types to be present without userspace needing
>> special knowledge of the kernel because it has the list of supported
>> types from the upcall. However, I don't think this is necessary.) If
>> entire wildcard flow is not present then the flow is exact match.
>
> It would be most convenient to take the existing Netlink protocol for
> exact-match flows and extend it in a backward-compatible way to also
> allow wildcarded matching.  However, the handling of omitted fields
> introduces a difficulty.  In an OpenFlow flow expressed in NXM or OXM,
> any field not included in the flow description is implicitly
> wildcarded.  This is a natural choice because most flows match only a
> small subset of the available fields and because it lends itself well
> to some extensions: if a switch adds support for matching a field, the
> controller need not be changed unless it actually wants to match on
> that field.
>
> In a kernel flow expressed as Netlink attibutes, an omitted field
> implicitly matches some "default" field value (e.g. zero).  This is
> the case for OVS_KEY_ATTR_PRIORITY, OVS_KEY_ATTR_SKB_MARK, and
> OVS_KEY_ATTR_TUNNEL, for example. Kernel megaflows cannot change this
> interpretation.  The natural conclusion is that a megaflow must
> explicitly list every wildcarded field.  Unfortunately this lends
> itself poorly to ex

[ovs-dev] [PATCH v4] dapapath: Kill VPORT_F_TUN_ID vport flag.

2013-05-03 Thread Pravin B Shelar 
VPORT_F_TUN_ID is last remaining flag, once we remove it, flags
field from vport-ops can be removed.  Since it does not complicate
much code, we decided to remove this flag.

Signed-off-by: Pravin B Shelar 
---
v3-v4:
 - Pass tun_key as param to ovs_vport_receive().

v1-v3:
New patch.
---
 datapath/tunnel.c |5 +++--
 datapath/tunnel.h |3 ++-
 datapath/vport-gre.c  |5 +
 datapath/vport-internal_dev.c |2 +-
 datapath/vport-lisp.c |4 +---
 datapath/vport-netdev.c   |2 +-
 datapath/vport-vxlan.c|4 +---
 datapath/vport.c  |7 +++
 datapath/vport.h  |8 ++--
 9 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/datapath/tunnel.c b/datapath/tunnel.c
index 8cf1c16..8c93e18 100644
--- a/datapath/tunnel.c
+++ b/datapath/tunnel.c
@@ -58,7 +58,8 @@
  * - skb->csum does not include the inner Ethernet header.
  * - The layer pointers are undefined.
  */
-void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb)
+void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
+struct ovs_key_ipv4_tunnel *tun_key)
 {
struct ethhdr *eh;
 
@@ -81,7 +82,7 @@ void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb)
return;
}
 
-   ovs_vport_receive(vport, skb);
+   ovs_vport_receive(vport, skb, tun_key);
 }
 
 static struct rtable *find_route(struct net *net,
diff --git a/datapath/tunnel.h b/datapath/tunnel.h
index 75a84d1..89c4e16 100644
--- a/datapath/tunnel.h
+++ b/datapath/tunnel.h
@@ -33,7 +33,8 @@ int ovs_tnl_send(struct vport *vport, struct sk_buff *skb,
  struct sk_buff *,
  int tunnel_hlen));
 
-void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb);
+void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
+struct ovs_key_ipv4_tunnel *tun_key);
 u16 ovs_tnl_get_src_port(struct sk_buff *skb);
 
 static inline void tnl_tun_key_init(struct ovs_key_ipv4_tunnel *tun_key,
diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
index 1f046ae..add17d9 100644
--- a/datapath/vport-gre.c
+++ b/datapath/vport-gre.c
@@ -270,12 +270,11 @@ static int gre_rcv(struct sk_buff *skb)
iph = ip_hdr(skb);
tnl_flags = gre_flags_to_tunnel_flags(gre_flags, is_gre64);
tnl_tun_key_init(&tun_key, iph, key, tnl_flags);
-   OVS_CB(skb)->tun_key = &tun_key;
 
__skb_pull(skb, hdr_len);
skb_postpull_rcsum(skb, skb_transport_header(skb), hdr_len + ETH_HLEN);
 
-   ovs_tnl_rcv(vport, skb);
+   ovs_tnl_rcv(vport, skb, &tun_key);
return 0;
 
 error:
@@ -375,7 +374,6 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff 
*skb)
 
 const struct vport_ops ovs_gre_vport_ops = {
.type   = OVS_VPORT_TYPE_GRE,
-   .flags  = VPORT_F_TUN_ID,
.create = gre_create,
.destroy= gre_tnl_destroy,
.get_name   = gre_get_name,
@@ -437,7 +435,6 @@ static int gre64_tnl_send(struct vport *vport, struct 
sk_buff *skb)
 
 const struct vport_ops ovs_gre64_vport_ops = {
.type   = OVS_VPORT_TYPE_GRE64,
-   .flags  = VPORT_F_TUN_ID,
.create = gre64_create,
.destroy= gre64_tnl_destroy,
.get_name   = gre_get_name,
diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
index 1b19bfc..9a159cd 100644
--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -94,7 +94,7 @@ static int internal_dev_xmit(struct sk_buff *skb, struct 
net_device *netdev)
vlan_copy_skb_tci(skb);
 
rcu_read_lock();
-   ovs_vport_receive(internal_dev_priv(netdev)->vport, skb);
+   ovs_vport_receive(internal_dev_priv(netdev)->vport, skb, NULL);
rcu_read_unlock();
return 0;
 }
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
index 554495d..ca2b441 100644
--- a/datapath/vport-lisp.c
+++ b/datapath/vport-lisp.c
@@ -219,7 +219,6 @@ static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
/* Save outer tunnel values */
iph = ip_hdr(skb);
tnl_tun_key_init(&tun_key, iph, key, OVS_TNL_F_KEY);
-   OVS_CB(skb)->tun_key = &tun_key;
 
/* Drop non-IP inner packets */
inner_iph = (struct iphdr *)(lisph + 1);
@@ -241,7 +240,7 @@ static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
ethh->h_source[0] = 0x02;
ethh->h_proto = protocol;
 
-   ovs_tnl_rcv(vport_from_priv(lisp_port), skb);
+   ovs_tnl_rcv(vport_from_priv(lisp_port), skb, &tun_key);
goto out;
 
 error:
@@ -390,7 +389,6 @@ static const char *lisp_get_name(const struct vport *vport)
 
 const struct vport_ops ovs_lisp_vport_ops = {
.type   = OVS_VPORT_TYPE_LISP,
-   .flags  = VPORT_F_TUN_ID,
.create = lisp_tnl_create,
.destroy= lisp_tnl_destroy,
.get_na

Re: [ovs-dev] [PATCH v4] dapapath: Kill VPORT_F_TUN_ID vport flag.

2013-05-03 Thread Jesse Gross 
On Fri, May 3, 2013 at 10:35 AM, Pravin B Shelar  wrote:
> VPORT_F_TUN_ID is last remaining flag, once we remove it, flags
> field from vport-ops can be removed.  Since it does not complicate
> much code, we decided to remove this flag.
>
> Signed-off-by: Pravin B Shelar 

Acked-by: Jesse Gross 

There's a typo in the subject line though.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH branch-1.4] ovsdb: Allow recovery from transient write errors in log implementation.

2013-05-03 Thread Zoltan Kiss 
Until now, the OVSDB data journaling implementation has made write errors
"sticky", so that a single write error persists as long as ovsdb-server is
alive.  However, some kinds of write errors (such as ENOSPC) can be
transient in practice.  I don't know of a good reason to make such errors
sticky, so this commit makes the journaling code retry writes even after
an error occurs, allowing ovsdb-server to recover from transient errors.

This is a crossport of commit a7bf837f065d81fbc0dab0a372a7b756094a5322
from master.

Reported-by: likunyun 
Signed-off-by: Ben Pfaff 
Acked-by: Ethan Jackson 
Backported-by: Zoltan Kiss 
Signed-off-by: Zoltan Kiss 
---
 ovsdb/log.c |   14 ++
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/ovsdb/log.c b/ovsdb/log.c
index f0926c0..6decf1f 100644
--- a/ovsdb/log.c
+++ b/ovsdb/log.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011 Nicira Networks
+/* Copyright (c) 2009, 2010, 2011, 2013 Nicira Networks
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -49,7 +49,7 @@ struct ovsdb_log {
 struct lockfile *lockfile;
 FILE *stream;
 struct ovsdb_error *read_error;
-struct ovsdb_error *write_error;
+bool write_error;
 enum ovsdb_log_mode mode;
 };
 
@@ -125,7 +125,7 @@ ovsdb_log_open(const char *name, enum ovsdb_log_open_mode 
open_mode,
 file->prev_offset = 0;
 file->offset = 0;
 file->read_error = NULL;
-file->write_error = NULL;
+file->write_error = false;
 file->mode = OVSDB_LOG_READ;
 *filep = file;
 return NULL;
@@ -146,7 +146,6 @@ ovsdb_log_close(struct ovsdb_log *file)
 fclose(file->stream);
 lockfile_unlock(file->lockfile);
 ovsdb_error_destroy(file->read_error);
-ovsdb_error_destroy(file->write_error);
 free(file);
 }
 }
@@ -324,10 +323,9 @@ ovsdb_log_write(struct ovsdb_log *file, struct json *json)
 
 json_string = NULL;
 
-if (file->write_error) {
-return ovsdb_error_clone(file->write_error);
-} else if (file->mode == OVSDB_LOG_READ) {
+if (file->mode == OVSDB_LOG_READ || file->write_error) {
 file->mode = OVSDB_LOG_WRITE;
+file->write_error = false;
 if (fseeko(file->stream, file->offset, SEEK_SET)) {
 error = ovsdb_io_error(errno, "%s: cannot seek to offset %lld",
file->name, (long long int) file->offset);
@@ -375,7 +373,7 @@ ovsdb_log_write(struct ovsdb_log *file, struct json *json)
 return NULL;
 
 error:
-file->write_error = ovsdb_error_clone(error);
+file->write_error = true;
 free(json_string);
 return error;
 }
-- 
1.7.0.4

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

Re: [ovs-dev] [PATCH] system-stats: Use getmntent_r() for thread-safety.

2013-05-03 Thread Andy Zhou 
Looks good.


On Wed, May 1, 2013 at 11:32 AM, Ben Pfaff  wrote:

> getmntent_r() is a GNU extension so we test for its existence and just
> disable this feature of system stats if it is not present, because this
> feature is not very important.
>
> Signed-off-by: Ben Pfaff 
> ---
>  configure.ac|2 +-
>  vswitchd/system-stats.c |8 +---
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7af8afb..bbb6dea 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -60,7 +60,7 @@ OVS_CHECK_IF_DL
>  OVS_CHECK_STRTOK_R
>  AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct stat.st_mtimensec],
>[], [], [[#include ]])
> -AC_CHECK_FUNCS([mlockall strnlen strsignal getloadavg statvfs setmntent])
> +AC_CHECK_FUNCS([mlockall strnlen strsignal getloadavg statvfs
> getmntent_r])
>  AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h])
>
>  OVS_CHECK_PKIDIR
> diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
> index 2e18b1b..842bc20 100644
> --- a/vswitchd/system-stats.c
> +++ b/vswitchd/system-stats.c
> @@ -447,9 +447,11 @@ get_process_stats(struct smap *stats)
>  static void
>  get_filesys_stats(struct smap *stats OVS_UNUSED)
>  {
> -#if HAVE_SETMNTENT && HAVE_STATVFS
> +#if HAVE_GETMNTENT_R && HAVE_STATVFS
>  static const char file_name[] = "/etc/mtab";
> +struct mntent mntent;
>  struct mntent *me;
> +char buf[4096];
>  FILE *stream;
>  struct ds s;
>
> @@ -460,7 +462,7 @@ get_filesys_stats(struct smap *stats OVS_UNUSED)
>  }
>
>  ds_init(&s);
> -while ((me = getmntent(stream)) != NULL) {
> +while ((me = getmntent_r(stream, &mntent, buf, sizeof buf)) != NULL) {
>  unsigned long long int total, free;
>  struct statvfs vfs;
>  char *p;
> @@ -494,7 +496,7 @@ get_filesys_stats(struct smap *stats OVS_UNUSED)
>  smap_add(stats, "file_systems", ds_cstr(&s));
>  }
>  ds_destroy(&s);
> -#endif  /* HAVE_SETMNTENT && HAVE_STATVFS */
> +#endif  /* HAVE_GETMNTENT_R && HAVE_STATVFS */
>  }
>
>  #define SYSTEM_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH branch-1.4] ovsdb: Allow recovery from transient write errors in log implementation.

2013-05-03 Thread Ben Pfaff 
On Fri, May 03, 2013 at 06:53:28PM +0100, Zoltan Kiss wrote:
> Until now, the OVSDB data journaling implementation has made write errors
> "sticky", so that a single write error persists as long as ovsdb-server is
> alive.  However, some kinds of write errors (such as ENOSPC) can be
> transient in practice.  I don't know of a good reason to make such errors
> sticky, so this commit makes the journaling code retry writes even after
> an error occurs, allowing ovsdb-server to recover from transient errors.
> 
> This is a crossport of commit a7bf837f065d81fbc0dab0a372a7b756094a5322
> from master.
> 
> Reported-by: likunyun 
> Signed-off-by: Ben Pfaff 
> Acked-by: Ethan Jackson 
> Backported-by: Zoltan Kiss 
> Signed-off-by: Zoltan Kiss 

I cherry-picked the original commit to branch-1.4 and branch-1.9.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/1] [branch-1.4] [ofproto-dpif] Memory leak at specific PACKET_INs

2013-05-03 Thread Ben Pfaff 
Thanks.  Do these messages appear in the log just as you show them, or
are they interspersed with other messages?

On Fri, May 03, 2013 at 05:40:57PM +0100, Zoltan Kiss wrote:
> Now I've tried with the latest 1.4 code, but it still produces these
> message intermittently. Here are some examples:
> 
> May  3 16:32:27 localhost ovs-vswitchd:
> 00093|ofproto_dpif|WARN|unexpected flow from datapath 
> in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
> May  3 16:32:28 localhost ovs-vswitchd:
> 00094|ofproto_dpif|WARN|unexpected flow from datapath 
> in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
> May  3 16:32:29 localhost ovs-vswitchd:
> 00095|ofproto_dpif|WARN|unexpected flow from datapath 
> in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
> May  3 16:32:30 localhost ovs-vswitchd:
> 00096|ofproto_dpif|WARN|unexpected flow from datapath 
> in_port(1),eth(src=00:21:1b:f3:63:45,dst=00:e0:ed:26:a9:bc),eth_type(0x0800),ipv4(src=10.80.2.155,dst=10.80.227.83,proto=6,tos=0x10,ttl=62,frag=no),tcp(src=53495,dst=22)
> 
> Regards,
> 
> Zoli
> 
> On 30/04/13 23:16, Ben Pfaff wrote:
> >Commit fdd80c39b1822db6c8b5c (datapath: Check gso_type for correct
> >sk_buff in queue_gso_packets().) could be related, but I am not
> >certain.  It's the mostly likely known-fixed problem I see in a quick
> >scan of branch-1.4 commits following cc8ccb5.
> >
> >Could you provide a log excerpt that includes some of these messages?
> >
> >On Tue, Apr 30, 2013 at 10:08:20AM +0100, Zoltan Kiss wrote:
> >>Hi,
> >>
> >>The latest commit we included in XS 6.1 is cc8ccb5 (26 Jun 2012
> >>14:43:54: lib: Do not assume sig_atomic_t is int)
> >>
> >>Regards,
> >>
> >>Zoli
> >>
> >>On 25/04/13 19:25, Ben Pfaff wrote:
> >>>It's not "normal".  It indicates a bug, although if they are only seen
> >>>occasionally it is not a big deal.  What commit is the original XS 6.1
> >>>OVS based on?
> >>>
> >>>On Wed, Apr 24, 2013 at 09:48:32PM +0100, Zoltan Kiss wrote:
> Hi,
> 
> Thanks, I've made my comments on that thread. Another related
> question came to my mind: I've seen from time to time some
> "ofproto_dpif|WARN|unexpected flow from datapath" messages, not just
> with these patches but with the original XS 6.1 ovs. I couldn't see
> a pattern in the type of flows. Is this normal?
> >>
> 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] system-stats: Use getmntent_r() for thread-safety.

2013-05-03 Thread Ben Pfaff 
Thanks, I applied this to master.

On Fri, May 03, 2013 at 11:24:46AM -0700, Andy Zhou wrote:
> Looks good.
> 
> 
> On Wed, May 1, 2013 at 11:32 AM, Ben Pfaff  wrote:
> 
> > getmntent_r() is a GNU extension so we test for its existence and just
> > disable this feature of system stats if it is not present, because this
> > feature is not very important.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  configure.ac|2 +-
> >  vswitchd/system-stats.c |8 +---
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 7af8afb..bbb6dea 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -60,7 +60,7 @@ OVS_CHECK_IF_DL
> >  OVS_CHECK_STRTOK_R
> >  AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct stat.st_mtimensec],
> >[], [], [[#include ]])
> > -AC_CHECK_FUNCS([mlockall strnlen strsignal getloadavg statvfs setmntent])
> > +AC_CHECK_FUNCS([mlockall strnlen strsignal getloadavg statvfs
> > getmntent_r])
> >  AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h])
> >
> >  OVS_CHECK_PKIDIR
> > diff --git a/vswitchd/system-stats.c b/vswitchd/system-stats.c
> > index 2e18b1b..842bc20 100644
> > --- a/vswitchd/system-stats.c
> > +++ b/vswitchd/system-stats.c
> > @@ -447,9 +447,11 @@ get_process_stats(struct smap *stats)
> >  static void
> >  get_filesys_stats(struct smap *stats OVS_UNUSED)
> >  {
> > -#if HAVE_SETMNTENT && HAVE_STATVFS
> > +#if HAVE_GETMNTENT_R && HAVE_STATVFS
> >  static const char file_name[] = "/etc/mtab";
> > +struct mntent mntent;
> >  struct mntent *me;
> > +char buf[4096];
> >  FILE *stream;
> >  struct ds s;
> >
> > @@ -460,7 +462,7 @@ get_filesys_stats(struct smap *stats OVS_UNUSED)
> >  }
> >
> >  ds_init(&s);
> > -while ((me = getmntent(stream)) != NULL) {
> > +while ((me = getmntent_r(stream, &mntent, buf, sizeof buf)) != NULL) {
> >  unsigned long long int total, free;
> >  struct statvfs vfs;
> >  char *p;
> > @@ -494,7 +496,7 @@ get_filesys_stats(struct smap *stats OVS_UNUSED)
> >  smap_add(stats, "file_systems", ds_cstr(&s));
> >  }
> >  ds_destroy(&s);
> > -#endif  /* HAVE_SETMNTENT && HAVE_STATVFS */
> > +#endif  /* HAVE_GETMNTENT_R && HAVE_STATVFS */
> >  }
> >
> >  #define SYSTEM_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
> > --
> > 1.7.2.5
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [const 0/9] Mark many static data structures as const.

2013-05-03 Thread Andy Zhou 
I have reviewed all 9 patches. They look fine. Thanks.


On Wed, May 1, 2013 at 11:20 AM, Ben Pfaff  wrote:

> In preparation for multithreading OVS, it makes sense to mark any
> static data that we can "const", because read-only access to data
> is obviously thread-safe.
>
> Ben Pfaff (9):
>   dpif-linux: Make dummy_action const in dpif_linux_init_flow_put().
>   Make most "struct option" instances "const".
>   hmap: Make HMAP_INITIALIZER a valid initializer for a const hmap.
>   netdev-linux: Mark more static data as "const".
>   netdev: Make 'smap' variable const in netdev_set_qos().
>   stream-fd: Mark 'fd_pstream_class' const.
>   vlog: Mark more static data const.
>   ofp-util: Make names[] in ofputil_action_code_from_name() const-ier.
>   vconn: Mark class structures as const.
>
>  lib/dpif-linux.c   |6 --
>  lib/hmap.h |3 ++-
>  lib/netdev-linux.c |   30 ++
>  lib/netdev.c   |2 +-
>  lib/ofp-util.c |4 ++--
>  lib/stream-fd.c|4 ++--
>  lib/vconn-provider.h   |   20 ++--
>  lib/vconn-stream.c |   20 ++--
>  lib/vconn.c|   30 +++---
>  lib/vlog.c |6 +++---
>  ovsdb/ovsdb-client.c   |4 ++--
>  ovsdb/ovsdb-server.c   |2 +-
>  ovsdb/ovsdb-tool.c |2 +-
>  tests/test-jsonrpc.c   |4 ++--
>  tests/test-netflow.c   |2 +-
>  tests/test-ovsdb.c |2 +-
>  tests/test-sflow.c |2 +-
>  tests/test-util.c  |2 +-
>  utilities/ovs-benchmark.c  |4 ++--
>  utilities/ovs-controller.c |2 +-
>  utilities/ovs-dpctl.c  |2 +-
>  utilities/ovs-ofctl.c  |2 +-
>  vswitchd/ovs-vswitchd.c|2 +-
>  23 files changed, 79 insertions(+), 78 deletions(-)
>
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] Change sFlow model to reflect per-bridge sampling

2013-05-03 Thread Ben Pfaff 
Thanks, I did want your opinion.  I applied this to branch-1.10.

On Thu, May 02, 2013 at 05:19:42PM -0700, Neil Mckee wrote:
> Thanks!
> 
> If that was a question to me, then yes, I think it would be good to
> fix this on branch 1.10 too.  The old code resulted in ifIndex==-95
> being offered to the sFlow module for traffic from a
> non-ifindex-port.  -95 being the -errno that the netdev replies with
> to indicate that it doesn't have an ifindex.  That might have been
> tolerable except that every sub-agent (bridge) was claiming to
> represent this same bogus datasource, and that's what was violating
> the sFlow containment model.  Traffic on ifIndex-ports was still
> being reported OK, but non-ifindex-port traffic on different bridges
> (e.g. gre tunnel traffic) was being aliased together.  Given the
> importance of tunnel traffic...
> 
> But it's up to you.
> 
> Neil
> 
> 
> On May 2, 2013, at 12:38 PM, Ben Pfaff wrote:
> 
> > Applied to master and branch-1.11, thanks.  Do we need this on
> > branch-1.10 also?
> > 
> > On Tue, Apr 30, 2013 at 10:38:53PM -0700, Neil Mckee wrote:
> >> I learned how to squash patches...
> >> 
> >> Change sFlow model to reflect per-bridge sampling.  Before we were 
> >> presenting a separate
> >> sFlow data-source (sampler) for each ifIndex-interface,  and it was 
> >> causing problems with
> >> samples that did not easily map to an ifIndex being aliased together and 
> >> breaking the sFlow
> >> containment rules.  This patch changes the model to present a single sFlow 
> >> data-source for
> >> each bridge.  Now we can still make all reasonable effort to map packet 
> >> samples to
> >> ingress/egress ifIndex numbers,  knowing that the fallback to "unknown" 
> >> does not break the
> >> sFlow model.   Note that interface-counter-polling is still handled the 
> >> same way as before,
> >> with sFlow counter-polling data only being exported for ifIndex-interfaces.
> >> 
> >> Signed-off-by: Neil McKee 
> >> 
> >> ---
> >> lib/sflow.h  |  7 
> >> ofproto/ofproto-dpif-sflow.c | 80 
> >> +---
> >> ofproto/ofproto-dpif.c   |  6 +++-
> >> ofproto/tunnel.c |  1 -
> >> tests/ofproto-dpif.at| 48 +-
> >> 5 files changed, 75 insertions(+), 67 deletions(-)
> >> 
> >> diff --git a/lib/sflow.h b/lib/sflow.h
> >> index 8ea9693..0d1f2b9 100644
> >> --- a/lib/sflow.h
> >> +++ b/lib/sflow.h
> >> @@ -8,6 +8,13 @@
> >> #ifndef SFLOW_H
> >> #define SFLOW_H 1
> >> 
> >> +typedef enum {
> >> +SFL_DSCLASS_IFINDEX = 0,
> >> +SFL_DSCLASS_VLAN = 1,
> >> +SFL_DSCLASS_PHYSICAL_ENTITY = 2,
> >> +SFL_DSCLASS_LOGICAL_ENTITY = 3
> >> +} SFL_DSCLASS;
> >> +
> >> enum SFLAddress_type {
> >> SFLADDRESSTYPE_IP_V4 = 1,
> >> SFLADDRESSTYPE_IP_V6 = 2
> >> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> >> index 69362ab..9ad0eaf 100644
> >> --- a/ofproto/ofproto-dpif-sflow.c
> >> +++ b/ofproto/ofproto-dpif-sflow.c
> >> @@ -341,39 +341,32 @@ dpif_sflow_add_poller(struct dpif_sflow *ds, struct 
> >> dpif_sflow_port *dsp)
> >> sfl_poller_set_bridgePort(poller, dsp->odp_port);
> >> }
> >> 
> >> -static void
> >> -dpif_sflow_add_sampler(struct dpif_sflow *ds, struct dpif_sflow_port *dsp)
> >> -{
> >> -SFLSampler *sampler = sfl_agent_addSampler(ds->sflow_agent, 
> >> &dsp->dsi);
> >> -sfl_sampler_set_sFlowFsPacketSamplingRate(sampler, 
> >> ds->options->sampling_rate);
> >> -sfl_sampler_set_sFlowFsMaximumHeaderSize(sampler, 
> >> ds->options->header_len);
> >> -sfl_sampler_set_sFlowFsReceiver(sampler, RECEIVER_INDEX);
> >> -}
> >> -
> >> void
> >> dpif_sflow_add_port(struct dpif_sflow *ds, struct ofport *ofport,
> >> uint32_t odp_port)
> >> {
> >> struct dpif_sflow_port *dsp;
> >> -uint32_t ifindex;
> >> +int ifindex;
> >> 
> >> dpif_sflow_del_port(ds, odp_port);
> >> 
> >> -/* Add to table of ports. */
> >> -dsp = xmalloc(sizeof *dsp);
> >> ifindex = netdev_get_ifindex(ofport->netdev);
> >> +
> >> if (ifindex <= 0) {
> >> -ifindex = (ds->sflow_agent->subId << 16) + odp_port;
> >> +/* Not an ifindex port, so do not add a cross-reference to it 
> >> here */
> >> +return;
> >> }
> >> +
> >> +/* Add to table of ports. */
> >> +dsp = xmalloc(sizeof *dsp);
> >> dsp->ofport = ofport;
> >> dsp->odp_port = odp_port;
> >> -SFL_DS_SET(dsp->dsi, 0, ifindex, 0);
> >> +SFL_DS_SET(dsp->dsi, SFL_DSCLASS_IFINDEX, ifindex, 0);
> >> hmap_insert(&ds->ports, &dsp->hmap_node, hash_int(odp_port, 0));
> >> 
> >> -/* Add poller and sampler. */
> >> +/* Add poller. */
> >> if (ds->sflow_agent) {
> >> dpif_sflow_add_poller(ds, dsp);
> >> -dpif_sflow_add_sampler(ds, dsp);
> >> }
> >> }
> >> 
> >> @@ -406,6 +399,9 @@ dpif_sflow_set_options(struct dpif_sflow *ds,
> >> SFLReceiver *receiver;
> >> SFLAddress agentIP;
> >> tim

[ovs-dev] [PATCH] socket-util: restore building on FreeBSD.

2013-05-03 Thread Ed Maste 
FreeBSD does not have EAI_ADDRFAMILY or EAI_NODATA and thus failed to build
after commit 3cbb5dc7e89df2b40bb6f715873cf2b6b25a7054 "socket-util: Use
getaddrinfo() instead of gethostbyname() for thread safety."

Signed-off-by: Ed Maste 
---
 lib/socket-util.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/socket-util.c b/lib/socket-util.c
index 906b970..2dff9f5 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -201,7 +201,9 @@ lookup_hostname(const char *host_name, struct in_addr *addr)
 freeaddrinfo(result);
 return 0;
 
+#ifdef EAI_ADDRFAMILY
 case EAI_ADDRFAMILY:
+#endif
 case EAI_NONAME:
 case EAI_SERVICE:
 return ENOENT;
@@ -220,8 +222,10 @@ lookup_hostname(const char *host_name, struct in_addr 
*addr)
 case EAI_MEMORY:
 return ENOMEM;
 
+#ifdef EAI_NODATA
 case EAI_NODATA:
 return ENXIO;
+#endif
 
 case EAI_SYSTEM:
 return errno;
-- 
1.7.11.5

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

Re: [ovs-dev] [PATCH] socket-util: restore building on FreeBSD.

2013-05-03 Thread Ben Pfaff 
On Fri, May 03, 2013 at 04:31:02PM -0400, Ed Maste wrote:
> FreeBSD does not have EAI_ADDRFAMILY or EAI_NODATA and thus failed to build
> after commit 3cbb5dc7e89df2b40bb6f715873cf2b6b25a7054 "socket-util: Use
> getaddrinfo() instead of gethostbyname() for thread safety."
> 
> Signed-off-by: Ed Maste 

Applied to master, thanks a lot.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [const 0/9] Mark many static data structures as const.

2013-05-03 Thread Ben Pfaff 
I applied them to master.  Thanks a lot for the reviews.

On Fri, May 03, 2013 at 01:13:09PM -0700, Andy Zhou wrote:
> I have reviewed all 9 patches. They look fine. Thanks.
> 
> 
> On Wed, May 1, 2013 at 11:20 AM, Ben Pfaff  wrote:
> 
> > In preparation for multithreading OVS, it makes sense to mark any
> > static data that we can "const", because read-only access to data
> > is obviously thread-safe.
> >
> > Ben Pfaff (9):
> >   dpif-linux: Make dummy_action const in dpif_linux_init_flow_put().
> >   Make most "struct option" instances "const".
> >   hmap: Make HMAP_INITIALIZER a valid initializer for a const hmap.
> >   netdev-linux: Mark more static data as "const".
> >   netdev: Make 'smap' variable const in netdev_set_qos().
> >   stream-fd: Mark 'fd_pstream_class' const.
> >   vlog: Mark more static data const.
> >   ofp-util: Make names[] in ofputil_action_code_from_name() const-ier.
> >   vconn: Mark class structures as const.
> >
> >  lib/dpif-linux.c   |6 --
> >  lib/hmap.h |3 ++-
> >  lib/netdev-linux.c |   30 ++
> >  lib/netdev.c   |2 +-
> >  lib/ofp-util.c |4 ++--
> >  lib/stream-fd.c|4 ++--
> >  lib/vconn-provider.h   |   20 ++--
> >  lib/vconn-stream.c |   20 ++--
> >  lib/vconn.c|   30 +++---
> >  lib/vlog.c |6 +++---
> >  ovsdb/ovsdb-client.c   |4 ++--
> >  ovsdb/ovsdb-server.c   |2 +-
> >  ovsdb/ovsdb-tool.c |2 +-
> >  tests/test-jsonrpc.c   |4 ++--
> >  tests/test-netflow.c   |2 +-
> >  tests/test-ovsdb.c |2 +-
> >  tests/test-sflow.c |2 +-
> >  tests/test-util.c  |2 +-
> >  utilities/ovs-benchmark.c  |4 ++--
> >  utilities/ovs-controller.c |2 +-
> >  utilities/ovs-dpctl.c  |2 +-
> >  utilities/ovs-ofctl.c  |2 +-
> >  vswitchd/ovs-vswitchd.c|2 +-
> >  23 files changed, 79 insertions(+), 78 deletions(-)
> >
> > --
> > 1.7.2.5
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [monitor 3/3] ovs-ofctl: Make "ovs-ofctl monitor" respond to echo requests.

2013-05-03 Thread Ethan Jackson 
> +case OFPTYPE_ECHO_REQUEST:
> +if (reply_to_echo_requests) {
> +struct ofpbuf *reply;
> +
> +reply = make_echo_reply(b->data);
> +retval = vconn_send(vconn, reply);
> +if (retval && retval != EAGAIN) {
> +ovs_fatal(retval, "failed to send echo reply");
> +}
> +}

I think we need to destroy 'reply' when vconn_send() returns EAGAIN.
Otherwise I think we leak it.

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

Re: [ovs-dev] [monitor 3/3] ovs-ofctl: Make "ovs-ofctl monitor" respond to echo requests.

2013-05-03 Thread Ben Pfaff 
On Fri, May 03, 2013 at 01:36:28PM -0700, Ethan Jackson wrote:
> > +case OFPTYPE_ECHO_REQUEST:
> > +if (reply_to_echo_requests) {
> > +struct ofpbuf *reply;
> > +
> > +reply = make_echo_reply(b->data);
> > +retval = vconn_send(vconn, reply);
> > +if (retval && retval != EAGAIN) {
> > +ovs_fatal(retval, "failed to send echo reply");
> > +}
> > +}
> 
> I think we need to destroy 'reply' when vconn_send() returns EAGAIN.
> Otherwise I think we leak it.

How about if I change "vconn_send()" to "vconn_send_block()" here?

Thanks,

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

Re: [ovs-dev] [PATCH] bridge: Correctly omit unsupported interface statistics from database.

2013-05-03 Thread Ethan Jackson 
> +#define IFACE_STAT(MEMBER, NAME) +1

You need a space after the + here.

Otherwise looks good, thanks.

Acked-by: Ethan Jackson 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ofp-util: Fix type of 'port' param to ofputil_encode_dump_ports_request().

2013-05-03 Thread Ethan Jackson 
Acked-by: Ethan Jackson 

On Wed, Feb 13, 2013 at 11:06 PM, Ben Pfaff  wrote:
> We always use unsigned types for port numbers elsewhere, so use one here
> too.
>
> Signed-off-by: Ben Pfaff 
> ---
>  lib/ofp-util.c |2 +-
>  lib/ofp-util.h |2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index dd6eed8..2716f82 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -4519,7 +4519,7 @@ ofputil_parse_key_value(char **stringp, char **keyp, 
> char **valuep)
>   * will be for Open Flow version 'ofp_version'. Returns message
>   * as a struct ofpbuf. Returns encoded message on success, NULL on error */
>  struct ofpbuf *
> -ofputil_encode_dump_ports_request(enum ofp_version ofp_version, int16_t port)
> +ofputil_encode_dump_ports_request(enum ofp_version ofp_version, uint16_t 
> port)
>  {
>  struct ofpbuf *request;
>
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 955886d..5698491 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -694,7 +694,7 @@ struct ofputil_port_stats {
>  };
>
>  struct ofpbuf *ofputil_encode_dump_ports_request(enum ofp_version 
> ofp_version,
> - int16_t port);
> + uint16_t port);
>  void ofputil_append_port_stat(struct list *replies,
>const struct ofputil_port_stats *ops);
>  size_t ofputil_count_port_stats(const struct ofp_header *);
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [monitor 3/3] ovs-ofctl: Make "ovs-ofctl monitor" respond to echo requests.

2013-05-03 Thread Ethan Jackson 
Fine with me.

Acked-by: Ethan Jackson 

On Fri, May 3, 2013 at 1:45 PM, Ben Pfaff  wrote:
> On Fri, May 03, 2013 at 01:36:28PM -0700, Ethan Jackson wrote:
>> > +case OFPTYPE_ECHO_REQUEST:
>> > +if (reply_to_echo_requests) {
>> > +struct ofpbuf *reply;
>> > +
>> > +reply = make_echo_reply(b->data);
>> > +retval = vconn_send(vconn, reply);
>> > +if (retval && retval != EAGAIN) {
>> > +ovs_fatal(retval, "failed to send echo reply");
>> > +}
>> > +}
>>
>> I think we need to destroy 'reply' when vconn_send() returns EAGAIN.
>> Otherwise I think we leak it.
>
> How about if I change "vconn_send()" to "vconn_send_block()" here?
>
> Thanks,
>
> Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] netdev-bsd: Use UINT64_MAX for unsupported stats.

2013-05-03 Thread Ed Maste 
As documented in netdev-provider.h for the get_stats method.

Signed-off-by: Ed Maste 
---
 lib/netdev-bsd.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index e3461b8..7ab9d3e 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -907,22 +907,22 @@ netdev_bsd_get_stats(const struct netdev *netdev_, struct 
netdev_stats *stats)
 stats->rx_errors = ifmd.ifmd_data.ifi_ierrors;
 stats->tx_errors = ifmd.ifmd_data.ifi_oerrors;
 stats->rx_dropped = ifmd.ifmd_data.ifi_iqdrops;
-stats->tx_dropped = 0;
+stats->tx_dropped = UINT64_MAX;
 stats->multicast = ifmd.ifmd_data.ifi_imcasts;
 stats->collisions = ifmd.ifmd_data.ifi_collisions;
 
-stats->rx_length_errors = 0;
-stats->rx_over_errors = 0;
-stats->rx_crc_errors = 0;
-stats->rx_frame_errors = 0;
-stats->rx_fifo_errors = 0;
-stats->rx_missed_errors = 0;
-
-stats->tx_aborted_errors = 0;
-stats->tx_carrier_errors = 0;
-stats->tx_fifo_errors = 0;
-stats->tx_heartbeat_errors = 0;
-stats->tx_window_errors = 0;
+stats->rx_length_errors = UINT64_MAX;
+stats->rx_over_errors = UINT64_MAX;
+stats->rx_crc_errors = UINT64_MAX;
+stats->rx_frame_errors = UINT64_MAX;
+stats->rx_fifo_errors = UINT64_MAX;
+stats->rx_missed_errors = UINT64_MAX;
+
+stats->tx_aborted_errors = UINT64_MAX;
+stats->tx_carrier_errors = UINT64_MAX;
+stats->tx_fifo_errors = UINT64_MAX;
+stats->tx_heartbeat_errors = UINT64_MAX;
+stats->tx_window_errors = UINT64_MAX;
 break;
 }
 }
-- 
1.7.11.5

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

Re: [ovs-dev] [monitor 3/3] ovs-ofctl: Make "ovs-ofctl monitor" respond to echo requests.

2013-05-03 Thread Ben Pfaff 
Thanks.  I made that change, and removed "&& retval != EAGAIN" from
the "if" condition (since vconn_send_block() does not return EAGAIN),
and applied this series to master.

On Fri, May 03, 2013 at 01:49:03PM -0700, Ethan Jackson wrote:
> Fine with me.
> 
> Acked-by: Ethan Jackson 
> 
> On Fri, May 3, 2013 at 1:45 PM, Ben Pfaff  wrote:
> > On Fri, May 03, 2013 at 01:36:28PM -0700, Ethan Jackson wrote:
> >> > +case OFPTYPE_ECHO_REQUEST:
> >> > +if (reply_to_echo_requests) {
> >> > +struct ofpbuf *reply;
> >> > +
> >> > +reply = make_echo_reply(b->data);
> >> > +retval = vconn_send(vconn, reply);
> >> > +if (retval && retval != EAGAIN) {
> >> > +ovs_fatal(retval, "failed to send echo reply");
> >> > +}
> >> > +}
> >>
> >> I think we need to destroy 'reply' when vconn_send() returns EAGAIN.
> >> Otherwise I think we leak it.
> >
> > How about if I change "vconn_send()" to "vconn_send_block()" here?
> >
> > Thanks,
> >
> > Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ofp-util: Fix type of 'port' param to ofputil_encode_dump_ports_request().

2013-05-03 Thread Ben Pfaff 
Thanks, I applied this to master.

On Fri, May 03, 2013 at 01:48:10PM -0700, Ethan Jackson wrote:
> Acked-by: Ethan Jackson 
> 
> On Wed, Feb 13, 2013 at 11:06 PM, Ben Pfaff  wrote:
> > We always use unsigned types for port numbers elsewhere, so use one here
> > too.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  lib/ofp-util.c |2 +-
> >  lib/ofp-util.h |2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index dd6eed8..2716f82 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -4519,7 +4519,7 @@ ofputil_parse_key_value(char **stringp, char **keyp, 
> > char **valuep)
> >   * will be for Open Flow version 'ofp_version'. Returns message
> >   * as a struct ofpbuf. Returns encoded message on success, NULL on error */
> >  struct ofpbuf *
> > -ofputil_encode_dump_ports_request(enum ofp_version ofp_version, int16_t 
> > port)
> > +ofputil_encode_dump_ports_request(enum ofp_version ofp_version, uint16_t 
> > port)
> >  {
> >  struct ofpbuf *request;
> >
> > diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> > index 955886d..5698491 100644
> > --- a/lib/ofp-util.h
> > +++ b/lib/ofp-util.h
> > @@ -694,7 +694,7 @@ struct ofputil_port_stats {
> >  };
> >
> >  struct ofpbuf *ofputil_encode_dump_ports_request(enum ofp_version 
> > ofp_version,
> > - int16_t port);
> > + uint16_t port);
> >  void ofputil_append_port_stat(struct list *replies,
> >const struct ofputil_port_stats *ops);
> >  size_t ofputil_count_port_stats(const struct ofp_header *);
> > --
> > 1.7.10.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] netdev-bsd: Use UINT64_MAX for unsupported stats.

2013-05-03 Thread Ben Pfaff 
On Fri, May 03, 2013 at 04:57:38PM -0400, Ed Maste wrote:
> As documented in netdev-provider.h for the get_stats method.
> 
> Signed-off-by: Ed Maste 

Thanks, applied to master, branch-1.11, and branch-1.10.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ofp-util: OpenFlow 1.0 can match IPv6 Ethertype even though not L3 or L4.

2013-05-03 Thread Ben Pfaff 
OpenFlow 1.0 can match on flows that have the IPv6 Ethertype, but
ofputil_usable_protocols() incorrectly reported that such a match required
NXM or OXM.  This commit fixes the problem.

Also, add some related tests.

Reported-by: Nagi Reddy Jonnala 
Signed-off-by: Ben Pfaff 
---
 AUTHORS|1 +
 lib/ofp-util.c |   21 +++
 tests/ovs-ofctl.at |  101 
 3 files changed, 115 insertions(+), 8 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index 593776d..be0e637 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -175,6 +175,7 @@ Mike Kruze  mkr...@nicira.com
 Min Chenustcer.tonyc...@gmail.com
 Murphy McCauley murphy.mccau...@gmail.com
 Mikael Doverhag mdover...@nicira.com
+Nagi Reddy Jonnala  njonn...@brocade.com
 Niklas Anderssonnanders...@nicira.com
 Pankaj Thakkar  thak...@nicira.com
 Paul Ingram p...@nicira.com
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 44d3c4e..07c6ce2 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1095,8 +1095,19 @@ ofputil_usable_protocols(const struct match *match)
 | OFPUTIL_P_OF13_OXM;
 }
 
-/* NXM and OXM support matching IPv6 traffic. */
-if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) {
+/* NXM and OXM support matching L3 and L4 fields within IPv6.
+ *
+ * (arp_sha, arp_tha, nw_frag, and nw_ttl are covered elsewhere so they
+ * don't need to be included in this test too.) */
+if (match->flow.dl_type == htons(ETH_TYPE_IPV6)
+&& (!ipv6_mask_is_any(&wc->masks.ipv6_src)
+|| !ipv6_mask_is_any(&wc->masks.ipv6_dst)
+|| !ipv6_mask_is_any(&wc->masks.nd_target)
+|| wc->masks.ipv6_label
+|| wc->masks.tp_src
+|| wc->masks.tp_dst
+|| wc->masks.nw_proto
+|| wc->masks.nw_tos)) {
 return OFPUTIL_P_OF10_NXM_ANY | OFPUTIL_P_OF12_OXM
 | OFPUTIL_P_OF13_OXM;
 }
@@ -1119,12 +1130,6 @@ ofputil_usable_protocols(const struct match *match)
 | OFPUTIL_P_OF13_OXM;
 }
 
-/* NXM and OXM support matching IPv6 flow label. */
-if (wc->masks.ipv6_label) {
-return OFPUTIL_P_OF10_NXM_ANY | OFPUTIL_P_OF12_OXM
-| OFPUTIL_P_OF13_OXM;
-}
-
 /* NXM and OXM support matching IP ECN bits. */
 if (wc->masks.nw_tos & IP_ECN_MASK) {
 return OFPUTIL_P_OF10_NXM_ANY | OFPUTIL_P_OF12_OXM
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index db19e01..295cdc7 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -1,5 +1,106 @@
 AT_BANNER([ovs-ofctl])
 
+AT_SETUP([ovs-ofctl parse-flows choice of protocol])
+# This doesn't cover some potential vlan_tci test cases.
+for test_case in \
+'tun_id=0NXM,OXM' \
+'tun_src=1.2.3.4 none' \
+'tun_dst=1.2.3.4 none' \
+'tun_flags=0 none' \
+'tun_tos=0   none' \
+'tun_ttl=0   none' \
+'metadata=0  NXM,OXM' \
+'in_port=1   any' \
+'skb_priority=0  none' \
+'skb_mark=1  none' \
+'reg0=0  NXM,OXM' \
+'reg1=1  NXM,OXM' \
+'reg2=2  NXM,OXM' \
+'reg3=3  NXM,OXM' \
+'reg4=4  NXM,OXM' \
+'reg5=5  NXM,OXM' \
+'reg6=6  NXM,OXM' \
+'reg7=7  NXM,OXM' \
+'dl_src=00:11:22:33:44:55any' \
+'dl_src=00:11:22:33:44:55/00:ff:ff:ff:ff:ff  NXM,OXM' \
+'dl_dst=00:11:22:33:44:55any' \
+'dl_dst=00:11:22:33:44:55/00:ff:ff:ff:ff:ff  NXM,OXM' \
+'dl_type=0x1234  any' \
+'dl_type=0x0800  any' \
+'dl_type=0x0806  any' \
+'dl_type=0x86dd  any' \
+'vlan_tci=0  any' \
+'vlan_tci=0x1009 any' \
+'dl_vlan=9   any' \
+'vlan_vid=11 any' \
+'dl_vlan_pcp=6   any' \
+'vlan_pcp=5  any' \
+'mpls,mpls_label=5   NXM,OXM' \
+'mpls,mpls_tc=1  NXM,OXM' \
+'mpls,mpls_bos=0 NXM,OXM' \
+'ip,ip_src=1.2.3.4   any' \
+'ip,ip_src=192.168.0.0/24any' \
+'ip,ip_src=192.0.168.0/255.0.255.0

[ovs-dev] [PATCH] DESIGN: Fix typo in "VLAN Matching" section.

2013-05-03 Thread Ben Pfaff 
Signed-off-by: Ben Pfaff 
---
 DESIGN |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/DESIGN b/DESIGN
index f3e9309..0037eea 100644
--- a/DESIGN
+++ b/DESIGN
@@ -185,7 +185,7 @@ Each column is interpreted as follows.
 
 - OF1.0 and OF1.1: /x,yy/z means dl_vlan , OFPFW_DL_VLAN
   x, dl_vlan_pcp yy, and OFPFW_DL_VLAN_PCP z.  ? means that the
-  given nibble is ignored (and conventionally 0 for  or z,
+  given nibble is ignored (and conventionally 0 for  or yy,
   conventionally 1 for x or z).   means that the given match
   is not supported.
 
-- 
1.7.2.5

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

[ovs-dev] [PATCH] meta-flow: Be pickier about format of Ethernet addresses.

2013-05-03 Thread Ben Pfaff 
Otherwise, input with invalid trailing data was accepted, such as input
that had 7 colon-separated segments instead of 6.

Signed-off-by: Ben Pfaff 
---
 lib/meta-flow.c |   21 +
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 9296faa..9ca16e2 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -2073,20 +2073,25 @@ mf_from_ethernet_string(const struct mf_field *mf, 
const char *s,
 uint8_t mac[ETH_ADDR_LEN],
 uint8_t mask[ETH_ADDR_LEN])
 {
-ovs_assert(mf->n_bytes == ETH_ADDR_LEN);
+int n;
 
-switch (sscanf(s, ETH_ADDR_SCAN_FMT"/"ETH_ADDR_SCAN_FMT,
-   ETH_ADDR_SCAN_ARGS(mac), ETH_ADDR_SCAN_ARGS(mask))){
-case ETH_ADDR_SCAN_COUNT * 2:
-return NULL;
+ovs_assert(mf->n_bytes == ETH_ADDR_LEN);
 
-case ETH_ADDR_SCAN_COUNT:
+n = -1;
+if (sscanf(s, ETH_ADDR_SCAN_FMT"%n", ETH_ADDR_SCAN_ARGS(mac), &n) > 0
+&& n == strlen(s)) {
 memset(mask, 0xff, ETH_ADDR_LEN);
 return NULL;
+}
 
-default:
-return xasprintf("%s: invalid Ethernet address", s);
+n = -1;
+if (sscanf(s, ETH_ADDR_SCAN_FMT"/"ETH_ADDR_SCAN_FMT"%n",
+   ETH_ADDR_SCAN_ARGS(mac), ETH_ADDR_SCAN_ARGS(mask), &n) > 0
+&& n == strlen(s)) {
+return NULL;
 }
+
+return xasprintf("%s: invalid Ethernet address", s);
 }
 
 static char *
-- 
1.7.2.5

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

Re: [ovs-dev] [PATCH] bridge: Correctly omit unsupported interface statistics from database.

2013-05-03 Thread Ben Pfaff 
On Fri, May 03, 2013 at 01:47:05PM -0700, Ethan Jackson wrote:
> > +#define IFACE_STAT(MEMBER, NAME) +1
> 
> You need a space after the + here.
> 
> Otherwise looks good, thanks.
> 
> Acked-by: Ethan Jackson 

Thanks, I fixed that and applied this to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] datapath: Remove unused get_config vport op.

2013-05-03 Thread Jesse Gross 
The get_config vport op is left over from old compatibility code,
it is neither used nor implemented any more.

Signed-off-by: Jesse Gross 
---
 datapath/vport-netdev.h |1 -
 datapath/vport.h|3 ---
 2 files changed, 4 deletions(-)

diff --git a/datapath/vport-netdev.h b/datapath/vport-netdev.h
index a3cb3a3..dd298b5 100644
--- a/datapath/vport-netdev.h
+++ b/datapath/vport-netdev.h
@@ -39,6 +39,5 @@ netdev_vport_priv(const struct vport *vport)
 }
 
 const char *ovs_netdev_get_name(const struct vport *);
-const char *ovs_netdev_get_config(const struct vport *);
 
 #endif /* vport_netdev.h */
diff --git a/datapath/vport.h b/datapath/vport.h
index 7233e4f..c23d35c 100644
--- a/datapath/vport.h
+++ b/datapath/vport.h
@@ -138,8 +138,6 @@ struct vport_parms {
  * existing vport to a &struct sk_buff.  May be %NULL for a vport that does not
  * have any configuration.
  * @get_name: Get the device's name.
- * @get_config: Get the device's configuration.
- * May be null if the device does not have an ifindex.
  * @send: Send a packet on the device.  Returns the length of the packet sent.
  */
 struct vport_ops {
@@ -159,7 +157,6 @@ struct vport_ops {
 
/* Called with rcu_read_lock or ovs_mutex. */
const char *(*get_name)(const struct vport *);
-   void (*get_config)(const struct vport *, void *);
 
int (*send)(struct vport *, struct sk_buff *);
 };
-- 
1.7.10.4

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