[ovs-dev] Brfendshyfoedv

[Mail Delivery Subsystem]

This message was not delivered due to the following reason:

Your message was not delivered because the destination server was
not reachable within the allowed queue period. The amount of time
a message is queued before it is returned depends on local configura-
tion parameters.

Most likely there is a network problem that prevented delivery, but
it is also possible that the computer is turned off, or does not
have a mail system running right now.

Your message was not delivered within 2 days:
Host 4.102.239.252 is not responding.

The following recipients could not receive this message:


Please reply to postmas...@openvswitch.org
if you feel this message to be in error.

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

[ovs-dev] Unable to hit group table entry

[Ning Wu]

Hi All,

I want to add some flow rules with actions as a group entry. The group and
flow rule have been installed successfully. Outputs below are obtained by
the ovs-ofctl command dump-flows, dump-groups and dump-group-stats.
According to the stats for the flow, the flow rule was hit, but the group
stats showed packet_count=0, and the actions for the group was not executed.

The ovs-vswtichd log did not give me any error though. I am wondering what
would be the potential reasons for this issue.

-- flow entry
cookie=0x2d8226e53b, duration=57.152s, table=2, n_packets=38,
n_bytes=3724, send_flow_rem priority=100,ip,nw_dst=10.0.2.0/24
actions=group:5

-- group entry
group_id=5,type=select,bucket=actions=dec_mpls_ttl,set_field:00:00:00:00:01:80->eth_src,set_field:00:00:03:00:33:80->eth_dst,output:3

-- group stats
group_id=5,duration=60.242s,ref_count=1,packet_count=0,byte_count=0,bucket0:packet_count=0,byte_count=0

Thanks in advance!

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

[ovs-dev] Meter implementation in openvswitch

[deepanshu . saxena1]

Hi Ben, 

I want to implement and contribute B.19.13-Meter action (EXT-379) of OF 1.5.1 
to openvswitch. 

As per my understanding, meter support in openvswitch datapath is not present 
as stated in the FAQs. Some patches[1] for meter implementation have been 
submitted but not applied to ovs master branch. 

Is there any specific reason why meters have not been implemented in datapath ? 

[1] http://openvswitch.org/pipermail/dev/2015-November/062428.html 

-- 
Deepanshu Saxena 
Assistant System Engineer, 
Tata Consultancy Services 
Mailto: deepanshu.saxe...@tcs.com 
Website: http://www.tcs.com 
=-=-=
Notice: The information contained in this e-mail
message and/or attachments to it may contain 
confidential or privileged information. If you are 
not the intended recipient, any dissemination, use, 
review, distribution, printing or copying of the 
information contained in this e-mail message 
and/or attachments to it are strictly prohibited. If 
you have received this communication in error, 
please notify us by reply e-mail or telephone and 
immediately and permanently delete the message 
and any attachments. Thank you


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

Re: [ovs-dev] [PATCH] system-traffic: Fix packet-in format for tests.

[Daniele Di Proietto]

Thanks for fixing this!

Acked-by: Daniele Di Proietto 

On 06/04/2016 15:07, "Joe Stringer"  wrote:

>Since continuations were introduced, the system-traffic tests which use
>OpenFlow monitors to check the results of datapath execution have been
>failing, because the new PACKET_IN2 format is used rather than
>PACKET_IN. Switch the expected output over to PACKET_IN2.
>
>Signed-off-by: Joe Stringer 
>---
> tests/system-traffic.at | 8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>index 28adbdcb9ee6..58212c1ed014 100644
>--- a/tests/system-traffic.at
>+++ b/tests/system-traffic.at
>@@ -181,9 +181,9 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2
>ct\(table=0\) '5054000a50
> 
> dnl Check this output. We only see the latter two packets, not the first.
> AT_CHECK([cat ofctl_monitor.log], [0], [dnl
>-NXT_PACKET_IN (xid=0x0): total_len=42 in_port=1 (via action) data_len=42
>(unbuffered)
>+NXT_PACKET_IN2 (xid=0x0): total_len=42 in_port=1 (via action)
>data_len=42 (unbuffered)
> 
>udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_s
>rc=10.1.1.1,nw_dst=10.1.1.2,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=1,tp_dst=2
>udp_csum:0
>-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=42
>ct_state=est|rpl|trk,in_port=2 (via action) data_len=42 (unbuffered)
>+NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=42
>ct_state=est|rpl|trk,in_port=2 (via action) data_len=42 (unbuffered)
> 
>udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_s
>rc=10.1.1.2,nw_dst=10.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=2,tp_dst=1
>udp_csum:0
> ])
> 
>@@ -925,9 +925,9 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2
>ct\(table=0\) 'e64c473528c9c6
> 
> dnl Check this output. We only see the latter two packets, not the first.
> AT_CHECK([cat ofctl_monitor.log], [0], [dnl
>-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=47
>ct_state=new|trk,in_port=1 (via action) data_len=47 (unbuffered)
>+NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=47
>ct_state=new|trk,in_port=1 (via action) data_len=47 (unbuffered)
> 
>udp,vlan_tci=0x,dl_src=e6:4c:47:35:28:c9,dl_dst=c6:f9:4e:cb:72:db,nw_s
>rc=172.16.0.1,nw_dst=172.16.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=41614,t
>p_dst= udp_csum:2096
>-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=75
>ct_state=rel|rpl|trk,in_port=2 (via action) data_len=75 (unbuffered)
>+NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=75
>ct_state=rel|rpl|trk,in_port=2 (via action) data_len=75 (unbuffered)
> 
>icmp,vlan_tci=0x,dl_src=c6:f9:4e:cb:72:db,dl_dst=e6:4c:47:35:28:c9,nw_
>src=172.16.0.2,nw_dst=172.16.0.1,nw_tos=192,nw_ecn=0,nw_ttl=64,icmp_type=3
>,icmp_code=3 icmp_csum:553f
> ])
> 
>-- 
>2.1.4
>

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

Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router

[Guru Shetty]

>
>
>
> Steve My understanding of last point,
> 1. How to static routes?
> format: "10.0.0.0/24 nexthop 10.1.1.1" or "10.0.0.0/24 nexthop 10.1.1.1
> dev port1"
> For the latter format, it need the networking-ovn to lookup logical router
> port table.
> It will do what this patch did in ovn-northd.
>
> 2. Routing recursion
> For example, "10.0.0.0/24 nexthop 10.1.1.1"
> "10.0.1.0/24 nexthop 10.0.0.1"
> 10.1.1.0/24 is route port subnet,
> The second route need do recursion lookup.
>

Can you please elaborate with a logical topology that has 2 routers (or 3).
I don't think I understand what you guys are referring to.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2] dp-packet: Fix use of uninitialised value at emc_lookup.

[Daniele Di Proietto]

Thanks for the fix!

I've applied this to master and branch-2.5

2016-04-06 16:28 GMT-07:00 William Tu :
> Valgrind reports "Conditional jump or move depends on uninitialised value"
> and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3
> lports/HV.  It is caused by 1) assigning an uninitialized value to 'key->hash'
> at emc_processing(). Due to uninit rss_hash_valid, dp_packet_rss_valid() might
> return true and undefined hash value is returned, and 2) at emc_lookup, the
> 'current_entry->key.hash' could be uninitialized due to dp_packet_clone().
> The patch fixes the two and as a result, a couple of calls to
> dp_packet_rss_valid() become redundant and thus are removed.
>
> Call stacks:
> - Connditional jump or move depends on uninitialised value(s)
> dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334)
> emc_processing (dpif-netdev.c:3455)
> dp_netdev_input__ (dpif-netdev.c:3639)
> and,
> - Use of uninitialised value of size 8
> emc_lookup (dpif-netdev.c:1785)
> emc_processing (dpif-netdev.c:3457)
> dp_netdev_input__ (dpif-netdev.c:3639)
>
> Signed-off-by: William Tu 
> ---
> v1->v2
> - use dp_packet_rss_invalidate() instead of direct assignment
> - fix dp_packet_clone_with_headroom()
> - remove redundant dp_packet_rss_invalidate()
> ---
>  lib/dp-packet.c| 16 +++-
>  lib/netdev-bsd.c   |  1 -
>  lib/netdev-dummy.c |  1 -
>  lib/netdev-linux.c |  1 -
>  4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index aec7fe7..0c85d50 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -29,6 +29,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, 
> enum dp_packet_source so
>  b->source = source;
>  dp_packet_reset_offsets(b);
>  pkt_metadata_init(>md, 0);
> +dp_packet_rss_invalidate(b);
>  }
>
>  static void
> @@ -167,6 +168,19 @@ dp_packet_clone_with_headroom(const struct dp_packet 
> *buffer, size_t headroom)
>  new_buffer->l3_ofs = buffer->l3_ofs;
>  new_buffer->l4_ofs = buffer->l4_ofs;
>  new_buffer->md = buffer->md;
> +#ifdef DPDK_NETDEV
> +new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> +#else
> +new_buffer->rss_hash_valid = buffer->rss_hash_valid;
> +#endif
> +
> +if (dp_packet_rss_valid(new_buffer)) {
> +#ifdef DPDK_NETDEV
> +new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
> +#else
> +new_buffer->rss_hash = buffer->rss_hash;
> +#endif
> +}
>
>  return new_buffer;
>  }
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 75bd5a3..49c05f4 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -641,7 +641,6 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct 
> dp_packet **packets,
>  dp_packet_delete(packet);
>  } else {
>  dp_packet_pad(packet);
> -dp_packet_rss_invalidate(packet);
>  packets[0] = packet;
>  *c = 1;
>  }
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index edc86fa..a1013ff 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -905,7 +905,6 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct 
> dp_packet **arr,
>  ovs_mutex_unlock(>mutex);
>
>  dp_packet_pad(packet);
> -dp_packet_rss_invalidate(packet);
>
>  arr[0] = packet;
>  *c = 1;
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 3f5b608..a7d7ac7 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1116,7 +1116,6 @@ netdev_linux_rxq_recv(struct netdev_rxq *rxq_, struct 
> dp_packet **packets,
>  dp_packet_delete(buffer);
>  } else {
>  dp_packet_pad(buffer);
> -dp_packet_rss_invalidate(buffer);
>  packets[0] = buffer;
>  *c = 1;
>  }
> --
> 2.5.0
>
> ___
> 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/1] Add Static route to logical router

[Darrell Ball]

On Wed, Apr 6, 2016 at 5:58 PM, Guru Shetty  wrote:

> On 6 April 2016 at 16:55, Mickey Spiegel  wrote:
>
> > Steve and Guru,
> >
> > I am not all that concerned about the "valid" column, but I do think that
> > we will need a different additional column in the near future for output
> > port.
> >
> > There are three different motivations for allowing output port to be
> > specified in the static route:
> > 1) In order to support static routes with a link local next hop. If a
> link
> > local next hop is used, it is possible that the same link local address
> > appears on different router ports with different meanings. By specifying
> > the port, this disambiguates the specific link local next hop that was
> > desired.
> > Note: Neutron does not yet support static routes with link local next
> hop.
> > We need to drive the feature in Neutron as well, optionally allowing a
> > router interface to be specified in addition to the next hop.
> > 2) This feature should not really be specific to static routes, it should
> > also apply to dynamic routes when we add that in the future. Basically
> > anything that looks up an IP address prefix and returns a next hop and
> > optionally an output port. There are cases where explicitly specifying
> the
> > output port makes sense.
> >
> For point 1 and 2, I am not sure whether we should do anything till there
> is code in ovn-northd that actually uses it.
>
>
>
> > 3) In order to optimize processing of the routing recursion (Steve's code
> > loops over the router's ports in ovn-northd.c to carry out this routing
> > recursion), we might want to do it above OVN in an event triggered
> manner,
> > rather than every time ovn-northd.c recalculates the flows that it places
> > into the southbound database.
> >
> I don't think I understand the above point. The static_route I have in mind
> need not recursively look through routers. All they need is to see whether
> the router peer has the next hop IP address and the packet is just sent to
> that router. From there on it is a fresh start.
>

The last point that Mickey mentioned is relevant even in this simplest form
of
routing.

Specifying both outport (eg logical router port) and next hop has
significant
advantages in processing time even for tiny route scales of a few 1000s
routes.
This is because finding the outport for each single non-recursive route is
O(1)
with smallest possible constant.



>
>
>
> >
> > For these reasons, I prefer to keep a separate table for static routes.
> >
> > Mickey
> >
> >
> > -"dev"  wrote: -
> > To: Shi Xin Ruan 
> > From: Guru Shetty
> > Sent by: "dev"
> > Date: 04/04/2016 08:41AM
> > Cc: ovs dev 
> > Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router
> >
> > On 31 March 2016 at 19:11, Shi Xin Ruan  wrote:
> >
> > > Thanks Guru.
> > >
> > > The column "valid" will indicate whether the routes has been transfer
> > into
> > > logical flow.
> > > Thinking about this case, deleting the logical router port which is out
> > > going interface of some static routes.
> > > The first possible way is preventing the deleting, the second way is
> > > removing the stactic routing from logical flow.
> > >
> > [Trying this again as my previous reply got rejected by the mailing list]
> > I feel this is an unnecessary complication (unless it becomes a real use
> > case). Let us start with not adding a new table and try to do this with a
> > column. If you are fine with it, would you mind re-spinning a non-RFC
> patch
> > with a unit test? If you want, I can provide the unit tests. If you
> prefer
> > that I do the entire thing, I am happy with that too.
> >
> >
> >
> > >
> > >
> > > From my points, both can work fine and has their advantages.
> > >
> > >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
> ___
> 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/1] Add Static route to logical router

[Guru Shetty]

On 6 April 2016 at 16:55, Mickey Spiegel  wrote:

> Steve and Guru,
>
> I am not all that concerned about the "valid" column, but I do think that
> we will need a different additional column in the near future for output
> port.
>
> There are three different motivations for allowing output port to be
> specified in the static route:
> 1) In order to support static routes with a link local next hop. If a link
> local next hop is used, it is possible that the same link local address
> appears on different router ports with different meanings. By specifying
> the port, this disambiguates the specific link local next hop that was
> desired.
> Note: Neutron does not yet support static routes with link local next hop.
> We need to drive the feature in Neutron as well, optionally allowing a
> router interface to be specified in addition to the next hop.
> 2) This feature should not really be specific to static routes, it should
> also apply to dynamic routes when we add that in the future. Basically
> anything that looks up an IP address prefix and returns a next hop and
> optionally an output port. There are cases where explicitly specifying the
> output port makes sense.
>
For point 1 and 2, I am not sure whether we should do anything till there
is code in ovn-northd that actually uses it.



> 3) In order to optimize processing of the routing recursion (Steve's code
> loops over the router's ports in ovn-northd.c to carry out this routing
> recursion), we might want to do it above OVN in an event triggered manner,
> rather than every time ovn-northd.c recalculates the flows that it places
> into the southbound database.
>
I don't think I understand the above point. The static_route I have in mind
need not recursively look through routers. All they need is to see whether
the router peer has the next hop IP address and the packet is just sent to
that router. From there on it is a fresh start.



>
> For these reasons, I prefer to keep a separate table for static routes.
>
> Mickey
>
>
> -"dev"  wrote: -
> To: Shi Xin Ruan 
> From: Guru Shetty
> Sent by: "dev"
> Date: 04/04/2016 08:41AM
> Cc: ovs dev 
> Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router
>
> On 31 March 2016 at 19:11, Shi Xin Ruan  wrote:
>
> > Thanks Guru.
> >
> > The column "valid" will indicate whether the routes has been transfer
> into
> > logical flow.
> > Thinking about this case, deleting the logical router port which is out
> > going interface of some static routes.
> > The first possible way is preventing the deleting, the second way is
> > removing the stactic routing from logical flow.
> >
> [Trying this again as my previous reply got rejected by the mailing list]
> I feel this is an unnecessary complication (unless it becomes a real use
> case). Let us start with not adding a new table and try to do this with a
> column. If you are fine with it, would you mind re-spinning a non-RFC patch
> with a unit test? If you want, I can provide the unit tests. If you prefer
> that I do the entire thing, I am happy with that too.
>
>
>
> >
> >
> > From my points, both can work fine and has their advantages.
> >
> >
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-docker: Update overlay mode for new tcp ports.

[Gurucharan Shetty]

There were changes made recently wherein 2 ovsdb-server is
started for northbound and southbound databases with tcp ports
6641 and 6642. This breaks Docker integration. This commit
fixes it.

Signed-off-by: Gurucharan Shetty 
---
 INSTALL.Docker.md   |   10 ++
 ovn/utilities/ovn-docker-overlay-driver |   12 ++--
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/INSTALL.Docker.md b/INSTALL.Docker.md
index 85d9122..eb27756 100644
--- a/INSTALL.Docker.md
+++ b/INSTALL.Docker.md
@@ -56,12 +56,6 @@ in a database.  On one of your machines, with an IP Address 
of $CENTRAL_IP,
 where you have installed and started Open vSwitch, you will need to start some
 central components.
 
-Begin by making ovsdb-server listen on a TCP port by running:
-
-```
-ovs-appctl -t ovsdb-server ovsdb-server/add-remote ptcp:6640
-```
-
 Start ovn-northd daemon.  This daemon translates networking intent from Docker
 stored in the OVN_Northbound database to logical flows in OVN_Southbound
 database.
@@ -89,8 +83,8 @@ support in upstream Linux.  You can verify whether you have 
the support in your
 kernel by doing a "lsmod | grep $ENCAP_TYPE".)
 
 ```
-ovs-vsctl set Open_vSwitch . external_ids:ovn-remote="tcp:$CENTRAL_IP:6640" \
-  external_ids:ovn-encap-ip=$LOCAL_IP external_ids:ovn-encap-type="$ENCAP_TYPE"
+ovs-vsctl set Open_vSwitch . external_ids:ovn-remote="tcp:$CENTRAL_IP:6642" \
+  external_ids:ovn-nb="tcp:$CENTRAL_IP:6641" 
external_ids:ovn-encap-ip=$LOCAL_IP external_ids:ovn-encap-type="$ENCAP_TYPE"
 ```
 
 And finally, start the ovn-controller.  (You need to run the below command
diff --git a/ovn/utilities/ovn-docker-overlay-driver 
b/ovn/utilities/ovn-docker-overlay-driver
index 26ed1fe..e4b7d95 100755
--- a/ovn/utilities/ovn-docker-overlay-driver
+++ b/ovn/utilities/ovn-docker-overlay-driver
@@ -36,7 +36,7 @@ app = Flask(__name__)
 vlog = ovs.vlog.Vlog("ovn-docker-overlay-driver")
 
 OVN_BRIDGE = "br-int"
-OVN_REMOTE = ""
+OVN_NB = ""
 PLUGIN_DIR = "/etc/docker/plugins"
 PLUGIN_FILE = "/etc/docker/plugins/openvswitch.spec"
 
@@ -64,7 +64,7 @@ def ovs_vsctl(*args):
 
 def ovn_nbctl(*args):
 args_list = list(args)
-database_option = "%s=%s" % ("--db", OVN_REMOTE)
+database_option = "%s=%s" % ("--db", OVN_NB)
 args_list.insert(0, database_option)
 return call_prog("ovn-nbctl", args_list)
 
@@ -82,10 +82,10 @@ def ovn_init_overlay():
   "external_ids:bridge-id=" + OVN_BRIDGE,
   "other-config:disable-in-band=true", "fail-mode=secure")
 
-global OVN_REMOTE
-OVN_REMOTE = ovs_vsctl("get", "Open_vSwitch", ".",
-   "external_ids:ovn-remote").strip('"')
-if not OVN_REMOTE:
+global OVN_NB
+OVN_NB = ovs_vsctl("get", "Open_vSwitch", ".",
+   "external_ids:ovn-nb").strip('"')
+if not OVN_NB:
 sys.exit("OVN central database's ip address not set")
 
 ovs_vsctl("set", "open_vswitch", ".",
-- 
1.7.9.5

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

[ovs-dev] (no subject)

[Mail Administrator]

This message was undeliverable due to the following reason(s):

Your message could not be delivered because the destination server was
unreachable within the allowed queue period. The amount of time
a message is queued before it is returned depends on local configura-
tion parameters.

Most likely there is a network problem that prevented delivery, but
it is also possible that the computer is turned off, or does not
have a mail system running right now.

Your message was not delivered within 1 days:
Host 43.3.205.152 is not responding.

The following recipients did not receive this message:


Please reply to postmas...@openvswitch.org
if you feel this message to be in error.

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

Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router

[Mickey Spiegel]

Steve and Guru,

I am not all that concerned about the "valid" column, but I do think that we 
will need a different additional column in the near future for output port.

There are three different motivations for allowing output port to be specified 
in the static route:
1) In order to support static routes with a link local next hop. If a link 
local next hop is used, it is possible that the same link local address appears 
on different router ports with different meanings. By specifying the port, this 
disambiguates the specific link local next hop that was desired.
Note: Neutron does not yet support static routes with link local next hop. We 
need to drive the feature in Neutron as well, optionally allowing a router 
interface to be specified in addition to the next hop.
2) This feature should not really be specific to static routes, it should also 
apply to dynamic routes when we add that in the future. Basically anything that 
looks up an IP address prefix and returns a next hop and optionally an output 
port. There are cases where explicitly specifying the output port makes sense.
3) In order to optimize processing of the routing recursion (Steve's code loops 
over the router's ports in ovn-northd.c to carry out this routing recursion), 
we might want to do it above OVN in an event triggered manner, rather than 
every time ovn-northd.c recalculates the flows that it places into the 
southbound database.

For these reasons, I prefer to keep a separate table for static routes.

Mickey


-"dev"  wrote: -
To: Shi Xin Ruan 
From: Guru Shetty 
Sent by: "dev" 
Date: 04/04/2016 08:41AM
Cc: ovs dev 
Subject: Re: [ovs-dev] [PATCH 1/1] Add Static route to logical router

On 31 March 2016 at 19:11, Shi Xin Ruan  wrote:

> Thanks Guru.
>
> The column "valid" will indicate whether the routes has been transfer into
> logical flow.
> Thinking about this case, deleting the logical router port which is out
> going interface of some static routes.
> The first possible way is preventing the deleting, the second way is
> removing the stactic routing from logical flow.
>
[Trying this again as my previous reply got rejected by the mailing list]
I feel this is an unnecessary complication (unless it becomes a real use
case). Let us start with not adding a new table and try to do this with a
column. If you are fine with it, would you mind re-spinning a non-RFC patch
with a unit test? If you want, I can provide the unit tests. If you prefer
that I do the entire thing, I am happy with that too.



>
>
> From my points, both can work fine and has their advantages.
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[ovs-dev] [PATCH v2] dp-packet: Fix use of uninitialised value at emc_lookup.

[William Tu]

Valgrind reports "Conditional jump or move depends on uninitialised value"
and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3
lports/HV.  It is caused by 1) assigning an uninitialized value to 'key->hash'
at emc_processing(). Due to uninit rss_hash_valid, dp_packet_rss_valid() might
return true and undefined hash value is returned, and 2) at emc_lookup, the
'current_entry->key.hash' could be uninitialized due to dp_packet_clone().
The patch fixes the two and as a result, a couple of calls to
dp_packet_rss_valid() become redundant and thus are removed.

Call stacks:
- Connditional jump or move depends on uninitialised value(s)
dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334)
emc_processing (dpif-netdev.c:3455)
dp_netdev_input__ (dpif-netdev.c:3639)
and,
- Use of uninitialised value of size 8
emc_lookup (dpif-netdev.c:1785)
emc_processing (dpif-netdev.c:3457)
dp_netdev_input__ (dpif-netdev.c:3639)

Signed-off-by: William Tu 
---
v1->v2
- use dp_packet_rss_invalidate() instead of direct assignment
- fix dp_packet_clone_with_headroom()
- remove redundant dp_packet_rss_invalidate()
---
 lib/dp-packet.c| 16 +++-
 lib/netdev-bsd.c   |  1 -
 lib/netdev-dummy.c |  1 -
 lib/netdev-linux.c |  1 -
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index aec7fe7..0c85d50 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,6 +29,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum 
dp_packet_source so
 b->source = source;
 dp_packet_reset_offsets(b);
 pkt_metadata_init(>md, 0);
+dp_packet_rss_invalidate(b);
 }
 
 static void
@@ -167,6 +168,19 @@ dp_packet_clone_with_headroom(const struct dp_packet 
*buffer, size_t headroom)
 new_buffer->l3_ofs = buffer->l3_ofs;
 new_buffer->l4_ofs = buffer->l4_ofs;
 new_buffer->md = buffer->md;
+#ifdef DPDK_NETDEV
+new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
+#else
+new_buffer->rss_hash_valid = buffer->rss_hash_valid;
+#endif
+
+if (dp_packet_rss_valid(new_buffer)) {
+#ifdef DPDK_NETDEV
+new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
+#else
+new_buffer->rss_hash = buffer->rss_hash;
+#endif
+}
 
 return new_buffer;
 }
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 75bd5a3..49c05f4 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -641,7 +641,6 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct 
dp_packet **packets,
 dp_packet_delete(packet);
 } else {
 dp_packet_pad(packet);
-dp_packet_rss_invalidate(packet);
 packets[0] = packet;
 *c = 1;
 }
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index edc86fa..a1013ff 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -905,7 +905,6 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct 
dp_packet **arr,
 ovs_mutex_unlock(>mutex);
 
 dp_packet_pad(packet);
-dp_packet_rss_invalidate(packet);
 
 arr[0] = packet;
 *c = 1;
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 3f5b608..a7d7ac7 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1116,7 +1116,6 @@ netdev_linux_rxq_recv(struct netdev_rxq *rxq_, struct 
dp_packet **packets,
 dp_packet_delete(buffer);
 } else {
 dp_packet_pad(buffer);
-dp_packet_rss_invalidate(buffer);
 packets[0] = buffer;
 *c = 1;
 }
-- 
2.5.0

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

Re: [ovs-dev] [PATCH 3/3] ovs-dpctl: Document conntrack "zone" arguments in help output.

[Justin Pettit]

> On Apr 6, 2016, at 8:29 AM, Russell Bryant  wrote:
> 
> 
> On Mon, Mar 28, 2016 at 11:35 PM, Justin Pettit  wrote:
> Signed-off-by: Justin Pettit 
> ---
>  utilities/ovs-dpctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Acked-by: Russell Bryant 

Thanks for the reviews.  I pushed the series.

--Justin


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

[ovs-dev] [PATCH] system-traffic: Fix packet-in format for tests.

[Joe Stringer]

Since continuations were introduced, the system-traffic tests which use
OpenFlow monitors to check the results of datapath execution have been
failing, because the new PACKET_IN2 format is used rather than
PACKET_IN. Switch the expected output over to PACKET_IN2.

Signed-off-by: Joe Stringer 
---
 tests/system-traffic.at | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 28adbdcb9ee6..58212c1ed014 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -181,9 +181,9 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 
ct\(table=0\) '5054000a50
 
 dnl Check this output. We only see the latter two packets, not the first.
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
-NXT_PACKET_IN (xid=0x0): total_len=42 in_port=1 (via action) data_len=42 
(unbuffered)
+NXT_PACKET_IN2 (xid=0x0): total_len=42 in_port=1 (via action) data_len=42 
(unbuffered)
 
udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.1.1.1,nw_dst=10.1.1.2,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=1,tp_dst=2
 udp_csum:0
-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=42 
ct_state=est|rpl|trk,in_port=2 (via action) data_len=42 (unbuffered)
+NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=42 
ct_state=est|rpl|trk,in_port=2 (via action) data_len=42 (unbuffered)
 
udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.1.1.2,nw_dst=10.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=2,tp_dst=1
 udp_csum:0
 ])
 
@@ -925,9 +925,9 @@ AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 2 
ct\(table=0\) 'e64c473528c9c6
 
 dnl Check this output. We only see the latter two packets, not the first.
 AT_CHECK([cat ofctl_monitor.log], [0], [dnl
-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=47 ct_state=new|trk,in_port=1 
(via action) data_len=47 (unbuffered)
+NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=47 ct_state=new|trk,in_port=1 
(via action) data_len=47 (unbuffered)
 
udp,vlan_tci=0x,dl_src=e6:4c:47:35:28:c9,dl_dst=c6:f9:4e:cb:72:db,nw_src=172.16.0.1,nw_dst=172.16.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=41614,tp_dst=
 udp_csum:2096
-NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=75 
ct_state=rel|rpl|trk,in_port=2 (via action) data_len=75 (unbuffered)
+NXT_PACKET_IN2 (xid=0x0): cookie=0x0 total_len=75 
ct_state=rel|rpl|trk,in_port=2 (via action) data_len=75 (unbuffered)
 
icmp,vlan_tci=0x,dl_src=c6:f9:4e:cb:72:db,dl_dst=e6:4c:47:35:28:c9,nw_src=172.16.0.2,nw_dst=172.16.0.1,nw_tos=192,nw_ecn=0,nw_ttl=64,icmp_type=3,icmp_code=3
 icmp_csum:553f
 ])
 
-- 
2.1.4

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

Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Justin Pettit]

I think you might be able to write a slightly simpler patch by using 
ip_format_masked() like the following:

-=-=-=-=-=-=-=-=-=-
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4b1d611..890b17c 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1179,8 +1179,11 @@ build_port_security_nd(struct ovn_port *op, struct hmap *
 if (ps.n_ipv4_addrs) {
 ds_put_cstr(, " && (");
 for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, "arp.spa == "IP_FMT" || ",
-  IP_ARGS(ps.ipv4_addrs[i].addr));
+ds_put_cstr(, "arp.spa == ");
+ip_format_masked(ps.ipv4_addrs[i].addr,
+ be32_prefix_mask(ps.ipv4_addrs[i].plen),
+ );
+ds_put_cstr(, " || ");
 }
 ds_chomp(, ' ');
 ds_chomp(, '|');
@@ -1264,7 +1267,10 @@ build_port_security_ip(enum ovn_pipeline pipeline, struct
 }
 
 for (int i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, IP_FMT", ", IP_ARGS(ps.ipv4_addrs[i].addr
+ip_format_masked(ps.ipv4_addrs[i].addr,
+ be32_prefix_mask(ps.ipv4_addrs[i].plen),
+ );
+ds_put_cstr(, ", ");
 }
 
 /* Replace ", " by "}". */
-=-=-=-=-=-=-=-=-=-

What do you think?

--Justin


> On Apr 6, 2016, at 8:18 AM, Numan Siddique  wrote:
> 
> Initial implementation of port security, missed out this feature.
> 
> Reported-by: Na Zhu 
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1564414
> Signed-off-by: Numan Siddique 
> ---
> ovn/northd/ovn-northd.c | 31 ---
> tests/ovn.at| 19 +++
> 2 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 4b1d611..975ca23 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1048,6 +1048,16 @@ extract_lport_addresses(char *address, struct 
> lport_addresses *laddrs,
> return true;
> }
> 
> +static inline bool
> +is_host_part_zero(ovs_be32 ip4, unsigned int plen)
> +{
> +ovs_be32 mask = be32_prefix_mask(plen);
> +if (plen != 32 && (ip4 & mask) == ip4) {
> +return true;
> +}
> +return false;
> +}
> +
> /* Appends port security constraints on L2 address field 'eth_addr_field'
>  * (e.g. "eth.src" or "eth.dst") to 'match'.  'port_security', with
>  * 'n_port_security' elements, is the collection of port_security constraints
> @@ -1179,8 +1189,15 @@ build_port_security_nd(struct ovn_port *op, struct 
> hmap *lflows)
> if (ps.n_ipv4_addrs) {
> ds_put_cstr(, " && (");
> for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
> -ds_put_format(, "arp.spa == "IP_FMT" || ",
> -  IP_ARGS(ps.ipv4_addrs[i].addr));
> +if (is_host_part_zero(ps.ipv4_addrs[i].addr,
> +  ps.ipv4_addrs[i].plen)) {
> +ds_put_format(, "arp.spa == "IP_FMT"/%d || ",
> +  IP_ARGS(ps.ipv4_addrs[i].addr),
> +  ps.ipv4_addrs[i].plen);
> +} else {
> +ds_put_format(, "arp.spa == "IP_FMT" || ",
> +  IP_ARGS(ps.ipv4_addrs[i].addr));
> +}
> }
> ds_chomp(, ' ');
> ds_chomp(, '|');
> @@ -1264,7 +1281,15 @@ build_port_security_ip(enum ovn_pipeline pipeline, 
> struct ovn_port *op,
> }
> 
> for (int i = 0; i < ps.n_ipv4_addrs; i++) {
> -ds_put_format(, IP_FMT", ", 
> IP_ARGS(ps.ipv4_addrs[i].addr));
> +if (is_host_part_zero(ps.ipv4_addrs[i].addr,
> +  ps.ipv4_addrs[i].plen)) {
> +ds_put_format(, IP_FMT"/%d, ",
> +  IP_ARGS(ps.ipv4_addrs[i].addr),
> +  ps.ipv4_addrs[i].plen);
> +} else {
> +ds_put_format(, IP_FMT", ",
> +  IP_ARGS(ps.ipv4_addrs[i].addr));
> +}
> }
> 
> /* Replace ", " by "}". */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 22121e1..441dd2b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1930,6 +1930,25 @@ for i in 1 2 3; do
> test_ipv6 ${i}3 f${i}${i}3 f021 $sip $tip
> done
> 
> +# configure lport13 to send and received IPv4 packets with an address range
> +ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168.0.13 
> 

Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.

[Russell Bryant]

On Wed, Apr 6, 2016 at 5:43 PM, Ramu Ramamurthy 
wrote:

> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant  wrote:
> > This patch implements one approach to using ovn-controller to implement
> > a software l2 gateway between logical and physical networks.
> >
> > A new logical port type called "gateway" is introduced here.  It is very
> > close to how localnet ports work, with the following exception:
> >
> > - A localnet port makes OVN use the physical network as the
> >   transport between hypervisors instead of tunnels. A gateway port still
> >   uses tunnels between all hypervisors, and packets only go to/from the
> >   specified physical network as needed via the chassis the gateway port
> >   is bound to.
> >
> > - A gateway port also gets bound to a chassis while a localnet port does
> >   not.  This binding is not done by ovn-controller.  It is left as an
> >   administrative function.  In the case of OpenStack, the Neutron plugin
> >   will do this.
> >
> > Signed-off-by: Russell Bryant 
> > ---
> >
>
> Can there be more than 1 gateway to the same l2 network  (say for HA) ?
> If gateway port G1 on chassis1 and gateway port G2 on chassis2,
> Then, when a VM a chassis3 sends a broadcast, it will tunnel to
> chassis1, and chassis2,
> and then get sent out of G1 and G2 to the same L2 network, ie, Can
> that broadcast packet
> get sent in duplicate to the physical network ? If so, would you have
> to run xSTP on the
> gateway ports ?
>

You can only have a single gateway port for a given network at this stage.

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

Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.

[Ramu Ramamurthy]

On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant  wrote:
> This patch implements one approach to using ovn-controller to implement
> a software l2 gateway between logical and physical networks.
>
> A new logical port type called "gateway" is introduced here.  It is very
> close to how localnet ports work, with the following exception:
>
> - A localnet port makes OVN use the physical network as the
>   transport between hypervisors instead of tunnels. A gateway port still
>   uses tunnels between all hypervisors, and packets only go to/from the
>   specified physical network as needed via the chassis the gateway port
>   is bound to.
>
> - A gateway port also gets bound to a chassis while a localnet port does
>   not.  This binding is not done by ovn-controller.  It is left as an
>   administrative function.  In the case of OpenStack, the Neutron plugin
>   will do this.
>
> Signed-off-by: Russell Bryant 
> ---
>

Can there be more than 1 gateway to the same l2 network  (say for HA) ?
If gateway port G1 on chassis1 and gateway port G2 on chassis2,
Then, when a VM a chassis3 sends a broadcast, it will tunnel to
chassis1, and chassis2,
and then get sent out of G1 and G2 to the same L2 network, ie, Can
that broadcast packet
get sent in duplicate to the physical network ? If so, would you have
to run xSTP on the
gateway ports ?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.

[Russell Bryant]

On Wed, Apr 6, 2016 at 3:46 PM, Joe Stringer  wrote:

> Although tests ideally also stick to shorter line lengths, it is very
> common for fixed text blocks like flows or large packets to be specified
> within tests. Checkpatch shouldn't complain about cases like these.
>
> Signed-off-by: Joe Stringer 
>

Acked-by: Russell Bryant 

 @@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):

>  co_authors = []
>  parse = 0
>  current_file = ''
> +previous_file = ''
>  scissors = re.compile(r'^[\w]*---[\w]*')
>  hunks = re.compile('^(---|\+\+\+) (\S+)')
>  is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>re.I | re.M | re.S)
>  is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>re.I | re.M | re.S)
> +skip_line_length_check = False
>
>  for line in text.split('\n'):
> +if current_file is not previous_file:
> +previous_file = current_file
> +for filetype in line_length_blacklist:
> +if filetype in current_file:
> +skip_line_length_check = True
> +break
> +
>

Since you made a comment about Python style, the above could also be:

if current_file != previous_file:
previous_file = current_file
if any([ft in current_file for ft in line_length_blacklist]):
skip_line_length_check = True
break

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

[ovs-dev] [PATCH 1/4 v2] ovsdb-idl: Compound Indexes Design Document

[Rodriguez Betancourt, Esteban]

In the work made in our projects, it was found the need to have a faster
access to the rows contained in tables in the replica, as well to have
the possibility to loop over a subset of rows that meet some specified
criteria.
Those needs lead us to design and implement a functionality that
satisfies those requirements, so an implementation of special indexes were
done.
In order to keep the OVSDB server implementation unmodified and avoid
extra load of processing, the indexes are created as part of the IDL.
The indexes are created as part of the initialization of the replica request
and are maintained automatically when there are changes in the replica.

This document explains the design rationale of the compound indexes feature.

Signed-off-by: Javier Albornoz 
Signed-off-by: Esteban Rodriguez Betancourt 
Signed-off-by: Jorge Arturo Sauma Vargas 
Co-authored-by: Javier Albornoz 
Co-authored-by: Esteban Rodriguez Betancourt 
Co-authored-by: Jorge Arturo Sauma Vargas 
---
 Documentation/automake.mk   |   1 +
 Documentation/c-idl-compound-indexes.md | 226 
 2 files changed, 227 insertions(+)
 create mode 100644 Documentation/c-idl-compound-indexes.md

diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index 5903c22..011855b 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -1,4 +1,5 @@
 docs += \
+   Documentation/c-idl-compound-indexes.md \
Documentation/committer-responsibilities.md \
Documentation/committer-grant-revocation.md \
Documentation/group-selection-method-property.txt
diff --git a/Documentation/c-idl-compound-indexes.md 
b/Documentation/c-idl-compound-indexes.md
new file mode 100644
index 000..1cdf1f8
--- /dev/null
+++ b/Documentation/c-idl-compound-indexes.md
@@ -0,0 +1,226 @@
+C IDL Compound Indexes
+==
+
+## Introduction
+
+This document describes the design and usage of the C IDL Compound Indexes
+feature that allows the developer to create indexes over any number of columns
+on the IDL side, and query them.
+
+This feature works completely on the IDL, without requiring changes to the
+OVSDB Server, OVSDB Protocol (OVSDB RFC (RFC 7047)) or
+performing additional communication with the server.
+
+Please note that in this document, the term "index" refers to the common
+database term defined as "a data structure that improves data retrieval". 
Unless
+stated otherwise, the definition for index from the OVSDB RFC (RFC 7047) is not
+used.
+
+## Typical Use Cases
+
+### Fast lookups
+
+Depending on the topology, the route table of a network device could manage
+thousands of routes. Commands such as "show ip route <*specific route*>" would
+need to do a sequential lookup of the routing table to find the specific route.
+With an index created, the lookup time could be faster.
+
+This same scenario could be applied to other features such as Access List rules
+and even interfaces lists.
+
+### Lexicographic order
+
+There are several cases where retrieving data in lexicographic order is needed.
+For example, SNMP. When an administrator or even a NMS would like to retrieve
+data from a specific device, it's possible that they will request data from 
full
+tables instead of just specific values. Also, they would like to have this
+information displayed in lexicographic order. This operation could be done by
+the SNMP daemon or by the CLI, but it would be better if the database could
+provide the data ready for consumption. Also, duplicate efforts by different
+processes will be avoided. Another use case for requesting data in 
lexicographic
+order is for user interfaces (web or CLI) where it would be better and quicker
+if the DB sends the data sorted instead of letting each process to sort the 
data
+by itself.
+
+## Implementation Design
+
+This feature maintains a collection of indexes per table. The developer can
+define any number of indexes per table.
+
+An index can be defined over any number of columns, and support the following
+options:
+
+-   Add a column with type string, int or real (using default comparators).
+-   Select ordering direction of a column (must be selected when creating the
+index).
+-   Use a custom iterator (eg: treat a string column like a IP, or sort by the
+value of "config" key in a map).
+
+For querying the index the user needs to create a cursor. That cursor points to
+a position in the index. With that, the user can perform lookups
+(by key) and/or get the following rows. The user can also compare the current
+value of the cursor to a record.
+
+For faster lookups, user would need to provide a key which will be used for 
finding
+the specific rows that meet this criteria. This key could be an IP address, a
+MAC address, an ACL rule, etc. When the information is found in the data
+structure the user's 

[ovs-dev] [PATCH 3/4 v2] ovsdb-idl: IDL Compound Indexes Implementation

[Rodriguez Betancourt, Esteban]

In the C IDL, allows to create multicolumn indexes in the
tables, that are keep synched with the data in the replica.

Signed-off-by: Esteban Rodriguez Betancourt 
---
lib/ovsdb-idl-provider.h |  30 
lib/ovsdb-idl.c  | 378 ++-
lib/ovsdb-idl.h  |  52 +++
3 files changed, 459 insertions(+), 1 deletion(-)

diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 027f79b..d9597be 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -1,4 +1,5 @@
/* Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (C) 2016 Hewlett Packard Enterprise Development LP
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -49,6 +50,7 @@ struct ovsdb_idl_column {
 bool mutable;
 void (*parse)(struct ovsdb_idl_row *, const struct ovsdb_datum *);
 void (*unparse)(struct ovsdb_idl_row *);
+int (*compare)(const void *, const void *); /* Perform a comparison over 
ovsrec_* */
};
 struct ovsdb_idl_table_class {
@@ -69,6 +71,7 @@ struct ovsdb_idl_table {
 struct hmap rows;/* Contains "struct ovsdb_idl_row"s. */
 struct ovsdb_idl *idl;   /* Containing idl. */
 unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX];
+struct shash indexes;/* Contains "struct ovsdb_idl_index"s */
 struct ovs_list track_list; /* Tracked rows (ovsdb_idl_row.track_node). */
};
@@ -78,6 +81,33 @@ struct ovsdb_idl_class {
 size_t n_tables;
};
+/*
+ * Contains the configuration per column of the index
+ */
+struct ovsdb_idl_index_column {
+const struct ovsdb_idl_column *column;
+column_comparator *comparer;
+int sorting_order;
+};
+
+/*
+ * Defines a IDL compound index
+ */
+struct ovsdb_idl_index {
+struct skiplist *skiplist;  /* Skiplist with pointer to
+ * rows*/
+struct ovsdb_idl_index_column *columns; /* Columns configuration */
+size_t n_columns;   /* Number of columns in index 
*/
+size_t alloc_columns;   /* Size allocated memory for
+ * columns, comparers and
+ * sorting order */
+bool row_sync;  /* Determines if the replica
+ * is modifying its content or
+ * not */
+const struct ovsdb_idl_table *table;/* Table that owns this index 
*/
+const char* index_name; /* The name of this index */
+};
+
struct ovsdb_idl_row *ovsdb_idl_get_row_arc(
 struct ovsdb_idl_row *src,
 struct ovsdb_idl_table_class *dst_table,
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 3ab05a3..06be5e8 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1,4 +1,5 @@
/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
+ * Copyright (C) 2016 Hewlett Packard Enterprise Development LP
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -36,8 +37,10 @@
#include "ovsdb-parser.h"
#include "poll-loop.h"
#include "shash.h"
+#include "skiplist.h"
#include "sset.h"
#include "util.h"
+#include "uuid.h"
#include "openvswitch/vlog.h"
 VLOG_DEFINE_THIS_MODULE(ovsdb_idl);
@@ -204,6 +207,16 @@ ovsdb_idl_table_from_class(const struct ovsdb_idl *,
const struct ovsdb_idl_table_class *);
static bool ovsdb_idl_track_is_set(struct ovsdb_idl_table *table);
+static int
+ ovsdb_idl_index_generic_comparer(const void *, const void *, const void *);
+static struct ovsdb_idl_index *ovsdb_idl_create_index_(const struct
+   ovsdb_idl_table *table,
+   size_t allocated_cols);
+static void
+ ovsdb_idl_destroy_indexes(struct ovsdb_idl_table *table);
+static void ovsdb_idl_add_to_indexes(const struct ovsdb_idl_row *);
+static void ovsdb_idl_remove_from_indexes(const struct ovsdb_idl_row *);
+
/* Creates and returns a connection to database 'remote', which should be in a
  * form acceptable to jsonrpc_session_open().  The connection will maintain an
  * in-memory replica of the remote database whose schema is described by
@@ -250,6 +263,7 @@ ovsdb_idl_create(const char *remote, const struct 
ovsdb_idl_class *class,
 memset(table->modes, default_mode, tc->n_columns);
 table->need_table = false;
 shash_init(>columns);
+shash_init(>indexes);
 for (j = 0; j < tc->n_columns; j++) {
 const struct ovsdb_idl_column *column = >columns[j];
@@ -285,6 +299,8 @@ ovsdb_idl_destroy(struct ovsdb_idl *idl)
 for (i = 0; i < idl->class->n_tables; i++) {
 

[ovs-dev] [PATCH 4/4 v2] ovsdb-idl: Autogenerated functions for compound indexes

[Rodriguez Betancourt, Esteban]

From:  Arnoldo Lutz Guevara 

Generates and fill the default comparators for columns with
type int, real, string. Also creates the macros that allow
to iterate over the contents of the index, and perform
queries.

Signed-off-by: Arnoldo Lutz Guevara 
Signed-off-by: Esteban Rodriguez Betancourt 
---
ovsdb/ovsdb-idlc.in | 116 +
tests/idltest.ovsschema |   6 +-
tests/ovsdb-idl.at  | 267 
tests/test-ovsdb.c  | 213 +-
4 files changed, 599 insertions(+), 3 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 26b0de4..38c0d51 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -8,9 +8,15 @@ import sys
import ovs.json
import ovs.db.error
import ovs.db.schema
+from ovs.db.types import StringType, IntegerType, RealType
 argv0 = sys.argv[0]
+def isColumnIndexable(column):
+return not column.type.is_map()  and column.type.key.is_valid() \
+   and (column.type.is_scalar())  and \
+column.type.key.toAtomicType() in ['OVSDB_TYPE_STRING', 
'OVSDB_TYPE_REAL', 'OVSDB_TYPE_INTEGER']
+
 def parseSchema(filename):
 return ovs.db.schema.IdlSchema.from_json(ovs.json.from_file(filename))
@@ -186,6 +192,25 @@ const struct %(s)s *%(s)s_track_get_next(const struct 
%(s)s *);
  (ROW); \\
  (ROW) = %(s)s_track_get_next(ROW))
+int %(s)s_index_compare(struct ovsdb_idl_index_cursor *, const struct %(s)s *, 
const struct %(s)s *);
+const struct %(s)s *%(s)s_index_first(struct ovsdb_idl_index_cursor *);
+const struct %(s)s *%(s)s_index_next(struct ovsdb_idl_index_cursor *);
+const struct %(s)s *%(s)s_index_find(struct ovsdb_idl_index_cursor *, const 
struct %(s)s *);
+const struct %(s)s *%(s)s_index_forward_to(struct ovsdb_idl_index_cursor *, 
const struct %(s)s *);
+const struct %(s)s *%(s)s_index_get_data(const struct ovsdb_idl_index_cursor 
*);
+#define %(S)s_FOR_EACH_RANGE(ROW, CURSOR, FROM, TO) \\
+for ((ROW) = %(s)s_index_forward_to(CURSOR, FROM); \\
+ ((ROW) && %(s)s_index_compare(CURSOR, ROW, TO) <= 0); \\
+ (ROW) = %(s)s_index_next(CURSOR))
+#define %(S)s_FOR_EACH_EQUAL(ROW, CURSOR, KEY) \\
+for ((ROW) = %(s)s_index_find(CURSOR, KEY); \\
+ ((ROW) && %(s)s_index_compare(CURSOR, ROW, KEY) == 0); \\
+ (ROW) = %(s)s_index_next(CURSOR))
+#define %(S)s_FOR_EACH_BYINDEX(ROW, CURSOR) \\
+for ((ROW) = %(s)s_index_first(CURSOR); \\
+ (ROW); \\
+ (ROW) = %(s)s_index_next(CURSOR))
+
void %(s)s_init(struct %(s)s *);
void %(s)s_delete(const struct %(s)s *);
struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *);
@@ -216,6 +241,10 @@ bool %(s)s_is_updated(const struct %(s)s *, enum 
%(s)s_column_id);
 print '%s);' % ', '.join(args)
 print
+for columnName, column in sorted(table.columns.iteritems()):
+if isColumnIndexable(column):
+print 'int %(s)s_index_%(c)s_cmp(const void *, const void *);' 
% {'s': structName, 'c': columnName}
+print
 # Table indexes.
 printEnum("%stable_id" % prefix.lower(), ["%sTABLE_%s" % (prefix.upper(), 
tableName.upper()) for tableName in sorted(schema.tables)] + ["%sN_TABLES" % 
prefix.upper()])
@@ -746,7 +775,90 @@ const struct ovsdb_datum *
'S': structName.upper(),
'C': columnName.upper()}
 print "}"
+# Column Index compare functions
+for columnName, column in sorted(table.columns.iteritems()):
+type = column.type
+funcDict = {'OVSDB_TYPE_STRING': 'str', 'OVSDB_TYPE_REAL': 
'double', 'OVSDB_TYPE_INTEGER': 'int'}
+if isColumnIndexable(column):
+print '''
+/*  Call an internal function defined to compare  "%(f)s" type columns for 
"%(c)s" columns
+in "%(s)s" tables.
+Parameters: void *row1, void * row2. Data to be compared. Must be of type 
corresponding the record of
+the table.
+Return value: 0 if both data values are equal, -1 if first parameter is 
less than second and 1 otherwise.
+For internal use only. Not recomended to be called directly. */ ''' % {'s' 
: structName, 'c' : columnName, 'f': type.key.toAtomicType()}
+print 'int'
+print '%(s)s_index_%(c)s_cmp(const void *row1, const void 
*row2)' % \
+{'s': structName, 'c': columnName}
+print '{'
+print 'struct %(s)s *data1 = (struct %(s)s *)row1;' % { 
's' : structName }
+print 'struct %(s)s *data2 = (struct %(s)s *)row2;' % { 
's' : structName }
+print 'return ovsdb_idl_index_%(f)scmp(data1->%(c)s, 
data2->%(c)s);' % \
+{'c': columnName, 'f': funcDict[type.key.toAtomicType()] }
+print "}"
+# Index table related functions

[ovs-dev] [PATCH 2/4 v2] lib:Data structures: Skiplist implementation

[Rodriguez Betancourt, Esteban]

Skiplist implementation intended for the IDL compound indexes
feature.

Signed-off-by: Esteban Rodriguez Betancourt 
---
lib/automake.mk   |   2 +
lib/skiplist.c| 313 ++
lib/skiplist.h|  54 +
tests/.gitignore  |   1 +
tests/automake.mk |   2 +
tests/library.at  |  11 ++
tests/test-skiplist.c | 210 +
7 files changed, 593 insertions(+)
create mode 100644 lib/skiplist.c
create mode 100644 lib/skiplist.h
create mode 100644 tests/test-skiplist.c

diff --git a/lib/automake.mk b/lib/automake.mk
index 7b829d0..5e08b44 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -220,6 +220,8 @@ lib_libopenvswitch_la_SOURCES = \
   lib/shash.h \
   lib/simap.c \
   lib/simap.h \
+ lib/skiplist.c \
+ lib/skiplist.h \
   lib/smap.c \
   lib/smap.h \
   lib/socket-util.c \
diff --git a/lib/skiplist.c b/lib/skiplist.c
new file mode 100644
index 000..fcf24f6
--- /dev/null
+++ b/lib/skiplist.c
@@ -0,0 +1,313 @@
+/* Copyright (C) 2016 Hewlett Packard Enterprise Development LP
+ * All Rights Reserved.
+ *
+ * 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.
+ */
+
+/*
+ * Skiplist implementationn based on:
+ * "Skip List: A Probabilistic Alternative to Balanced Trees",
+ * by William Pugh.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "skiplist.h"
+#include "random.h"
+#include "util.h"
+
+/*
+ * The primary usage of the skiplists are the compound indexes
+ * at the IDL.
+ * For that use case 32 height levels is more than enough as
+ * it could indexes a table with 4.294.967.296 rows.
+ * In case that a new use case require more than that then this
+ * number can be changed, but also the way in which the random
+ * numbers are generated must be changed.
+ */
+#define SKIPLIST_MAX_LEVELS 32
+
+/*
+ * Skiplist node container
+ */
+struct skiplist_node {
+const void *data;   /* Pointer to saved data */
+uint64_t height;/* Height of this node */
+struct skiplist_node *forward[];/* Links to the next nodes */
+};
+
+/*
+ * Skiplist container
+ */
+
+struct skiplist {
+struct skiplist_node *header;   /* Pointer to head node (not first
+ * data node) */
+skiplist_comparator *cmp;   /* Pointer to the skiplist's comparison
+ * function */
+void *cfg;  /* Pointer to optional comparison
+ * configuration, used by the comparator */
+int max_levels; /* Max levels of the skiplist. */
+int64_t size;   /* Current size of the skiplist. */
+int64_t level;  /* Max number of levels used in this skiplist 
*/
+void (*free_func) (void *); /* Function that free the value's memory */
+};
+
+static int skiplist_determine_level(struct skiplist *sl);
+
+static struct skiplist_node *skiplist_create_node(int, const void *);
+
+static struct skiplist_node *skiplist_forward_to_(struct skiplist *sl,
+  const void *value,
+  struct skiplist_node
+  **update);
+
+/*
+ * skiplist_create returns a new skiplist, configured with given max_levels,
+ * data comparer and configuration.
+ * Sets a probability of 0.5 (RAND_MAX / 2).
+ */
+struct skiplist *
+skiplist_create(int max_levels, skiplist_comparator object_comparator,
+void *configuration)
+{
+random_init();
+struct skiplist *sl;
+
+sl = xmalloc(sizeof (struct skiplist));
+sl->cfg = configuration;
+sl->max_levels = max_levels < SKIPLIST_MAX_LEVELS ?
+max_levels : SKIPLIST_MAX_LEVELS;
+sl->size = 0;
+sl->level = 0;
+sl->cmp = object_comparator;
+sl->header = skiplist_create_node(sl->max_levels, NULL);
+sl->free_func = NULL;
+
+return sl;
+}
+
+/*
+ * Set a custom function that free the value's memory when
+ * destroying the skiplist.
+ */
+void
+skiplist_set_free_func(struct skiplist *sl, void (*func) (void *))
+{
+sl->free_func = func;
+}
+
+/*
+ * Determines the correspondent level for a skiplist node.
+ * Guarantees that the returned integer is less or equal
+ * to the current height of the skiplist plus 

Re: [ovs-dev] [PATCH 3/5] ovsdb-idl: IDL Compound Indexes Implementation

[Rodriguez Betancourt, Esteban]

> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: martes, 22 de marzo de 2016 14:55
> To: Rodriguez Betancourt, Esteban 
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 3/5] ovsdb-idl: IDL Compound Indexes 
> Implementation
> 
> On Wed, Mar 09, 2016 at 12:03:54AM +, Rodriguez Betancourt, 
> Esteban
> wrote:
> > In the C IDL, allows to create multicolumn indexes in the tables, 
> > that are keep synched with the data in the replica.
> >
> > Signed-off-by: Esteban Rodriguez Betancourt 
> 
> Until now, ovsdb-idl.h has been divided into sections separated by 
> page- breaks (control-L), and each section has had a comment 
> explaining basically what that section describes.  Some of the 
> sections, like the one for transactions, has a lot of detail.  This 
> commit adds a new section, but it doesn't add a page break or an 
> explanatory comment.  I think it should do both.  The design document 
> from patch 1 might be a good place from which to draw for the comment.

Changed in v2 patch.

> This patch and patch 2 has a couple of typedefs for function pointers.
> CodingStyle.md says:
> 
>   A function type is a good use for a typedef because it can clarify
> code.  The type should be a function type, not a pointer-to-function
> type.  That way, the typedef name can be used to declare function
> prototypes.  (It cannot be used for function definitions, because that
> is explicitly prohibited by C89 and C99.)

Changed in v2 patch.

> Do you think that the sorting order in struct ovsdb_idl_index is 
> useful, that is, do you see a common use for indexes in descending 
> order?  If indexes in descending order are only rarely useful, then 
> they could be implemented through a custom comparison function.

This is more needed by tools that expose the data to a user, and the user wants 
to sort the data in different ways.
Also this enables the option to have in a single index some columns in one 
direction and other columns in the other direction.

> I don't understand, in ovsdb_idl_index_compare(), why NULL != NULL.  
> It seems to me that this is risky and likely to cause confusion, if 
> not now then later.
Changed in v2 patch.

> I don't understand why indexes have string names.  It seems like kind 
> of a curious design.  Why isn't the client expected to retain a 
> pointer to the index that it wants to traverse, instead of a name?


One reason is to prevent the developer from modifying the values of the index by
referencing it using a string instead of giving direct access to the index data 
structure.
The other one is to allow a more intuitive way to refer to the indexes, and to
select them when programming. 

> Please use xmalloc() instead of malloc().
Changed in v2 patch.

> I found myself wondering whether there's value in doing dynamic 
> allocation of the three parallel arrays inside struct ovsdb_idl_index.
> It would be very unusual to have an index over more than, say, 3 
> columns, and so it might be easier to have fixed-size arrays of 3 
> elements (or a few more, to allow room for expansion).  I imagine that 
> the columns in indexes will be fixed at build time, after all.
> 
> ovsdb_idl_index_add_column() seems to be working hard to avoid using 
> xrealloc(), but I don't know why.

Changed in v2 patch. The indexes support any number of "columns" for doing the 
comparison. But that number isn't limited to the number of columns in a row, 
because eventually one column can have several values indexed, for example, 
different keys from a map column.

For some "queries", the need of filtering the data first raise the number of 
columns need to 5 or more.

> I don't understand why only certain columns have a default comparison 
> function.  Why not use ovsdb_datum_compare_3way() as the default for 
> every column?  I guess that the ovsdb_datums and ovsdb_type would have 
> to be passed into column_comparator instead of a pair of void * 
> pointers, but that would arguably be an improvement anyhow, certainly 
> from a type- safety viewpoint.

We decided against using the datums for the comparison because it seems 
intuitive that the parsed values in the ovsrec are easier and faster to compare.

In some cases, a table could have a really really high number of rows, and 
receive a lot of updates per second, so is important that the comparison 
functions are fast.

> Do you have an example of a useful use for a custom comparison function?

Sorting a string column, that contains IPv4 or IPv6 values.
Sort an specific member inside a map column Sort a string as a number Sort 
strings as dates Sort by a value contained in a referenced row

> Did you consider defining a struct of three members instead of using 
> three parallel arrays in struct ovsdb_idl_index?  It might well be 
> cleaner, and certainly would be for memory allocation issues.

Changed in v2 patch

> It looks like the clients don't actually need to see the definition of 
> struct 

[ovs-dev] Use of uninitialized value at testcase OVN 3 HVs, 3 LS, 3 lports/LS, 1 LR

[William Tu]

Hi,

Valgrind reports "Conditional jump or move depends on uninitialised
value(s)" on test case 2019, "ovn.at:1229 ovn -- 3 HVs, 3 LS, 3 lports/LS,
1 LR". I have no clue about how to fix this error. Any comments are
appreciated.

At the end of the message, valgrind reports "Uninitialised value was
created by a stack allocation at 0x4404A0: xlate_actions
(ofproto-dpif-xlate.c:5061)". So I suspect that due to deep call stacks
caused by xlate_recursively(), maybe the stack size is not enough. So I
increase stack size to 512MB but still the same.

If you want to try, make valgrind more verbose by adding
VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full \
--track-origins=yes \
--suppressions=$(abs_top_srcdir)/tests/glibc.supp \
--suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=80

Regards,
William

-
==2306== Conditional jump or move depends on uninitialised value(s)
==2306==at 0x4E8E08: is_all_ones (util.c:1145)
==2306==by 0x4911B5: commit (odp-util.c:5410)
==2306==by 0x4913DB: commit_set_ether_addr_action (odp-util.c:5459)
==2306==by 0x4913DB: commit_odp_actions (odp-util.c:5923)
==2306==by 0x4385ED: xlate_commit_actions (ofproto-dpif-xlate.c:2919)
==2306==by 0x43E91F: compose_output_action__ (ofproto-dpif-xlate.c:3142)
==2306==by 0x43BEBD: compose_output_action (ofproto-dpif-xlate.c:3210)
==2306==by 0x43BEBD: xlate_output_action (ofproto-dpif-xlate.c:3895)
==2306==by 0x43D2E6: do_xlate_actions (ofproto-dpif-xlate.c:4474)
==2306==by 0x43E155: xlate_recursively (ofproto-dpif-xlate.c:3229)
<... skip ...>
==2306==by 0x43D544: do_xlate_actions (ofproto-dpif-xlate.c:4609)
==2306==by 0x43E155: xlate_recursively (ofproto-dpif-xlate.c:3229)
==2306==by 0x43E155: xlate_table_action (ofproto-dpif-xlate.c:3292)
==2306==by 0x43D544: xlate_ofpact_resubmit (ofproto-dpif-xlate.c:3555)
==2306==by 0x43D544: do_xlate_actions (ofproto-dpif-xlate.c:4609)
==2306==by 0x440BFD: xlate_actions (ofproto-dpif-xlate.c:5324)
==2306==by 0x429E2F: ofproto_dpif_execute_actions__
(ofproto-dpif.c:3736)
==2306==by 0x42A064: ofproto_dpif_execute_actions (ofproto-dpif.c:3774)
==2306==by 0x42A064: packet_out (ofproto-dpif.c:4424)
==2306==by 0x41CDB1: handle_packet_out (ofproto.c:3402)
==2306==by 0x4214CA: handle_openflow__ (ofproto.c:7239)
==2306==by 0x4214CA: handle_openflow (ofproto.c:7403)
==2306==by 0x4461D2: ofconn_run (connmgr.c:1379)
==2306==by 0x4461D2: connmgr_run (connmgr.c:323)
==2306==by 0x41AC05: ofproto_run (ofproto.c:1762)
==2306==by 0x409DE3: bridge_run__ (bridge.c:2888)
==2306==by 0x40F773: bridge_run (bridge.c:2943)
==2306==by 0x406274: main (ovs-vswitchd.c:120)
==2306==  Uninitialised value was created by a stack allocation
==2306==at 0x4404A0: xlate_actions (ofproto-dpif-xlate.c:5061)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.

[Joe Stringer]

Although tests ideally also stick to shorter line lengths, it is very
common for fixed text blocks like flows or large packets to be specified
within tests. Checkpatch shouldn't complain about cases like these.

Signed-off-by: Joe Stringer 
---
v2: Broaden the set of blacklisted files to not check.
---
 utilities/checkpatch.py | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index d9011f370816..83d14e89269e 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -56,6 +56,12 @@ skip_trailing_whitespace_check = False
 skip_block_whitespace_check = False
 skip_signoff_check = False
 
+# Don't enforce character limit on files that include these characters in their
+# name, as they may have legitimate reasons to have longer lines.
+#
+# Python isn't checked as flake8 performs these checks during build.
+line_length_blacklist = ['.am', '.at', 'etc', '.in', '.mk', '.patch', '.py']
+
 
 def is_added_line(line):
 """Returns TRUE if the line in question is an added line.
@@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):
 co_authors = []
 parse = 0
 current_file = ''
+previous_file = ''
 scissors = re.compile(r'^[\w]*---[\w]*')
 hunks = re.compile('^(---|\+\+\+) (\S+)')
 is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
   re.I | re.M | re.S)
 is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
   re.I | re.M | re.S)
+skip_line_length_check = False
 
 for line in text.split('\n'):
+if current_file is not previous_file:
+previous_file = current_file
+for filetype in line_length_blacklist:
+if filetype in current_file:
+skip_line_length_check = True
+break
+
 lineno = lineno + 1
 if len(line) <= 0:
 continue
@@ -154,7 +169,7 @@ def ovs_checkpatch_parse(text):
 if trailing_whitespace_or_crlf(line[1:]):
 print_line = True
 print_warning("Line has trailing whitespace", lineno)
-if len(line[1:]) > 79:
+if len(line[1:]) > 79 and not skip_line_length_check:
 print_line = True
 print_warning("Line is greater than 79-characters long",
   lineno)
-- 
2.1.4

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

Re: [ovs-dev] [PATCH] checkpatch: Don't enforce char limit on tests.

[Joe Stringer]

On 5 April 2016 at 11:37, Russell Bryant  wrote:
>
>
> On Tue, Apr 5, 2016 at 2:17 PM, Joe Stringer  wrote:
>>
>> Although tests ideally also stick to shorter line lengths, it is very
>> common for fixed text blocks like flows or large packets to be specified
>> within tests. Checkpatch shouldn't complain about cases like these.
>>
>> Signed-off-by: Joe Stringer 
>
>
> Acked-by: Russell Bryant 
>
> Some thoughts below ...
>
>> ---
>>  utilities/checkpatch.py | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index d9011f370816..791c7d902fa5 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -154,7 +154,10 @@ def ovs_checkpatch_parse(text):
>>  if trailing_whitespace_or_crlf(line[1:]):
>>  print_line = True
>>  print_warning("Line has trailing whitespace", lineno)
>> -if len(line[1:]) > 79:
>> +
>> +# Don't enforce character limit on test files, since they
>> commonly
>> +# include long pieces of text like flows or hex dumps of
>> packets
>> +if len(line[1:]) > 79 and '.at' not in current_file:
>>  print_line = True
>>  print_warning("Line is greater than 79-characters long",
>>lineno)
>
>
> I believe there are other examples that would hit this same problem, such as
> *.am at least.  flake8 is already checking the equivalent for Python files.
> An alternative would be to just whitelist what we want to check. (.c, .h,
> .md?)

Hmm. A whitelist wouldn't cover things like NEWS, NOTICE,
debian/changelog, rhel/README.RHEL, etc.

I'm leaning towards blacklisting files with the following strings in
their name at the moment:

[ '.am', '.at', 'etc', '.in', '.mk', '.patch', '.py']
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] checkpatch: Don't enforce char limit on tests.

[Joe Stringer]

On 6 April 2016 at 07:33, Aaron Conole  wrote:
> Russell Bryant  writes:
>
>> On Tue, Apr 5, 2016 at 2:17 PM, Joe Stringer  wrote:
>>
>>> Although tests ideally also stick to shorter line lengths, it is very
>>> common for fixed text blocks like flows or large packets to be specified
>>> within tests. Checkpatch shouldn't complain about cases like these.
>>>
>>> Signed-off-by: Joe Stringer 
>>>
>>
>> Acked-by: Russell Bryant 
>>
>> Some thoughts below ...
>>
>> ---
>>>  utilities/checkpatch.py | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>>> index d9011f370816..791c7d902fa5 100755
>>> --- a/utilities/checkpatch.py
>>> +++ b/utilities/checkpatch.py
>>> @@ -154,7 +154,10 @@ def ovs_checkpatch_parse(text):
>>>  if trailing_whitespace_or_crlf(line[1:]):
>>>  print_line = True
>>>  print_warning("Line has trailing whitespace", lineno)
>>> -if len(line[1:]) > 79:
>>> +
>>> +# Don't enforce character limit on test files, since they
>>> commonly
>>> +# include long pieces of text like flows or hex dumps of
>>> packets
>>> +if len(line[1:]) > 79 and '.at' not in current_file:
>>>  print_line = True
>>>  print_warning("Line is greater than 79-characters long",
>>>lineno)
>>
>>
>> I believe there are other examples that would hit this same problem, such
>> as *.am at least.  flake8 is already checking the equivalent for Python
>> files.  An alternative would be to just whitelist what we want to check.
>> (.c, .h, .md?)
>
> I think either a whitelist or blacklist work fine for this check. There are
> certain files which just don't make sense to check for line
> lengths.
>
> That said, the proposed patch looks good to me.

Thanks for the reviews, I'll send a follow up (in case my python style
is a bit too C-like ;) )
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Ryan Moats]

"dev"  wrote on 04/06/2016 10:18:57 AM:

> From: Numan Siddique 
> To: ovs dev 
> Date: 04/06/2016 10:19 AM
> Subject: [ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with
> prefixes in lport port security
> Sent by: "dev" 
>
> Initial implementation of port security, missed out this feature.
>
> Reported-by: Na Zhu 
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1564414
> Signed-off-by: Numan Siddique 
> ---

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

[ovs-dev] [PATCH v4] datapath-windows: Hot add CPU support.

[Sorin Vinturis]

Hot add CPU is the ability to dynamically add CPUs to a running
system. Adding CPUs can occur physically by adding new hardware,
logically by online hardware partitioning, or virtually through
a virtualization layer.

This patch add support to reallocate any per-cpu resources, in
case a new processor is added.

Signed-off-by: Sorin Vinturis 
Reported-by: Sorin Vinturis 
Reported-at: https://github.com/openvswitch/ovs-issues/issues/112
---
v2: Correctly reallocate per-cpu data structures when a new processor
is about to be added to the system.
v3: Per-cpu variables are allocated for the maximum number of processors
supported by the system.
---
 datapath-windows/ovsext/Datapath.c |  13 +++--
 datapath-windows/ovsext/Datapath.h |   2 +-
 datapath-windows/ovsext/Driver.c   |   5 +-
 datapath-windows/ovsext/Recirc.c   | 100 -
 datapath-windows/ovsext/Recirc.h   |  45 +++--
 datapath-windows/ovsext/Util.c |  34 -
 datapath-windows/ovsext/Util.h |  20 +++-
 7 files changed, 114 insertions(+), 105 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c 
b/datapath-windows/ovsext/Datapath.c
index 464fa97..8c0c246 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -379,26 +379,29 @@ FreeUserDumpState(POVS_OPEN_INSTANCE instance)
 }
 }
 
-VOID
+NDIS_STATUS
 OvsInit()
 {
+NDIS_STATUS status = NDIS_STATUS_SUCCESS;
+
 gOvsCtrlLock = 
 NdisAllocateSpinLock(gOvsCtrlLock);
 OvsInitEventQueue();
-OvsDeferredActionsQueueAlloc();
-OvsDeferredActionsLevelAlloc();
+
+status = OvsPerCpuDataInit();
+
+return status;
 }
 
 VOID
 OvsCleanup()
 {
+OvsPerCpuDataCleanup();
 OvsCleanupEventQueue();
 if (gOvsCtrlLock) {
 NdisFreeSpinLock(gOvsCtrlLock);
 gOvsCtrlLock = NULL;
 }
-OvsDeferredActionsQueueFree();
-OvsDeferredActionsLevelFree();
 }
 
 VOID
diff --git a/datapath-windows/ovsext/Datapath.h 
b/datapath-windows/ovsext/Datapath.h
index 2c61d82..09e233f 100644
--- a/datapath-windows/ovsext/Datapath.h
+++ b/datapath-windows/ovsext/Datapath.h
@@ -65,7 +65,7 @@ typedef struct _OVS_OPEN_INSTANCE {
 
 NDIS_STATUS OvsCreateDeviceObject(NDIS_HANDLE ovsExtDriverHandle);
 VOID OvsDeleteDeviceObject();
-VOID OvsInit();
+NDIS_STATUS OvsInit();
 VOID OvsCleanup();
 
 POVS_OPEN_INSTANCE OvsGetOpenInstance(PFILE_OBJECT fileObject,
diff --git a/datapath-windows/ovsext/Driver.c b/datapath-windows/ovsext/Driver.c
index 853886e..2771015 100644
--- a/datapath-windows/ovsext/Driver.c
+++ b/datapath-windows/ovsext/Driver.c
@@ -96,7 +96,10 @@ DriverEntry(PDRIVER_OBJECT driverObject,
 UNREFERENCED_PARAMETER(registryPath);
 
 /* Initialize driver associated data structures. */
-OvsInit();
+status = OvsInit();
+if (status != NDIS_STATUS_SUCCESS) {
+goto cleanup;
+}
 
 gOvsExtDriverObject = driverObject;
 
diff --git a/datapath-windows/ovsext/Recirc.c b/datapath-windows/ovsext/Recirc.c
index 86e6f51..2febf06 100644
--- a/datapath-windows/ovsext/Recirc.c
+++ b/datapath-windows/ovsext/Recirc.c
@@ -18,71 +18,61 @@
 #include "Flow.h"
 #include "Jhash.h"
 
-static POVS_DEFERRED_ACTION_QUEUE ovsDeferredActionQueue = NULL;
-static UINT32* ovsDeferredActionLevel = NULL;
-
 /*
  * --
- * OvsDeferredActionsQueueAlloc --
- * The function allocates per-cpu deferred actions queue.
+ * '_OVS_DEFERRED_ACTION_QUEUE' structure is responsible for keeping track of
+ * all deferred actions. The maximum number of deferred actions should not
+ * exceed 'DEFERRED_ACTION_QUEUE_SIZE'.
  * --
  */
-BOOLEAN
-OvsDeferredActionsQueueAlloc()
-{
-ovsDeferredActionQueue =
-OvsAllocateMemoryPerCpu(sizeof(*ovsDeferredActionQueue),
-OVS_RECIRC_POOL_TAG);
-if (!ovsDeferredActionQueue) {
-return FALSE;
-}
-return TRUE;
-}
+typedef struct _OVS_DEFERRED_ACTION_QUEUE {
+UINT32  head;
+UINT32  tail;
+OVS_DEFERRED_ACTION deferredActions[DEFERRED_ACTION_QUEUE_SIZE];
+} OVS_DEFERRED_ACTION_QUEUE, *POVS_DEFERRED_ACTION_QUEUE;
 
-/*
- * --
- * OvsDeferredActionsQueueFree --
- * The function frees per-cpu deferred actions queue.
- * --
- */
-VOID
-OvsDeferredActionsQueueFree()
-{
-OvsFreeMemoryWithTag(ovsDeferredActionQueue,
- OVS_RECIRC_POOL_TAG);
-ovsDeferredActionQueue = NULL;
-}
+typedef struct _OVS_DEFERRED_ACTION_DATA {
+OVS_DEFERRED_ACTION_QUEUE   queue;
+UINT32  level;
+} OVS_DEFERRED_ACTION_DATA, *POVS_DEFERRED_ACTION_DATA;
+
+static POVS_DEFERRED_ACTION_DATA 

Re: [ovs-dev] [PATCH] dp-packet: Fix use of uninitialised value at emc_lookup.

[William Tu]

Hi Darrell and Daniele,

Thanks for the comments! I will use dp_packet_rss_invalidate() instead and
send v2 patch.

Regards,
William

On Wed, Apr 6, 2016 at 11:26 AM, Daniele Di Proietto 
wrote:

> 2016-04-06 10:09 GMT-07:00 Darrell Ball :
> > On Wed, Apr 6, 2016 at 9:37 AM, William Tu  wrote:
> >
> >> Valgrind reports "Conditional jump or move depends on uninitialised
> value"
> >> and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3
> >> lports/HV.  It is caused by reading uninitialized 'key->hash' at
> >> emc_lookup()
> >> and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(),
> >> the value of key->hash is initilized by
> dpif_netdev_packet_get_rss_hash(),
> >> which returns an uninitialized hash value.  Call stacks below:
> >>
> >> - Connditional jump or move depends on uninitialised value(s)
> >> dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334)
> >> emc_processing (dpif-netdev.c:3455)
> >> dp_netdev_input__ (dpif-netdev.c:3639)
> >> and,
> >> - Use of uninitialised value of size 8
> >> emc_lookup (dpif-netdev.c:1785)
> >> emc_processing (dpif-netdev.c:3457)
> >> dp_netdev_input__ (dpif-netdev.c:3639)
> >>
> >> Signed-off-by: William Tu 
> >> ---
> >>  lib/dp-packet.c | 9 -
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> >> index aec7fe7..87ed329 100644
> >> --- a/lib/dp-packet.c
> >> +++ b/lib/dp-packet.c
> >> @@ -1,5 +1,5 @@
> >>  /*
> >> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> >> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
> >>   *
> >>   * Licensed under the Apache License, Version 2.0 (the "License");
> >>   * you may not use this file except in compliance with the License.
> >> @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t
> allocated,
> >> enum dp_packet_source so
> >>  b->source = source;
> >>  dp_packet_reset_offsets(b);
> >>  pkt_metadata_init(>md, 0);
> >> +#ifdef DPDK_NETDEV
> >> +b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
> >> +b->mbuf.hash.rss = 0;
> >> +#else
> >> +b->rss_hash_valid = false;
> >> +b->rss_hash = 0;
> >> +#endif
> >>
> >
> >
> > Just a general comment, not a review:
> >
> > Do you need to set the hash value to zero as well as set
> > the "hash_valid" flag to false; should not setting the "hash_valid"
> > flag to false be enough to handle a  initialization issue ?
> >
> > I think there is already an API for setting "hash_valid"
> > to false here
> >
> > static inline void
> > dp_packet_rss_invalidate(struct dp_packet *p)
> > {
> > #ifdef DPDK_NETDEV
> > p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
> > #else
> > p->rss_hash_valid = false;
> > #endif
> > }
> >
> >
>
> I agree with Darrell, I think it's better to use
> dp_packet_rss_invalidate().
>
> Also, if we include dp_packet_rss_invalidate() in dp_packet_init__(),
> we will have redundant calls to dp_packet_rss_invalidate() in
> netdev-{bsd,dummy,linux}.c. Would you mind removing those? There's
> another one in netdev-dpdk.c, but that will be requires anyway.
>
> Would you mind sending a v2 with the suggested changes?
>
> Thanks!
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] dp-packet: Fix use of uninitialised value at emc_lookup.

[Daniele Di Proietto]

2016-04-06 10:09 GMT-07:00 Darrell Ball :
> On Wed, Apr 6, 2016 at 9:37 AM, William Tu  wrote:
>
>> Valgrind reports "Conditional jump or move depends on uninitialised value"
>> and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3
>> lports/HV.  It is caused by reading uninitialized 'key->hash' at
>> emc_lookup()
>> and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(),
>> the value of key->hash is initilized by dpif_netdev_packet_get_rss_hash(),
>> which returns an uninitialized hash value.  Call stacks below:
>>
>> - Connditional jump or move depends on uninitialised value(s)
>> dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334)
>> emc_processing (dpif-netdev.c:3455)
>> dp_netdev_input__ (dpif-netdev.c:3639)
>> and,
>> - Use of uninitialised value of size 8
>> emc_lookup (dpif-netdev.c:1785)
>> emc_processing (dpif-netdev.c:3457)
>> dp_netdev_input__ (dpif-netdev.c:3639)
>>
>> Signed-off-by: William Tu 
>> ---
>>  lib/dp-packet.c | 9 -
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index aec7fe7..87ed329 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>   * you may not use this file except in compliance with the License.
>> @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t allocated,
>> enum dp_packet_source so
>>  b->source = source;
>>  dp_packet_reset_offsets(b);
>>  pkt_metadata_init(>md, 0);
>> +#ifdef DPDK_NETDEV
>> +b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
>> +b->mbuf.hash.rss = 0;
>> +#else
>> +b->rss_hash_valid = false;
>> +b->rss_hash = 0;
>> +#endif
>>
>
>
> Just a general comment, not a review:
>
> Do you need to set the hash value to zero as well as set
> the "hash_valid" flag to false; should not setting the "hash_valid"
> flag to false be enough to handle a  initialization issue ?
>
> I think there is already an API for setting "hash_valid"
> to false here
>
> static inline void
> dp_packet_rss_invalidate(struct dp_packet *p)
> {
> #ifdef DPDK_NETDEV
> p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
> #else
> p->rss_hash_valid = false;
> #endif
> }
>
>

I agree with Darrell, I think it's better to use dp_packet_rss_invalidate().

Also, if we include dp_packet_rss_invalidate() in dp_packet_init__(),
we will have redundant calls to dp_packet_rss_invalidate() in
netdev-{bsd,dummy,linux}.c. Would you mind removing those? There's
another one in netdev-dpdk.c, but that will be requires anyway.

Would you mind sending a v2 with the suggested changes?

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

Re: [ovs-dev] [PATCH v2] ofp-actions: Add a new action to truncate a packet.

[Joe Stringer]

On 1 April 2016 at 17:37, William Tu  wrote:
> The patch proposes adding a new action to support packet truncation.  The new
> action is formatted as 'output(port=n,max_len=m)', as output to port n, with
> packet size being MIN(original_size, m).
>
> One use case is to enable port mirroring to send smaller packets to the
> destination port so that only useful packet information is mirrored/copied,
> saving some performance overhead of copying entire packet payload.  Example
> use case is below as well as shown in the testcases:
>
> - Output to port 1 with max_len 100 bytes.
> - The output packet size on port 1 will be MIN(original_packet_size, 100).
> # ovs-ofctl add-flow br0 'actions=output(port=1,max_len=100)'
>
> - The scope of max_len is limited to output action itself.  The following
>   output:1 and output:2 will be the original packet size.
> # ovs-ofctl add-flow br0 
> 'actions=output(port=1,max_len=100),output:1,output:2'
>
> Implementation/Limitaions:
>
> - The patch adds a new OpenFlow extension action OFPACT_OUTPUT_TRUNC, and
>   a new datapath action, OVS_ACTION_ATTR_OUTPUT_TRUNC.
> - The minimum value of max_len is 60 byte (minimum Ethernet frame size).
>   This is defined in OVS_ACTION_OUTPUT_MIN.
> - Only "output(port=[0-9]*,max_len=)" is supported.  Output to IN_PORT,
>   TABLE, NORMAL, FLOOD, ALL, and LOCAL are not supported.
>
> Signed-off-by: William Tu 

Great to see another user of the kernel testsuite :-)

One broad piece of feedback, what happens if you run an old
openvswitch kernel module with this new userspace that supports the
action? Eg if you took your stock openvswitch module that comes with
your distribution kernel and run this new userspace against it. Can
you still execute this action?



> diff --git a/datapath/actions.c b/datapath/actions.c
> index dcf8591..a1a1241 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c

...

> @@ -1055,6 +1060,13 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
> prev_port = nla_get_u32(a);
> break;
>
> +case OVS_ACTION_ATTR_OUTPUT_TRUNC: {
> +   struct ovs_action_output_trunc *output_trunc = 
> nla_data(a);
> +   prev_port = output_trunc->port;
> +   max_len = output_trunc->max_len;
> +   break;
> +}
> +

Whitespace.

> case OVS_ACTION_ATTR_USERSPACE:
> output_userspace(dp, skb, key, a, attr, len);
> break;



> @@ -536,6 +540,36 @@ encode_OUTPUT(const struct ofpact_output *output,
>  }
>
>  static char * OVS_WARN_UNUSED_RESULT
> +parse_truncate_subfield(struct ofpact_output_trunc *output_trunc,
> +const char *arg_)
> +{
> +char *key, *value;
> +char *error = NULL;
> +char *arg = CONST_CAST(char *, arg_);
> +
> +while (ofputil_parse_key_value(, , )) {
> +char *error;
> +if (!strcmp(key, "port")) {
> +if (!ofputil_port_from_string(value, _trunc->port)) {
> +return xasprintf("%s: output to unknown port", value);
> +}
> +} else if (!strcmp(key, "max_len")) {
> +error = str_to_u16(value, key, _trunc->max_len);
> +} else {
> +error = xasprintf("invalid key '%s' in output_trunc argument",
> +  key);
> +}
> +if (error) {
> +return error;
> +}
> +}
> +if (!error && arg[0]) {
> +error = xstrdup("unexpected input following field syntax");
> +}
> +return error;
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
>  parse_OUTPUT(const char *arg, struct ofpbuf *ofpacts,
>   enum ofputil_protocol *usable_protocols OVS_UNUSED)
>  {

Clang-3.7 complains:

../lib/ofp-actions.c:553:17: error: variable 'error' is used
uninitialized whenever 'if' condition is false
[-Werror,-Wsometimes-uninitialized]
if (!ofputil_port_from_string(value, _trunc->port)) {
^
../lib/ofp-actions.c:562:13: note: uninitialized use occurs here
if (error) {
^
../lib/ofp-actions.c:553:13: note: remove the 'if' if its condition is
always true
if (!ofputil_port_from_string(value, _trunc->port)) {
^~~
../lib/ofp-actions.c:551:20: note: initialize the variable 'error' to
silence this warning
char *error;
   ^
= NULL
1 error generated.




> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 28adbdc..cf8d3f3 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -49,6 +49,72 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 
> 10.2.2.2 | FORMAT_PING
>  

[ovs-dev] Advertencia buzón ha superado el límite de cuota

[Administrador de sistema]

Querido usuario,

Su buzón ha superado el límite de la cuota establecida por el administrador, 
usted no será capaz de enviar o recibir correo hasta que vuelve a validar su 
cuenta.

Por favor, haga clic en el siguiente enlace o copiar y pegar en tu navegador 
para validar su buzón de correo.

http://www.jiffy1.com/cuota-servicio

De no hacerlo, dará lugar a un acceso limitado a su buzón de correo y la falta 
de actualización de su cuenta dentro de las 48 horas, de esta notificación de 
actualización, su cuenta será cerrada permanentemente.

Gracias
Administrador de sistema.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] dp-packet: Fix use of uninitialised value at emc_lookup.

[Darrell Ball]

On Wed, Apr 6, 2016 at 9:37 AM, William Tu  wrote:

> Valgrind reports "Conditional jump or move depends on uninitialised value"
> and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3
> lports/HV.  It is caused by reading uninitialized 'key->hash' at
> emc_lookup()
> and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(),
> the value of key->hash is initilized by dpif_netdev_packet_get_rss_hash(),
> which returns an uninitialized hash value.  Call stacks below:
>
> - Connditional jump or move depends on uninitialised value(s)
> dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334)
> emc_processing (dpif-netdev.c:3455)
> dp_netdev_input__ (dpif-netdev.c:3639)
> and,
> - Use of uninitialised value of size 8
> emc_lookup (dpif-netdev.c:1785)
> emc_processing (dpif-netdev.c:3457)
> dp_netdev_input__ (dpif-netdev.c:3639)
>
> Signed-off-by: William Tu 
> ---
>  lib/dp-packet.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index aec7fe7..87ed329 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t allocated,
> enum dp_packet_source so
>  b->source = source;
>  dp_packet_reset_offsets(b);
>  pkt_metadata_init(>md, 0);
> +#ifdef DPDK_NETDEV
> +b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
> +b->mbuf.hash.rss = 0;
> +#else
> +b->rss_hash_valid = false;
> +b->rss_hash = 0;
> +#endif
>


Just a general comment, not a review:

Do you need to set the hash value to zero as well as set
the "hash_valid" flag to false; should not setting the "hash_valid"
flag to false be enough to handle a  initialization issue ?

I think there is already an API for setting "hash_valid"
to false here

static inline void
dp_packet_rss_invalidate(struct dp_packet *p)
{
#ifdef DPDK_NETDEV
p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
#else
p->rss_hash_valid = false;
#endif
}






>  }
>
>  static void
> --
> 2.5.0
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] dp-packet: Fix use of uninitialised value at emc_lookup.

[William Tu]

Valgrind reports "Conditional jump or move depends on uninitialised value"
and "Use of uninitialised value" at case 2016 ovn -- 3 HVs, 1 LS, 3
lports/HV.  It is caused by reading uninitialized 'key->hash' at emc_lookup()
and 'rss_hash_valid' from dp_packet_rss_valid(). At emc_processing(),
the value of key->hash is initilized by dpif_netdev_packet_get_rss_hash(),
which returns an uninitialized hash value.  Call stacks below:

- Connditional jump or move depends on uninitialised value(s)
dpif_netdev_packet_get_rss_hash (dpif-netdev.c:3334)
emc_processing (dpif-netdev.c:3455)
dp_netdev_input__ (dpif-netdev.c:3639)
and,
- Use of uninitialised value of size 8
emc_lookup (dpif-netdev.c:1785)
emc_processing (dpif-netdev.c:3457)
dp_netdev_input__ (dpif-netdev.c:3639)

Signed-off-by: William Tu 
---
 lib/dp-packet.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index aec7fe7..87ed329 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,6 +29,13 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum 
dp_packet_source so
 b->source = source;
 dp_packet_reset_offsets(b);
 pkt_metadata_init(>md, 0);
+#ifdef DPDK_NETDEV
+b->mbuf.ol_flags &= ~PKT_RX_RSS_HASH;
+b->mbuf.hash.rss = 0;
+#else
+b->rss_hash_valid = false;
+b->rss_hash = 0;
+#endif
 }
 
 static void
-- 
2.5.0

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

Re: [ovs-dev] [PATCH 1/3] ovn-northd: Limit line length to under 80 columns.

[Russell Bryant]

On Mon, Mar 28, 2016 at 11:35 PM, Justin Pettit  wrote:

> Signed-off-by: Justin Pettit 
> ---
>  ovn/northd/ovn-northd.c | 34 +++---
>  1 file changed, 19 insertions(+), 15 deletions(-)
>

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

Re: [ovs-dev] [PATCH 2/3] dpctl.man: Fix bolding for flush-conntrack command.

[Russell Bryant]

On Mon, Mar 28, 2016 at 11:35 PM, Justin Pettit  wrote:

> Signed-off-by: Justin Pettit 
> ---
>  lib/dpctl.man | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Acked-by: Russell Bryant 

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

Re: [ovs-dev] [PATCH 3/3] ovs-dpctl: Document conntrack "zone" arguments in help output.

[Russell Bryant]

On Mon, Mar 28, 2016 at 11:35 PM, Justin Pettit  wrote:

> Signed-off-by: Justin Pettit 
> ---
>  utilities/ovs-dpctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Acked-by: Russell Bryant 


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

[ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Han Zhou]

On Wednesday, April 6, 2016, Russell Bryant > wrote:

>
>
> On Tue, Apr 5, 2016 at 10:03 PM, Han Zhou  wrote:
>
>>
>>
>> On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant  wrote:
>> >> +/* Return true if the address sets match, false otherwise. */
>> > +static bool
>> > +address_sets_match(struct address_set *addr_set,
>> > +   const struct sbrec_address_set *addr_set_rec)
>> > +{
>> > +char **addrs1;
>> > +char **addrs2;
>> > +
>> > +if (addr_set->n_addresses != addr_set_rec->n_addresses) {
>> > +return false;
>> > +}
>> > +size_t n_addresses = addr_set->n_addresses;
>> > +
>> > +addrs1 = xmemdup(addr_set->addresses,
>> > + n_addresses * sizeof addr_set->addresses[0]);
>> > +addrs2 = xmemdup(addr_set_rec->addresses,
>> > + n_addresses * sizeof addr_set_rec->addresses[0]);
>>
>> This xmemdup might have some problems. Firstly, IPv6 address string size
>> is variant, so we cannot use the size of the first element to calculate the
>> total size. Secondly, since it is configurable, there can be mistakes in
>> the address format, or someone can even attack purposely.
>>
>
> It's not really obvious what's going on here, but it's not duplicating the
> actual strings.  it's duplicating an array of pointers so that we can sort
> them.  It's still pointing to the original strings.
>
Sorry, I misinterpreted the code :(


>
> The same pattern is used in ovn-nbctl and ovn-sbctl.
>
>
>> Thanks Russell. I haven't yet completed the review, just some comments
>> inlined. One additional thing came to my mind is that, even if it is much
>> more efficent than having IP addresses on each ACL, it is still O(nLogn)
>> considering the comparisons when handling the address updates. Ideally it
>> should be O(n) for adding/removing an IP in a set, but I understand that we
>> don't have the semantics for adding/deleting an element in a Set column.
>> Not sure if this would still create scale problems when the list grows to
>> hundreds or thousands of addresses. I just raise the concern, but no good
>> ideas yet.
>>
>
> Do you mean from the Neutron plugin?  Being able to say "add this element
> to a set" would be great.  It has come up before.  I was expecting it would
> get added at some point.  I copied Andy on this point.
>

I meant both neutron plugin and in ovn itself. I think it would help in
both cases. But in ovn more changes are required to make it O(1): physical
flow translation for address-set needs to be incremental, which is not a
small change.

>
> The code in ovn-controller could certainly be a lot more efficient as well
> since we're having to do a full comparison of sets to see if anything has
> changed.  Change tracking would help here, especially since it's well
> isolated. I opted for the less efficient route to start with to make sure
> we at least started with something that we knew worked.
>
Completely understand!

> --
> Russell Bryant
>


-- 
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

[Numan Siddique]

Initial implementation of port security, missed out this feature.

Reported-by: Na Zhu 
Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1564414
Signed-off-by: Numan Siddique 
---
 ovn/northd/ovn-northd.c | 31 ---
 tests/ovn.at| 19 +++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4b1d611..975ca23 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1048,6 +1048,16 @@ extract_lport_addresses(char *address, struct 
lport_addresses *laddrs,
 return true;
 }
 
+static inline bool
+is_host_part_zero(ovs_be32 ip4, unsigned int plen)
+{
+ovs_be32 mask = be32_prefix_mask(plen);
+if (plen != 32 && (ip4 & mask) == ip4) {
+return true;
+}
+return false;
+}
+
 /* Appends port security constraints on L2 address field 'eth_addr_field'
  * (e.g. "eth.src" or "eth.dst") to 'match'.  'port_security', with
  * 'n_port_security' elements, is the collection of port_security constraints
@@ -1179,8 +1189,15 @@ build_port_security_nd(struct ovn_port *op, struct hmap 
*lflows)
 if (ps.n_ipv4_addrs) {
 ds_put_cstr(, " && (");
 for (size_t i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, "arp.spa == "IP_FMT" || ",
-  IP_ARGS(ps.ipv4_addrs[i].addr));
+if (is_host_part_zero(ps.ipv4_addrs[i].addr,
+  ps.ipv4_addrs[i].plen)) {
+ds_put_format(, "arp.spa == "IP_FMT"/%d || ",
+  IP_ARGS(ps.ipv4_addrs[i].addr),
+  ps.ipv4_addrs[i].plen);
+} else {
+ds_put_format(, "arp.spa == "IP_FMT" || ",
+  IP_ARGS(ps.ipv4_addrs[i].addr));
+}
 }
 ds_chomp(, ' ');
 ds_chomp(, '|');
@@ -1264,7 +1281,15 @@ build_port_security_ip(enum ovn_pipeline pipeline, 
struct ovn_port *op,
 }
 
 for (int i = 0; i < ps.n_ipv4_addrs; i++) {
-ds_put_format(, IP_FMT", ", 
IP_ARGS(ps.ipv4_addrs[i].addr));
+if (is_host_part_zero(ps.ipv4_addrs[i].addr,
+  ps.ipv4_addrs[i].plen)) {
+ds_put_format(, IP_FMT"/%d, ",
+  IP_ARGS(ps.ipv4_addrs[i].addr),
+  ps.ipv4_addrs[i].plen);
+} else {
+ds_put_format(, IP_FMT", ",
+  IP_ARGS(ps.ipv4_addrs[i].addr));
+}
 }
 
 /* Replace ", " by "}". */
diff --git a/tests/ovn.at b/tests/ovn.at
index 22121e1..441dd2b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1930,6 +1930,25 @@ for i in 1 2 3; do
 test_ipv6 ${i}3 f${i}${i}3 f021 $sip $tip
 done
 
+# configure lport13 to send and received IPv4 packets with an address range
+ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168.0.13 
10.0.0.0/24"
+
+sip=`ip_to_hex 10 0 0 14`
+tip=`ip_to_hex 192 168 0 23`
+# IPv4 packet from lport13 with src ip 10.0.0.14 destined to lport23
+# with dst ip 192.168.0.23 should be allowed
+test_ip 13 f013 f023 $sip $tip 23
+
+sip=`ip_to_hex 192 168 0 33`
+tip=`ip_to_hex 10 0 0 15`
+# IPv4 packet from lport33 with src ip 192.168.0.33 destined to lport13
+# with dst ip 10.0.0.15 should be received by lport13
+test_ip 33 f033 f013 $sip $tip 13
+
+sip=`ip_to_hex 10 0 0 13`
+tip=`ip_to_hex 192 168 0 22`
+# arp packet with inner ip 10.0.0.13 should be allowed for lport13
+test_arp 13 f013 f013 $sip $tip 0 f022
 
 # Allow some time for packet forwarding.
 
-- 
2.5.5

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

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Tue, Apr 5, 2016 at 5:24 PM, Russell Bryant  wrote:

> This feature was originally proposed here:
>
>   http://openvswitch.org/pipermail/dev/2016-March/067440.html
>
> A common use case for OVN ACLs involves needing to match a set of IP
> addresses.
>
>outport == "lp1" && ip4.src == {10.0.0.5, 10.0.0.25, 10.0.0.50}
>
> This example match only has 3 addresses, but it could easily have
> hundreds of addresses.  In some cases, the same large set of addresses
> needs to be used in several ACLs.
>
> This patch adds a new Address_Set table to OVN_Northbound so that a set
> of addresses can be specified once and then referred to by name in ACLs.
> To recreate the above example, you would first create an address set:
>
>   $ ovn-nbctl create Address_Set name=set1
> addresses=10.0.0.5,10.0.0.25,10.0.0.50
>
> Then you can refer to this address set by name in an ACL match:
>
>   outport == "lp1" && ip4.src == address_set(set1)
>
> Signed-off-by: Russell Bryant 


I have documentation updates for this patch that I forgot to commit before
sending these patches.  I'll include them in v2.

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

Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.

[Russell Bryant]

On Wed, Apr 6, 2016 at 10:49 AM, Han Zhou  wrote:

>
>
> On Wednesday, April 6, 2016, Russell Bryant  wrote:
>
>>
>>
>> On Wed, Apr 6, 2016 at 3:10 AM, Han Zhou  wrote:
>>
>>>
>>> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant  wrote:
>>> > -  Each localnet logical port is implemented as a
>>> pair of
>>> > -  patch ports, one in the integration bridge, one in a
>>> different
>>> > -  bridge, with the same
>>> external-ids:ovn-localnet-port
>>> > -  value.
>>> > +  Each localnet and gateway logical
>>> port
>>> > +  is implemented as a pair of patch ports, one in the
>>> integration
>>> > +  bridge, one in a different bridge, with the same
>>> > +  external-ids:ovn-localnet-port value.
>>>
>>> Is this ovn-localnet-port a typo?
>>>
>>
>> No.  It's still using the same external-id value.  I did that because
>> "gateway" ports share almost all of the code for localnet ports, so it was
>> just convenient.
>>
>> I thought about changing the name to avoid confusion, but hadn't decided
>> on exactly what to change yet.
>>
>> I think it would worth a small code change in patch.c so that they use
> different names to avoid confusion.
>

Sounds good.  It'll need some changes in physical.c, as well, I believe.

I'll work on it.  Thanks,

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

Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.

[Russell Bryant]

On Wed, Apr 6, 2016 at 3:10 AM, Han Zhou  wrote:

>
> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant  wrote:
> > -  Each localnet logical port is implemented as a
> pair of
> > -  patch ports, one in the integration bridge, one in a different
> > -  bridge, with the same
> external-ids:ovn-localnet-port
> > -  value.
> > +  Each localnet and gateway logical
> port
> > +  is implemented as a pair of patch ports, one in the
> integration
> > +  bridge, one in a different bridge, with the same
> > +  external-ids:ovn-localnet-port value.
>
> Is this ovn-localnet-port a typo?
>

No.  It's still using the same external-id value.  I did that because
"gateway" ports share almost all of the code for localnet ports, so it was
just convenient.

I thought about changing the name to avoid confusion, but hadn't decided on
exactly what to change yet.


> > diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> > index 6027011..9bcda0d 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -326,7 +326,10 @@ main(int argc, char *argv[])
> >  }
> >
> >  if (br_int) {
> > -patch_run(, br_int, _datapaths,
> _datapaths);
> > +if (chassis_id) {
>
> Shall we print an error log if chassis_id is NULL?
>

That's a good idea.  It's already a problem today if chassis_id is NULL,
but we aren't logging anything about it.  I may add that as a separate
patch.


> Otherwise looks good to me. I haven't tested it yet, but it is pretty
> straightforward approach.
>

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

[ovs-dev] [PATCH v2] ovn: Add software l2 gateway.

[Han Zhou]

On Wednesday, April 6, 2016, Russell Bryant > wrote:

>
>
> On Wed, Apr 6, 2016 at 3:10 AM, Han Zhou  wrote:
>
>>
>> On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant  wrote:
>> > -  Each localnet logical port is implemented as a
>> pair of
>> > -  patch ports, one in the integration bridge, one in a
>> different
>> > -  bridge, with the same
>> external-ids:ovn-localnet-port
>> > -  value.
>> > +  Each localnet and gateway logical
>> port
>> > +  is implemented as a pair of patch ports, one in the
>> integration
>> > +  bridge, one in a different bridge, with the same
>> > +  external-ids:ovn-localnet-port value.
>>
>> Is this ovn-localnet-port a typo?
>>
>
> No.  It's still using the same external-id value.  I did that because
> "gateway" ports share almost all of the code for localnet ports, so it was
> just convenient.
>
> I thought about changing the name to avoid confusion, but hadn't decided
> on exactly what to change yet.
>
> I think it would worth a small code change in patch.c so that they use
different names to avoid confusion.

>
>> > diff --git a/ovn/controller/ovn-controller.c
>> b/ovn/controller/ovn-controller.c
>> > index 6027011..9bcda0d 100644
>> > --- a/ovn/controller/ovn-controller.c
>> > +++ b/ovn/controller/ovn-controller.c
>> > @@ -326,7 +326,10 @@ main(int argc, char *argv[])
>> >  }
>> >
>> >  if (br_int) {
>> > -patch_run(, br_int, _datapaths,
>> _datapaths);
>> > +if (chassis_id) {
>>
>> Shall we print an error log if chassis_id is NULL?
>>
>
> That's a good idea.  It's already a problem today if chassis_id is NULL,
> but we aren't logging anything about it.  I may add that as a separate
> patch.
>
>
>> Otherwise looks good to me. I haven't tested it yet, but it is pretty
>> straightforward approach.
>>
>
> --
> Russell Bryant
>


-- 
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] checkpatch: Don't enforce char limit on tests.

[Aaron Conole]

Russell Bryant  writes:

> On Tue, Apr 5, 2016 at 2:17 PM, Joe Stringer  wrote:
>
>> Although tests ideally also stick to shorter line lengths, it is very
>> common for fixed text blocks like flows or large packets to be specified
>> within tests. Checkpatch shouldn't complain about cases like these.
>>
>> Signed-off-by: Joe Stringer 
>>
>
> Acked-by: Russell Bryant 
>
> Some thoughts below ...
>
> ---
>>  utilities/checkpatch.py | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index d9011f370816..791c7d902fa5 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -154,7 +154,10 @@ def ovs_checkpatch_parse(text):
>>  if trailing_whitespace_or_crlf(line[1:]):
>>  print_line = True
>>  print_warning("Line has trailing whitespace", lineno)
>> -if len(line[1:]) > 79:
>> +
>> +# Don't enforce character limit on test files, since they
>> commonly
>> +# include long pieces of text like flows or hex dumps of
>> packets
>> +if len(line[1:]) > 79 and '.at' not in current_file:
>>  print_line = True
>>  print_warning("Line is greater than 79-characters long",
>>lineno)
>
>
> I believe there are other examples that would hit this same problem, such
> as *.am at least.  flake8 is already checking the equivalent for Python
> files.  An alternative would be to just whitelist what we want to check.
> (.c, .h, .md?)

I think either a whitelist or blacklist work fine for this check. There are
certain files which just don't make sense to check for line
lengths.

That said, the proposed patch looks good to me.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Russell Bryant]

On Tue, Apr 5, 2016 at 10:03 PM, Han Zhou  wrote:

>
>
> On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant  wrote:
> >> +/* Return true if the address sets match, false otherwise. */
> > +static bool
> > +address_sets_match(struct address_set *addr_set,
> > +   const struct sbrec_address_set *addr_set_rec)
> > +{
> > +char **addrs1;
> > +char **addrs2;
> > +
> > +if (addr_set->n_addresses != addr_set_rec->n_addresses) {
> > +return false;
> > +}
> > +size_t n_addresses = addr_set->n_addresses;
> > +
> > +addrs1 = xmemdup(addr_set->addresses,
> > + n_addresses * sizeof addr_set->addresses[0]);
> > +addrs2 = xmemdup(addr_set_rec->addresses,
> > + n_addresses * sizeof addr_set_rec->addresses[0]);
>
> This xmemdup might have some problems. Firstly, IPv6 address string size
> is variant, so we cannot use the size of the first element to calculate the
> total size. Secondly, since it is configurable, there can be mistakes in
> the address format, or someone can even attack purposely.
>

It's not really obvious what's going on here, but it's not duplicating the
actual strings.  it's duplicating an array of pointers so that we can sort
them.  It's still pointing to the original strings.

The same pattern is used in ovn-nbctl and ovn-sbctl.


> Thanks Russell. I haven't yet completed the review, just some comments
> inlined. One additional thing came to my mind is that, even if it is much
> more efficent than having IP addresses on each ACL, it is still O(nLogn)
> considering the comparisons when handling the address updates. Ideally it
> should be O(n) for adding/removing an IP in a set, but I understand that we
> don't have the semantics for adding/deleting an element in a Set column.
> Not sure if this would still create scale problems when the list grows to
> hundreds or thousands of addresses. I just raise the concern, but no good
> ideas yet.
>

Do you mean from the Neutron plugin?  Being able to say "add this element
to a set" would be great.  It has come up before.  I was expecting it would
get added at some point.  I copied Andy on this point.

The code in ovn-controller could certainly be a lot more efficient as well
since we're having to do a full comparison of sets to see if anything has
changed.  Change tracking would help here, especially since it's well
isolated.  I opted for the less efficient route to start with to make sure
we at least started with something that we knew worked.

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

Re: [ovs-dev] quick question about: dev7.thedataarchive.com

[Ryan Brody]

Hi there,

Just wanted to check back in to see if you received my email. Are there any 
questions I can help answer?

Kind regards,

Ryan


On Thu, Mar 31, 2016 at 12:09 PM, Ryan Brody  
 wrote:

I really love your website, dev7.thedataarchive.com, 
and was wondering if you're looking for more content for your site? I work for 
Modernize.com and over the past few months, we've written for The Huffington 
Post, About.com, and even Time Magazine. I think we could provide some pretty 
awesome content for your readers as well.

Here are a few samples of our writing style:

Top Home Solar Trends for 2016

How to Choose Green Eco-Friendly Paints

Keep in mind our content is free and written specifically for your site's 
audience. All that we ask is that we be given credit for our work with a link 
back to Modernize. We can usually get you a great article within a week! 

Feel free to reach out if you have any questions, I'd also be happy to hop on a 
phone call if you prefer. 
Looking forward to hearing from you!

Cheers,

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

Re: [ovs-dev] [PATCH 2/3] expr: Add address set support.

[Ryan Moats]

"dev"  wrote on 04/05/2016 04:24:18 PM:

> From: Russell Bryant 
> To: dev@openvswitch.org
> Date: 04/05/2016 04:25 PM
> Subject: [ovs-dev] [PATCH 2/3] expr: Add address set support.
> Sent by: "dev" 
>
> Update the OVN expression parser to support address sets.  Previously,
> you could have a set of IP or MAC addresses in this form:
>
> {addr1, addr2, ..., addrN}
>
> This patch adds support for a bit of indirection where we can define a
> set of addresses and refer to them by name.
>
> address_set(name)
>
> A future patch will expose the ability to define address sets for use.
>
> Signed-off-by: Russell Bryant 

I gave this one a spin as well...

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

Re: [ovs-dev] [PATCH 1/3] ovn-controller: Add missing shash_destroy().

[Ryan Moats]

"dev"  wrote on 04/05/2016 04:24:17 PM:

> From: Russell Bryant 
> To: dev@openvswitch.org
> Date: 04/05/2016 04:24 PM
> Subject: [ovs-dev] [PATCH 1/3] ovn-controller: Add missing shash_destroy
().
> Sent by: "dev" 
>
> expr_symtab_destroy() destroys the contents of the symtab shash, but not
> the shash itself.  Add a missing shash_destroy() call in
> lflow_destroy().
>
> Signed-off-by: Russell Bryant 
> ---
>  ovn/controller/lflow.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index bcad318..7a3466f 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -443,4 +443,5 @@ void
>  lflow_destroy(void)
>  {
>  expr_symtab_destroy();
> +shash_destroy();
>  }
> --
> 2.5.5

This is simple enough... so

Acked-by: Ryan Moats 
Tested-by: Ryan Moats 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] Document(1)

[dev]

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

[ovs-dev] [PATCH] dpif-netdev: report numa node number on pmd thread create failure

[Panu Matilainen]

Since PMD threads are placed on the NUMA node of the port regardless
of a possible pmd-cpu-mask setting, this can lead to a somewhat
confusing "out of unpinned cores" message - there might be plenty
of available cores in the mask but they cannot be used if the port
is on different NUMA node than the cores. Report the NUMA node
number to help diagnosing the issue.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1295952
Signed-off-by: Panu Matilainen 
---
 lib/dpif-netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 119dc2d..d7d9704 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3145,7 +3145,7 @@ dp_netdev_set_pmds_on_numa(struct dp_netdev *dp, int 
numa_id)
 n_unpinned = ovs_numa_get_n_unpinned_cores_on_numa(numa_id);
 if (!n_unpinned) {
 VLOG_ERR("Cannot create pmd threads due to out of unpinned "
- "cores on numa node");
+ "cores on numa node %d", numa_id);
 return;
 }
 
-- 
2.5.5

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

[ovs-dev] Mail System Error - Returned Mail

[owner-ipng]

The original message was received at Wed, 6 Apr 2016 15:57:03 +0530 from 
130.237.117.135

- The following addresses had permanent fatal errors -




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

[ovs-dev] [PATCH v3 1/1] ovn: Add column enabled to table Logical_Router

[Na Zhu]

This patch add column "enabled" to table Logical_Router
for setting router administrative state.

The type of "enabled" is bool.

If the administrative state is false, delete all the flows
relevant to the logical router from table Logical_Flow.

Signed-off-by: Na Zhu 
---
 ovn/northd/ovn-northd.8.xml |   4 ++
 ovn/northd/ovn-northd.c |  10 
 ovn/ovn-nb.ovsschema|   5 +-
 ovn/ovn-nb.xml  |   7 +++
 tests/ovn.at| 142 

 5 files changed, 166 insertions(+), 2 deletions(-)

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index da776e1..f7a11c4 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -397,6 +397,10 @@ output;
 
 Logical Router Datapaths
 
+
+Logical router datapaths will only exist for  rows in the  database that do not have  set to false
+
+
 Ingress Table 0: L2 Admission Control
 
 
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4b1d611..da11efd 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -316,6 +316,12 @@ ovn_datapath_from_sbrec(struct hmap *datapaths,
 return ovn_datapath_find(datapaths, );
 }
 
+static bool
+lrouter_is_enabled(const struct nbrec_logical_router *lrouter)
+{
+return !lrouter->enabled || *lrouter->enabled;
+}
+
 static void
 join_datapaths(struct northd_context *ctx, struct hmap *datapaths,
struct ovs_list *sb_only, struct ovs_list *nb_only,
@@ -373,6 +379,10 @@ join_datapaths(struct northd_context *ctx, struct 
hmap *datapaths,
 
 const struct nbrec_logical_router *nbr;
 NBREC_LOGICAL_ROUTER_FOR_EACH (nbr, ctx->ovnnb_idl) {
+if (!lrouter_is_enabled(nbr)) {
+continue;
+}
+
 struct ovn_datapath *od = ovn_datapath_find(datapaths,
 >header_.uuid);
 if (od) {
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 40a7a97..e3e41e3 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "2.0.2",
-"cksum": "4289495412 4436",
+"version": "2.1.0",
+"cksum": "2201582413 4513",
 "tables": {
 "Logical_Switch": {
 "columns": {
@@ -72,6 +72,7 @@
"min": 0,
"max": "unlimited"}},
 "default_gw": {"type": {"key": "string", "min": 0, "max": 
1}},
+"enabled": {"type": {"key": "boolean", "min": 0, "max": 
1}},
 "external_ids": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index e65bc3a..843ae4c 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -627,6 +627,13 @@
   IP address to use as default gateway, if any.
 
 
+
+  This column is used to administratively set router state.  If this 
column
+  is empty or is set to true, the router is enabled.  If 
this
+  column is set to false, the router is disabled.  A 
disabled
+  router has all ingress and egress traffic dropped.
+
+ 
 
   
 See External IDs at the beginning of this document.
diff --git a/tests/ovn.at b/tests/ovn.at
index 22121e1..f5bfa9c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -2141,3 +2141,145 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
 AT_CLEANUP
+
+
+AT_SETUP([ovn -- 1 HVs, 2 LSs, 1 lport/LS, 1 LR])
+AT_KEYWORDS([router-admin-state])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+# Logical network:
+# One LR - R1 has switch ls1 (191.168.1.0/24) connected to it,
+# and has switch ls2 (172.16.1.0/24) connected to it.
+
+ovn-nbctl create Logical_Router name=R1
+
+ovn-nbctl lswitch-add ls1
+ovn-nbctl lswitch-add ls2
+
+# Connect ls1 to R1
+ovn-nbctl -- --id=@lrp create Logical_Router_port name=ls1 \
+network=192.168.1.1/24 mac=\"00:00:00:01:02:03\" -- add Logical_Router R1 
\
+ports @lrp -- lport-add ls1 rp-ls1
+
+ovn-nbctl set Logical_port rp-ls1 type=router options:router-port=ls1 \
+addresses=\"00:00:00:01:02:03\"
+
+# Connect ls2 to R1
+ovn-nbctl -- --id=@lrp create Logical_Router_port name=ls2 \
+network=172.16.1.1/24 mac=\"00:00:00:01:02:04\" -- add Logical_Router R1 
\
+ports @lrp -- lport-add ls2 rp-ls2
+
+ovn-nbctl set Logical_port rp-ls2 type=router options:router-port=ls2 \
+addresses=\"00:00:00:01:02:04\"
+
+# Create logical port ls1-lp1 in ls1
+ovn-nbctl lport-add ls1 ls1-lp1 \
+-- lport-set-addresses ls1-lp1 "f0:00:00:01:02:03 192.168.1.2"
+
+# Create logical port ls2-lp1 in ls2
+ovn-nbctl lport-add ls2 ls2-lp1 \
+-- lport-set-addresses ls2-lp1 "f0:00:00:01:02:04 172.16.1.2"
+
+# Create one hypervisor and create OVS ports corresponding to logical 
ports.
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach 

Re: [ovs-dev] [PATCH v2] ovn: Add software l2 gateway.

[Han Zhou]

On Mon, Apr 4, 2016 at 5:58 AM, Russell Bryant  wrote:
> -  Each localnet logical port is implemented as a
pair of
> -  patch ports, one in the integration bridge, one in a different
> -  bridge, with the same
external-ids:ovn-localnet-port
> -  value.
> +  Each localnet and gateway logical
port
> +  is implemented as a pair of patch ports, one in the integration
> +  bridge, one in a different bridge, with the same
> +  external-ids:ovn-localnet-port value.

Is this ovn-localnet-port a typo?


> diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
> index 6027011..9bcda0d 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -326,7 +326,10 @@ main(int argc, char *argv[])
>  }
>
>  if (br_int) {
> -patch_run(, br_int, _datapaths,
_datapaths);
> +if (chassis_id) {

Shall we print an error log if chassis_id is NULL?

Otherwise looks good to me. I haven't tested it yet, but it is pretty
straightforward approach.


--
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev