Re: [ovs-dev] [PATCH 0/7] OpenFlow-level flow-based tunneling support

[Rajahalme, Jarno (NSN - FI/Espoo)]

On May 2, 2013, at 21:21 , ext Ben Pfaff wrote:

> There has been a lot of discussion on patch 4.  Do you plan to update
> and re-send the series, or do you expect further discussion or review
> of the rest of the series before we go on?

I have the rest of the series done, I was waiting for some feedback on the 
output action restriction thing. I'll send updated patches shortly.

  Jarno

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

Re: [ovs-dev] [PATCH 4/7] Keep all of tunnel metadata in flow.

[Rajahalme, Jarno (NSN - FI/Espoo)]

On May 2, 2013, at 21:19 , ext Ben Pfaff wrote:

> On Tue, Apr 30, 2013 at 05:21:28AM +, Rajahalme, Jarno (NSN - FI/Espoo) 
> wrote:
>> 
>> On Apr 29, 2013, at 20:49 , ext Jesse Gross wrote:
>> 
>>> On Sun, Apr 28, 2013 at 11:29 AM, Rajahalme, Jarno (NSN - FI/Espoo)
>>>  wrote:
 
 Another thing that I came to think only when reading Ben's new tutorial:
 Output to the input port is skipped. This would be a problem if you only
 have one generic flow based GRE port (as enabled by the last patch in
 this series), and you should forward GRE input to another GRE output.
 This could be fixed by always allowing output (in xlate_output_action())
 to a tunnel that has cfg.ip_dst_flow set, even if the output port is the
 same as the input port. Thoughts?
>>> 
>>> I know that Ben was thinking about ways to relax this check anyways,
>>> so it might be good to see how that fits with this.
>> 
>> 
>> It seems to me that it should be relatively safe to allow packets that have
>> been modified by actions to be sent to the input port, if so directed by the
>> flow action. At least when the destination mac has been changed.
> 
> I agree (or perhaps if the VLAN has changed) but it seems to me that
> it's not hard to add new actions.
> 
> Another approach would be to allow the in_port field to be changed.
> Change it to 0 or OFPP_NONE then do your output.  Wrap it in
> NXAST_STACK_PUSH and NXAST_STACK_POP if you don't want it changed
> permanently.



I actually tried changing the "in_port" before realizing it is not writeable. 
Making it writeable would be one line change. However, setting the in_port for 
this purpose would add some overhead and maybe seem like a kludge in 
retrospect, but it might easily become OF compatible, if the "in_port is not 
writeable" phrase is later removed from the spec. 

In another mail you said you'd take in a patch to a new action 
(NXAST_ALLWAYS_OUTPUT?). Which one you prefer?

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

[ovs-dev] [PATCH v2 0/4] OpenFlow-level flow-based tunneling support

[Jarno Rajahalme]

Adds tun_src and tun_dst match and set capabilities via new NXM fields
NXM_NX_TUN_IPV4_SRC and NXM_NX_TUN_IPV4_DST.  This allows management of
large number of tunnels via flow tables, without requiring the tunnels
to be pre-configured.

Flow-based tunnels can be configured with options remote_ip=flow and
local_ip=flow.  local_ip=flow requires remote_ip=flow.  When set, the
tunnel remote IP address and/or local IP address is set from the flow,
instead of the tunnel configuration.

Example:

$ ovs-vsctl add-port br0 gre -- set Interface gre ofport_request=1 type=gre 
options:remote_ip=flow options:key=flow
$ ovs-ofctl add-flow br0 "in_port=LOCAL 
actions=set_tunnel:1,set_field:192.168.0.1->tun_dst,output:1"
$ ovs-ofctl add-flow br0 "in_port=1 tun_src=192.168.0.1 tun_id=1 actions=LOCAL"

Jarno Rajahalme (4):
  Keep all of tunnel metadata in flow.
  ofproto-dpif: Remove unnecessary initial_vals.tunnel_ip_tos.
  ofproto-dpif: Keep perfect fitness on tunnel input.
  OpenFlow-level flow-based tunneling support.

---
v2:
- Rebase
- Keep ctx->base_flow.tunnel zeroed to reflect datapath view of the flow
- Add orig_tunnel_ip_dst to struct action_xlate_ctx and prevent
  tunnel output with tunnel ip_dst the same is the original tunnel IP
  destination address.  This helps avoiding loops otherwise easy to create
  accidentally with the new 'ip_remote=flow' tunnel configuration option.

 include/openflow/nicira-ext.h |3 +++
 lib/flow.c|2 ++
 lib/flow.h|2 ++
 lib/match.c   |4 
 lib/meta-flow.c   |   20 +---
 lib/netdev-vport.c|   21 +---
 lib/netdev.h  |2 ++
 lib/nx-match.c|8 ++-
 lib/ofp-print.c   |8 +++
 lib/ofp-util.c|   29 +++---
 ofproto/ofproto-dpif.c|   53 +++--
 ofproto/tunnel.c  |   41 ++-
 tests/ofproto.at  |   27 +
 tests/tunnel.at   |   28 ++
 14 files changed, 183 insertions(+), 65 deletions(-)

-- 
1.7.10.4

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

[ovs-dev] [PATCH v2 1/4] Keep all of tunnel metadata in flow.

[Jarno Rajahalme]

Do not clear tunnel metadata on tunnel input. This is used by a later patch.

Signed-off-by: Jarno Rajahalme 
---
v2:
- Keep ctx->base_flow.tunnel zeroed to reflect datapath view of the flow

 ofproto/ofproto-dpif.c |   17 +++--
 ofproto/tunnel.c   |3 +--
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 89a4668..3f420af 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -7025,8 +7025,6 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
   struct rule_dpif *rule,
   uint8_t tcp_flags, const struct ofpbuf *packet)
 {
-ovs_be64 initial_tun_id = flow->tunnel.tun_id;
-
 /* Flow initialization rules:
  * - 'base_flow' must match the kernel's view of the packet at the
  *   time that action processing starts.  'flow' represents any
@@ -7038,23 +7036,22 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
  *   to another device without any modifications this will cause us to
  *   insert a new tag since the original one was stripped off by the
  *   VLAN device.
- * - Tunnel 'flow' is largely cleared when transitioning between
- *   the input and output stages since it does not make sense to output
- *   a packet with the exact headers that it was received with (i.e.
- *   the destination IP is us).  The one exception is the tun_id, which
- *   is preserved to allow use in later resubmit lookups and loads into
- *   registers.
+ * - Tunnel metadata as received is retained in 'flow'. This allows
+ *   tunnel metadata matching also in later tables.
+ *   Since a kernel action for setting the tunnel metadata will only be
+ *   generated with actual tunnel output, changing the tunnel metadata
+ *   values in 'flow' (such as tun_id) will only have effect with a later
+ *   tunnel output action.
  * - Tunnel 'base_flow' is completely cleared since that is what the
  *   kernel does.  If we wish to maintain the original values an action
  *   needs to be generated. */
 
 ctx->ofproto = ofproto;
 ctx->flow = *flow;
-memset(&ctx->flow.tunnel, 0, sizeof ctx->flow.tunnel);
 ctx->base_flow = ctx->flow;
+memset(&ctx->base_flow.tunnel, 0, sizeof ctx->base_flow.tunnel);
 ctx->base_flow.vlan_tci = initial_vals->vlan_tci;
 ctx->base_flow.tunnel.ip_tos = initial_vals->tunnel_ip_tos;
-ctx->flow.tunnel.tun_id = initial_tun_id;
 ctx->rule = rule;
 ctx->packet = packet;
 ctx->may_learn = packet != NULL;
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 8d29184..f5bbfb9 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -197,8 +197,7 @@ tnl_port_receive(struct flow *flow)
 }
 
 flow->in_port = tnl_port->ofport->ofp_port;
-memset(&flow->tunnel, 0, sizeof flow->tunnel);
-flow->tunnel.tun_id = match.in_key;
+/* Keep flow->tunnel to allow matching on tunnel metadata */
 
 if (pre_flow_str) {
 char *post_flow_str = flow_to_string(flow);
-- 
1.7.10.4

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

[ovs-dev] [PATCH v2 2/4] ofproto-dpif: Remove initial_vals.tunnel_ip_tos.

[Jarno Rajahalme]

As tunnel metadata is no longer cleared on input, and the input
values are retained in 'ctx->flow' accross tunnel output actions,
there is no need to store the tunnel.ip_tos to initial_vals.

Signed-off-by: Jarno Rajahalme 
---
v2: no change

 ofproto/ofproto-dpif.c |   15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3f420af..2459e5e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -304,9 +304,6 @@ struct initial_vals {
 * This member should be removed when the VLAN splinters feature is no
 * longer needed. */
 ovs_be16 vlan_tci;
-
-/* If received on a tunnel, the IP TOS value of the tunnel. */
-uint8_t tunnel_ip_tos;
 };
 
 static void action_xlate_ctx_init(struct action_xlate_ctx *,
@@ -3837,8 +3834,7 @@ drop_key_clear(struct dpif_backer *backer)
  * to the VLAN TCI with which the packet was really received, that is, the
  * actual VLAN TCI extracted by odp_flow_key_to_flow().  (This differs from
  * the value returned in flow->vlan_tci only for packets received on
- * VLAN splinters.)  Also, if received on an IP tunnel, sets
- * 'initial_vals->tunnel_ip_tos' to the tunnel's IP TOS.
+ * VLAN splinters.)
  *
  * Similarly, this function also includes some logic to help with tunnels.  It
  * may modify 'flow' as necessary to make the tunneling implementation
@@ -3865,7 +3861,6 @@ ofproto_receive(const struct dpif_backer *backer, struct 
ofpbuf *packet,
 
 if (initial_vals) {
 initial_vals->vlan_tci = flow->vlan_tci;
-initial_vals->tunnel_ip_tos = flow->tunnel.ip_tos;
 }
 
 if (odp_in_port) {
@@ -5752,7 +5747,6 @@ rule_dpif_execute(struct rule_dpif *rule, const struct 
flow *flow,
 rule_credit_stats(rule, &stats);
 
 initial_vals.vlan_tci = flow->vlan_tci;
-initial_vals.tunnel_ip_tos = flow->tunnel.ip_tos;
 ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
 action_xlate_ctx_init(&ctx, ofproto, flow, &initial_vals,
   rule, stats.tcp_flags, packet);
@@ -6750,7 +6744,7 @@ static bool
 tunnel_ecn_ok(struct action_xlate_ctx *ctx)
 {
 if (is_ip_any(&ctx->base_flow)
-&& (ctx->base_flow.tunnel.ip_tos & IP_ECN_MASK) == IP_ECN_CE) {
+&& (ctx->flow.tunnel.ip_tos & IP_ECN_MASK) == IP_ECN_CE) {
 if ((ctx->base_flow.nw_tos & IP_ECN_MASK) == IP_ECN_NOT_ECT) {
 VLOG_WARN_RL(&rl, "dropping tunnel packet marked ECN CE"
  " but is not ECN capable");
@@ -7051,7 +7045,6 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
 ctx->base_flow = ctx->flow;
 memset(&ctx->base_flow.tunnel, 0, sizeof ctx->base_flow.tunnel);
 ctx->base_flow.vlan_tci = initial_vals->vlan_tci;
-ctx->base_flow.tunnel.ip_tos = initial_vals->tunnel_ip_tos;
 ctx->rule = rule;
 ctx->packet = packet;
 ctx->may_learn = packet != NULL;
@@ -7137,7 +7130,6 @@ xlate_actions(struct action_xlate_ctx *ctx,
 uint32_t local_odp_port;
 
 initial_vals.vlan_tci = ctx->base_flow.vlan_tci;
-initial_vals.tunnel_ip_tos = ctx->base_flow.tunnel.ip_tos;
 
 add_sflow_action(ctx);
 add_ipfix_action(ctx);
@@ -7902,7 +7894,6 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf 
*packet,
 dpif_flow_stats_extract(flow, packet, time_msec(), &stats);
 
 initial_vals.vlan_tci = flow->vlan_tci;
-initial_vals.tunnel_ip_tos = 0;
 action_xlate_ctx_init(&ctx, ofproto, flow, &initial_vals, NULL,
   packet_get_tcp_flags(packet, flow), packet);
 ctx.resubmit_stats = &stats;
@@ -8205,7 +8196,6 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int 
argc, const char *argv[],
 }
 
 initial_vals.vlan_tci = flow.vlan_tci;
-initial_vals.tunnel_ip_tos = flow.tunnel.ip_tos;
 }
 
 /* Generate a packet, if requested. */
@@ -8240,7 +8230,6 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int 
argc, const char *argv[],
 flow_extract(packet, priority, mark, NULL, in_port, &flow);
 flow.tunnel.tun_id = tun_id;
 initial_vals.vlan_tci = flow.vlan_tci;
-initial_vals.tunnel_ip_tos = flow.tunnel.ip_tos;
 } else {
 unixctl_command_reply_error(conn, "Bad command syntax");
 goto exit;
-- 
1.7.10.4

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

[ovs-dev] [PATCH v2 3/4] ofproto-dpif: Keep perfect fitness on tunnel input.

[Jarno Rajahalme]

As the flow is no longer modified (apart from the in_port) on tunnel
input, the perfect fitness can be retained.

Signed-off-by: Jarno Rajahalme 
---
v2: no change

 ofproto/ofproto-dpif.c |3 ---
 1 file changed, 3 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2459e5e..02f86e1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3875,9 +3875,6 @@ ofproto_receive(const struct dpif_backer *backer, struct 
ofpbuf *packet,
 }
 port = ofport_dpif_cast(ofport);
 
-/* We can't reproduce 'key' from 'flow'. */
-fitness = fitness == ODP_FIT_PERFECT ? ODP_FIT_TOO_MUCH : fitness;
-
 /* XXX: Since the tunnel module is not scoped per backer, it's
  * theoretically possible that we'll receive an ofport belonging to an
  * entirely different datapath.  In practice, this can't happen because
-- 
1.7.10.4

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

[ovs-dev] [PATCH v2 4/4] OpenFlow-level flow-based tunneling support.

[Jarno Rajahalme]

Adds tun_src and tun_dst match and set capabilities via new NXM fields
NXM_NX_TUN_IPV4_SRC and NXM_NX_TUN_IPV4_DST.  This allows management of
large number of tunnels via the flow tables, without requiring the tunnels
to be pre-configured.

Flow-based tunnels can be configured with options remote_ip=flow and
local_ip=flow.  local_ip=flow requires remote_ip=flow.  When set, the
tunnel remote IP address and/or local IP address is set from the flow,
instead of the tunnel configuration.

Example:

$ ovs-vsctl add-port br0 gre -- set Interface gre ofport_request=1 type=gre 
options:remote_ip=flow options:key=flow
$ ovs-ofctl add-flow br0 "in_port=LOCAL 
actions=set_tunnel:1,set_field:192.168.0.1->tun_dst,output:1"
$ ovs-ofctl add-flow br0 "in_port=1 tun_src=192.168.0.1 tun_id=1 actions=LOCAL"

Signed-off-by: Jarno Rajahalme 
---
v2:
- Add orig_tunnel_ip_dst to struct action_xlate_ctx and prevent
  tunnel output with tunnel ip_dst the same is the original tunnel IP
  destination address.  This helps avoiding loops otherwise easy to create
  accidentally with the new 'ip_remote=flow' tunnel configuration option.

 include/openflow/nicira-ext.h |3 +++
 lib/flow.c|2 ++
 lib/flow.h|2 ++
 lib/match.c   |4 
 lib/meta-flow.c   |   20 +++-
 lib/netdev-vport.c|   21 ++---
 lib/netdev.h  |2 ++
 lib/nx-match.c|8 +++-
 lib/ofp-print.c   |8 
 lib/ofp-util.c|   29 +++--
 ofproto/ofproto-dpif.c|   18 --
 ofproto/tunnel.c  |   38 ++
 tests/ofproto.at  |   27 +++
 tests/tunnel.at   |   28 
 14 files changed, 173 insertions(+), 37 deletions(-)

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index c80ff95..cfef5ef 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1746,6 +1746,9 @@ OFP_ASSERT(sizeof(struct nx_action_output_reg) == 24);
 #define NXM_NX_COOKIE NXM_HEADER  (0x0001, 30, 8)
 #define NXM_NX_COOKIE_W   NXM_HEADER_W(0x0001, 30, 8)
 
+#define NXM_NX_TUN_IPV4_SRC   NXM_HEADER  (0x0001, 31, 4)
+#define NXM_NX_TUN_IPV4_DST   NXM_HEADER  (0x0001, 32, 4)
+
 /* ## - ## */
 /* ## Requests and replies. ## */
 /* ## - ## */
diff --git a/lib/flow.c b/lib/flow.c
index 678b8f0..9a5f4b1 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -495,6 +495,8 @@ flow_get_metadata(const struct flow *flow, struct 
flow_metadata *fmd)
 BUILD_ASSERT_DECL(FLOW_WC_SEQ == 20);
 
 fmd->tun_id = flow->tunnel.tun_id;
+fmd->tun_src = flow->tunnel.ip_src;
+fmd->tun_dst = flow->tunnel.ip_dst;
 fmd->metadata = flow->metadata;
 memcpy(fmd->regs, flow->regs, sizeof fmd->regs);
 fmd->in_port = flow->in_port;
diff --git a/lib/flow.h b/lib/flow.h
index 6e169d6..0fdf4ef 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -118,6 +118,8 @@ BUILD_ASSERT_DECL(sizeof(struct flow) == sizeof(struct 
flow_tnl) + 160 &&
 /* Represents the metadata fields of struct flow. */
 struct flow_metadata {
 ovs_be64 tun_id; /* Encapsulating tunnel ID. */
+ovs_be32 tun_src;/* Tunnel outer IPv4 src addr */
+ovs_be32 tun_dst;/* Tunnel outer IPv4 dst addr */
 ovs_be64 metadata;   /* OpenFlow 1.1+ metadata field. */
 uint32_t regs[FLOW_N_REGS];  /* Registers. */
 uint16_t in_port;/* OpenFlow port or zero. */
diff --git a/lib/match.c b/lib/match.c
index fc3c540..512253e 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -135,13 +135,9 @@ match_wc_init(struct match *match, const struct flow *flow)
 void
 match_init_exact(struct match *match, const struct flow *flow)
 {
-ovs_be64 tun_id = flow->tunnel.tun_id;
-
 match->flow = *flow;
 match->flow.skb_priority = 0;
 match->flow.skb_mark = 0;
-memset(&match->flow.tunnel, 0, sizeof match->flow.tunnel);
-match->flow.tunnel.tun_id = tun_id;
 flow_wildcards_init_exact(&match->wc);
 }
 
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 9296faa..5eebffd 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -57,21 +57,21 @@ static const struct mf_field mf_fields[MFF_N_IDS] = {
 }, {
 MFF_TUN_SRC, "tun_src", NULL,
 MF_FIELD_SIZES(be32),
-MFM_NONE,
+MFM_FULLY,
 MFS_IPV4,
 MFP_NONE,
-false,
-0, NULL,
-0, NULL,
+true,
+NXM_NX_TUN_IPV4_SRC, "NXM_NX_TUN_IPV4_SRC",
+NXM_NX_TUN_IPV4_SRC, "NXM_NX_TUN_IPV4_SRC",
 }, {
 MFF_TUN_DST, "tun_dst", NULL,
 MF_FIELD_SIZES(be32),
-MFM_NONE,
+MFM_FULLY,
 MFS_IPV4,
 MFP_NONE,
-false,
-0, NULL,
-0, NULL,
+true,
+  

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

[Kyle Mestery (kmestery)]

On May 3, 2013, at 7:52 PM, Jesse Gross  wrote:
> 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 


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

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

[Jesse Gross]

On Mon, May 6, 2013 at 8:04 AM, Kyle Mestery (kmestery)
 wrote:
> On May 3, 2013, at 7:52 PM, Jesse Gross  wrote:
>> 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 
>
>
> Looks good Jesse.

Applied, thanks for the review.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/4] ofproto-dpif.c: Re-implement the ofproto/trace command

[Alex Wang]

From: Alex Wang 

Since the use of single datapath, all bridges belonging to the same type of
datapath will use the same (single) datapath. This causes confusion in the
current 'ofproto/trace' command. Especially, when given the unrelated
'bridge' and 'in_port' combination, the current implementation will still
be able to process and give misleading output. Thusly, this patch changes
the 'ofproto/trace' command syntax to formats shown as follow.

ofproto/trace [datapath] priority tun_id in_port mark packet
ofproto/trace [datapath] dp_flow [-generate]
ofproto/trace bridge br_flow [-generate]

Therein, the bridge name is replaced by datapath name in the first format,
since the mapping between in_port and the corresponding datapath is unique.
The second format requires datapath name since it uses 'ovs-dpctl dump-flows'
output. And the thrid format requires bridge name since it uses 'ovs-ofctl
add-flow' input like flow.

Signed-off-by: Alex Wang 
---
 ofproto/ofproto-dpif.c |  176 
 1 file changed, 133 insertions(+), 43 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6ec1c23..6550af3 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6073,7 +6073,7 @@ fix_sflow_action(struct action_xlate_ctx *ctx)
 }
 
 cookie = ofpbuf_at(ctx->odp_actions, ctx->user_cookie_offset,
-   sizeof cookie->sflow);
+   sizeof(*cookie));
 ovs_assert(cookie->type == USER_ACTION_COOKIE_SFLOW);
 
 compose_sflow_cookie(ctx->ofproto, base->vlan_tci,
@@ -8138,7 +8138,9 @@ static void
 ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
   void *aux OVS_UNUSED)
 {
-const char *dpname = argv[1];
+const char **adjusted_argv;
+const char *dpname;
+const struct dpif_backer *backer;
 struct ofproto_dpif *ofproto;
 struct ofpbuf odp_key;
 struct ofpbuf *packet;
@@ -8146,59 +8148,108 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int 
argc, const char *argv[],
 struct ds result;
 struct flow flow;
 char *s;
+bool is_dp;
+bool has_name;
 
+is_dp = false;
+has_name = false;
+dpname = NULL;
 packet = NULL;
+backer = NULL;
 ofpbuf_init(&odp_key, 0);
 ds_init(&result);
 
-ofproto = ofproto_dpif_lookup(dpname);
-if (!ofproto) {
-unixctl_command_reply_error(conn, "Unknown ofproto (use ofproto/list "
-"for help)");
-goto exit;
+/* Check the existence of datapath name in the argv. */
+if (argc == 2 || (argc == 3 && !strcmp(argv[2], "-generate"))
+|| argc == 6) {
+/* Check the number of datapaths. */
+if (shash_count(&all_dpif_backers) == 1) {
+   is_dp = true;
+has_name = false;
+} else {
+unixctl_command_reply_error(conn, "Must specify datapath name, "
+ "there is more than one type of datapath");
+goto exit;
+}
+} else {
+/* Else, the argv[1] is the datapath or bridge name.
+ * And since there are only three types of datapath (ovs-system,
+ * ovs-netdev and ovs-dummy), exact match is used to distingush
+ * datapath name and bridge name. */
+if (!strcmp(argv[1], "ovs-system") || !strcmp(argv[1], "ovs-netdev")
+|| !strcmp(argv[1], "ovs-dummy")) {
+is_dp = true;
+/* extract the type of datapath */
+dpname = argv[1] + 4;
+} else {
+is_dp = false;
+dpname = argv[1];
+}
+has_name = true;
 }
-if (argc == 3 || (argc == 4 && !strcmp(argv[3], "-generate"))) {
-/* ofproto/trace dpname flow [-generate] */
-const char *flow_s = argv[2];
-const char *generate_s = argv[3];
 
-/* Allow 'flow_s' to be either a datapath flow or an OpenFlow-like
- * flow.  We guess which type it is based on whether 'flow_s' contains
- * an '(', since a datapath flow always contains '(') but an
- * OpenFlow-like flow should not (in fact it's allowed but I believe
- * that's not documented anywhere).
- *
- * An alternative would be to try to parse 'flow_s' both ways, but then
- * it would be tricky giving a sensible error message.  After all, do
- * you just say "syntax error" or do you present both error messages?
- * Both choices seem lousy. */
-if (strchr(flow_s, '(')) {
-int error;
+/* If datapath is specified, extract the corresponding backer object. */
+if (is_dp) {
+if (has_name) {
+backer = shash_find_data(&all_dpif_backers, dpname);
+} else {
+struct shash_node *node;
+adjusted_argv = argv;
+node = shash_first(&all_dpif_backers);
+backer = node->data;
+}
+if (!b

[ovs-dev] [PATCH 2/4] tests: Fixes the tests affected by the new ofproto/trace command

[Alex Wang]

From: Alex Wang 

The following three tests are affected by the re-implemented ofproto/trace
command.

tests/learn.at
tests/ofproto-dpif.at
tests/tunnel.at

This patch fixes these affected tests.

Signed-off-by: Alex Wang 
---
 tests/learn.at|   16 +--
 tests/ofproto-dpif.at |   76 -
 tests/tunnel.at   |   50 
 3 files changed, 71 insertions(+), 71 deletions(-)

diff --git a/tests/learn.at b/tests/learn.at
index 8f59b63..eecbe81 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -73,7 +73,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 # Trace an ARP packet arriving on port 3, to create a MAC learning entry.
 
flow="in_port(3),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)"
-AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout])
 actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
 expected="1,2,100"
@@ -90,7 +90,7 @@ NXST_FLOW reply:
 
 # Trace a packet arrival destined for the learned MAC.
 # (This will also learn a MAC.)
-AT_CHECK([ovs-appctl ofproto/trace br0 
'in_port(1),eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:05),eth_type(0x0806),arp(sip=192.168.0.2,tip=192.168.0.1,op=2,sha=50:54:00:00:00:06,tha=50:54:00:00:00:05)'
 -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:05),eth_type(0x0806),arp(sip=192.168.0.2,tip=192.168.0.1,op=2,sha=50:54:00:00:00:06,tha=50:54:00:00:00:05)'
 -generate], [0], [stdout])
 AT_CHECK([tail -1 stdout], [0], [Datapath actions: 3
 ])
 
@@ -104,7 +104,7 @@ NXST_FLOW reply:
 
 # Trace a packet arrival that updates the first learned MAC entry.
 
flow="in_port(2),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)"
-AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout])
 actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
 expected="1,3,100"
@@ -145,7 +145,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 # Trace an ICMP packet arriving on port 3, to create a MAC learning entry.
 
flow="in_port(3),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)"
-AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout])
 actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
 expected="1,2,100"
@@ -164,7 +164,7 @@ NXST_FLOW reply:
 # disappear as long as we refresh it every second.
 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25; do
 ovs-appctl time/warp 1000
-AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], 
[stdout])
 
 # Check that the entry is there.
 AT_CHECK([ovs-ofctl dump-flows br0 table=1], [0], [stdout])
@@ -207,7 +207,7 @@ AT_CHECK([[ovs-ofctl add-flow br0 'table=0 tcp 
actions=learn(table=1, hard_timeo
 
 # Trace a TCPv4 packet arriving on port 3.
 
flow="in_port(3),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:06),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no),tcp(src=4,dst=80)"
-AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout])
 actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
 expected="1,2,100"
@@ -235,7 +235,7 @@ AT_CHECK([[ovs-ofctl add-flow br0 'table=0 tcp6 
actions=learn(table=1, hard_time
 
 # Trace a TCPv6 packet arriving on port 3.
 
flow="in_port(3),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:06),eth_type(0x86dd),ipv6(src=fec0::2,dst=fec0::1,label=0,proto=6,tclass=0,hlimit=255,frag=no),tcp(src=4,dst=80)"
-AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0], [stdout])
 actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
 expected="1,2,100"
@@ -294,7 +294,7 @@ AT_SETUP([learning action - fin_timeout feature])
 OVS_VSWITCHD_START(
 [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
 AT_CHECK([[ovs-ofctl add-flow br0 'actions=learn(fin_hard_timeout=10, 
fin_idle_timeout=5, NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], 
output:NXM_OF_IN_PORT[])']])
-AT_CHECK([ovs-appctl ofproto/trace br0 
'in_port(1),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)'
 -generate], [0], [ignore])
+

[ovs-dev] [PATCH 3/4] tests: Add tests for the ofproto/trace command

[Alex Wang]

From: Alex Wang 

Three testcases are added to the testsuite, which test the three command
formats and the corresponding corner cases.

Signed-off-by: Alex Wang 
---
 tests/ofproto-dpif.at |  209 +
 1 file changed, 209 insertions(+)

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 8cbbd80..3406b01 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1043,6 +1043,215 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow" 
"$actual"], [0], [expout])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+# Three testcases for the ofproto/trace command
+# The first one tests the normal input and corner cases
+# of the "ofproto/trace [datapath_name] datapath_flow"
+# and the "ofproto/trace bridge_name openflow_flow"
+AT_SETUP([ofproto-dpif - ofproto/trace command 1])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2)
+
+AT_DATA([flows.txt], [dnl
+in_port=1 actions=output:2
+in_port=2 actions=output:1
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+dp_flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
+of_flow="in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0"
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test command: ofproto/trace dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test incorrect command: ofproto/trace wrong_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace wrong_name "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Unknown bridge or datapath name
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test incorrect command: ofproto/trace nonexist_dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-system "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Cannot find datapath of this name
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test incorrect command: ofproto/trace br_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Cannot parse the openflow flow
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Add another bridge
+AT_CHECK([ovs-vsctl add-br br1 -- set bridge br1 datapath-type=netdev \
+  -- set bridge br1 fail-mode=secure])
+
+# Test incorrect command: ofproto/trace dp_flow, since there is more than one 
dp
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Must specify datapath name, there is more than one type of datapath
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test command: ofproto/trace dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Delete the added bridge
+AT_CHECK([ovs-vsctl del-br br1])
+
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test commmand: ofproto/trace br_name of_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$of_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test incorrect command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$of_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Bad datapath flow syntax
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+OVS_VSWITCHD_STOP(["/couldn't open old datapath ovs-dummy to remove it/d; 
/obtaining netdev stats via vport failed/d"])
+AT_CLEANUP
+
+# The second test tests the -generate option
+AT_SETUP([ofproto-dpif - ofproto/trace command 2])
+OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone])
+ADD_OF_PORTS([br0], 1, 2, 3)
+
+dp_flow='in_port(3),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff)'
+of_flow='arp,metadata=0,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=ff:ff:ff:ff:ff:ff'
+
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+
+# Check for no MAC learning entry.
+AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], 
[dnl
+ port  VLAN  MACAge
+])
+
+# Test command: ofproto/trace br_name of_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$of_flow"], [0], [stdout])
+
+# Check for no MAC learning entry.
+AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], 
[dnl
+ port  VLAN  MACAge
+])
+
+# Test command: ofproto/trace dp_flow -generate
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow" -generate], [

[ovs-dev] [PATCH 4/4] ofproto-unixctl.man: Modify the man page for ofproto/trace command

[Alex Wang]

From: Alex Wang 

This patch Modifies the man page for the ofproto/trace command in
accordance with the new implementation.

Signed-off-by: Alex Wang 
---
 ofproto/ofproto-unixctl.man |   62 ++-
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
index 8890343..0dbd5f5 100644
--- a/ofproto/ofproto-unixctl.man
+++ b/ofproto/ofproto-unixctl.man
@@ -6,12 +6,18 @@ These commands manage the core OpenFlow switch implementation 
(called
 Lists the names of the running ofproto instances.  These are the names
 that may be used on \fBofproto/trace\fR.
 .
-.IP "\fBofproto/trace \fIswitch priority tun_id in_port mark packet\fR"
-.IQ "\fBofproto/trace \fIswitch flow \fB\-generate\fR"
-Traces the path of an imaginary packet through \fIswitch\fR.  Both
-forms require \fIswitch\fR, the switch on which the packet arrived
-(one of those listed by \fBofproto/list\fR).  The first form specifies
-a packet's contents explicitly:
+.IP "\fBofproto/trace\fR [\fIdatapath\fR] \fIpriority tun_id in_port \
+mark packet\fR"
+.IQ "\fBofproto/trace\fR [\fIdatapath\fR] \fIdp_flow \fB\-generate\fR"
+.IQ "\fBofproto/trace \fIswitch br_flow \fB\-generate\fR"
+Traces the path of an imaginary packet through \fiswitch\fR.  The first 
+two forms require an optional \fIdatapath\fR argument, which is the datapath 
+the \fiswitch\fR is on (can be found by \fBdpif/show\fR command).  If more 
+than one datapath exists, the \fIdatapath\fR argument is compulsory.  
+The third form requires a \fIswitch\fR argument (one of those listed by 
+\fBofproto/list\fR).
+.IP
+The first form specifies a packet's contents explicitly:
 .RS
 .IP "\fIpriority\fR"
 Packet QoS priority. Use \fB0\fR if QoS is not setup.
@@ -19,8 +25,8 @@ Packet QoS priority. Use \fB0\fR if QoS is not setup.
 The tunnel ID on which the packet arrived.  Use
 \fB0\fR if the packet did not arrive through a tunnel.
 .IP "\fIin_port\fR"
-The OpenFlow port on which the packet arrived.  Use \fB65534\fR if the
-packet arrived on \fBOFPP_LOCAL\fR, the local port.
+The datapath port on which the packet arrived.  The port mapping can
+be found by \fBdpif/show\fR.
 .IP "\fImark\fR"
 SKB mark of the packet. Use \fB0\fR if Netfilter marks are not used.
 .IP "\fIpacket\fR"
@@ -31,38 +37,46 @@ by hand, so the \fBovs\-pcap\fR(1) and 
\fBovs\-tcpundump\fR(1)
 utilities provide easier ways.
 .RE
 .IP
-The second form specifies the packet's contents implicitly:
+The second and thrid form specify the packet's contents implicitly:
 .RS
-.IP "\fIflow\fR"
-A flow in one of two forms: either the form printed by
-\fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR command, or in a format
-similar to that accepted by \fBovs\-ofctl\fR(8)'s \fBadd\-flow\fR
-command.  This is not an OpenFlow flow: besides other differences, it
-never contains wildcards.  \fB\*(PN\fR generates an arbitrary packet
-that has the specified \fIflow\fR.
+.IP "\fIdp_flow\fR"
+A flow in the form printed by \fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR 
+command. \fB\*(PN\fR generates an arbitrary packet that has the specified 
+\fIdp_flow\fR.
+.IP "\fIbr_flow\fR"
+A flow in the form similar to that accepted by \fBovs\-ofctl\fR(8)'s 
+\fBadd\-flow\fR command.  This is not an OpenFlow flow: besides other 
+differences, it never contains wildcards.  \fB\*(PN\fR generates an 
+arbitrary packet that has the specified \fIbr_flow\fR.
 .RE
 .IP
 \fB\*(PN\fR will respond with extensive information on how the packet
 would be handled if it were to be received.  The packet will not
 actually be sent, but side effects such as MAC learning will occur.
 .
-.IP "\fBofproto/trace \fIswitch flow\fR"
+.IP "\fBofproto/trace\fR [\fIdatapath\fR] \fIdp_flow\fR"
+.IQ "\fBofproto/trace \fIswitch br_flow\fR"
 Traces the path of a packet in an imaginary flow through
 \fIswitch\fR.  The arguments are:
 .RS
+.IP "\fIdatapath\fR"
+The \fIdatapath\fR of the switch on which the packet arrived (can be 
+found by \fBdpif/show\fR).  If more than one datapath exists, the 
+\fIdatapath\fR argument is compulsory.
+.IP "\fIdp_flow\fR"
+A flow in the form printed by \fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR 
+command.
 .IP "\fIswitch\fR"
 The switch on which the packet arrived (one of those listed by
 \fBofproto/list\fR).
-.IP "\fIflow\fR"
-A flow in one of two forms: either the form printed by
-\fBovs\-dpctl\fR(8)'s \fBdump\-flows\fR command, or in a format
-similar to that accepted by \fBovs\-ofctl\fR(8)'s \fBadd\-flow\fR
-command.  This is not an OpenFlow flow: besides other differences, it
-never contains wildcards.
+.IP "\fIbr_flow\fR"
+A flow in the form similar to that accepted by \fBovs\-ofctl\fR(8)'s 
+\fBadd\-flow\fR command.  This is not an OpenFlow flow: besides other 
+differences, it never contains wildcards.
 .RE
 .IP
 \fB\*(PN\fR will respond with extensive information on how a packet
-in \fIflow\fR would be handled if it were received by
+in flow would be handled if it were 

[ovs-dev] [PATCH] Allow master to build on Fedora with the recent threading changes

[Kyle Mestery]

The recent threading changes have broken the build on Fedora,
and presumably other Red Hat based distributions. This adds an explicit
"-lpthread" to the linker command line and allows the latest master to build
on Fedora. I've also tested this on Ubuntu and it builds fine there.

Signed-off-by: Kyle Mestery 
---
 configure.ac | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure.ac b/configure.ac
index bbb6dea..fe3f9c7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -44,6 +44,7 @@ AC_SYS_LARGEFILE
 AC_SEARCH_LIBS([pow], [m])
 AC_SEARCH_LIBS([clock_gettime], [rt])
 AC_SEARCH_LIBS([timer_create], [rt])
+AC_SEARCH_LIBS([pthread_sigmask], [pthread])
 
 OVS_CHECK_ESX
 OVS_CHECK_COVERAGE
-- 
1.8.1.4

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

Re: [ovs-dev] [PATCH] Allow master to build on Fedora with the recent threading changes

[Ed Maste]

On 6 May 2013 13:47, Kyle Mestery  wrote:
> The recent threading changes have broken the build on Fedora,
> and presumably other Red Hat based distributions. This adds an explicit
> "-lpthread" to the linker command line and allows the latest master to build
> on Fedora. I've also tested this on Ubuntu and it builds fine there.
>
> Signed-off-by: Kyle Mestery 

Acked-by: Ed Maste 

Builds fine for me on FreeBSD with this change.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Pravin Shelar]

On Fri, May 3, 2013 at 10:46 AM, Jesse Gross  wrote:
> 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.

Thanks, I fixed it and push all four patches to master.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [bfd] bfd: Implement Bidirectional Forwarding Detection.

[Ethan Jackson]

Sorry to not have responded in so long on this.  It's become important
again, so I'm going to try to get it merged soon.  Seems pretty close.

> No, that's fine, but I found it hard to infer the units other than
> from the type.  Could some comments mention that they are msecs?

I added a comment in the struct indicating that all timers are in msecs.

> (x & 16383) == (x % 16384) for nonnegative integer x.  If you like %
> better then by all means use x % 16384.

I agree, doesn't matter much. I find modulo easier to reason about so
I've used that.

>> > I don't see a check for this requirement from 6.8.6 (I think it only
>> > requires comparing p->size to msg->length):
>>
>> I think we implicitly make this check by using ofpbuf_at() which will return
>> NULL if there aren't at least BFD_PACKET_LEN bytes.
>
> I agree that we know there are at least BFD_PACKET_LEN bytes.  We
> don't know that there are at least msg->length bytes, since
> msg->length might be greater than BFD_PACKET_LEN.

Actually, msg->length can't be greater that BFD_PACKET_LEN, Since we
don't allow authenticated mode, we require msg->length to be precisely
BFD_PACKET_LEN, otherwise we drop the packet. The real issue here is
that the code is confusing to someone reading the spec and cross
checking it.  I'll add a comment explaining what's going on.

>>
>> > The pseudocode at the end of 6.8.6 only mentions updating
>> > bfd.LocalDiag in a couple of cases, but the implementation appears to
>> > update LocalDiag in every case where it changes the session state.
>>
>> I found the RFC a bit confusing in this respect.  For example,
>> suppose you follow their instructions with respect to the diagnostic
>> strictly.  Then if the remote neighbor goes down, you will set
>> DIAG_RMT_DOWN, and have no way of going back to DIAG_NONE when the
>> session comes back up.  It isn't done upon reception of BFD packet
>> in either the DOWN or INIT states.  This didn't seem correct, to me,
>> so I choose to interpret the spec as setting the diagnostic to none
>> if not otherwise specified.  Does that sound reasonable?
>
> LocalDiag is described as:
>
>   The diagnostic code specifying the reason for the most recent
>   change in the local session state.  This MUST be initialized to
>   zero (No Diagnostic).
>
> I assumed, without carefully thinking it through, that this was sort
> of like errno, in that it can get set to some kind of error but
> nothing ever sets it back to 0, so that you can always use it to find
> out what the most recent error was, even after recovery.  I don't know
> whether that is the intent, but if it is then it would explain why it
> is never set back to "No Diagnostic".

It's all a bit confusing.  How about this: I'll change it to never set
"No Diagnostic" and add a note in the TODO at the top of the file to
check what another implementation does.  That should clear up the
ambiguity.

>
>> > I don't see anything in bfd_process_packet() that corresponds to:
>> >
>> >   If the packet was not discarded, it has been received for purposes
>> >   of the Detection Time expiration rules in section 6.8.4.
>> >
>> > but maybe I just missed it.
>>
>> I interpreted this line as satisfying the requirement:
>>
>> bfd->detect_time = bfd_rx_interval(bfd) * bfd->mult + time_msec();
>
> Oh, then I think it's in the wrong place.  For example, this:
>
>   If bfd.SessionState is AdminDown
>
>   Discard the packet
>
> is implemented as:
>
> if (bfd->state == STATE_ADMIN_DOWN) {
> VLOG_DBG_RL(&rl, "Administratively down, dropping control message.");
> return;
> }
>
> but
>
>  bfd->detect_time = bfd_rx_interval(bfd) * bfd->mult + time_msec();
>
> happens before that, so that the detection time is reset even if the
> packet was discarded.

I think the spec is inconsistent here, or at the very least I don't
know how to interpret it.

First it tells us

  Update the Detection Time as described in section 6.8.4.

  If bfd.SessionState is AdminDown

  Discard the packet

Then much later it tells us

  If the packet was not discarded, it has been received for purposes
  of the Detection Time expiration rules in section 6.8.4.

It's not clear to me what it's expecting us to do in this second
location that we haven't already done in the first.  At any rate, I
think the code is reasonable as is.  If we find it's inconsistent with
other implementations, we can change it later.

In debugging the BFD implementation as I developed it, I found the
current format easier to interpret.  I'll go ahead and leave it for
now.

Since it's been so long, I'll go ahead and resend the latest version
of the patch.  I think it's pretty much reviewed and ready to go, so
don't feel obligated to comb over it as nothings changed.  Feel free
to make additional comments if you'd like of course.

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

Re: [ovs-dev] [bfd] bfd: Implement Bidirectional Forwarding Detection.

[Ethan Jackson]

Traditionally, Open vSwitch has used a variant of 802.1ag "CFM" for
interface liveness detection.  This has served us well until now,
but has several serious drawbacks which have steadily become more
inconvenient.  First, the 802.1ag standard does not implement
several useful features forcing us to (optionally) break
compatibility.  Second, 802.1.ag is not particularly popular
outside of carrier grade networking equipment.  Third, 802.1ag is
simply quite awkward.

In an effort to solve the aforementioned problems, this patch
implements BFD which is ubiquitous, well designed, straight
forward, and implements required features in a standard way.  The
initial cut of the protocol focuses on getting the basics of the
specification correct, leaving performance optimizations, and
advanced features as future work.  The protocol should be
considered experimental pending future testing.

Signed-off-by: Ethan Jackson 
---
 lib/automake.mk|2 +
 lib/bfd.c  |  873 
 lib/bfd.h  |   46 +++
 lib/odp-util.c |2 +
 lib/odp-util.h |7 +-
 lib/util.h |1 +
 ofproto/ofproto-dpif.c |   64 +++-
 ofproto/ofproto-provider.h |   16 +
 ofproto/ofproto.c  |   39 ++
 ofproto/ofproto.h  |7 +
 vswitchd/bridge.c  |   12 +
 vswitchd/vswitch.ovsschema |   10 +-
 vswitchd/vswitch.xml   |   92 +
 13 files changed, 1164 insertions(+), 7 deletions(-)
 create mode 100644 lib/bfd.c
 create mode 100644 lib/bfd.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 1d58604..f340c60 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -12,6 +12,8 @@ lib_libopenvswitch_a_SOURCES = \
lib/aes128.h \
lib/backtrace.c \
lib/backtrace.h \
+   lib/bfd.c \
+   lib/bfd.h \
lib/bitmap.c \
lib/bitmap.h \
lib/bond.c \
diff --git a/lib/bfd.c b/lib/bfd.c
new file mode 100644
index 000..95dad2d
--- /dev/null
+++ b/lib/bfd.c
@@ -0,0 +1,873 @@
+/* Copyright (c) 2013 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. */
+
+#include 
+#include "bfd.h"
+
+#include 
+
+#include "csum.h"
+#include "dpif.h"
+#include "dynamic-string.h"
+#include "flow.h"
+#include "hash.h"
+#include "hmap.h"
+#include "list.h"
+#include "netlink.h"
+#include "odp-util.h"
+#include "ofpbuf.h"
+#include "openvswitch/types.h"
+#include "packets.h"
+#include "poll-loop.h"
+#include "random.h"
+#include "smap.h"
+#include "timeval.h"
+#include "unixctl.h"
+#include "util.h"
+#include "vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(bfd);
+
+/* XXX Finish BFD.
+ *
+ * The goal of this module is to replace CFM with something both more flexible
+ * and standards compliant.  In service of this goal, the following needs to be
+ * done.
+ *
+ * - Compliance
+ *   * Implement Demand mode.
+ *   * Go through the RFC line by line and verify we comply.
+ *   * Test against a hardware implementation.  Preferably a popular one.
+ *   * Delete BFD packets with nw_ttl != 255 in the datapath to prevent DOS
+ * attacks.
+ *
+ * - Unit tests.
+ *
+ * - BFD show into ovs-bugtool.
+ *
+ * - Set TOS/PCP on inner BFD frame, and outer tunnel header when encapped.
+ *
+ * - CFM "check_tnl_key" option equivalent.
+ *
+ * - CFM "fault override" equivalent.
+ *
+ * - Sending BFD messages should be in its own thread/process.
+ *
+ * - Scale testing.  How does it operate when there are large number of bfd
+ *   sessions?  Do we ever have random flaps?  What's the CPU utilization?
+ *
+ * - Rely on data traffic for liveness by using BFD demand mode.
+ *   If we're receiving traffic on a port, we can safely assume it's up (modulo
+ *   unidrectional failures).  BFD has a demand mode in which it can stay quiet
+ *   unless it feels the need to check the status of the port.  Using this, we
+ *   can implement a strategy in which BFD only sends control messages on dark
+ *   interfaces.
+ *
+ * - Depending on how one interprets the spec, it appears that a BFD session
+ *   can never change bfd.LocalDiag to "No Diagnostic".  We should verify that
+ *   this is what hardware implementations actually do.  Seems like "No
+ *   Diagnostic" should be set once a BFD session state goes UP. */
+
+#define BFD_VERSION 1
+
+enum flags {
+FLAG_MULTIPOINT = 1 << 0,
+FLAG_DEMAND = 1 << 1,
+FLAG_AUTH = 1 << 2,
+FLAG_CTL = 1 << 3,
+FLAG_FINAL = 1 << 4,
+FLAG_POLL = 1 

[ovs-dev] [PATCH] ovsdb-client: Avoid assertion with multiple databases.

[Justin Pettit]

When using ovsdb-client with an ovsdb-server with multiple databases, an
assertion could trigger due to them being returned in non-sorted order.
This commit changes the fetch_dbs() function to always return databases
in sorted order, since both callers are expecting that behavior.

Signed-off-by: Justin Pettit 
Reported-by: Spiro Kourtessis 
---
 AUTHORS  |1 +
 ovsdb/ovsdb-client.c |2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index 593776d..c113f16 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -194,6 +194,7 @@ Scott Hendricks shendri...@nicira.com
 Sean Brady  sbr...@gtfservices.com
 Sebastian Andrzej Siewior  sebast...@breakpoint.cc
 Sébastien RICCIOs...@swisscenter.com
+Spiro Kourtessissp...@vmware.com
 Srini Seetharaman   seeth...@stanford.edu
 Stephen Hemminger   shemmin...@vyatta.com
 Takayuki HAMA   t-h...@cb.jp.nec.com
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 37bd1aa..bba9782 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -392,6 +392,7 @@ fetch_dbs(struct jsonrpc *rpc, struct svec *dbs)
 svec_add(dbs, name->u.string);
 }
 jsonrpc_msg_destroy(reply);
+svec_sort(&dbs);
 }
 
 static void
@@ -404,7 +405,6 @@ do_list_dbs(struct jsonrpc *rpc, const char *database 
OVS_UNUSED,
 
 svec_init(&dbs);
 fetch_dbs(rpc, &dbs);
-svec_sort(&dbs);
 SVEC_FOR_EACH (i, db_name, &dbs) {
 puts(db_name);
 }
-- 
1.7.5.4

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

Re: [ovs-dev] [PATCH] Always check return value of strftime().

[Andy Zhou]

Looks good. Thanks for addressing it cleanly.


On Thu, May 2, 2013 at 4:16 PM, Ben Pfaff  wrote:

> strftime() returns 0 and leaves the contents of the output buffer
> unspecified if the output buffer is not big enough.  Thus, one should
> check strftime()'s return value.  Until now, OVS has had a few invocations
> of strftime() that did not check the return value.  This commit fixes
> those.  I believe that the buffers were always large enough in each case,
> but it's better to be safe.
>
> Reported-by: Andy Zhou 
> Signed-off-by: Ben Pfaff 
> ---
>  lib/dynamic-string.c  |   23 +--
>  lib/dynamic-string.h  |   12 +++-
>  lib/table.c   |   18 +++---
>  lib/vlog.c|   11 ---
>  ovsdb/ovsdb-tool.c|8 +++-
>  utilities/ovs-ofctl.c |8 ++--
>  6 files changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> index c373601..ba9aa6d 100644
> --- a/lib/dynamic-string.c
> +++ b/lib/dynamic-string.c
> @@ -183,17 +183,16 @@ ds_put_printable(struct ds *ds, const char *s,
> size_t n)
>  }
>  }
>
> -/* Writes the current time to 'string' based on 'template'.
> - * The current time is either local time or UTC based on 'utc'. */
> +/* Writes time 'when' to 'string' based on 'template', in local time or
> UTC
> + * based on 'utc'. */
>  void
> -ds_put_strftime(struct ds *ds, const char *template, bool utc)
> +ds_put_strftime(struct ds *ds, const char *template, time_t when, bool
> utc)
>  {
>  struct tm tm;
> -time_t now = time_wall();
>  if (utc) {
> -gmtime_r(&now, &tm);
> +gmtime_r(&when, &tm);
>  } else {
> -localtime_r(&now, &tm);
> +localtime_r(&when, &tm);
>  }
>
>  for (;;) {
> @@ -207,6 +206,18 @@ ds_put_strftime(struct ds *ds, const char *template,
> bool utc)
>  }
>  }
>
> +/* Returns a malloc()'d string for time 'when' based on 'template', in
> local
> + * time or UTC based on 'utc'. */
> +char *
> +xastrftime(const char *template, time_t when, bool utc)
> +{
> +struct ds s;
> +
> +ds_init(&s);
> +ds_put_strftime(&s, template, when, utc);
> +return s.string;
> +}
> +
>  int
>  ds_get_line(struct ds *ds, FILE *file)
>  {
> diff --git a/lib/dynamic-string.h b/lib/dynamic-string.h
> index 098caaf..b988e1f 100644
> --- a/lib/dynamic-string.h
> +++ b/lib/dynamic-string.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -22,10 +22,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "compiler.h"
>
> -struct tm;
> -
>  /* A "dynamic string", that is, a buffer that can be used to construct a
>   * string across a series of operations that extend or modify it.
>   *
> @@ -56,14 +55,17 @@ void ds_put_format(struct ds *, const char *, ...)
> PRINTF_FORMAT(2, 3);
>  void ds_put_format_valist(struct ds *, const char *, va_list)
>  PRINTF_FORMAT(2, 0);
>  void ds_put_printable(struct ds *, const char *, size_t);
> -void ds_put_strftime(struct ds *, const char *, bool utc)
> -STRFTIME_FORMAT(2);
>  void ds_put_hex_dump(struct ds *ds, const void *buf_, size_t size,
>   uintptr_t ofs, bool ascii);
>  int ds_get_line(struct ds *, FILE *);
>  int ds_get_preprocessed_line(struct ds *, FILE *);
>  int ds_get_test_line(struct ds *, FILE *);
>
> +void ds_put_strftime(struct ds *, const char *template, time_t when, bool
> utc)
> +STRFTIME_FORMAT(2);
> +char *xastrftime(const char *template, time_t when, bool utc)
> +STRFTIME_FORMAT(1);
> +
>  char *ds_cstr(struct ds *);
>  const char *ds_cstr_ro(const struct ds *);
>  char *ds_steal_cstr(struct ds *);
> diff --git a/lib/table.c b/lib/table.c
> index 266c410..21f4905 100644
> --- a/lib/table.c
> +++ b/lib/table.c
> @@ -218,22 +218,19 @@ table_print_table_line__(struct ds *line)
>  ds_clear(line);
>  }
>
> -static void
> -table_format_timestamp__(char *s, size_t size)
> +static char *
> +table_format_timestamp__(void)
>  {
> -time_t now = time_wall();
> -struct tm tm;
> -strftime(s, size, "%Y-%m-%d %H:%M:%S", gmtime_r(&now, &tm));
> +return xastrftime("%Y-%m-%d %H:%M:%S", time_wall(), true);
>  }
>
>  static void
>  table_print_timestamp__(const struct table *table)
>  {
>  if (table->timestamp) {
> -char s[32];
> -
> -table_format_timestamp__(s, sizeof s);
> +char *s = table_format_timestamp__();
>  puts(s);
> +free(s);
>  }
>  }
>
> @@ -500,10 +497,9 @@ table_print_json__(const struct table *table, const
> struct table_style *style)
>  json_object_put_string(json, "caption", table->caption);
>  }
>  if (table->timestamp) {
> -char s[32];
> -
> -table_format_timestamp__(s,

[ovs-dev] Check out my photos on Facebook

[Eagle Hostor]

Hi Eagle,

I set up a Facebook profile where I can post my pictures, videos and events and 
I want to add you as a friend so you can see it. First, you need to join 
Facebook! Once you join, you can also create your own profile.

--
hi can we be friends?thanks
--

Thanks,
Eagle

To sign up for Facebook, follow the link below:
http://www.facebook.com/p.php?i=15518058419&k=AQCPAmVSLNtWNXl2LjAWU0da1ZVTvwTZLMB4yHdqqm-QdKdAy2keTEtzTLfIiiO2epFwl48eizpYw0fHv-GTPaFyDuI&r

Already have an account? Add this email address to your account:
http://www.facebook.com/merge_accounts.php?e=dev%40openvswitch.org&c=AQDP8E4igcXhww8-zPv_zkl40ktYC4bmJCjABAc2UKdKYg&source=unknown

===
This message was sent to dev@openvswitch.org. If you don't want to receive 
these emails from Facebook in the future or have your email address used for 
friend suggestions, please follow the link below to unsubscribe.
http://www.facebook.com/o.php?k=AS1rhWdad304gGVq&e=dev%40openvswitch.org&mid=HMTMzMzEwNzk0OmRldkBvcGVudnN3aXRjaC5vcmc6OA
Facebook, Inc., Attention: Department 415, PO Box 10005, Palo Alto, CA 94303

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

[ovs-dev] [PATCH 1/2] ofproto-dpif: Avoid figuring out sFlow and IPFIX actions twice.

[Ben Pfaff]

Not only is it easier to re-use the actions we already have, this avoids
potential problems due to the state that add_sflow_action() and
add_ipfix_action() look at having possibly been changed by
do_xlate_actions().  Currently those functions appear to look only at
the flow's 'in_port', which currently can't change.  However, an upcoming
commit will make it possible for actions to change the flow's 'in_port',
and in addition, with this change, one doesn't have to wonder whether these
functions might look at other state that translation might change.

Signed-off-by: Ben Pfaff 
---
 ofproto/ofproto-dpif.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 89a4668..d7d21fa 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -7137,6 +7137,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
 } else {
 static struct vlog_rate_limit trace_rl = VLOG_RATE_LIMIT_INIT(1, 1);
 struct initial_vals initial_vals;
+size_t sample_actions_len;
 uint32_t local_odp_port;
 
 initial_vals.vlan_tci = ctx->base_flow.vlan_tci;
@@ -7144,6 +7145,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
 
 add_sflow_action(ctx);
 add_ipfix_action(ctx);
+sample_actions_len = ctx->odp_actions->size;
 
 if (tunnel_ecn_ok(ctx) && (!in_port || may_receive(in_port, ctx))) {
 do_xlate_actions(ofpacts, ofpacts_len, ctx);
@@ -7151,9 +7153,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
 /* We've let OFPP_NORMAL and the learning action look at the
  * packet, so drop it now if forwarding is disabled. */
 if (in_port && !stp_forward_in_state(in_port->stp_state)) {
-ofpbuf_clear(ctx->odp_actions);
-add_sflow_action(ctx);
-add_ipfix_action(ctx);
+ctx->odp_actions->size = sample_actions_len;
 }
 }
 
-- 
1.7.2.5

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

[ovs-dev] [PATCH 2/2] meta-flow: Make 'in_port' field writable.

[Ben Pfaff]

OpenFlow says that an "output" action to a flow's input port is ordinarily
dropped, unless the flow explicitly outputs to OFPP_IN_PORT.  We've
occasionally been asked to implement some way to avoid this behavior in
cases where it is not easily known in advance whether a given port is the
input port (so that OFPP_IN_PORT is not easy to use).

This commit implements such a feature.  With this commit, one may write:
actions=load:0->NXM_OF_IN_PORT[],output:123
which will output to port 123 regardless of whether it is the input port.
If the input port is important, then one may save and restore it on the
stack:
actions=push:NXM_OF_IN_PORT[],load:0->NXM_OF_IN_PORT[],output:123,
pop:NXM_OF_IN_PORT[]

(Sometimes I am asked whether "resubmit" changes the in_port and would
therefore interact badly with this feature.  It does not.   "resubmit" only
(optionally) changes the in_port used for the resubmit's flow table lookup.
It does not otherwise have any effect on in_port.)

Bug #14091.
CC: Jarno Rajahalme 
CC: Ronghua Zhang 
Signed-off-by: Ben Pfaff 
---
 NEWS  |4 
 include/openflow/nicira-ext.h |2 ++
 lib/meta-flow.c   |2 +-
 tests/ofproto-dpif.at |5 +++--
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 3a7123b..87f9bde 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,9 @@
 post-v1.11.0
 -
+- OpenFlow:
+  * The "load" and "set_field" actions can now modify the "in_port".  (This
+allows one to enable output to a flow's input port by setting the
+in_port to some unused value, such as OFPP_NONE.)
 
 
 v1.11.0 - xx xxx 
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index c80ff95..8c9fab1 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -493,6 +493,8 @@ OFP_ASSERT(sizeof(struct nx_action_pop_queue) == 16);
  * Modifying any of the above fields changes the corresponding packet
  * header.
  *
+ *   - NXM_OF_IN_PORT
+ *
  *   - NXM_NX_REG(idx) for idx in the switch's accepted range.
  *
  *   - NXM_OF_VLAN_TCI.  Modifying this field's value has side effects on the
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 9296faa..a75e526 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -114,7 +114,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = {
 MFM_NONE,
 MFS_OFP_PORT,
 MFP_NONE,
-false,
+true,
 NXM_OF_IN_PORT, "NXM_OF_IN_PORT",
 OXM_OF_IN_PORT, "OXM_OF_IN_PORT",
 }, {
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 2b9df96..1fdbac3 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -92,18 +92,19 @@ AT_SETUP([ofproto-dpif - output])
 OVS_VSWITCHD_START
 ADD_OF_PORTS([br0], [1], [9], [10], [11], [55], [66], [77], [88])
 AT_DATA([flows.txt], [dnl
-in_port=1 
actions=resubmit:2,resubmit:3,resubmit:4,resubmit:5,resubmit:6,resubmit:7
+in_port=1 
actions=resubmit:2,resubmit:3,resubmit:4,resubmit:5,resubmit:6,resubmit:7,resubmit:8
 in_port=2 actions=output:9
 in_port=3 
actions=load:55->NXM_NX_REG0[[]],output:NXM_NX_REG0[[]],load:66->NXM_NX_REG1[[]]
 in_port=4 
actions=output:10,output:NXM_NX_REG0[[]],output:NXM_NX_REG1[[]],output:11
 in_port=5 actions=load:77->NXM_NX_REG0[[0..15]],load:88->NXM_NX_REG0[[16..31]]
 in_port=6 actions=output:NXM_NX_REG0[[0..15]],output:NXM_NX_REG0[[16..31]]
 in_port=7 actions=load:0x11ff->NXM_NX_REG0[[]],output:NXM_NX_REG0[[]]
+in_port=8 actions=1,9,load:9->NXM_OF_IN_PORT[[]],1,9
 ])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 AT_CHECK([ovs-appctl ofproto/trace br0 
'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'],
 [0], [stdout])
 AT_CHECK([tail -1 stdout], [0],
-  [Datapath actions: 9,55,10,55,66,11,77,88
+  [Datapath actions: 9,55,10,55,66,11,77,88,9,1
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
-- 
1.7.2.5

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

[ovs-dev] [PATCH 1/3] ofp-actions: Switch away from odd use of "q" in "enqueue" action format.

[Ben Pfaff]

The formatting of the "enqueue" action uses a "q" to separate the port
number from the queue number, as in "enqueue:123q456".  This is different
from every other action.  This commit improves the situation by:

* Switching the formatting to use a colon (e.g. "enqueue:123:456"),
  which is a little less odd-looking but still accepted by older
  versions of Open vSwitch.

* Improving the parser to accept "enqueue(123,456)" also.

Signed-off-by: Ben Pfaff 
---
 lib/ofp-actions.c|2 +-
 lib/ofp-parse.c  |5 +++--
 tests/ofp-actions.at |2 +-
 utilities/ovs-ofctl.8.in |4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 068699f..8d429fc 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -2095,7 +2095,7 @@ ofpact_format(const struct ofpact *a, struct ds *s)
 enqueue = ofpact_get_ENQUEUE(a);
 ds_put_format(s, "enqueue:");
 ofputil_format_port(enqueue->port, s);
-ds_put_format(s, "q%"PRIu32, enqueue->queue);
+ds_put_format(s, ":%"PRIu32, enqueue->queue);
 break;
 
 case OFPACT_OUTPUT_REG:
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 295c038..6282c0b 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -126,12 +126,13 @@ static void
 parse_enqueue(char *arg, struct ofpbuf *ofpacts)
 {
 char *sp = NULL;
-char *port = strtok_r(arg, ":q", &sp);
+char *port = strtok_r(arg, ":q,", &sp);
 char *queue = strtok_r(NULL, "", &sp);
 struct ofpact_enqueue *enqueue;
 
 if (port == NULL || queue == NULL) {
-ovs_fatal(0, "\"enqueue\" syntax is \"enqueue:PORT:QUEUE\"");
+ovs_fatal(0, "\"enqueue\" syntax is \"enqueue:PORT:QUEUE\" or "
+  "\"enqueue(PORT,QUEUE)\"");
 }
 
 enqueue = ofpact_put_ENQUEUE(ofpacts);
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 2ecbdb5..53c9c12 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -39,7 +39,7 @@ AT_DATA([test-data], [dnl
 # actions=mod_tp_dst:443
 000a 0008 01bb 
 
-# actions=enqueue:10q55
+# actions=enqueue:10:55
 000b 0010 000a  0037
 
 # actions=resubmit:5
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index f46b9dc..4f60776 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -833,10 +833,10 @@ written in the upper half of register 0.  This form of 
\fBoutput\fR
 uses an OpenFlow extension that is not supported by standard OpenFlow
 switches.
 .
-.IP \fBenqueue:\fIport\fB:\fIqueue\fR
+.IP \fBenqueue(\fIport\fB,\fIqueue\fB)\fR
 Enqueues the packet on the specified \fIqueue\fR within port
 \fIport\fR, which must be an OpenFlow port number or keyword
-(e.g. \fBLOCAL\fR)..  The number of supported queues depends on the
+(e.g. \fBLOCAL\fR).  The number of supported queues depends on the
 switch; some OpenFlow implementations do not support queuing at all.
 .
 .IP \fBnormal\fR
-- 
1.7.2.5

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

[ovs-dev] [PATCH 2/3] ofp-parse: Accept port names in "output" and "enqueue" actions.

[Ben Pfaff]

The "output" and "enqueue" actions did not accept port names.  The
ovs-ofctl manpage claimed that "enqueue" did accept them.  This fixes
the problem.

As a side effect, this change begins validating numeric port numbers used
in "output" and "enqueue" actions.  We are trying to get users to use
keywords such as "local" instead of the corresponding numeric values for
OpenFlow 1.1+ compatibility.  As NEWS for 1.9.0 says:

- ovs-ofctl:
  - Commands and actions that accept port numbers now also accept keywords
that represent those ports (such as LOCAL, NONE, and ALL).  This is
also the recommended way to specify these ports, for compatibility
with OpenFlow 1.1 and later (which use the OpenFlow 1.0 numbers
for these ports for different purposes).

I had overlooked these actions in previous versions.

Signed-off-by: Ben Pfaff 
---
 lib/ofp-parse.c   |   10 +++---
 tests/ofproto-dpif.at |6 +++---
 tests/ovs-ofctl.at|4 ++--
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 6282c0b..5204c78 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -136,7 +136,9 @@ parse_enqueue(char *arg, struct ofpbuf *ofpacts)
 }
 
 enqueue = ofpact_put_ENQUEUE(ofpacts);
-enqueue->port = str_to_u32(port);
+if (!ofputil_port_from_string(port, &enqueue->port)) {
+ovs_fatal(0, "%s: invalid port", port);
+}
 enqueue->queue = str_to_u32(queue);
 }
 
@@ -153,7 +155,9 @@ parse_output(char *arg, struct ofpbuf *ofpacts)
 struct ofpact_output *output;
 
 output = ofpact_put_OUTPUT(ofpacts);
-output->port = str_to_u32(arg);
+if (!ofputil_port_from_string(arg, &output->port)) {
+ovs_fatal(0, "%s: invalid port", arg);
+}
 output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
 }
 }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1fdbac3..80f7975 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -166,7 +166,7 @@ AT_SETUP([ofproto-dpif - DSCP])
 OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
 ADD_OF_PORTS([br0], [9])
 AT_DATA([flows.txt], [dnl
-actions=output:65534,enqueue:1:1,enqueue:1:2,enqueue:1:2,enqueue:1:1,output:1,mod_nw_tos:0,output:1,output:65534
+actions=local,enqueue(1,1),enqueue:1:2,enqueue:1:2,enqueue:1:1,output:1,mod_nw_tos:0,output:1,local
 ])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 AT_CHECK([ovs-vsctl -- \
@@ -197,8 +197,8 @@ AT_DATA([flows.txt], [dnl
 in_port=local actions=local,flood
 in_port=1 actions=flood
 in_port=2 actions=all
-in_port=3 
actions=output:65534,output:1,output:2,output:3,output:4,output:5,output:6,output:7
-in_port=4 
actions=enqueue:65534:1,enqueue:1:1,enqueue:2:1,enqueue:3:2,enqueue:4:1,enqueue:5:1,enqueue:6:1,enqueue:7:1
+in_port=3 
actions=local,output:1,output:2,output:3,output:4,output:5,output:6,output:7
+in_port=4 
actions=enqueue:local:1,enqueue:1:1,enqueue:2:1,enqueue:3:2,enqueue:4:1,enqueue:5:1,enqueue:6:1,enqueue:7:1
 ])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 AT_CHECK([ovs-ofctl mod-port br0 5 noforward])
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index db19e01..2c99bb2 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -11,7 +11,7 @@ udp,nw_src=192.168.0.3,tp_dst=53 actions=pop_queue,output:1
 cookie=0x123456789abcdef hard_timeout=10 priority=6 actions=controller
 actions=note:41.42.43,note:00.01.02.03.04.05.06.07,note
 actions=set_field:fe80:0123:4567:890a:a6ba:dbff:fefe:59fa->ipv6_src
-in_port=0 actions=resubmit:0
+in_port=0 actions=resubmit:0,enqueue:local:5,enqueue:123:456
 
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
 ]])
 
@@ -28,7 +28,7 @@ OFPT_FLOW_MOD: ADD udp,nw_src=192.168.0.3,tp_dst=53 
actions=pop_queue,output:1
 OFPT_FLOW_MOD: ADD priority=6 cookie:0x123456789abcdef hard:10 
actions=CONTROLLER:65535
 OFPT_FLOW_MOD: ADD 
actions=note:41.42.43.00.00.00,note:00.01.02.03.04.05.06.07.00.00.00.00.00.00,note:00.00.00.00.00.00
 OFPT_FLOW_MOD: ADD 
actions=load:0xa6badbfffefe59fa->NXM_NX_IPV6_SRC[0..63],load:0xfe8001234567890a->NXM_NX_IPV6_SRC[64..127]
-OFPT_FLOW_MOD: ADD in_port=0 actions=resubmit:0
+OFPT_FLOW_MOD: ADD in_port=0 actions=resubmit:0,enqueue:LOCAL:5,enqueue:123:456
 OFPT_FLOW_MOD: ADD 
actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
 ]])
 AT_CLEANUP
-- 
1.7.2.5

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

[ovs-dev] [PATCH 3/3] ovs-ofctl: Improve manpage.

[Ben Pfaff]

Fixes misspelling of "OpenFlow".

Fixes the indentation of the paragraph beginning "Any \fIreason\fR...".

Changes "target" to "action" in the introduction of actions, which seems
like a better name.

Reorders action descriptions to group all the simple forms of output
together.

Consistently mentions that output actions don't output to the input port.

Signed-off-by: Ben Pfaff 
---
 utilities/ovs-ofctl.8.in |   52 -
 1 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 4f60776..37cc39d 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -813,31 +813,28 @@ command to be used as input for other commands that parse 
flows.
 The \fBadd\-flow\fR, \fBadd\-flows\fR, and \fBmod\-flows\fR commands
 require an additional field, which must be the final field specified:
 .
-.IP \fBactions=\fR[\fItarget\fR][\fB,\fItarget\fR...]\fR
+.IP \fBactions=\fR[\fIaction\fR][\fB,\fIaction\fR...]\fR
 Specifies a comma-separated list of actions to take on a packet when the 
-flow entry matches.  If no \fItarget\fR is specified, then packets
-matching the flow are dropped.  The \fItarget\fR may be an OpenFlow port 
-number designating the physical port on which to output the packet, or one 
-of the following keywords:
+flow entry matches.  If no \fIaction\fR is specified, then packets
+matching the flow are dropped.  The following forms of \fIaction\fR
+are supported:
 .
 .RS
-.IP \fBoutput:\fIport\fR
-Outputs the packet to \fIport\fR, which must be an OpenFlow port
-number or keyword (e.g. \fBLOCAL\fR).
+.IP \fIport\fR
+.IQ \fBoutput:\fIport\fR
+Outputs the packet to OpenFlow port number \fIport\fR.  If \fIport\fR
+is the packet's input port, the packet is not output.
 .
 .IP \fBoutput:\fIsrc\fB[\fIstart\fB..\fIend\fB]
 Outputs the packet to the OpenFlow port number read from \fIsrc\fR,
 which must be an NXM field as described above.  For example,
 \fBoutput:NXM_NX_REG0[16..31]\fR outputs to the OpenFlow port number
-written in the upper half of register 0.  This form of \fBoutput\fR
-uses an OpenFlow extension that is not supported by standard OpenFlow
-switches.
-.
-.IP \fBenqueue(\fIport\fB,\fIqueue\fB)\fR
-Enqueues the packet on the specified \fIqueue\fR within port
-\fIport\fR, which must be an OpenFlow port number or keyword
-(e.g. \fBLOCAL\fR).  The number of supported queues depends on the
-switch; some OpenFlow implementations do not support queuing at all.
+written in the upper half of register 0.  If the port number is the
+packet's input port, the packet is not output.
+.IP
+This form of \fBoutput\fR was added in Open vSwitch 1.3.0.  This form
+of \fBoutput\fR uses an OpenFlow extension that is not supported by
+standard OpenFlow switches.
 .
 .IP \fBnormal\fR
 Subjects the packet to the device's normal L2/L3 processing.  (This
@@ -853,6 +850,13 @@ tree protocol).
 Outputs the packet on all switch physical ports other than the port on
 which it was received.
 .
+.IP \fBlocal\fR
+Outputs the packet on the ``local port,'' which corresponds to the
+network device that has the same name as the bridge.
+.
+.IP \fBin_port\fR
+Outputs the packet on the port from which it was received.
+.
 .IP \fBcontroller(\fIkey\fB=\fIvalue\fR...\fB)
 Sends the packet to the OpenFlow controller as a ``packet in''
 message.  The supported key-value pairs are:
@@ -873,6 +877,7 @@ controller connection will only have a nonzero connection 
ID if its
 controller uses the \fBNXT_SET_CONTROLLER_ID\fR Nicira extension to
 OpenFlow.
 .RE
+.IP
 Any \fIreason\fR other than \fBaction\fR and any nonzero
 \fIcontroller-id\fR uses a Nicira vendor extension that, as of this
 writing, is only known to be implemented by Open vSwitch (version 1.6
@@ -883,12 +888,11 @@ or later).
 Shorthand for \fBcontroller()\fR or
 \fBcontroller(max_len=\fInbytes\fB)\fR, respectively.
 .
-.IP \fBlocal\fR
-Outputs the packet on the ``local port,'' which corresponds to the
-network device that has the same name as the bridge.
-.
-.IP \fBin_port\fR
-Outputs the packet on the port from which it was received.
+.IP \fBenqueue(\fIport\fB,\fIqueue\fB)\fR
+Enqueues the packet on the specified \fIqueue\fR within port
+\fIport\fR, which must be an OpenFlow port number or keyword
+(e.g. \fBLOCAL\fR).  The number of supported queues depends on the
+switch; some OpenFlow implementations do not support queuing at all.
 .
 .IP \fBdrop\fR
 Discards the packet, so no further processing or forwarding takes place.
@@ -1084,7 +1088,7 @@ value just popped.
 Writes the literal \fIvalue\fR into the field \fIdst\fR, which should
 be specified as a name used for matching.  (This is similar to
 \fBload\fR but more closely matches the set-field action defined in
-Open Flow 1.2 and above.)
+OpenFlow 1.2 and above.)
 .
 .IP
 Example: \fBset_field:fe80:0123:4567:890a:a6ba:dbff:fefe:59fa\->ipv6_src\fR
-- 
1.7.2.5

___
dev mailing li

Re: [ovs-dev] [PATCH] Allow master to build on Fedora with the recent threading changes

[Ben Pfaff]

On Mon, May 06, 2013 at 02:37:37PM -0400, Ed Maste wrote:
> On 6 May 2013 13:47, Kyle Mestery  wrote:
> > The recent threading changes have broken the build on Fedora,
> > and presumably other Red Hat based distributions. This adds an explicit
> > "-lpthread" to the linker command line and allows the latest master to build
> > on Fedora. I've also tested this on Ubuntu and it builds fine there.
> >
> > Signed-off-by: Kyle Mestery 
> 
> Acked-by: Ed Maste 
> 
> Builds fine for me on FreeBSD with this change.

Applied to master with your ack.  Thanks, Kyle and Ed.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/2] ofproto-dpif: Avoid figuring out sFlow and IPFIX actions twice.

[Romain Lenglet]

Looks good to me.
--
Romain Lenglet

- Original Message -
> From: "Ben Pfaff" 
> To: dev@openvswitch.org
> Cc: "Ben Pfaff" 
> Sent: Monday, May 6, 2013 3:38:58 PM
> Subject: [ovs-dev] [PATCH 1/2] ofproto-dpif: Avoid figuring out sFlow and 
> IPFIX actions twice.
> 
> Not only is it easier to re-use the actions we already have, this avoids
> potential problems due to the state that add_sflow_action() and
> add_ipfix_action() look at having possibly been changed by
> do_xlate_actions().  Currently those functions appear to look only at
> the flow's 'in_port', which currently can't change.  However, an upcoming
> commit will make it possible for actions to change the flow's 'in_port',
> and in addition, with this change, one doesn't have to wonder whether these
> functions might look at other state that translation might change.
> 
> Signed-off-by: Ben Pfaff 
> ---
>  ofproto/ofproto-dpif.c |6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 89a4668..d7d21fa 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -7137,6 +7137,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
>  } else {
>  static struct vlog_rate_limit trace_rl = VLOG_RATE_LIMIT_INIT(1, 1);
>  struct initial_vals initial_vals;
> +size_t sample_actions_len;
>  uint32_t local_odp_port;
>  
>  initial_vals.vlan_tci = ctx->base_flow.vlan_tci;
> @@ -7144,6 +7145,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
>  
>  add_sflow_action(ctx);
>  add_ipfix_action(ctx);
> +sample_actions_len = ctx->odp_actions->size;
>  
>  if (tunnel_ecn_ok(ctx) && (!in_port || may_receive(in_port, ctx))) {
>  do_xlate_actions(ofpacts, ofpacts_len, ctx);
> @@ -7151,9 +7153,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
>  /* We've let OFPP_NORMAL and the learning action look at the
>   * packet, so drop it now if forwarding is disabled. */
>  if (in_port && !stp_forward_in_state(in_port->stp_state)) {
> -ofpbuf_clear(ctx->odp_actions);
> -add_sflow_action(ctx);
> -add_ipfix_action(ctx);
> +ctx->odp_actions->size = sample_actions_len;
>  }
>  }
>  
> --
> 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 1/4] ofproto-dpif.c: Re-implement the ofproto/trace command

[Ben Pfaff]

On Mon, May 06, 2013 at 10:11:14AM -0700, Alex Wang  wrote:
> From: Alex Wang 
> 
> Since the use of single datapath, all bridges belonging to the same type of
> datapath will use the same (single) datapath. This causes confusion in the
> current 'ofproto/trace' command. Especially, when given the unrelated
> 'bridge' and 'in_port' combination, the current implementation will still
> be able to process and give misleading output. Thusly, this patch changes
> the 'ofproto/trace' command syntax to formats shown as follow.
> 
> ofproto/trace [datapath] priority tun_id in_port mark packet
> ofproto/trace [datapath] dp_flow [-generate]
> ofproto/trace bridge br_flow [-generate]
> 
> Therein, the bridge name is replaced by datapath name in the first format,
> since the mapping between in_port and the corresponding datapath is unique.
> The second format requires datapath name since it uses 'ovs-dpctl dump-flows'
> output. And the thrid format requires bridge name since it uses 'ovs-ofctl
> add-flow' input like flow.
> 
> Signed-off-by: Alex Wang 

Hi Alex.  Before I start reviewing the patches themselves, it looks to
me like patch 1 breaks the tests, then patch 2 fixes them, then patch
4 updates the documentation to match the change from patch 1.  If I'm
understanding that correctly, then patches 1, 2, and 4 should all be a
single patch: we don't want test failures after any single patch is
applied, and similarly we don't want incorrect or incomplete
documentation after any single patch is applied.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb-client: Avoid assertion with multiple databases.

[Ben Pfaff]

On Mon, May 06, 2013 at 12:50:52PM -0700, Justin Pettit wrote:
> When using ovsdb-client with an ovsdb-server with multiple databases, an
> assertion could trigger due to them being returned in non-sorted order.
> This commit changes the fetch_dbs() function to always return databases
> in sorted order, since both callers are expecting that behavior.
> 
> Signed-off-by: Justin Pettit 
> Reported-by: Spiro Kourtessis 

Looks good, thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [bfd] bfd: Implement Bidirectional Forwarding Detection.

[Ben Pfaff]

On Mon, May 06, 2013 at 12:10:29PM -0700, Ethan Jackson wrote:
> Since it's been so long, I'll go ahead and resend the latest version
> of the patch.  I think it's pretty much reviewed and ready to go, so
> don't feel obligated to comb over it as nothings changed.  Feel free
> to make additional comments if you'd like of course.

Sounds fine, I'll do a quick skim.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] tests: Add tests for the ofproto/trace command

[Alex Wang]

From: Alex Wang 

Three testcases are added to the testsuite, which test the three command
formats and the corresponding corner cases.

Signed-off-by: Alex Wang 
---
 tests/ofproto-dpif.at |  209 +
 1 file changed, 209 insertions(+)

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 8cbbd80..3406b01 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1043,6 +1043,215 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow" 
"$actual"], [0], [expout])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+# Three testcases for the ofproto/trace command
+# The first one tests the normal input and corner cases
+# of the "ofproto/trace [datapath_name] datapath_flow"
+# and the "ofproto/trace bridge_name openflow_flow"
+AT_SETUP([ofproto-dpif - ofproto/trace command 1])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2)
+
+AT_DATA([flows.txt], [dnl
+in_port=1 actions=output:2
+in_port=2 actions=output:1
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+dp_flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
+of_flow="in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0"
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test command: ofproto/trace dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test incorrect command: ofproto/trace wrong_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace wrong_name "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Unknown bridge or datapath name
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test incorrect command: ofproto/trace nonexist_dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-system "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Cannot find datapath of this name
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test incorrect command: ofproto/trace br_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Cannot parse the openflow flow
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Add another bridge
+AT_CHECK([ovs-vsctl add-br br1 -- set bridge br1 datapath-type=netdev \
+  -- set bridge br1 fail-mode=secure])
+
+# Test incorrect command: ofproto/trace dp_flow, since there is more than one 
dp
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Must specify datapath name, there is more than one type of datapath
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test command: ofproto/trace dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Delete the added bridge
+AT_CHECK([ovs-vsctl del-br br1])
+
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test commmand: ofproto/trace br_name of_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$of_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test incorrect command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$of_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Bad datapath flow syntax
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+OVS_VSWITCHD_STOP(["/couldn't open old datapath ovs-dummy to remove it/d; 
/obtaining netdev stats via vport failed/d"])
+AT_CLEANUP
+
+# The second test tests the -generate option
+AT_SETUP([ofproto-dpif - ofproto/trace command 2])
+OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone])
+ADD_OF_PORTS([br0], 1, 2, 3)
+
+dp_flow='in_port(3),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff)'
+of_flow='arp,metadata=0,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=ff:ff:ff:ff:ff:ff'
+
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+
+# Check for no MAC learning entry.
+AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], 
[dnl
+ port  VLAN  MACAge
+])
+
+# Test command: ofproto/trace br_name of_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$of_flow"], [0], [stdout])
+
+# Check for no MAC learning entry.
+AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], 
[dnl
+ port  VLAN  MACAge
+])
+
+# Test command: ofproto/trace dp_flow -generate
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow" -generate], [

[ovs-dev] [PATCH 2/2] tests: Add tests for the ofproto/trace command

[Alex Wang]

From: Alex Wang 

Three testcases are added to the testsuite, which test the three command
formats and the corresponding corner cases.

Signed-off-by: Alex Wang 
---
 tests/ofproto-dpif.at |  209 +
 1 file changed, 209 insertions(+)

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 8cbbd80..3406b01 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1043,6 +1043,215 @@ AT_CHECK([ovs-dpctl normalize-actions "$flow" 
"$actual"], [0], [expout])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+# Three testcases for the ofproto/trace command
+# The first one tests the normal input and corner cases
+# of the "ofproto/trace [datapath_name] datapath_flow"
+# and the "ofproto/trace bridge_name openflow_flow"
+AT_SETUP([ofproto-dpif - ofproto/trace command 1])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], 1, 2)
+
+AT_DATA([flows.txt], [dnl
+in_port=1 actions=output:2
+in_port=2 actions=output:1
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+dp_flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
+of_flow="in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0"
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test command: ofproto/trace dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test incorrect command: ofproto/trace wrong_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace wrong_name "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Unknown bridge or datapath name
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test incorrect command: ofproto/trace nonexist_dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-system "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Cannot find datapath of this name
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test incorrect command: ofproto/trace br_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$dp_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Cannot parse the openflow flow
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Add another bridge
+AT_CHECK([ovs-vsctl add-br br1 -- set bridge br1 datapath-type=netdev \
+  -- set bridge br1 fail-mode=secure])
+
+# Test incorrect command: ofproto/trace dp_flow, since there is more than one 
dp
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Must specify datapath name, there is more than one type of datapath
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+# Test command: ofproto/trace dp_name dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Delete the added bridge
+AT_CHECK([ovs-vsctl del-br br1])
+
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test commmand: ofproto/trace br_name of_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$of_flow"], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [dnl
+Datapath actions: 2
+])
+
+# Test incorrect command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$of_flow"],
+  [2], [], [stderr])
+AT_CHECK([tail -2 stderr], [0], [dnl
+Bad datapath flow syntax
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+OVS_VSWITCHD_STOP(["/couldn't open old datapath ovs-dummy to remove it/d; 
/obtaining netdev stats via vport failed/d"])
+AT_CLEANUP
+
+# The second test tests the -generate option
+AT_SETUP([ofproto-dpif - ofproto/trace command 2])
+OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone])
+ADD_OF_PORTS([br0], 1, 2, 3)
+
+dp_flow='in_port(3),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff)'
+of_flow='arp,metadata=0,in_port=3,vlan_tci=0x,dl_src=50:54:00:00:00:05,dl_dst=ff:ff:ff:ff:ff:ff'
+
+# Test command: ofproto/trace dp_flow
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow"], [0], [stdout])
+
+# Check for no MAC learning entry.
+AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], 
[dnl
+ port  VLAN  MACAge
+])
+
+# Test command: ofproto/trace br_name of_flow
+AT_CHECK([ovs-appctl ofproto/trace br0 "$of_flow"], [0], [stdout])
+
+# Check for no MAC learning entry.
+AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], 
[dnl
+ port  VLAN  MACAge
+])
+
+# Test command: ofproto/trace dp_flow -generate
+AT_CHECK([ovs-appctl ofproto/trace "$dp_flow" -generate], [

Re: [ovs-dev] [PATCH] ovsdb-client: Avoid assertion with multiple databases.

[Justin Pettit]

On May 6, 2013, at 3:43 PM, Ben Pfaff  wrote:

> On Mon, May 06, 2013 at 12:50:52PM -0700, Justin Pettit wrote:
>> When using ovsdb-client with an ovsdb-server with multiple databases, an
>> assertion could trigger due to them being returned in non-sorted order.
>> This commit changes the fetch_dbs() function to always return databases
>> in sorted order, since both callers are expecting that behavior.
>> 
>> Signed-off-by: Justin Pettit 
>> Reported-by: Spiro Kourtessis 
> 
> Looks good, thanks.

Thanks.  I pushed this to master, branch-1.9, branch-1.10, and branch-1.11.

--Justin


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

[ovs-dev] [PATCH] ovsdb-client: Fix recently introduced svec_sort() bug.

[Justin Pettit]

Commit 66980be9 (ovsdb-client: Avoid assertion with multiple databases.)
passed in a pointer to an svec pointer, when it should have just been an
svec pointer.  This corrects the bug.

Signed-off-by: Justin Pettit 
---
 ovsdb/ovsdb-client.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 81dcc77..c25148d 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -392,7 +392,7 @@ fetch_dbs(struct jsonrpc *rpc, struct svec *dbs)
 svec_add(dbs, name->u.string);
 }
 jsonrpc_msg_destroy(reply);
-svec_sort(&dbs);
+svec_sort(dbs);
 }
 
 static void
-- 
1.7.5.4

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

Re: [ovs-dev] [PATCH] ovsdb-client: Fix recently introduced svec_sort() bug.

[Ben Pfaff]

On Mon, May 06, 2013 at 09:33:26PM -0700, Justin Pettit wrote:
> Commit 66980be9 (ovsdb-client: Avoid assertion with multiple databases.)
> passed in a pointer to an svec pointer, when it should have just been an
> svec pointer.  This corrects the bug.
> 
> Signed-off-by: Justin Pettit 

Thanks, "make check" passes for me with this change.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovsdb-client: Fix recently introduced svec_sort() bug.

[Justin Pettit]

On May 6, 2013, at 9:42 PM, Ben Pfaff  wrote:

> On Mon, May 06, 2013 at 09:33:26PM -0700, Justin Pettit wrote:
>> Commit 66980be9 (ovsdb-client: Avoid assertion with multiple databases.)
>> passed in a pointer to an svec pointer, when it should have just been an
>> svec pointer.  This corrects the bug.
>> 
>> Signed-off-by: Justin Pettit 
> 
> Thanks, "make check" passes for me with this change.

Thanks for the quick review.  I pushed it to all affected branches.

--Justin


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