[ovs-dev] [PATCH] vtep/ovs-vtep: support multiple ovs-vtep processes which using the isolated logical switch

[nickcooper-zhangtonghao]

signed-off-by: nickcooper-zhangtong...@opencloud.tech 


The computer can run multiple ovs-vtep processes, but ovs-vtep processes share 
the “vtep_lsX” bridge when binding the physical-switch and logical-switch. The 
bridge created by logical-switch should be named  according to logical-switch.

 
--- vtep/ovs-vtep   2016-05-01 09:42:08.903583112 -0400
+++ vtep/ovs-vtep.new   2016-05-01 08:15:19.352586815 -0400
@@ -81,11 +81,11 @@ def unixctl_exit(conn, unused_argv, unus


 class Logical_Switch(object):
-def __init__(self, ls_name):
+def __init__(self, ls_name, ps_name):
 global ls_count
 self.name = ls_name
 ls_count += 1
-self.short_name = "vtep_ls" + str(ls_count)
+self.short_name = ps_name + "_vtep_ls" + str(ls_count)
 vlog.info("creating lswitch %s (%s)" % (self.name, self.short_name))
 self.ports = {}
 self.tunnels = {}
@@ -583,7 +583,7 @@ def handle_physical():
 for b in binding_set:
 vlan, ls_name = b.split()
 if ls_name not in Lswitches:
-Lswitches[ls_name] = Logical_Switch(ls_name)
+Lswitches[ls_name] = Logical_Switch(ls_name, ps_name)

 binding = "%s-%s" % (vlan, pp_name)
 ls = Lswitches[ls_name]






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

[ovs-dev] [PATCH] vtep/ovs-vtep: support multiple ovs-vtep processes which using the isolated logical-switch

[nickcooper-zhangtonghao]

Hi all,
The computer can run multiple ovs-vtep processes, but ovs-vtep 
processes share the “vtep_lsX” bridge when binding the physical-switch and 
logical-switch. The bridge created by logical-switch should be named  according 
to logical-switch.

signed-off-by: nickcooper-zhangtonghao 

--- vtep/ovs-vtep   2016-05-01 09:42:08.903583112 -0400
+++ vtep/ovs-vtep.new   2016-05-01 08:15:19.352586815 -0400
@@ -81,11 +81,11 @@ def unixctl_exit(conn, unused_argv, unus


 class Logical_Switch(object):
-def __init__(self, ls_name):
+def __init__(self, ls_name, ps_name):
 global ls_count
 self.name = ls_name
 ls_count += 1
-self.short_name = "vtep_ls" + str(ls_count)
+self.short_name = ps_name + "_vtep_ls" + str(ls_count)
 vlog.info("creating lswitch %s (%s)" % (self.name, self.short_name))
 self.ports = {}
 self.tunnels = {}
@@ -583,7 +583,7 @@ def handle_physical():
 for b in binding_set:
 vlan, ls_name = b.split()
 if ls_name not in Lswitches:
-Lswitches[ls_name] = Logical_Switch(ls_name)
+Lswitches[ls_name] = Logical_Switch(ls_name, ps_name)

 binding = "%s-%s" % (vlan, pp_name)
 ls = Lswitches[ls_name]

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

Re: [ovs-dev] [PATCH] vtep/ovs-vtep: support multiple ovs-vtep processes which using the isolated logical-switch

[nickcooper-zhangtonghao]

Hi ,
I have changed the commit message in my github repository. Thank 
Darrell Ball for your OVN gateway testing.

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

Re: [ovs-dev] [PATCH] vtep/ovs-vtep: support multiple ovs-vtep processes

[nickcooper-zhangtonghao]

Hi Ben,
Thank you for your advice. In general case, we run an ovs-vtep process 
on the hypervisor. 
In our network virtualization solution, we want to run multiple copies of the 
vtep simulator - ovs-vtep for the different network, 
such as the data/management/storage network.

Start the VTEP emulator:
ovs-vtep --detach mgmt-network
ovs-vtep --detach stor-network
ovs-vtep --detach data-network

Bind the logical switch to a port:
vtep-ctl bind-ls mgmt-network vnet0 0 ls0
vtep-ctl bind-ls stor-network vnet1 0 ls1
vtep-ctl bind-ls data-network vnet2 0 ls2


In fact, the vnet0/vnet1/vnet2 have been connected to the “vtep_ls1” bridge. 
So, the VMs of the different physical switches have been connected. 
This is arguably an ovs-vtep bug. I fixed this bug by including the physical 
switch name as part of the logical switch name.  
Darrell Ball did an initial test using the OVN gateway test case and did not 
see any issues with the proposed diff.
I have submited a pull request. https://github.com/openvswitch/ovs/pull/126 
<https://github.com/openvswitch/ovs/pull/126>



Thanks，
nickcooper-zhangtonghao.


> On May 18, 2016, at 10:26 AM, Ben Pfaff  wrote:
> 
> On Sun, May 01, 2016 at 01:44:26PM +0800, nickcooper-zhangtonghao wrote:
>> signed-off-by: nickcooper-zhangtong...@opencloud.tech 
>> <mailto:nickcooper-zhangtong...@opencloud.tech>
>> 
>> The computer can run multiple ovs-vtep processes, but ovs-vtep processes 
>> share the “vtep_lsX” bridge when binding the physical-switch and 
>> logical-switch. The bridge created by logical-switch should be named  
>> according to logical-switch.
> 
> Hi Nick, thanks for submitting a patch.
> 
> You'll need to resubmit it for a number of reasons.
> 
> Your return email address was totally wrong and definitely not usable.
> I think I've corrected it; we'll see if my reply bounces.
> 
> Please use a short (<75 character) title.
> 
> Please limit lines in the commit message to 75 characters or so.
> 
> "Signed-off-by" goes at the end of the message and it should be
> formatted as:
>Signed-off-by: Your Real Name 
> not as you did it.
> 
> Thanks,
> 
> Ben.

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

[ovs-dev] [PATCH 2/4] ovn: ovn-northd, the implementation of icmp4 reject actions

[nickcooper-zhangtonghao]

ovn: the implementation of icmp4 reject actions.

ACLs support the "icmp-reject" actions. Adds a row with the icmp4-reject 
contents to the Logical_Flow table.

Signed-off-by: nickcooper-zhangtonghao 


diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 6712d21..78679b1 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -1625,11 +1625,42 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows)
   acl->priority + OVN_ACL_PRI_OFFSET,
   acl->match, "drop;");
 } else if (!strcmp(acl->action, "reject")) {
-/* xxx Need to support "reject". */
-VLOG_INFO("reject is not a supported action");
+
+/* ACLs support the "icmp-reject" actions.
+ * icmp-net-unreachable
+ * icmp-host-unreachable
+ * icmp-proto-unreachable
+ * icmp-port-unreachable
+ * icmp-net-prohibited
+ * icmp-host-prohibited
+ * icmp-admin-prohibited */
+
+char reject_action[200];
+uint8_t icmp_type = 3, icmp_code = 0;
+
+if (!strcmp(acl->reject_action, "icmp-net-unreachable")) {
+icmp_code = 0;
+} else if (!strcmp(acl->reject_action, "icmp-host-unreachable")) {
+icmp_code = 1;
+} else if (!strcmp(acl->reject_action, "icmp-proto-unreachable")) {
+icmp_code = 2;
+} else if (!strcmp(acl->reject_action, "icmp-port-unreachable")) {
+icmp_code = 3;
+} else if (!strcmp(acl->reject_action, "icmp-net-prohibited")) {
+icmp_code = 9;
+} else if (!strcmp(acl->reject_action, "icmp-host-prohibited")) {
+icmp_code = 10;
+} else if (!strcmp(acl->reject_action, "icmp-admin-prohibited")) {
+icmp_code = 13;
+}
+
+sprintf(reject_action, "icmp4 { eth.src<->eth.dst; 
ip4.src<->ip4.dst;"
+"icmp4.type = %d; icmp4.code = %d; outport = inport; 
inport = \"\";output;};",
+icmp_type, icmp_code);
+
 ovn_lflow_add(lflows, od, stage,
   acl->priority + OVN_ACL_PRI_OFFSET,
-  acl->match, "drop;");
+  acl->match, reject_action);
 }
 }
 }





thanks,
nick




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

[ovs-dev] [PATCH 1/4] ovn: ovn-nbctl, the implementation of icmp4 reject actions

[nickcooper-zhangtonghao]

Hi,

Now that some reject functions have been implemented and tested，other 
functions(e.g. TCP RST) need perfect!


ovn: the implementation of icmp4 reject actions.

It support icmp4 reject (e.g. icmp-net-unreachable, icmp-host-prohibited, 
tcp-reset,
icmp-admin-prohibited, icmp-port-unreachable, icmp-net-prohibited, 
icmp-host-unreachable,
and icmp-proto-unreachable). The icmp-net-unreachable is default. The "TCP RST” 
function
will be completed soon. Reject action support only "from-lport" direction. In 
general, 
considering performance requirements, it might make sense to support only 
“from-lport” direction.

Signed-off-by: nickcooper-zhangtonghao 


diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 25916da..55cc1da 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -302,7 +302,7 @@ Logical switch commands:\n\
   ls-list   print the names of all logical switches\n\
 \n\
 ACL commands:\n\
-  acl-add SWITCH DIRECTION PRIORITY MATCH ACTION [log]\n\
+  acl-add SWITCH DIRECTION PRIORITY MATCH ACTION [REJECT-ACTION] [log]\n\
 add an ACL to SWITCH\n\
   acl-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\
 remove ACLs from SWITCH\n\
@@ -1082,9 +1082,11 @@ nbctl_acl_list(struct ctl_context *ctx)

 for (i = 0; i < ls->n_acls; i++) {
 const struct nbrec_acl *acl = acls[i];
-ds_put_format(&ctx->output, "%10s %5"PRId64" (%s) %s%s\n",
+ds_put_format(&ctx->output, "%10s %5"PRId64" (%s) %s %s %s\n",
   acl->direction, acl->priority,
-  acl->match, acl->action, acl->log ? " log" : "");
+  acl->match, acl->action,
+  strcmp(acl->action, "reject") ? "" : acl->reject_action,
+  acl->log ? "log" : "");
 }

 free(acls);
@@ -1120,6 +1122,7 @@ nbctl_acl_add(struct ctl_context *ctx)
 {
 const struct nbrec_logical_switch *ls;
 const char *action = ctx->argv[5];
+const char *reject_action = ctx->argv[6];

 ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);

@@ -1133,6 +1136,35 @@ nbctl_acl_add(struct ctl_context *ctx)
   "\"drop\", and \"reject\"", action);
 return;
 }
+
+/* Validate reject action. */
+if (strcmp(action, "reject")) {
+reject_action = "";
+} else {
+if (strcmp(direction, "from-lport"))
+ctl_fatal("%s: reject action support only \"from-lport\" 
direction.",
+direction);
+
+if (ctx->argc == 6) {
+reject_action = "icmp-net-unreachable";
+} else if (ctx->argc == 7) {
+if (strcmp(reject_action, "icmp-net-unreachable")
+&& strcmp(reject_action, "icmp-host-unreachable")
+&& strcmp(reject_action, "icmp-proto-unreachable")
+&& strcmp(reject_action, "icmp-port-unreachable")
+&& strcmp(reject_action, "icmp-net-prohibited")
+&& strcmp(reject_action, "icmp-host-prohibited")
+&& strcmp(reject_action, "icmp-admin-prohibited")
+&& strcmp(reject_action, "tcp-reset")) {
+ctl_fatal("%s: reject action must be one of 
\"icmp-net-unreachable\", "
+"\"icmp-host-unreachable\", 
\"icmp-proto-unreachable\", "
+"\"icmp-port-unreachable\", \"icmp-net-prohibited\", "
+"\"icmp-host-prohibited\", \"icmp-admin-prohibited\", "
+  "and \"tcp-reset\"", reject_action);
+return;
+}
+}
+}

 /* Create the acl. */
 struct nbrec_acl *acl = nbrec_acl_insert(ctx->txn);
@@ -1140,6 +1172,7 @@ nbctl_acl_add(struct ctl_context *ctx)
 nbrec_acl_set_direction(acl, direction);
 nbrec_acl_set_match(acl, ctx->argv[4]);
 nbrec_acl_set_action(acl, action);
+nbrec_acl_set_reject_action(acl, reject_action);
 if (shash_find(&ctx->options, "--log") != NULL) {
 nbrec_acl_set_log(acl, true);
 }
@@ -2139,7 +2172,7 @@ static const struct ctl_command_syntax nbctl_commands[] = 
{
 { "ls-list", 0, 0, "", NULL, nbctl_ls_list, NULL, "", RO },

 /* acl commands. */
-{ "acl-add", 5, 5, "SWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
+{ "acl-add", 5, 6, "SWITCH DIRECTION PRIORITY MATCH ACTION 
[REJECT-ACTIONS]", NULL,
   nbctl_acl_add, NULL, "--log", RW },
 { "acl-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL,
   nbctl_acl_del, NULL, "", RW },





thanks,
nick






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

[ovs-dev] [PATCH 4/4] ovn: ovn-contrller, the implementation of icmp4 reject actions

[nickcooper-zhangtonghao]

ovn: the implementation of icmp4 reject actions.

Update the ovn manpage and ovn-nb.ovsschema for icmp4 reject actions.

Signed-off-by: nickcooper-zhangtonghao 


diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 460d5bd..9842664 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.0.0",
-"cksum": "849073644 7576",
+"cksum": "2026671360 7919",
 "tables": {
 "Logical_Switch": {
 "columns": {
@@ -87,6 +87,10 @@
 "match": {"type": "string"},
 "action": {"type": {"key": {"type": "string",
 "enum": ["set", ["allow", 
"allow-related", "drop", "reject"]]}}},
+"reject_action": {"type": {"key": {"type": "string",
+"enum": ["set", 
["icmp-net-unreachable", "icmp-host-unreachable",
+   "icmp-proto-unreachable", 
"icmp-port-unreachable", "icmp-net-prohibited",
+   "icmp-host-prohibited", 
"icmp-admin-prohibited", "tcp-reset", ""]]}}},
 "log": {"type": "boolean"},
 "external_ids": {
 "type": {"key": "string", "value": "string”,



diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index e571eeb..70d20da 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -681,13 +681,51 @@
 

 
-  reject: Drop the packet, replying with a RST for TCP or
-  ICMP unreachable message for other IP-based protocols.
-  Not implemented--currently treated as drop
+  reject: Reject the packet, replying with a RST for TCP 
or
+  ICMP unreachable message for other IP-based protocols. Reject action 
support only from-lport direction.
 
   
 

+
+  The action to take when the reject ACL rule matches:
+  
+
+  icmp-net-unreachable: ICMP network unreachable 
(default).
+
+
+
+  icmp-host-unreachable: ICMP host unreachable.
+
+
+
+  icmp-proto-unreachable: ICMP protocol unreachable.
+
+
+
+  icmp-port-unreachable: ICMP port unreachable.
+
+
+
+  icmp-net-prohibited: ICMP network prohibited.
+
+
+
+  icmp-host-prohibited: ICMP host prohibited.
+
+
+
+  icmp-admin-prohibited: ICMP administratively prohibited.
+
+
+
+  tcp-reset: TCP RST packet.
+  Not implemented--currently treated as 
icmp-net-unreachable
+
+
+  
+
+
 
   
 If set to true, packets that match the ACL will trigger a


diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 7b45bbb..73e9799 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1251,10 +1251,6 @@
 icmp4.code = 1 (host unreachable)
   

-  
-Details TBD.
-  
-
   Prerequisite: ip4
 




thanks,
nick







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

[ovs-dev] [PATCH 3/4] ovn: ovn-contrller, the implementation of icmp4 reject actions

[nickcooper-zhangtonghao]

ovn: the implementation of icmp4 reject actions.

The ovn-contoller will process the packet-in data packet and compose an ICMP4 
packet，
whose icmp4 data contains the IP header and first 8 bytes of original 
datagram's data.

Signed-off-by: nickcooper-zhangtonghao 


diff --git a/lib/packets.c b/lib/packets.c
index a27264c..de90c9b 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -1251,6 +1251,9 @@ packet_format_tcp_flags(struct ds *s, uint16_t tcp_flags)
 #define ARP_PACKET_SIZE  (2 + ETH_HEADER_LEN + VLAN_HEADER_LEN + \
   ARP_ETH_HEADER_LEN)

+#define ICMP4_PACKET_SIZE  (2 + ETH_HEADER_LEN + VLAN_HEADER_LEN + \
+IP_HEADER_LEN + ICMP_HEADER_LEN)
+
 /* Clears 'b' and replaces its contents by an ARP frame with the specified
  * 'arp_op', 'arp_sha', 'arp_tha', 'arp_spa', and 'arp_tpa'.  The outer
  * Ethernet frame is initialized with Ethernet source 'arp_sha' and destination
@@ -1299,6 +1302,24 @@ compose_arp__(struct dp_packet *b)
 dp_packet_set_l3(b, arp);
 }

+void
+compose_icmp4__(struct dp_packet *b)
+{
+dp_packet_clear(b);
+dp_packet_prealloc_tailroom(b, ICMP4_PACKET_SIZE);
+dp_packet_reserve(b, 2 + VLAN_HEADER_LEN);
+
+struct eth_header *eth = dp_packet_put_zeros(b, sizeof *eth);
+eth->eth_type = htons(ETH_TYPE_IP);
+
+struct ip_header *ip4 = dp_packet_put_zeros(b, sizeof *ip4);
+struct icmp_header *icmp4 = dp_packet_put_zeros(b, sizeof *icmp4);
+
+dp_packet_reset_offsets(b);
+dp_packet_set_l4(b, icmp4);
+dp_packet_set_l3(b, ip4);
+}
+
 /* This function expect packet with ethernet header with correct
  * l3 pointer set. */


diff --git a/lib/packets.h b/lib/packets.h
index 077ccfa..adfb02b 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -1067,6 +1067,7 @@ void compose_arp(struct dp_packet *, uint16_t arp_op,
  const struct eth_addr arp_sha,
  const struct eth_addr arp_tha, bool broadcast,
  ovs_be32 arp_spa, ovs_be32 arp_tpa);
+void compose_icmp4__(struct dp_packet *);
 void compose_nd(struct dp_packet *, const struct eth_addr eth_src,
 struct in6_addr *, struct in6_addr *);
 void compose_na(struct dp_packet *,


diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 1370301..3556409 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -72,6 +72,10 @@ static void send_garp_run(const struct ovsrec_bridge *,
 static void pinctrl_handle_na(const struct flow *ip_flow,
   const struct match *md,
   struct ofpbuf *userdata);
+static void pinctrl_handle_icmp4(struct dp_packet *pkt_in,
+const struct flow *ip_flow,
+const struct match *md,
+struct ofpbuf *userdata);
 static void reload_metadata(struct ofpbuf *ofpacts,
 const struct match *md);

@@ -412,6 +416,10 @@ process_packet_in(const struct ofp_header *msg)
 case ACTION_OPCODE_NA:
 pinctrl_handle_na(&headers, &pin.flow_metadata, &userdata);
 break;
+
+case ACTION_OPCODE_ICMP4:
+pinctrl_handle_icmp4(&packet, &headers, &pin.flow_metadata, &userdata);
+break;

 default:
 VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32,
@@ -1003,3 +1011,95 @@ exit:
 dp_packet_uninit(&packet);
 ofpbuf_uninit(&ofpacts);
 }
+
+static void
+pinctrl_handle_icmp4(struct dp_packet *pkt_in,
+const struct flow *ip_flow,
+const struct match *md,
+struct ofpbuf *userdata)
+{
+/* This action only works for ICMP packets, and the switch should only send
+ * us ICMP packets this way, but check here just to be sure. */
+if (ip_flow->dl_type != htons(ETH_TYPE_IP) ||
+ip_flow->nw_proto != IPPROTO_ICMP) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(&rl, "ICMP4 action on non-IP packet (Ethertype %"PRIx16") 
and (IP Protocol %"PRIx16")",
+ ntohs(ip_flow->dl_type), ntohs(ip_flow->nw_proto));
+return;
+}
+
+/* Compose an ICMP4 packet. */
+struct icmp_header *icmp4;
+struct ip_header *ip4;
+struct eth_header *eth;
+uint64_t packet_stub[128 / 8];
+struct dp_packet pkt_out;
+
+dp_packet_use_stub(&pkt_out, packet_stub, sizeof packet_stub);
+compose_icmp4__(&pkt_out);
+
+eth = dp_packet_l2(&pkt_out);
+eth->eth_dst = ip_flow->dl_dst;
+eth->eth_src = ip_flow->dl_src;
+
+ip4 = dp_packet_l3(&pkt_out);
+ip4->ip_ihl_ver = IP_IHL_VER(5, 4);
+ip4->ip_tos = ip_flow->nw_tos;
+ip4->ip_tot_len = htons(sizeof *ip4 + sizeof *icmp4 + sizeof 

[ovs-dev] [PATCH v2] ovn: the implementation of icmp4 reject actions.

[nickcooper-zhangtonghao]

Hi Ryan,
Thank you for your advice. The controller will drop the icmp4 packet whose 
eth.src or eth.dst is
broadcast/multicast address and the type of icmp4 is “destination unreachable”. 
That may reduce 
the broadcast storm attack(e.g. icmp flood, dhcp flood). I write the rate 
limiting of icmp4 into 
my to-do list. Thanks.

ovn: the implementation of icmp4 reject actions.

It support icmp4 reject (e.g. icmp-net-unreachable, icmp-host-prohibited, 
tcp-reset,
icmp-admin-prohibited, icmp-port-unreachable, icmp-net-prohibited, 
icmp-host-unreachable,
and icmp-proto-unreachable). The icmp-net-unreachable is default. The "TCP RST" 
function
will be completed soon. Reject action support only "from-lport" direction. In 
general,
considering performance requirements, it might make sense to support only 
"from-lport" direction.

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/dp-packet.h   |  16 +++
 lib/packets.c |  21 +
 lib/packets.h |   1 +
 ovn/controller/pinctrl.c  | 113 ++
 ovn/lib/actions.c |   4 +-
 ovn/lib/actions.h |   6 +++
 ovn/northd/ovn-northd.c   |  37 +--
 ovn/ovn-nb.ovsschema  |   6 ++-
 ovn/ovn-nb.xml|  44 --
 ovn/ovn-sb.xml|   4 --
 ovn/utilities/ovn-nbctl.c |  41 +++--
 11 files changed, 277 insertions(+), 16 deletions(-)

diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 7c1e637..a6df36f 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -318,6 +318,15 @@ dp_packet_set_l3(struct dp_packet *b, void *l3)
 b->l3_ofs = l3 ? (char *) l3 - (char *) dp_packet_data(b) : UINT16_MAX;
 }
 
+static inline size_t
+dp_packet_l3_size(const struct dp_packet *b)
+{
+return b->l3_ofs != UINT16_MAX
+? (const char *)dp_packet_tail(b) - (const char *)dp_packet_l3(b)
+- dp_packet_l2_pad_size(b)
+: 0;
+}
+
 static inline void *
 dp_packet_l4(const struct dp_packet *b)
 {
@@ -342,6 +351,13 @@ dp_packet_l4_size(const struct dp_packet *b)
 }
 
 static inline const void *
+dp_packet_get_ip4_payload(const struct dp_packet *b)
+{
+return OVS_LIKELY(dp_packet_l3_size(b) >= IP_HEADER_LEN)
+? (const char *)dp_packet_l3(b) + IP_HEADER_LEN : NULL;
+}
+
+static inline const void *
 dp_packet_get_tcp_payload(const struct dp_packet *b)
 {
 size_t l4_size = dp_packet_l4_size(b);
diff --git a/lib/packets.c b/lib/packets.c
index a27264c..de90c9b 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -1251,6 +1251,9 @@ packet_format_tcp_flags(struct ds *s, uint16_t tcp_flags)
 #define ARP_PACKET_SIZE  (2 + ETH_HEADER_LEN + VLAN_HEADER_LEN + \
   ARP_ETH_HEADER_LEN)
 
+#define ICMP4_PACKET_SIZE  (2 + ETH_HEADER_LEN + VLAN_HEADER_LEN + \
+IP_HEADER_LEN + ICMP_HEADER_LEN)
+
 /* Clears 'b' and replaces its contents by an ARP frame with the specified
  * 'arp_op', 'arp_sha', 'arp_tha', 'arp_spa', and 'arp_tpa'.  The outer
  * Ethernet frame is initialized with Ethernet source 'arp_sha' and destination
@@ -1299,6 +1302,24 @@ compose_arp__(struct dp_packet *b)
 dp_packet_set_l3(b, arp);
 }
 
+void
+compose_icmp4__(struct dp_packet *b)
+{
+dp_packet_clear(b);
+dp_packet_prealloc_tailroom(b, ICMP4_PACKET_SIZE);
+dp_packet_reserve(b, 2 + VLAN_HEADER_LEN);
+
+struct eth_header *eth = dp_packet_put_zeros(b, sizeof *eth);
+eth->eth_type = htons(ETH_TYPE_IP);
+
+struct ip_header *ip4 = dp_packet_put_zeros(b, sizeof *ip4);
+struct icmp_header *icmp4 = dp_packet_put_zeros(b, sizeof *icmp4);
+
+dp_packet_reset_offsets(b);
+dp_packet_set_l4(b, icmp4);
+dp_packet_set_l3(b, ip4);
+}
+
 /* This function expect packet with ethernet header with correct
  * l3 pointer set. */
 static void *
diff --git a/lib/packets.h b/lib/packets.h
index 077ccfa..adfb02b 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -1067,6 +1067,7 @@ void compose_arp(struct dp_packet *, uint16_t arp_op,
  const struct eth_addr arp_sha,
  const struct eth_addr arp_tha, bool broadcast,
  ovs_be32 arp_spa, ovs_be32 arp_tpa);
+void compose_icmp4__(struct dp_packet *);
 void compose_nd(struct dp_packet *, const struct eth_addr eth_src,
 struct in6_addr *, struct in6_addr *);
 void compose_na(struct dp_packet *,
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 1370301..7893872 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -72,6 +72,10 @@ static void send_garp_run(const struct ovsrec_bridge *,
 static void pinctrl_handle_na(const struct flow *ip_flow,
   const struct match *md,
   struct ofpbuf *userdata);
+static void pinctrl_handle_icmp4(struct dp_packet *pkt_in,
+   

[ovs-dev] [PATCH] ovn: will not create patch port whose peer named "".

[nickcooper-zhangtonghao]

Hi,
When linking the logical switch to the logical router, I find some WARN info in 
the ovs-vswitch log file.
I think it is not necessary to create the logical patch port whose peer named 
“”.

commands:
ovn-nbctl lr-add lr0
ovn-nbctl lrp-add lr0 lr0-p0 00:11:22:33:44:55 192.168.100.10/24
ovn-nbctl ls-add ls0
ovn-nbctl lsp-add ls0 ls0-p0
ovn-nbctl set Logical_switch_port ls0-p0 type=router 
options:router-port=lr0-p0

ovs-vswitch.log
23 2016-07-20T09:00:00.309Z|00022|bridge|INFO|bridge br-int: added 
interface patch-lr0-p0-to- on port 1
24 2016-07-20T09:00:00.513Z|00023|bridge|INFO|bridge br-int: added 
interface patch-ls0-p0-to-lr0-p0 on port 2
25 2016-07-20T09:00:00.513Z|00024|bridge|INFO|bridge br-int: added 
interface patch-lr0-p0-to-ls0-p0 on port 3
26 2016-07-20T09:00:00.514Z|00025|dpif|WARN|system@ovs-system: failed 
to query port patch-lr0-p0-to-: Invalid argument


ovn: will not create patch port whose peer named “”.

It is not necessary to create the logical patch port whose peer named "".

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/patch.c | 4 
 tutorial/ovs-sandbox   | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 52d9e8d..ce22317 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -353,6 +353,10 @@ add_logical_patch_ports(struct controller_ctx *ctx,
 continue;
 }
 
+if (!strcmp(peer, "")) {
+continue;
+}
+
 char *src_name = patch_port_name(local, peer);
 char *dst_name = patch_port_name(peer, local);
 create_patch_port(ctx, "ovn-logical-patch-port", local,
-- 
1.8.3.1



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

[ovs-dev] [PATCH] ovn: remove the dead code.

[nickcooper-zhangtonghao]

It is not necessary to assign 0 to ip_csum.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/pinctrl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 7893872..b35576a 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -349,7 +349,6 @@ pinctrl_handle_put_dhcp_opts(
 struct ip_header *out_ip = dp_packet_l3(&pkt_out);
 out_ip->ip_tot_len = htons(pkt_out.l4_ofs - pkt_out.l3_ofs + new_l4_size);
 udp->udp_csum = 0;
-out_ip->ip_csum = 0;
 out_ip->ip_csum = csum(out_ip, sizeof *out_ip);
 
 pin->packet = dp_packet_data(&pkt_out);
-- 
1.8.3.1




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

[ovs-dev] [PATCH] ovn: will not create patch port whose peer named "".

[nickcooper-zhangtonghao]

Hi,
When linking the logical switch to the logical router, I find some WARN info in 
the ovs-vswitch log file.
I think it is not necessary to create the logical patch port whose peer named 
"".

commands:
ovn-nbctl lr-add lr0
ovn-nbctl lrp-add lr0 lr0-p0 00:11:22:33:44:55 192.168.100.10/24
ovn-nbctl ls-add ls0
ovn-nbctl lsp-add ls0 ls0-p0
ovn-nbctl set Logical_switch_port ls0-p0 type=router options:router-port=lr0-p0

ovs-vswitch.log
23 2016-07-20T09:00:00.309Z|00022|bridge|INFO|bridge br-int: added interface 
patch-lr0-p0-to- on port 1
24 2016-07-20T09:00:00.513Z|00023|bridge|INFO|bridge br-int: added interface 
patch-ls0-p0-to-lr0-p0 on port 2
25 2016-07-20T09:00:00.513Z|00024|bridge|INFO|bridge br-int: added interface 
patch-lr0-p0-to-ls0-p0 on port 3
26 2016-07-20T09:00:00.514Z|00025|dpif|WARN|system@ovs-system: failed to query 
port patch-lr0-p0-to-: Invalid argument

ovn: will not create patch port whose peer named "".

It is not necessary to create the logical patch port whose peer named "".

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/patch.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 52d9e8d..ce22317 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -353,6 +353,10 @@ add_logical_patch_ports(struct controller_ctx *ctx,
 continue;
 }
 
+if (!strcmp(peer, "")) {
+continue;
+}
+
 char *src_name = patch_port_name(local, peer);
 char *dst_name = patch_port_name(peer, local);
 create_patch_port(ctx, "ovn-logical-patch-port", local,
-- 
1.8.3.1



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

[ovs-dev] [PATCH] ovn: ovn-northd and von-controller take a high amount of CPU time.

[nickcooper-zhangtonghao]

Hi all,
When setting the logical switch port as the peer of logical router,
I found a large number of ovn-northd log. The process of ovn-northd write 
quickly the log
all the time. The worse thing that the process of ovn-northd and von-controller 
take a high amount of CPU time. I think this is a bug， undoubtedly.
The path I provide may fix the bug.

commands:
ovn-nbctl ls-add ls0
ovn-nbctl lsp-add ls0 ls0-p0

ovn-nbctl lr-add lr0
ovn-nbctl lrp-add lr0 lr0-p0 00:11:22:33:44:55 192.168.100.10/24 peer=ls0-p0



When setting the logical switch port as the peer of logical router,
the process of ovn-northd write quickly the log and take a high amount of CPU 
time.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/northd/ovn-northd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 78679b1..4e83ad1 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2634,7 +2634,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
  * should be on peer's outport. */
 if (op->nbr->peer) {
 struct ovn_port *peer = ovn_port_find(ports, op->nbr->peer);
-if (!peer) {
+if (!peer || (peer && peer->nbs)) {
 continue;
 }
 
-- 
1.8.3.1



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

Re: [ovs-dev] [PATCH 1/3] ovn-northd: Only peer router ports to other router ports.

[nickcooper-zhangtonghao]

>> A router port's "peer", if set, must point to another router port, but the
>> code as written also accepted switch ports.  This caused problems when
>> switch ports were actually specified.
>> 
>> Reported-by: Gurucharan Shetty 
>> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/075524.html
>> Signed-off-by: Ben Pfaff 
>> 
> Acked-by: Gurucharan Shetty 
> 
>> ---
>> ovn/northd/ovn-northd.c | 23 ++-
>> 1 file changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index 7ce509d..9047635 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -496,6 +496,12 @@ struct ovn_port {
>> 
>> struct lport_addresses lrp_networks;
>> 
>> +/* The port's peer:
>> + *
>> + * - A switch port S of type "router" has a router port R as a
>> peer,
>> + *   and R in turn has S has its peer.
>> + *
>> + * - Two connected logical router ports have each other as peer.
>> */
>> struct ovn_port *peer;
>> 
>> struct ovn_datapath *od;
>> @@ -715,7 +721,22 @@ join_logical_ports(struct northd_context *ctx,
>> sizeof *op->od->router_ports * (op->od->n_router_ports +
>> 1));
>> op->od->router_ports[op->od->n_router_ports++] = op;
>> } else if (op->nbr && op->nbr->peer) {
>> -op->peer = ovn_port_find(ports, op->nbr->peer);
>> +struct ovn_port *peer = ovn_port_find(ports, op->nbr->peer);
>> +if (peer) {
>> +if (peer->nbr) {
>> +op->peer = peer;
>> +} else {
>> +/* An ovn_port for a switch port of type "router"
>> does have
>> + * a router port as its peer (see the case above for
>> + * "router" ports), but this is set via
>> options:router-port
>> + * in Logical_Switch_Port and does not involve the
>> + * Logical_Router_Port's 'peer' column. */
>> +static struct vlog_rate_limit rl =
>> +VLOG_RATE_LIMIT_INIT(5, 1);
>> +VLOG_WARN_RL(&rl, "Bad configuration: The peer of
>> router "
>> + "port %s is a switch port", op->key);
>> +}
>> +}
>> }
>> }
>> }
>> --
>> 2.1.3
>> 
>> 


This patch that fix the bug is more better than which I provide. Thank you for 
your tips, Gurucharan. 

Acked-by: nickcooper-zhangtonghao 








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

[ovs-dev] [PATCH] ovn: remove unnecessary conditional statements.

[nickcooper-zhangtonghao]

It is unnecessary to check ‘chassis_id’ again.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/ovn-controller.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index dcab918..4d9490a 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -443,11 +443,11 @@ main(int argc, char *argv[])
 
 lflow_run(&ctx, &lports, &mcgroups, &local_datapaths,
   &patched_datapaths, &group_table, &ct_zones);
-if (chassis_id) {
-physical_run(&ctx, mff_ovn_geneve,
- br_int, chassis_id, &ct_zones,
- &local_datapaths, &patched_datapaths);
-}
+
+physical_run(&ctx, mff_ovn_geneve,
+ br_int, chassis_id, &ct_zones,
+ &local_datapaths, &patched_datapaths);
+
 ofctrl_put(&group_table);
 }
 
-- 
1.8.3.1



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

[ovs-dev] [PATCH] ovn: don't create the "" patch port.

[nickcooper-zhangtonghao]

It is not necessary to create the logical patch port whose peer named "".

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/patch.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 707d08b..7e3ae39 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -357,7 +357,10 @@ add_logical_patch_ports(struct controller_ctx *ctx,
 if (!strcmp(binding->type, "patch") || local_port) {
 const char *local = binding->logical_port;
 const char *peer = smap_get(&binding->options, "peer");
-if (!peer) {
+
+/* It is not necessary to create the logical patch port 
+ * whose peer named "". */
+if (!peer || !strcmp(peer, "")) {
 continue;
 }
 
-- 
1.8.3.1




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

Re: [ovs-dev] [PATCH] ovn: remove the dead code.

[nickcooper-zhangtonghao]

I apologize for this mistake.

> On Jul 25, 2016, at 2:40 AM, Ryan Moats  wrote:
> 
> Ben Pfaff  wrote on 07/24/2016 12:56:29 PM:
> 
> > From: Ben Pfaff 
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: Numan Siddique , ovs dev 
> > , nickcooper-zhangtonghao  > zhangtong...@opencloud.tech>
> > Date: 07/24/2016 12:56 PM
> > Subject: Re: [ovs-dev] [PATCH] ovn: remove the dead code.
> > 
> > On Sun, Jul 24, 2016 at 12:04:43PM -0500, Ryan Moats wrote:
> > > Numan Siddique  wrote on 07/24/2016 11:21:07 AM:
> > > 
> > > > From: Numan Siddique 
> > > > To: Ryan Moats/Omaha/IBM@IBMUS
> > > > Cc: nickcooper-zhangtonghao  > > > zhangtong...@opencloud.tech>, ovs dev 
> > > > Date: 07/24/2016 11:21 AM
> > > > Subject: Re: [ovs-dev] [PATCH] ovn: remove the dead code.
> > > >
> > > > On Sun, Jul 24, 2016 at 6:26 AM, Ryan Moats  wrote:
> > > > "dev"  wrote on 07/21/2016 05:36:13 AM:
> > > >
> > > > > From: nickcooper-zhangtonghao 
> > > > > To: dev@openvswitch.org
> > > > > Cc: nickcooper-zhangtonghao 
> > > > > Date: 07/21/2016 05:36 AM
> > > > > Subject: [ovs-dev] [PATCH] ovn: remove the dead code.
> > > > > Sent by: "dev" 
> > > > >
> > > > > It is not necessary to assign 0 to ip_csum.
> > > > >
> > > > > Signed-off-by: nickcooper-zhangtonghao  > > > > zhangtong...@opencloud.tech>
> > > > > ---
> > > > >  ovn/controller/pinctrl.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > >
> > > > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > > > > index 7893872..b35576a 100644
> > > > > --- a/ovn/controller/pinctrl.c
> > > > > +++ b/ovn/controller/pinctrl.c
> > > > > @@ -349,7 +349,6 @@ pinctrl_handle_put_dhcp_opts(
> > > > >  struct ip_header *out_ip = dp_packet_l3(&pkt_out);
> > > > >  out_ip->ip_tot_len = htons(pkt_out.l4_ofs - pkt_out.l3_ofs +
> > > > > new_l4_size);
> > > > >  udp->udp_csum = 0;
> > > > > -out_ip->ip_csum = 0;
> > > >
> > > > ​This is not a dead code. It is required. The reason it is required
> > > > is because the DHCPv4 response packet is constructed by copying the
> > > > DHCPv4 request packet and then adding the dhcp options. So the
> > > > ip_csum needs to be set to 0 before calling "csum", otherwise,
> > > > csum() will calculate wrong checksum. because of which the VM will
> > > > reject the DHCPv4 response packet.
> > > >
> > > > ​You can verify yourself by applying the ovn-northd DHCP patch [1].
> > > >
> > > > ​[1] - https://patchwork.ozlabs.org/patch/651958/ 
> > > > <https://patchwork.ozlabs.org/patch/651958/>
> > > >
> > > > ​
> > > >
> > > > >  out_ip->ip_csum = csum(out_ip, sizeof *out_ip);
> > > > >
> > > > >  pin->packet = dp_packet_data(&pkt_out);
> > > >
> > > > lgtm...
> > > >
> > > > Acked-by: Ryan Moats 
> > > 
> > > Ok, I'll admit this is a mistaken ack, but I think adding
> > > a comment pointing out that the line is initializing the
> > > variable for the pending csum calculation makes it crystal
> > > clear for the future.
> > 
> > Sure, that would be a welcome patch.
> 
> Such a patch now exists at
> http://patchwork.ozlabs.org/patch/652122/ 
> <http://patchwork.ozlabs.org/patch/652122/>
> 
> 

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

[ovs-dev] [PATCH] ovn-northd: Only peer router ports to other router ports.

[nickcooper-zhangtonghao]

Deletion of a logical router port, which may be an other
router peer, result in a WARN, because the "ports" variable
still contains the logical port deleted. This port exists
but should not have been initialized fully.

nbsp == NULL, nbrp == NULL

It is necessary to check 'nbsp'.

A router port's "peer", if set, must point to another router port, but the
code as written also accepted switch ports.  This caused problems when
switch ports were actually specified.

Reported-by: Gurucharan Shetty 
Reported-at: http://openvswitch.org/pipermail/dev/2016-July/075524.html
Signed-off-by: Ben Pfaff 
Acked-by: Gurucharan Shetty 
Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/northd/ovn-northd.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index a3d1672..efc915c 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -746,9 +746,15 @@ join_logical_ports(struct northd_context *ctx,
 } else if (op->nbrp && op->nbrp->peer) {
 struct ovn_port *peer = ovn_port_find(ports, op->nbrp->peer);
 if (peer) {
+/* Deletion of a logical router port, which may be an other
+ * router peer, result in a WARN, because the "ports" variable
+ * still contains the logical port deleted. This port exists 
+ * but should not have been initialized fully.
+ *
+ * nbsp == NULL, nbrp == NULL */
 if (peer->nbrp) {
 op->peer = peer;
-} else {
+} else if (peer->nbsp) {
 /* An ovn_port for a switch port of type "router" does have
  * a router port as its peer (see the case above for
  * "router" ports), but this is set via options:router-port
-- 
1.8.3.1



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

[ovs-dev] [PATCH] ovn-northd: create patch ports when necessary.

[nickcooper-zhangtonghao]

If the logical router ports without 'peer' or the port named peer
is not created, it is unnecessary to insert the ports into the
southbound Port_Binding table.

Similarly, if logical switch ports of type 'router' don't contain 'router-port'
value, these ports will not be inserted.

This patch may optimize the process of 'build_ports'.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/northd/ovn-northd.c | 32 
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index efc915c..f85eb2e 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -770,6 +770,22 @@ join_logical_ports(struct northd_context *ctx,
 }
 }
 
+static bool
+ovn_port_is_insert(const struct ovn_port *op)
+{
+if (op->nbsp) {
+/* The logical switch port is connected to router*/
+if (!strcmp(op->nbsp->type, "router")) {
+const char *router_port = smap_get(&op->nbsp->options,
+   "router-port");
+return router_port ? true : false;
+}
+return true;
+}
+/* Check the peer of the logical router port*/
+return op->peer ? true : false;
+}
+
 static void
 ovn_port_update_sbrec(const struct ovn_port *op)
 {
@@ -869,11 +885,19 @@ build_ports(struct northd_context *ctx, struct hmap 
*datapaths,
 continue;
 }
 
-op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
-ovn_port_update_sbrec(op);
+/* If the logical router ports without 'peer' or the port named peer
+ * is not created, it is unnecessary to insert the ports into the 
+ * southbound Port_Binding table.
+ *
+ * Similarly, if logical switch ports of type 'router' lack 
'router-port'
+ * value, these ports will not be inserted. */
+if (ovn_port_is_insert(op)) {
+op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
+ovn_port_update_sbrec(op);
 
-sbrec_port_binding_set_logical_port(op->sb, op->key);
-sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key);
+sbrec_port_binding_set_logical_port(op->sb, op->key);
+sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key);
+}
 }
 
 /* Delete southbound records without northbound matches. */
-- 
1.8.3.1



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

Re: [ovs-dev] [PATCH] ovn: don't create the "" patch port.

[nickcooper-zhangtonghao]

ovn-northd: create patch ports when necessary.

If the logical router ports without 'peer' or the port named peer
is not created, it is unnecessary to insert the ports into the
southbound Port_Binding table.

Similarly, if logical switch ports of type 'router' don't contain 'router-port'
value, these ports will not be inserted.

This patch may optimize the process of 'build_ports'.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/northd/ovn-northd.c | 32 
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index efc915c..f85eb2e 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -770,6 +770,22 @@ join_logical_ports(struct northd_context *ctx,
 }
 }

+static bool
+ovn_port_is_insert(const struct ovn_port *op)
+{
+if (op->nbsp) {
+/* The logical switch port is connected to router*/
+if (!strcmp(op->nbsp->type, "router")) {
+const char *router_port = smap_get(&op->nbsp->options,
+   "router-port");
+return router_port ? true : false;
+}
+return true;
+}
+/* Check the peer of the logical router port*/
+return op->peer ? true : false;
+}
+
 static void
 ovn_port_update_sbrec(const struct ovn_port *op)
 {
@@ -869,11 +885,19 @@ build_ports(struct northd_context *ctx, struct hmap 
*datapaths,
 continue;
 }

-op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
-ovn_port_update_sbrec(op);
+/* If the logical router ports without 'peer' or the port named peer
+ * is not created, it is unnecessary to insert the ports into the
+ * southbound Port_Binding table.
+ *
+ * Similarly, if logical switch ports of type 'router' lack 
'router-port'
+ * value, these ports will not be inserted. */
+if (ovn_port_is_insert(op)) {
+op->sb = sbrec_port_binding_insert(ctx->ovnsb_txn);
+ovn_port_update_sbrec(op);

-sbrec_port_binding_set_logical_port(op->sb, op->key);
-sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key);
+sbrec_port_binding_set_logical_port(op->sb, op->key);
+sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key);
+}
 }

 /* Delete southbound records without northbound matches. */
--
1.8.3.1



> On Jul 25, 2016, at 1:38 AM, Ben Pfaff  wrote:
> 
> On Sun, Jul 24, 2016 at 07:58:19AM -0700, nickcooper-zhangtonghao wrote:
>> It is not necessary to create the logical patch port whose peer named 
>> "".
>> 
>> Signed-off-by: nickcooper-zhangtonghao 
>> 
> 
> It seems to me that this fix is in the wrong place.  If any change is
> needed, I would suggest that it should be to make ovn-northd not create
> ports named .



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

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[nickcooper-zhangtonghao]

Hi William,
I reviewed your patch codes and found other bugs.

If the ‘ovsdb_condition_from_json’ return the error, cnd->clauses will be
set NULL, so ‘ovsdb_condition_destroy’ should check the 'cnd->clauses’ before
free it.

It is applied to ‘ovsdb_row_from_json’.


Signed-off-by: nickcooper-zhangtonghao 
---
 ovsdb/column.c  | 5 -
 ovsdb/condition.c   | 6 +-
 ovsdb/replication.c | 3 +++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ovsdb/column.c b/ovsdb/column.c
index 8838df3..f01301f 100644
--- a/ovsdb/column.c
+++ b/ovsdb/column.c
@@ -129,7 +129,10 @@ ovsdb_column_set_init(struct ovsdb_column_set *set)
 void
 ovsdb_column_set_destroy(struct ovsdb_column_set *set)
 {
-free(set->columns);
+if (set->columns) {
+free(set->columns);
+set->columns = NULL;
+}
 }

 void
diff --git a/ovsdb/condition.c b/ovsdb/condition.c
index 6da3b08..f337a37 100644
--- a/ovsdb/condition.c
+++ b/ovsdb/condition.c
@@ -485,7 +485,11 @@ ovsdb_condition_destroy(struct ovsdb_condition *cnd)
 for (i = 0; i < cnd->n_clauses; i++) {
 ovsdb_clause_free(&cnd->clauses[i]);
 }
-free(cnd->clauses);
+
+if (cnd->clauses) {
+free(cnd->clauses);
+cnd->clauses = NULL;
+}
 cnd->n_clauses = 0;

 ovsdb_condition_optimize_destroy(cnd);
diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index 52b7085..bfd2ca1 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -568,6 +568,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
 }

 ovsdb_condition_destroy(&condition);
+json_destroy(CONST_CAST(struct json *, where));
+
 return error;
 }

@@ -625,6 +627,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
 ovsdb_row_destroy(row);
 ovsdb_column_set_destroy(&columns);
 ovsdb_condition_destroy(&condition);
+json_destroy(CONST_CAST(struct json *, where));

 return error;
 }
--
1.8.3.1


> From: William Tu 
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.
> Message-ID: <1469582910-64371-1-git-send-email-u9012...@gmail.com>
> 
> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by value
> reports the following leak.
>json_from_string (json.c:1025)
>execute_update (replication.c:614), similarily at execute_delete()
>process_table_update (replication.c:502)
>process_notification.part.5 (replication.c:445)
>process_notification (replication.c:402)
>check_for_notifications (replication.c:418)
>replication_run (replication.c:110)
> 
> Signed-off-by: William Tu 
> ---
> ovsdb/replication.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
> index af7ae5c..fe89d39 100644
> --- a/ovsdb/replication.c
> +++ b/ovsdb/replication.c
> @@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
> }
> 
> ovsdb_condition_destroy(&condition);
> +json_destroy(CONST_CAST(struct json *, where));
> +
> return error;
> }
> 
> @@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
> ovsdb_row_destroy(row);
> ovsdb_column_set_destroy(&columns);
> ovsdb_condition_destroy(&condition);
> +json_destroy(CONST_CAST(struct json *, where));
> 
> return error;
> }
> -- 
> 2.5.0



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

Re: [ovs-dev] [PATCH] ovn-northd: Only peer router ports to other router ports.

[nickcooper-zhangtonghao]

> On Jul 28, 2016, at 5:01 AM, Ben Pfaff  wrote:
> 
> On Mon, Jul 25, 2016 at 09:19:00AM -0700, nickcooper-zhangtonghao wrote:
>> Deletion of a logical router port, which may be an other
>> router peer, result in a WARN, because the "ports" variable
>> still contains the logical port deleted. This port exists
>> but should not have been initialized fully.
>> 
>> nbsp == NULL, nbrp == NULL
>> 
>> It is necessary to check 'nbsp'.
>> 
>> A router port's "peer", if set, must point to another router port, but the
>> code as written also accepted switch ports.  This caused problems when
>> switch ports were actually specified.
>> 
>> Reported-by: Gurucharan Shetty 
>> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/075524.html
>> Signed-off-by: Ben Pfaff 
>> Acked-by: Gurucharan Shetty 
>> Signed-off-by: nickcooper-zhangtonghao 
>> 
>> ---
>> ovn/northd/ovn-northd.c | 8 +++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index a3d1672..efc915c 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -746,9 +746,15 @@ join_logical_ports(struct northd_context *ctx,
>> } else if (op->nbrp && op->nbrp->peer) {
>> struct ovn_port *peer = ovn_port_find(ports, op->nbrp->peer);
>> if (peer) {
>> +/* Deletion of a logical router port, which may be an other
>> + * router peer, result in a WARN, because the "ports" 
>> variable
>> + * still contains the logical port deleted. This port 
>> exists 
>> + * but should not have been initialized fully.
>> + *
>> + * nbsp == NULL, nbrp == NULL */
>> if (peer->nbrp) {
>> op->peer = peer;
>> -} else {
>> +} else if (peer->nbsp) {
>> /* An ovn_port for a switch port of type "router" does 
>> have
>>  * a router port as its peer (see the case above for
>>  * "router" ports), but this is set via 
>> options:router-port
> 
> I don't understand this yet.  I don't see how an ovn_port's nbsp and
> nbrp can both be NULL.  I only see two calls to ovn_port_create(), and
> each of them supplies either nbsp or nbrp as nonnull.
> 
> Can you explain further?
> 
> Thanks,
> 
> Ben.


Run the commands below, you will get a WARN info in the ovn-northd.log

ovn-nbctl lr-add lr0
ovn-nbctl lr-add lr1
ovn-nbctl lrp-add lr0 lr0-p0 00:11:22:33:44:55 192.168.10.10/24 peer=lr1-p0
ovn-nbctl lrp-add lr1 lr1-p0 00:11:22:33:44:66 192.168.20.10/24 peer=lr0-p0
ovn-nbctl lrp-del lr1-p0

WARN INFO:
“Bad configuration: The peer of router port lr0-p0 is a switch port.”


Here’s is why:
In the ‘join_logical_ports’ function,  firstly, we get the ‘ovn_port’s from 
SB database and save them to “ports” variable. The ovn_port’s nbsp and nbrp both
are NULL via ovn_port_create.

Next, in the HMAP_FOR_EACH  loop, we initialize the ‘ovn_port’s except which 
were deleted on the NB database. The ovn_port which is deleted in NB database 
is still in SB database. so, still in “ports” variable.

Then, when connected to their peer, the ovn_port deleted may be found as a peer.
But the ovn_port's nbsp and nbsp both are NULL.

Thanks,
zhangtonghao  





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

Re: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.

[nickcooper-zhangtonghao]

Good, it is unnecessary to check again.

> On Jul 28, 2016, at 2:13 AM, William Tu  wrote:
> 
> Hi zhangtonghao,
> 
> Thanks for the patch!
> I think it's safe to call free(ptr) when ptr == NULL. The free() will
> be no operation.
> 
> Regards,
> William
> 
> On Tue, Jul 26, 2016 at 9:18 PM, nickcooper-zhangtonghao
>  wrote:
>> Hi William,
>> I reviewed your patch codes and found other bugs.
>> 
>> If the ‘ovsdb_condition_from_json’ return the error, cnd->clauses will be
>> set NULL, so ‘ovsdb_condition_destroy’ should check the 'cnd->clauses’ before
>> free it.
>> 
>> It is applied to ‘ovsdb_row_from_json’.
>> 
>> 
>> Signed-off-by: nickcooper-zhangtonghao 
>> 
>> ---
>> ovsdb/column.c  | 5 -
>> ovsdb/condition.c   | 6 +-
>> ovsdb/replication.c | 3 +++
>> 3 files changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/ovsdb/column.c b/ovsdb/column.c
>> index 8838df3..f01301f 100644
>> --- a/ovsdb/column.c
>> +++ b/ovsdb/column.c
>> @@ -129,7 +129,10 @@ ovsdb_column_set_init(struct ovsdb_column_set *set)
>> void
>> ovsdb_column_set_destroy(struct ovsdb_column_set *set)
>> {
>> -free(set->columns);
>> +if (set->columns) {
>> +free(set->columns);
>> +set->columns = NULL;
>> +}
>> }
>> 
>> void
>> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
>> index 6da3b08..f337a37 100644
>> --- a/ovsdb/condition.c
>> +++ b/ovsdb/condition.c
>> @@ -485,7 +485,11 @@ ovsdb_condition_destroy(struct ovsdb_condition *cnd)
>> for (i = 0; i < cnd->n_clauses; i++) {
>> ovsdb_clause_free(&cnd->clauses[i]);
>> }
>> -free(cnd->clauses);
>> +
>> +if (cnd->clauses) {
>> +free(cnd->clauses);
>> +cnd->clauses = NULL;
>> +}
>> cnd->n_clauses = 0;
>> 
>> ovsdb_condition_optimize_destroy(cnd);
>> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
>> index 52b7085..bfd2ca1 100644
>> --- a/ovsdb/replication.c
>> +++ b/ovsdb/replication.c
>> @@ -568,6 +568,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
>> }
>> 
>> ovsdb_condition_destroy(&condition);
>> +json_destroy(CONST_CAST(struct json *, where));
>> +
>> return error;
>> }
>> 
>> @@ -625,6 +627,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
>> ovsdb_row_destroy(row);
>> ovsdb_column_set_destroy(&columns);
>> ovsdb_condition_destroy(&condition);
>> +json_destroy(CONST_CAST(struct json *, where));
>> 
>> return error;
>> }
>> --
>> 1.8.3.1
>> 
>> 
>>> From: William Tu 
>>> To: dev@openvswitch.org
>>> Subject: [ovs-dev] [PATCH] ovsdb: Fix memory leak in execute_update.
>>> Message-ID: <1469582910-64371-1-git-send-email-u9012...@gmail.com>
>>> 
>>> Valgrind testcase 1804 ovsdb-server.at:1023 insert rows, update rows by 
>>> value
>>> reports the following leak.
>>>   json_from_string (json.c:1025)
>>>   execute_update (replication.c:614), similarily at execute_delete()
>>>   process_table_update (replication.c:502)
>>>   process_notification.part.5 (replication.c:445)
>>>   process_notification (replication.c:402)
>>>   check_for_notifications (replication.c:418)
>>>   replication_run (replication.c:110)
>>> 
>>> Signed-off-by: William Tu 
>>> ---
>>> ovsdb/replication.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/ovsdb/replication.c b/ovsdb/replication.c
>>> index af7ae5c..fe89d39 100644
>>> --- a/ovsdb/replication.c
>>> +++ b/ovsdb/replication.c
>>> @@ -573,6 +573,8 @@ execute_delete(struct ovsdb_txn *txn, const char *uuid,
>>>}
>>> 
>>>ovsdb_condition_destroy(&condition);
>>> +json_destroy(CONST_CAST(struct json *, where));
>>> +
>>>return error;
>>> }
>>> 
>>> @@ -630,6 +632,7 @@ execute_update(struct ovsdb_txn *txn, const char *uuid,
>>>ovsdb_row_destroy(row);
>>>ovsdb_column_set_destroy(&columns);
>>>ovsdb_condition_destroy(&condition);
>>> +json_destroy(CONST_CAST(struct json *, where));
>>> 
>>>return error;
>>> }
>>> --
>>> 2.5.0
>> 



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

Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when necessary.

[nickcooper-zhangtonghao]

> On Jul 28, 2016, at 5:03 AM, Ben Pfaff  wrote:
> 
> On Mon, Jul 25, 2016 at 09:23:19AM -0700, nickcooper-zhangtonghao wrote:
>> If the logical router ports without 'peer' or the port named peer
>> is not created, it is unnecessary to insert the ports into the
>> southbound Port_Binding table.
>> 
>> Similarly, if logical switch ports of type 'router' don't contain 
>> 'router-port'
>> value, these ports will not be inserted.
>> 
>> This patch may optimize the process of 'build_ports'.
>> 
>> Signed-off-by: nickcooper-zhangtonghao 
>> 
> 
> This doesn't seem like a common case.  Is it worth optimizing?

It seems to me that it’s unnecessary to create the ports unused currently.

Thanks,
zhangtonghao

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

Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when necessary.

[nickcooper-zhangtonghao]

I only consider the OVN without CMS. Your tips may sound great. 

> On Jul 29, 2016, at 10:48 AM, Ryan Moats  wrote:
> 
> "dev"  wrote on 07/27/2016 09:47:47 PM:
> 
> > From: nickcooper-zhangtonghao 
> > To: Ben Pfaff 
> > Cc: dev@openvswitch.org
> > Date: 07/27/2016 09:48 PM
> > Subject: Re: [ovs-dev] [PATCH] ovn-northd: create patch ports when 
> > necessary.
> > Sent by: "dev" 
> > 
> > 
> > > On Jul 28, 2016, at 5:03 AM, Ben Pfaff  wrote:
> > > 
> > > On Mon, Jul 25, 2016 at 09:23:19AM -0700, nickcooper-zhangtonghao wrote:
> > >> If the logical router ports without 'peer' or the port named peer
> > >> is not created, it is unnecessary to insert the ports into the
> > >> southbound Port_Binding table.
> > >> 
> > >> Similarly, if logical switch ports of type 'router' don't contain
> > 'router-port'
> > >> value, these ports will not be inserted.
> > >> 
> > >> This patch may optimize the process of 'build_ports'.
> > >> 
> > >> Signed-off-by: nickcooper-zhangtonghao  > zhangtong...@opencloud.tech>
> > > 
> > > This doesn't seem like a common case.  Is it worth optimizing?
> > 
> > It seems to me that it’s unnecessary to create the ports unused currently.
> 
> If I think about it, the router port and its peer will be created by the
> CMS.  That means this would optimize for the case where the CMS creates
> these two ports at substantially different times.  I can't think of any
> CMS that works that way today, so I'm with Ben, this doesn't sound like a
> common enough case.
> 

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

[ovs-dev] [PATCH] ovn-controller: if 'ovn-bridge-mappings' unconfigured, return directly.

[nickcooper-zhangtonghao]

If the chassis doesn't configure the 'external-ids:ovn-bridge-mappings' in
the OVSDB, the 'add_bridge_mappings' should return directly.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/patch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 012e6ba..4116944 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -152,7 +152,9 @@ add_bridge_mappings(struct controller_ctx *ctx,
 if (cfg) {
 mappings_cfg = smap_get(&cfg->external_ids, "ovn-bridge-mappings");
 if (!mappings_cfg) {
-mappings_cfg = "";
+/* This chassis doesn't configure the 'bridge-mappings'.
+ * Return directly*/
+return;
 }
 }
 
-- 
1.8.3.1




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

Re: [ovs-dev] [PATCH] ovn-controller: if 'ovn-bridge-mappings' unconfigured, return directly.

[nickcooper-zhangtonghao]

> On Aug 2, 2016, at 12:09 AM, Russell Bryant  wrote:
> 
> 
> 
> On Mon, Aug 1, 2016 at 12:00 PM, Ben Pfaff  <mailto:b...@ovn.org>> wrote:
> On Mon, Aug 01, 2016 at 02:55:26AM -0700, nickcooper-zhangtonghao wrote:
> > If the chassis doesn't configure the 'external-ids:ovn-bridge-mappings' in
> > the OVSDB, the 'add_bridge_mappings' should return directly.
> >
> > Signed-off-by: nickcooper-zhangtonghao 
> > 
> 
> What's the benefit?
> 
> I was looking at this earlier before I got on some calls.
> 
> There doesn't appear to be any bug here, but this patch does skip some 
> unnecessary code.  The most notable improvement seems to be skipping a loop 
> over all Port_Binding records in every call to patch_run.
> 
> -- 
> Russell Bryant

I’am with Russell.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ovn-controller:improve the ofctrl_run, pinctrl_run

[nickcooper-zhangtonghao]

Before calling the function "ofctrl_run" and "pinctrl_run",
the "br-int"" has been checked. Remove the conditional statements in
the function may make the code clearer.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/ofctrl.c  | 15 +--
 ovn/controller/pinctrl.c | 16 +---
 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 79d840d..9742e34 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -432,17 +432,12 @@ recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum 
ofptype type)
 enum mf_field_id
 ofctrl_run(const struct ovsrec_bridge *br_int)
 {
-if (br_int) {
-char *target;
-target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
-if (strcmp(target, rconn_get_target(swconn))) {
-VLOG_INFO("%s: connecting to switch", target);
-rconn_connect(swconn, target, target);
-}
-free(target);
-} else {
-rconn_disconnect(swconn);
+char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+if (strcmp(target, rconn_get_target(swconn))) {
+VLOG_INFO("%s: connecting to switch", target);
+rconn_connect(swconn, target, target);
 }
+free(target);
 
 rconn_run(swconn);
 
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 416dad6..0377ef3 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -454,18 +454,12 @@ pinctrl_run(struct controller_ctx *ctx, const struct 
lport_index *lports,
 const char *chassis_id,
 struct hmap *local_datapaths)
 {
-if (br_int) {
-char *target;
-
-target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
-if (strcmp(target, rconn_get_target(swconn))) {
-VLOG_INFO("%s: connecting to switch", target);
-rconn_connect(swconn, target, target);
-}
-free(target);
-} else {
-rconn_disconnect(swconn);
+char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+if (strcmp(target, rconn_get_target(swconn))) {
+VLOG_INFO("%s: connecting to switch", target);
+rconn_connect(swconn, target, target);
 }
+free(target);
 
 rconn_run(swconn);
 
-- 
1.8.3.1



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

[ovs-dev] [PATCH] ovn-controller: Combine two Logical_Flow loops into one.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/lflow.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index fda10eb..0cc92a7 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -412,18 +412,14 @@ add_logical_flows(struct controller_ctx *ctx, const 
struct lport_index *lports,
 }
 full_logical_flow_processing = false;
 } else {
-/* First, remove any flows that should be removed. */
 SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
+/* Remove any flows that should be removed. */
 if (sbrec_logical_flow_is_deleted(lflow)) {
 ofctrl_remove_flows(&lflow->header_.uuid);
-}
-}
-
-/* Now, add/modify existing flows. */
-SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
-if (!sbrec_logical_flow_is_deleted(lflow)) {
-/* if the logical flow is a modification, just remove
- * the flows for this row, and then add new flows. */
+} else {
+/* Now, add/modify existing flows. If the logical
+ * flow is a modification, just remove the flows
+ * for this row, and then add new flows. */
 if (!sbrec_logical_flow_is_new(lflow)) {
 ofctrl_remove_flows(&lflow->header_.uuid);
 }
-- 
1.8.3.1



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

[ovs-dev] [PATCH] ovn-controller: Remove the repeated code.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/lflow.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index fda10eb..e22923b 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -595,7 +595,6 @@ consider_logical_flow(const struct lport_index *lports,
 }
 ofctrl_add_flow(ptable, lflow->priority, &m->match, &conj,
 &lflow->header_.uuid);
-ofpbuf_uninit(&conj);
 ofpbuf_uninit(&conj);
 }
 }
-- 
1.8.3.1




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

Re: [ovs-dev] [PATCH 2/2] ovs-appctl: Fix potential crash with timeout argument

[nickcooper-zhangtonghao]

> On Aug 8, 2016, at 3:45 PM, dev-requ...@openvswitch.org wrote:
> 
> Date: Sun,  7 Aug 2016 22:06:05 +0100
> From: Bhanuprakash Bodireddy  >
> To: dev@openvswitch.org 
> Subject: [ovs-dev] [PATCH 2/2] ovs-appctl: Fix potential crash with
>   timeout argument
> Message-ID:
>   <1470603965-73273-2-git-send-email-bhanuprakash.bodire...@intel.com 
> >
> 
> ovs-appctl can crash with missing timeout argument.
>  # ovs-appctl --timeout= dpif-netdev/pmd-stats-show
> 
> Fix by using strtol and validating the timeout value.
> 
> Signed-off-by: Bhanuprakash Bodireddy  >
> ---
> utilities/ovs-appctl.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index 8f87cc4..2543ee9 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -127,6 +127,7 @@ parse_command_line(int argc, char *argv[])
> char *short_options_ = 
> ovs_cmdl_long_options_to_short_options(long_options);
> char *short_options = xasprintf("+%s", short_options_);
> const char *target;
> +int timeout;
> int e_options;
> 
> target = NULL;
> @@ -165,7 +166,13 @@ parse_command_line(int argc, char *argv[])
> exit(EXIT_SUCCESS);
> 
> case 'T':
> -time_alarm(atoi(optarg));
> +timeout = strtol(optarg, NULL, 10);
> +if (timeout <= 0) {
> +ovs_fatal(0, "timeout value %s on -t or --timeout is 
> invalid",
> +   optarg);
> +} else {
> +time_alarm(timeout);
> +}
> break;
> 
> case 'V':
> -- 
> 2.4.11

It seems to me that it’s unnecessary to change the codes. If the “timeout” is 
empty, the “atoi” function will convert it to 0, then time_alarm will disable 
the “timeout” feature, and return directly. If the “timeout” is not a positive 
number, (e.g. -100), the check will been done in the time_alarm.




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

Re: [ovs-dev] [PATCH v2 21/21] ovn-trace: New utility.

[nickcooper-zhangtonghao]

> On Aug 9, 2016, at 12:15 AM, dev-requ...@openvswitch.org wrote:
> 
> Date: Mon,  8 Aug 2016 09:14:32 -0700
> From: Ben Pfaff mailto:b...@ovn.org>>
> To: dev@openvswitch.org 
> Cc: Ben Pfaff mailto:b...@ovn.org>>
> Subject: [ovs-dev] [PATCH v2 21/21] ovn-trace: New utility.
> Message-ID: <1470672872-19450-22-git-send-email-...@ovn.org 
> >
> 
> This new utility is intended to fulfill for OVN the purpose that
> "ofproto/trace" has for Open vSwitch.  First, it's meant to be a useful
> tool for troubleshooting and diagnosis and in general for improving one's
> understanding of the emergent properties of a flow table.  Second, it
> simplifies and increases the practical scope of testing, as well as making
> testing more reliable and repeatable and failures easier to interpret.
> 
> This commit adds only a single test that uses the new utility, based on the
> oldest OVN end-to-end test "ovn -- 3 HVs, 1 LS, 3 lports/HV".  The
> differences between the old and the new test illustrate properties of
> tracing.  First, the new test does not start any ovn-controller processes
> or simulate any hypervisors in a nontrivial way.  This is because ovn-trace
> does not actually forward packets or rely on the physical structure of the
> system.  Second, whereas the old test tested not just the logical but also
> the physical structure of the system, it needed to have several logical
> ports, a total of 9 (3 on each of 3 HVs), whereas since this test only
> tests the logical network implementation it can use a smaller number.  This
> property also means that the new test runs signicantly faster than the old
> one (less than a second on my laptop).
> 
> In my opinion this approach points the way toward the future of OVN
> testing.  Certainly, we need end-to-end tests.  However, I believe that the
> bulk of our tests can be broken into ones that test the logical network
> implementation (using tracing) and ones that test physical/logical
> translation.
> 
> Signed-off-by: Ben Pfaff mailto:b...@ovn.org>>
> ---
> 
> +
> +/* --detailed: Show a detailed, table-by-table trace. */
> +static bool detailed;
> +
> +/* --summary: Show a trace that omits table information. */
> +static bool summary;
> +
> +/* --minimal: Show a trace with only minimal information. */
> +static bool minimal;
> +

As a little reminder, it is better to initialize ‘detailed’, ‘summary’, and 
‘minimal' variables with a value of ‘false'.


Thanks,
zhangtonghao.


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

Re: [ovs-dev] [PATCH] ovn-controller: Combine two Logical_Flow loops into one.

[nickcooper-zhangtonghao]

> On Aug 7, 2016, at 6:54 AM, Ryan Moats  wrote:
> 
> > From: nickcooper-zhangtonghao  > <mailto:nickcooper-zhangtong...@opencloud.tech>>
> > To: dev@openvswitch.org <mailto:dev@openvswitch.org>
> > Cc: nickcooper-zhangtonghao  > <mailto:nickcooper-zhangtong...@opencloud.tech>>
> > Date: 08/06/2016 06:43 AM
> > Subject: [ovs-dev] [PATCH] ovn-controller: Combine two Logical_Flow 
> > loops into one.
> > Sent by: "dev"  > <mailto:dev-boun...@openvswitch.org>>
> > 
> > Signed-off-by: nickcooper-zhangtonghao  > zhangtong...@opencloud.tech>
> > ---
> >  ovn/controller/lflow.c | 14 +-
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> > index fda10eb..0cc92a7 100644
> > --- a/ovn/controller/lflow.c
> > +++ b/ovn/controller/lflow.c
> > @@ -412,18 +412,14 @@ add_logical_flows(struct controller_ctx *ctx, 
> > const struct lport_index *lports,
> >  }
> >  full_logical_flow_processing = false;
> >  } else {
> > -/* First, remove any flows that should be removed. */
> >  SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
> > +/* Remove any flows that should be removed. */
> >  if (sbrec_logical_flow_is_deleted(lflow)) {
> >  ofctrl_remove_flows(&lflow->header_.uuid);
> > -}
> > -}
> > -
> > -/* Now, add/modify existing flows. */
> > -SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
> > -if (!sbrec_logical_flow_is_deleted(lflow)) {
> > -/* if the logical flow is a modification, just remove
> > - * the flows for this row, and then add new flows. */
> > +} else {
> > +/* Now, add/modify existing flows. If the logical
> > + * flow is a modification, just remove the flows
> > + * for this row, and then add new flows. */
> >  if (!sbrec_logical_flow_is_new(lflow)) {
> >  ofctrl_remove_flows(&lflow->header_.uuid);
> >  }
> > -- 
> 
> I had to go back through the July archives to remember why we ended
> up with two logical flow loops and this change was always on my
> radar, but I just forgot about it while chasing other higher priority
> items.  So, I went ahead and ran through the ovn unit tests with
> -j4 50 times to verify that the test failure rate I'm seeing is
> reasonable (I didn't get one...), so
> 
> Acked-by: Ryan Moats mailto:rmo...@us.ibm.com>>


Thanks for your review.



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

Re: [ovs-dev] [PATCH] ovn-controller:improve the ofctrl_run, pinctrl_run

[nickcooper-zhangtonghao]

> On Aug 7, 2016, at 5:44 AM, Ryan Moats  wrote:
> 
> "dev"  wrote on 08/04/2016 06:50:03 AM:
> 
> > From: nickcooper-zhangtonghao 
> > To: dev@openvswitch.org
> > Cc: nickcooper-zhangtonghao 
> > Date: 08/04/2016 06:50 AM
> > Subject: [ovs-dev] [PATCH] ovn-controller:improve the ofctrl_run, 
> > pinctrl_run
> > Sent by: "dev" 
> > 
> > Before calling the function "ofctrl_run" and "pinctrl_run",
> > the "br-int"" has been checked. Remove the conditional statements in
> > the function may make the code clearer.
> > 
> > Signed-off-by: nickcooper-zhangtonghao  > zhangtong...@opencloud.tech>
> > ---
> 
> This applies cleanly, it passes all the unit tests, and I understand
> the logic behind it, but I can't help but think it means we've missed
> something along the way... 
> 
> so, Reluctantly-Acked-by: Ryan Moats 
> 
Thanks for your review. This patch makes only the codes clearer, and doesn’t 
make 
much sense.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovn-controller: Remove the repeated code.

[nickcooper-zhangtonghao]

> On Aug 7, 2016, at 7:22 AM, Ryan Moats  wrote:
> 
> > From: nickcooper-zhangtonghao  > <mailto:nickcooper-zhangtong...@opencloud.tech>>
> > To: dev@openvswitch.org <mailto:dev@openvswitch.org>
> > Cc: nickcooper-zhangtonghao  > <mailto:nickcooper-zhangtong...@opencloud.tech>>
> > Date: 08/06/2016 07:18 AM
> > Subject: [ovs-dev] [PATCH] ovn-controller: Remove the repeated code.
> > Sent by: "dev"  > <mailto:dev-boun...@openvswitch.org>>
> > 
> > Signed-off-by: nickcooper-zhangtonghao  > zhangtong...@opencloud.tech>
> > ---
> >  ovn/controller/lflow.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> > index fda10eb..e22923b 100644
> > --- a/ovn/controller/lflow.c
> > +++ b/ovn/controller/lflow.c
> > @@ -595,7 +595,6 @@ consider_logical_flow(const struct lport_index *lports,
> >  }
> >  ofctrl_add_flow(ptable, lflow->priority, &m->match, &conj,
> >  &lflow->header_.uuid);
> > -ofpbuf_uninit(&conj);
> >  ofpbuf_uninit(&conj);
> >  }
> >  }
> 
> git blame shows what happened with this one - I didn't resolve one of
> the merge conflicts between fdbdb595 and a53d69c9 correctly
> 
> Double checked it with 25 cycles of ovn unit tests running with -j4
> and it looks fine, so...
> 
> Acked-by: Ryan Moats mailto:rmo...@us.ibm.com>>


Yes, the repeated code should be removed.



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

[ovs-dev] The name of logical switch and router is null

[nickcooper-zhangtonghao]

Hi all,
When I create one or more logical router named ’null', using 'ovn-nbctl 
lr-add’, there is no error information.
This is arguably an ovn-nbctl bug.

# ovn-nbctl lr-add
# ovn-nbctl lr-list
3aeff07a-cd16-45f3-ac5a-e4da634b81f6 ()

# ovn-nbctl lr-add
# ovn-nbctl lr-add
# ovn-nbctl lr-list
3aeff07a-cd16-45f3-ac5a-e4da634b81f6 ()
01dc4fe3-755f-4e88-9a53-8351b059d56a ()
1a47070b-f2ca-48a2-9ab9-661f008cc75d ()

What’s more, if you want to duplicate a logical router, you may use 
“--add-duplicate” option 
# ovn-nbctl --add-duplicate lr-add lr1
# ovn-nbctl lr-list
33eadbf1-59d6-42f2-aedd-6d5c64e2105d (lr1)
34cb1c7b-417d-4bde-b91a-3e0aae3e9e69 (lr1)


diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 7789cdd..a1d71c2 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1206,37 +1206,31 @@ static void
 nbctl_lr_add(struct ctl_context *ctx)
 {
 const char *lr_name = ctx->argc == 2 ? ctx->argv[1] : NULL;
+if (lr_name == NULL)
+ctl_fatal("lr-add requires specifying a name");

 bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
 bool add_duplicate = shash_find(&ctx->options, "--add-duplicate") != NULL;
 if (may_exist && add_duplicate) {
 ctl_fatal("--may-exist and --add-duplicate may not be used together");
 }
-
-if (lr_name) {
-if (!add_duplicate) {
-const struct nbrec_logical_router *lr;
-NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->idl) {
-if (!strcmp(lr->name, lr_name)) {
-if (may_exist) {
-return;
-}
-ctl_fatal("%s: a router with this name already exists",
-  lr_name);
-}
+
+if (!add_duplicate) {
+const struct nbrec_logical_router *lr;
+NBREC_LOGICAL_ROUTER_FOR_EACH (lr, ctx->idl) {
+if (!strcmp(lr->name, lr_name)) {
+if (may_exist)
+return;
+
+ctl_fatal("%s: a router with this name already exists",
+lr_name);
 }
 }
-} else if (may_exist) {
-ctl_fatal("--may-exist requires specifying a name");
-} else if (add_duplicate) {
-ctl_fatal("--add-duplicate requires specifying a name");
 }

 struct nbrec_logical_router *lr;
 lr = nbrec_logical_router_insert(ctx->txn);
-if (lr_name) {
-nbrec_logical_router_set_name(lr, lr_name);
-}
+   nbrec_logical_router_set_name(lr, lr_name);
 }




diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 7789cdd..520f7e8 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -526,6 +526,8 @@ static void
 nbctl_ls_add(struct ctl_context *ctx)
 {
 const char *ls_name = ctx->argc == 2 ? ctx->argv[1] : NULL;
+if (ls_name == NULL)
+ctl_fatal("ls-add requires specifying a name");

 bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
 bool add_duplicate = shash_find(&ctx->options, "--add-duplicate") != NULL;
@@ -533,30 +535,22 @@ nbctl_ls_add(struct ctl_context *ctx)
 ctl_fatal("--may-exist and --add-duplicate may not be used together");
 }

-if (ls_name) {
-if (!add_duplicate) {
-const struct nbrec_logical_switch *ls;
-NBREC_LOGICAL_SWITCH_FOR_EACH (ls, ctx->idl) {
-if (!strcmp(ls->name, ls_name)) {
-if (may_exist) {
-return;
-}
-ctl_fatal("%s: a switch with this name already exists",
-  ls_name);
-}
+if (!add_duplicate) {
+const struct nbrec_logical_switch *ls;
+NBREC_LOGICAL_SWITCH_FOR_EACH (ls, ctx->idl) {
+if (!strcmp(ls->name, ls_name)) {
+if (may_exist)
+return;
+
+ctl_fatal("%s: a switch with this name already exists",
+  ls_name);
 }
 }
-} else if (may_exist) {
-ctl_fatal("--may-exist requires specifying a name");
-} else if (add_duplicate) {
-ctl_fatal("--add-duplicate requires specifying a name");
 }

 struct nbrec_logical_switch *ls;
 ls = nbrec_logical_switch_insert(ctx->txn);
-if (ls_name) {
-nbrec_logical_switch_set_name(ls, ls_name);
-}
+nbrec_logical_switch_set_name(ls, ls_name);
 }



Thanks,
nick



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

[ovs-dev] [ovn-dev] ovn-nbctl: Improvement to the ovn-nbctl manpage

[nickcooper-zhangtonghao]

Hi Ben,
Thank you for your advice.


ovn-nbctl: the improvement to the ovn-nbctl manpage.

If there are multiple logical switches or routers with a duplicate name, 
the configuration is slightly different. You should configure the logical 
switches or routers using the UUID instead of the name.

Signed-off-by: nickcooper-zhangtonghao 


diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index c2ca420..b4b8501 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -50,7 +50,9 @@
   --may-exist, adding a duplicate name succeeds but does
   not create a new logical switch.  With --add-duplicate,
   the command really creates a new logical switch with a duplicate
-  name.  It is an error to specify both options.
+  name.  It is an error to specify both options.  If there are multiple
+  logical switches with a duplicate name, configure the logical 
switches
+  using the UUID instead of the switch name.
 
   

@@ -267,7 +269,9 @@
   --may-exist, adding a duplicate name succeeds but does
   not create a new logical router.  With --add-duplicate,
   the command really creates a new logical router with a duplicate
-  name.  It is an error to specify both options.
+  name.  It is an error to specify both options.  If there are multiple
+  logical routers with a duplicate name, configure the logical routers
+  using the UUID instead of the router name.
 
   



> On Jul 2, 2016, at 8:35 AM, Ben Pfaff  wrote:
> 
> On Fri, Jul 01, 2016 at 12:38:54AM +0800, nickcooper-zhangtonghao wrote:
>>  When I create one or more logical router named ’null', using 'ovn-nbctl 
>> lr-add’, there is no error information.
>> This is arguably an ovn-nbctl bug.
> 
> It's not a bug.  Any number of unnamed logical switches or logical
> routers may exist.
> 
> If the documentation is unclear then please submit an improvement to the
> ovn-nbctl manpge.
> 
> Thanks,
> 
> Ben.


Thanks,
nick




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

[ovs-dev] [PATCH] ovn: ARP reply security vulnerability.

[nickcooper-zhangtonghao]

The the logical routers check only the "arp.op == 2" for ARP replies
and then use ARP replies to populate the logical router's ARP table.
If we continue to send ARP replies, which have different "arp.spa" and
"arp.sha", to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and crashes.

So,
1. When logical router receive an ARP reply packet, we should check the
   packet's "arp.tpa" and "arp.spa".
2. The logical router uses a cache to store the ARP request. Only when
   logical routers send an ARP request packet, and get a corresponding
   ARP reply, will the 'ovn-controller' update MAC_Binding table of SB
   database.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/pinctrl.c | 152 ++-
 ovn/northd/ovn-northd.c  |  17 +-
 2 files changed, 166 insertions(+), 3 deletions(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index ca49ca6..afa1197 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -57,12 +57,18 @@ static void pinctrl_handle_put_mac_binding(const struct 
flow *md,
const struct flow *headers,
bool is_arp);
 static void init_put_mac_bindings(void);
+static void init_arp_request_caches(void);
 static void destroy_put_mac_bindings(void);
+static void destroy_arp_request_caches(void);
 static void run_put_mac_bindings(struct controller_ctx *,
  const struct lport_index *lports);
 static void wait_put_mac_bindings(struct controller_ctx *);
 static void flush_put_mac_bindings(void);
 
+static void flush_arp_request_caches(void);
+static void pinctrl_handle_arp_request_cache(uint32_t dp_key,
+ uint32_t port_key,
+ const char *ip_s);
 static void init_send_garps(void);
 static void destroy_send_garps(void);
 static void send_garp_wait(void);
@@ -84,6 +90,7 @@ pinctrl_init(void)
 swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
 conn_seq_no = 0;
 init_put_mac_bindings();
+init_arp_request_caches();
 init_send_garps();
 }
 
@@ -184,6 +191,17 @@ pinctrl_handle_arp(const struct flow *ip_flow, const 
struct match *md,
 enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
 queue_msg(ofputil_encode_packet_out(&po, proto));
 
+/* The logical router uses a cache to store the ARP request.
+ * Only when logical routers send an ARP request packet, and get a
+ * corresponding ARP reply, will the 'ovn-controller' update
+ * mac_binding table of SB database. */
+uint32_t dp_key = ntohll(md->flow.metadata);
+uint32_t port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
+char ip_s[INET_ADDRSTRLEN];
+inet_ntop(AF_INET, &ip_flow->nw_dst, ip_s, sizeof(ip_s));
+
+pinctrl_handle_arp_request_cache(dp_key, port_key, ip_s);
+
 exit:
 dp_packet_uninit(&packet);
 ofpbuf_uninit(&ofpacts);
@@ -511,6 +529,7 @@ pinctrl_destroy(void)
 {
 rconn_destroy(swconn);
 destroy_put_mac_bindings();
+destroy_arp_request_caches();
 destroy_send_garps();
 }
 
@@ -540,8 +559,22 @@ struct put_mac_binding {
 struct eth_addr mac;
 };
 
+/* Buffered "arp_request_cache" operation. */
+struct arp_request_cache {
+struct hmap_node hmap_node; /* In 'arp_request_caches'. */
+
+long long int timestamp;/* In milliseconds. */
+
+/* Key. */
+uint32_t dp_key;
+uint32_t port_key;
+char ip_s[INET6_ADDRSTRLEN + 1];
+};
+
 /* Contains "struct put_mac_binding"s. */
 static struct hmap put_mac_bindings;
+/* Contains "struct arp_request_cache"s. */
+static struct hmap arp_request_caches;
 
 static void
 init_put_mac_bindings(void)
@@ -572,6 +605,100 @@ pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t 
port_key,
 }
 
 static void
+init_arp_request_caches(void)
+{
+hmap_init(&arp_request_caches);
+}
+
+static void
+destroy_arp_request_caches(void)
+{
+flush_arp_request_caches();
+hmap_destroy(&arp_request_caches);
+}
+
+static bool
+pinctrl_find_delete_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+ const char *ip_s)
+{
+struct arp_request_cache *arc, *next;
+HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
+if (arc->dp_key == dp_key
+&& arc->port_key == port_key
+&& !strcmp(arc->ip_s, ip_s)) {
+hmap_remove(&arp_request_caches, &arc->hmap_node);
+free(arc);
+return true;
+}
+}
+
+return false;
+}
+
+static struct arp_request_cache *
+pinctrl_find_arp_request_cache(uint32_t dp_key, uint32_t

[ovs-dev] [PATCH v2] ovn: ARP reply security vulnerability.

[nickcooper-zhangtonghao]

The the logical routers check only the "arp.op == 2" for ARP replies
and then use ARP replies to populate the logical router's ARP table.
If we continue to send ARP replies, which have different "arp.spa" and
"arp.sha", to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and crashes.

So,
1. When logical router receive an ARP reply packet, we should check the
   packet's "arp.tpa" and "arp.spa".
2. The logical router uses a cache to store the ARP request. Only when
   logical routers send an ARP request packet, and get a corresponding
   ARP reply, will the 'ovn-controller' update MAC_Binding table of SB
   database.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/pinctrl.c | 152 ++-
 ovn/northd/ovn-northd.c  |  22 ---
 2 files changed, 166 insertions(+), 8 deletions(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 8a759cd..bba7b29 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -58,12 +58,18 @@ static void pinctrl_handle_put_mac_binding(const struct 
flow *md,
const struct flow *headers,
bool is_arp);
 static void init_put_mac_bindings(void);
+static void init_arp_request_caches(void);
 static void destroy_put_mac_bindings(void);
+static void destroy_arp_request_caches(void);
 static void run_put_mac_bindings(struct controller_ctx *,
  const struct lport_index *lports);
 static void wait_put_mac_bindings(struct controller_ctx *);
 static void flush_put_mac_bindings(void);

+static void flush_arp_request_caches(void);
+static void pinctrl_handle_arp_request_cache(uint32_t dp_key,
+ uint32_t port_key,
+ const char *ip_s);
 static void init_send_garps(void);
 static void destroy_send_garps(void);
 static void send_garp_wait(void);
@@ -85,6 +91,7 @@ pinctrl_init(void)
 swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
 conn_seq_no = 0;
 init_put_mac_bindings();
+init_arp_request_caches();
 init_send_garps();
 }
 
@@ -185,6 +192,17 @@ pinctrl_handle_arp(const struct flow *ip_flow, const 
struct match *md,
 enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
 queue_msg(ofputil_encode_packet_out(&po, proto));
 
+/* The logical router uses a cache to store the ARP request.
+ * Only when logical routers send an ARP request packet, and get a
+ * corresponding ARP reply, will the 'ovn-controller' update
+ * mac_binding table of SB database. */
+uint32_t dp_key = ntohll(md->flow.metadata);
+uint32_t port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
+char ip_s[INET_ADDRSTRLEN];
+inet_ntop(AF_INET, &ip_flow->nw_dst, ip_s, sizeof(ip_s));
+
+pinctrl_handle_arp_request_cache(dp_key, port_key, ip_s);
+
 exit:
 dp_packet_uninit(&packet);
 ofpbuf_uninit(&ofpacts);
@@ -801,6 +819,7 @@ pinctrl_destroy(void)
 {
 rconn_destroy(swconn);
 destroy_put_mac_bindings();
+destroy_arp_request_caches();
 destroy_send_garps();
 }
 
@@ -830,8 +849,22 @@ struct put_mac_binding {
 struct eth_addr mac;
 };
 
+/* Buffered "arp_request_cache" operation. */
+struct arp_request_cache {
+struct hmap_node hmap_node; /* In 'arp_request_caches'. */
+
+long long int timestamp;/* In milliseconds. */
+
+/* Key. */
+uint32_t dp_key;
+uint32_t port_key;
+char ip_s[INET6_ADDRSTRLEN + 1];
+};
+
 /* Contains "struct put_mac_binding"s. */
 static struct hmap put_mac_bindings;
+/* Contains "struct arp_request_cache"s. */
+static struct hmap arp_request_caches;
 
 static void
 init_put_mac_bindings(void)
@@ -862,6 +895,100 @@ pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t 
port_key,
 }
 
 static void
+init_arp_request_caches(void)
+{
+hmap_init(&arp_request_caches);
+}
+
+static void
+destroy_arp_request_caches(void)
+{
+flush_arp_request_caches();
+hmap_destroy(&arp_request_caches);
+}
+
+static bool
+pinctrl_find_delete_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+ const char *ip_s)
+{
+struct arp_request_cache *arc, *next;
+HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
+if (arc->dp_key == dp_key
+&& arc->port_key == port_key
+&& !strcmp(arc->ip_s, ip_s)) {
+hmap_remove(&arp_request_caches, &arc->hmap_node);
+free(arc);
+return true;
+}
+}
+
+return false;
+}
+
+static struct arp_request_cache *
+pinctrl_find_arp_request_cache(uint32_t dp_key, uint32_t

[ovs-dev] [PATCH] ovn: ND security vulnerability.

[nickcooper-zhangtonghao]

The the logical routers will populate the logical router's ND table when
receiving the NS/ND packets. If we continue to send ND advertisements or
solicitations to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and crashes.

So,
1. When logical router receive a neighbor advertisements, we should check the
   packet's "ip6.dst" and "ip6.src".
2. The logical router uses a cache to store the neighbor solicitations which
   router sends. Only when logical routers send a neighbor solicitations,
   and get a corresponding neighbor advertisements, will the 'ovn-controller'
   update MAC_Binding table of SB database.

Signed-off-by: nickcooper-zhangtonghao 
---
 include/ovn/actions.h|  7 +
 ovn/controller/pinctrl.c | 81 
 ovn/lib/actions.c| 28 +
 ovn/northd/ovn-northd.c  | 46 ++-
 4 files changed, 141 insertions(+), 21 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 6f6f858..2e41675 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -60,6 +60,7 @@ struct simap;
 OVNACT(CT_SNAT,   ovnact_ct_nat)\
 OVNACT(CT_LB, ovnact_ct_lb) \
 OVNACT(ARP,   ovnact_nest)  \
+OVNACT(ND_NS, ovnact_nest)  \
 OVNACT(ND_NA, ovnact_nest)  \
 OVNACT(GET_ARP,   ovnact_get_mac_bind)  \
 OVNACT(PUT_ARP,   ovnact_put_mac_bind)  \
@@ -321,6 +322,12 @@ enum action_opcode {
  */
 ACTION_OPCODE_PUT_DHCP_OPTS,
 
+/* "nd_ns { ...actions... }".
+ *
+ * The actions, in OpenFlow 1.3 format, follow the action_header.
+ */
+ACTION_OPCODE_ND_NS,
+
 /* "nd_na { ...actions... }".
  *
  * The actions, in OpenFlow 1.3 format, follow the action_header.
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index bba7b29..9728e12 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -77,6 +77,9 @@ static void send_garp_run(const struct ovsrec_bridge *,
   const char *chassis_id,
   const struct lport_index *lports,
   struct hmap *local_datapaths);
+static void pinctrl_handle_nd_ns(const struct flow *ip_flow,
+ const struct match *md,
+ struct ofpbuf *userdata);
 static void pinctrl_handle_nd_na(const struct flow *ip_flow,
  const struct match *md,
  struct ofpbuf *userdata);
@@ -715,6 +718,10 @@ process_packet_in(const struct ofp_header *msg)
 pinctrl_handle_put_dhcp_opts(&packet, &pin, &userdata, &continuation);
 break;
 
+case ACTION_OPCODE_ND_NS:
+pinctrl_handle_nd_ns(&headers, &pin.flow_metadata, &userdata);
+break;
+
 case ACTION_OPCODE_ND_NA:
 pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
 break;
@@ -1006,12 +1013,9 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
 
 /* Check to see whether the corresponding ARP request exists in the cache.
  * If we don't find it, return directly. */
-if (is_arp) {
-bool update = pinctrl_find_delete_arp_request_cache(dp_key, port_key, 
ip_s);
-
-if (!update) {
-return;
-}
+bool update = pinctrl_find_delete_arp_request_cache(dp_key, port_key, 
ip_s);
+if (!update) {
+return;
 }
 
 uint32_t hash = hash_string(ip_s, hash_2words(dp_key, port_key));
@@ -1398,6 +1402,71 @@ reload_metadata(struct ofpbuf *ofpacts, const struct 
match *md)
 }
 
 static void
+pinctrl_handle_nd_ns(const struct flow *ip_flow, const struct match *md,
+ struct ofpbuf *userdata)
+{
+/* This action only works for IPv6 ND packets, and the switch should only
+ * send us ND packets this way, but check here just to be sure. */
+if (ip_flow->dl_type != htons(ETH_TYPE_IPV6)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(&rl, "NS action on non-ND packet");
+return;
+}
+
+enum ofp_version version = rconn_get_version(swconn);
+enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
+
+uint64_t packet_stub[128 / 8];
+struct dp_packet packet;
+dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
+
+compose_nd_ns(&packet, ip_flow->dl_src,
+&ip_flow->ipv6_src, &ip_flow->ipv6_dst);
+
+/* Reload previous packet metadata. */
+uint64_t ofpacts_stub[4096 / 8];
+struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
+reload_metadata(&ofpacts, md);
+
+enum ofperr er

[ovs-dev] [PATCH v2 0/2] ovn: ARP reply security vulnerability.

[nickcooper-zhangtonghao]

The the logical routers check only the "arp.op == 2" for ARP replies
and then use ARP replies to populate the logical router's ARP table.
If we continue to send ARP replies, which have different "arp.spa" and
"arp.sha", to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and crashes.

So,
1. When logical router receive an ARP reply packet, we should check the
   packet's "arp.tpa" and "arp.spa".
2. The logical router uses a cache to store the ARP request. Only when
   logical routers send an ARP request packet, and get a corresponding
   ARP reply, will the 'ovn-controller' update MAC_Binding table of SB
   database.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/pinctrl.c | 152 ++-
 ovn/northd/ovn-northd.c  |  22 ---
 2 files changed, 166 insertions(+), 8 deletions(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 8a759cd..bba7b29 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -58,12 +58,18 @@ static void pinctrl_handle_put_mac_binding(const struct 
flow *md,
const struct flow *headers,
bool is_arp);
 static void init_put_mac_bindings(void);
+static void init_arp_request_caches(void);
 static void destroy_put_mac_bindings(void);
+static void destroy_arp_request_caches(void);
 static void run_put_mac_bindings(struct controller_ctx *,
  const struct lport_index *lports);
 static void wait_put_mac_bindings(struct controller_ctx *);
 static void flush_put_mac_bindings(void);

+static void flush_arp_request_caches(void);
+static void pinctrl_handle_arp_request_cache(uint32_t dp_key,
+ uint32_t port_key,
+ const char *ip_s);
 static void init_send_garps(void);
 static void destroy_send_garps(void);
 static void send_garp_wait(void);
@@ -85,6 +91,7 @@ pinctrl_init(void)
 swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
 conn_seq_no = 0;
 init_put_mac_bindings();
+init_arp_request_caches();
 init_send_garps();
 }
 
@@ -185,6 +192,17 @@ pinctrl_handle_arp(const struct flow *ip_flow, const 
struct match *md,
 enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
 queue_msg(ofputil_encode_packet_out(&po, proto));
 
+/* The logical router uses a cache to store the ARP request.
+ * Only when logical routers send an ARP request packet, and get a
+ * corresponding ARP reply, will the 'ovn-controller' update
+ * mac_binding table of SB database. */
+uint32_t dp_key = ntohll(md->flow.metadata);
+uint32_t port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
+char ip_s[INET_ADDRSTRLEN];
+inet_ntop(AF_INET, &ip_flow->nw_dst, ip_s, sizeof(ip_s));
+
+pinctrl_handle_arp_request_cache(dp_key, port_key, ip_s);
+
 exit:
 dp_packet_uninit(&packet);
 ofpbuf_uninit(&ofpacts);
@@ -801,6 +819,7 @@ pinctrl_destroy(void)
 {
 rconn_destroy(swconn);
 destroy_put_mac_bindings();
+destroy_arp_request_caches();
 destroy_send_garps();
 }
 
@@ -830,8 +849,22 @@ struct put_mac_binding {
 struct eth_addr mac;
 };
 
+/* Buffered "arp_request_cache" operation. */
+struct arp_request_cache {
+struct hmap_node hmap_node; /* In 'arp_request_caches'. */
+
+long long int timestamp;/* In milliseconds. */
+
+/* Key. */
+uint32_t dp_key;
+uint32_t port_key;
+char ip_s[INET6_ADDRSTRLEN + 1];
+};
+
 /* Contains "struct put_mac_binding"s. */
 static struct hmap put_mac_bindings;
+/* Contains "struct arp_request_cache"s. */
+static struct hmap arp_request_caches;
 
 static void
 init_put_mac_bindings(void)
@@ -862,6 +895,100 @@ pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t 
port_key,
 }
 
 static void
+init_arp_request_caches(void)
+{
+hmap_init(&arp_request_caches);
+}
+
+static void
+destroy_arp_request_caches(void)
+{
+flush_arp_request_caches();
+hmap_destroy(&arp_request_caches);
+}
+
+static bool
+pinctrl_find_delete_arp_request_cache(uint32_t dp_key, uint32_t port_key,
+ const char *ip_s)
+{
+struct arp_request_cache *arc, *next;
+HMAP_FOR_EACH_SAFE (arc, next, hmap_node, &arp_request_caches) {
+if (arc->dp_key == dp_key
+&& arc->port_key == port_key
+&& !strcmp(arc->ip_s, ip_s)) {
+hmap_remove(&arp_request_caches, &arc->hmap_node);
+free(arc);
+return true;
+}
+}
+
+return false;
+}
+
+static struct arp_request_cache *
+pinctrl_find_arp_request_cache(uint32_t dp_key, uint32_t

[ovs-dev] [PATCH v2 2/2] ovn: add "timeout" column in MAC_Binding

[nickcooper-zhangtonghao]

Add "timeout" column in MAC_Binding to support cache aging.
The ARP/NA cache expiring will be removed and logical router
send ARP/NS to get IP-MAC binding.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/pinctrl.c | 25 ++---
 ovn/ovn-sb.ovsschema |  3 ++-
 ovn/ovn-sb.xml   |  5 +
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 9728e12..fb8141b 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -1011,7 +1011,7 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
 inet_ntop(AF_INET6, &ip6, ip_s, sizeof(ip_s));
 }
 
-/* Check to see whether the corresponding ARP request exists in the cache.
+/* Check to see whether the corresponding ARP/NS request exists in the 
cache.
  * If we don't find it, return directly. */
 bool update = pinctrl_find_delete_arp_request_cache(dp_key, port_key, 
ip_s);
 if (!update) {
@@ -1042,9 +1042,6 @@ run_put_mac_binding(struct controller_ctx *ctx,
 const struct lport_index *lports,
 const struct put_mac_binding *pmb)
 {
-if (time_msec() > pmb->timestamp + 1000) {
-return;
-}
 
 /* Convert logical datapath and logical port key into lport. */
 const struct sbrec_port_binding *pb
@@ -1066,8 +1063,8 @@ run_put_mac_binding(struct controller_ctx *ctx,
  * port.
  *
  * XXX This is not very efficient. */
-const struct sbrec_mac_binding *b;
-SBREC_MAC_BINDING_FOR_EACH (b, ctx->ovnsb_idl) {
+const struct sbrec_mac_binding *b, *next;
+SBREC_MAC_BINDING_FOR_EACH_SAFE (b, next, ctx->ovnsb_idl) {
 if (!strcmp(b->logical_port, pb->logical_port)
 && !strcmp(b->ip, pmb->ip_s)) {
 if (strcmp(b->mac, mac_string)) {
@@ -1082,6 +1079,10 @@ run_put_mac_binding(struct controller_ctx *ctx,
 sbrec_mac_binding_set_logical_port(b, pb->logical_port);
 sbrec_mac_binding_set_ip(b, pmb->ip_s);
 sbrec_mac_binding_set_mac(b, mac_string);
+
+struct ds timeout = DS_EMPTY_INITIALIZER;
+ds_put_format(&timeout, "%lld", time_msec());
+sbrec_mac_binding_set_timeout(b, ds_cstr(&timeout));
 sbrec_mac_binding_set_datapath(b, pb->datapath);
 }
 
@@ -1093,9 +1094,19 @@ run_put_mac_bindings(struct controller_ctx *ctx,
 return;
 }
 
-/* The ARP request expiring will be remove.*/
+/* The ARP request expiring will be removed.*/
 run_update_arp_request_cache();
 
+/* The IP-MAC binding expiring for this logical
+ * port will be removed from the database. Expire time is
+ * 15min*/
+const struct sbrec_mac_binding *b, *next;
+SBREC_MAC_BINDING_FOR_EACH_SAFE (b, next, ctx->ovnsb_idl) {
+if (time_msec() > strtoll(b->timeout, NULL, 10) + 15*6) {
+sbrec_mac_binding_delete(b);
+}
+}
+
 const struct put_mac_binding *pmb;
 HMAP_FOR_EACH (pmb, hmap_node, &put_mac_bindings) {
 run_put_mac_binding(ctx, lports, pmb);
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
index 8604b4e..e9a68eb 100644
--- a/ovn/ovn-sb.ovsschema
+++ b/ovn/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Southbound",
 "version": "1.8.0",
-"cksum": "59582657 7376",
+"cksum": "413692163 7423",
 "tables": {
 "SB_Global": {
 "columns": {
@@ -126,6 +126,7 @@
 "logical_port": {"type": "string"},
 "ip": {"type": "string"},
 "mac": {"type": "string"},
+"timeout": {"type": "string"},
 "datapath": {"type": {"key": {"type": "uuid",
   "refTable": 
"Datapath_Binding",
 "indexes": [["logical_port", "ip"]],
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 0829cbc..233ea30 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1960,6 +1960,11 @@ tcp.flags = RST;
 
   The Ethernet address to which the IP is bound.
 
+
+
+  The ARP/ND cache timeout.
+
+
 
   The logical datapath to which the logical port belongs.
 
-- 
1.8.3.1



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

[ovs-dev] [PATCH v2 1/2] ovn: ND security vulnerability.

[nickcooper-zhangtonghao]

The the logical routers will populate the logical router's ND table when
receiving the NS/ND packets. If we continue to send ND advertisements or
solicitations to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and crashes.

So,
1. When logical router receive a neighbor advertisements, we should check the
   packet's "ip6.dst" and "ip6.src".
2. The logical router uses a cache to store the neighbor solicitations which
   router sends. Only when logical routers send a neighbor solicitations,
   and get a corresponding neighbor advertisements, will the 'ovn-controller'
   update MAC_Binding table of SB database.

Signed-off-by: nickcooper-zhangtonghao 
---
 include/ovn/actions.h|  7 +
 ovn/controller/pinctrl.c | 81 
 ovn/lib/actions.c| 28 +
 ovn/northd/ovn-northd.c  | 46 ++-
 4 files changed, 141 insertions(+), 21 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 6f6f858..2e41675 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -60,6 +60,7 @@ struct simap;
 OVNACT(CT_SNAT,   ovnact_ct_nat)\
 OVNACT(CT_LB, ovnact_ct_lb) \
 OVNACT(ARP,   ovnact_nest)  \
+OVNACT(ND_NS, ovnact_nest)  \
 OVNACT(ND_NA, ovnact_nest)  \
 OVNACT(GET_ARP,   ovnact_get_mac_bind)  \
 OVNACT(PUT_ARP,   ovnact_put_mac_bind)  \
@@ -321,6 +322,12 @@ enum action_opcode {
  */
 ACTION_OPCODE_PUT_DHCP_OPTS,
 
+/* "nd_ns { ...actions... }".
+ *
+ * The actions, in OpenFlow 1.3 format, follow the action_header.
+ */
+ACTION_OPCODE_ND_NS,
+
 /* "nd_na { ...actions... }".
  *
  * The actions, in OpenFlow 1.3 format, follow the action_header.
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index bba7b29..9728e12 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -77,6 +77,9 @@ static void send_garp_run(const struct ovsrec_bridge *,
   const char *chassis_id,
   const struct lport_index *lports,
   struct hmap *local_datapaths);
+static void pinctrl_handle_nd_ns(const struct flow *ip_flow,
+ const struct match *md,
+ struct ofpbuf *userdata);
 static void pinctrl_handle_nd_na(const struct flow *ip_flow,
  const struct match *md,
  struct ofpbuf *userdata);
@@ -715,6 +718,10 @@ process_packet_in(const struct ofp_header *msg)
 pinctrl_handle_put_dhcp_opts(&packet, &pin, &userdata, &continuation);
 break;
 
+case ACTION_OPCODE_ND_NS:
+pinctrl_handle_nd_ns(&headers, &pin.flow_metadata, &userdata);
+break;
+
 case ACTION_OPCODE_ND_NA:
 pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
 break;
@@ -1006,12 +1013,9 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
 
 /* Check to see whether the corresponding ARP request exists in the cache.
  * If we don't find it, return directly. */
-if (is_arp) {
-bool update = pinctrl_find_delete_arp_request_cache(dp_key, port_key, 
ip_s);
-
-if (!update) {
-return;
-}
+bool update = pinctrl_find_delete_arp_request_cache(dp_key, port_key, 
ip_s);
+if (!update) {
+return;
 }
 
 uint32_t hash = hash_string(ip_s, hash_2words(dp_key, port_key));
@@ -1398,6 +1402,71 @@ reload_metadata(struct ofpbuf *ofpacts, const struct 
match *md)
 }
 
 static void
+pinctrl_handle_nd_ns(const struct flow *ip_flow, const struct match *md,
+ struct ofpbuf *userdata)
+{
+/* This action only works for IPv6 ND packets, and the switch should only
+ * send us ND packets this way, but check here just to be sure. */
+if (ip_flow->dl_type != htons(ETH_TYPE_IPV6)) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(&rl, "NS action on non-ND packet");
+return;
+}
+
+enum ofp_version version = rconn_get_version(swconn);
+enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
+
+uint64_t packet_stub[128 / 8];
+struct dp_packet packet;
+dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
+
+compose_nd_ns(&packet, ip_flow->dl_src,
+&ip_flow->ipv6_src, &ip_flow->ipv6_dst);
+
+/* Reload previous packet metadata. */
+uint64_t ofpacts_stub[4096 / 8];
+struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
+reload_metadata(&ofpacts, md);
+
+enum ofperr er

[ovs-dev] [PATCH 0/3] ovn: Make the ARP/ND more secure.

[nickcooper-zhangtonghao]

IPv6 defines an NUD mechanism that can help determine quickly whether
neighbors have disconnected or gone down. The OVN uses the same mechanism
for both IPv4 and IPv6. According to Section 7.3.2 in RFC 2461, A Neighbor
Cache entry should be in one of five states: INCOMPLETE, REACHABLE, STALE,
DELAY, PROBE. The OVN supports only INCOMPLETE, REACHABLE, PROBE in this patch.

You can find more information in RFC 2461 section 7.3.2.

* The logical routers use a cache to store the ARP/NS requests and set their
state INCOMPLETE. Only when logical routers get a corresponding ARP/NS reply,
will the "ovn-controller" update the cache and set their state NUD_REACHABLE.

* More than ReachableTime milliseconds have elapsed since the last positive
confirmation was received that the forward path was functioning properly.
If no reachability confirmation is received within NEIGHBOUR_REACHABLE_TIME
seconds, send a Neighbor Solicitation and change the state to PROBE.

* According to Section 4.6 in RFC 5944, we should update the MAC address of 
existing
entries in the case of gratuitous ARP.

nickcooper-zhangtonghao (3):
  ovn: Don't learn from unrequested ARP replies.
  ovn: Don't learn from ND packets.
  ovn: update manpage, ovn-sb.xml

 include/ovn/actions.h   |   7 +
 ovn/controller/ovn-controller.c |   2 +-
 ovn/controller/pinctrl.c| 356 +---
 ovn/controller/pinctrl.h|   2 +-
 ovn/lib/actions.c   |  29 +++-
 ovn/lib/automake.mk |   1 +
 ovn/northd/ovn-northd.c |  84 +++---
 ovn/ovn-sb.xml  |  39 +
 ovn/utilities/ovn-trace.c   |  25 +++
 9 files changed, 423 insertions(+), 122 deletions(-)

-- 
1.8.3.1

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

[ovs-dev] [PATCH 2/3] ovn: Don't learn from ND packets.

[nickcooper-zhangtonghao]

The the logical routers will populate the logical router's ND table when
receiving the NS/NA packets. If we continue to send ND advertisements or
solicitations to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability and
crashes. This patches may fix this bug.

Signed-off-by: nickcooper-zhangtonghao 
---
 include/ovn/actions.h |  7 
 ovn/controller/pinctrl.c  | 99 +++
 ovn/lib/actions.c | 29 +-
 ovn/northd/ovn-northd.c   | 49 ---
 ovn/utilities/ovn-trace.c | 25 
 5 files changed, 186 insertions(+), 23 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index e2a716a..b1f67be 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -61,6 +61,7 @@ struct simap;
 OVNACT(CT_SNAT,   ovnact_ct_nat)\
 OVNACT(CT_LB, ovnact_ct_lb) \
 OVNACT(ARP,   ovnact_nest)  \
+OVNACT(ND_NS, ovnact_nest)  \
 OVNACT(ND_NA, ovnact_nest)  \
 OVNACT(GET_ARP,   ovnact_get_mac_bind)  \
 OVNACT(PUT_ARP,   ovnact_put_mac_bind)  \
@@ -329,6 +330,12 @@ enum action_opcode {
  */
 ACTION_OPCODE_PUT_DHCP_OPTS,
 
+/* "nd_ns { ...actions... }".
+ *
+ * The actions, in OpenFlow 1.3 format, follow the action_header.
+ */
+ACTION_OPCODE_ND_NS,
+
 /* "nd_na { ...actions... }".
  *
  * The actions, in OpenFlow 1.3 format, follow the action_header.
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 177d0d4..9c39681 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -79,6 +79,9 @@ static void send_garp_run(const struct ovsrec_bridge *,
 static void pinctrl_handle_nd_na(const struct flow *ip_flow,
  const struct match *md,
  struct ofpbuf *userdata);
+static void pinctrl_handle_nd_ns(const struct flow *ip_flow,
+ const struct match *md,
+ struct ofpbuf *userdata);
 static void reload_metadata(struct ofpbuf *ofpacts,
 const struct match *md);
 
@@ -717,6 +720,10 @@ process_packet_in(const struct ofp_header *msg)
 pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
 break;
 
+case ACTION_OPCODE_ND_NS:
+pinctrl_handle_nd_ns(&headers, &pin.flow_metadata, &userdata);
+break;
+
 case ACTION_OPCODE_PUT_ND:
 pinctrl_handle_put_mac_binding(&pin.flow_metadata.flow, &headers,
false);
@@ -868,17 +875,29 @@ pinctrl_handle_neighbour_cache(uint32_t dp_key, uint32_t 
port_key,
 static void
 send_solicitation_request(const struct neighbour_cache * nc)
 {
-/* Compose a ARP request packet. */
+/* Compose a ARP/NS request packet. */
 uint64_t packet_stub[128 / 8];
 struct dp_packet packet;
-ovs_be32 nw_dst, nw_src;
 dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
 
-inet_pton(AF_INET, nc->nw_dst, &nw_dst);
-inet_pton(AF_INET, nc->nw_src, &nw_src);
+if (nc->type == OVN_NEIGHBOUR_TYPE_ARP) {
+ovs_be32 ip_dst, ip_src;
+inet_pton(AF_INET, nc->nw_dst, &ip_dst);
+inet_pton(AF_INET, nc->nw_src, &ip_src);
 
-compose_arp(&packet, ARP_OP_REQUEST, nc->eth_src, nc->eth_dst,
- true, nw_src, nw_dst);
+compose_arp(&packet, ARP_OP_REQUEST, nc->eth_src, nc->eth_dst,
+ true, ip_src, ip_dst);
+} else if (nc->type == OVN_NEIGHBOUR_TYPE_ND) {
+struct in6_addr ip6_dst, ip6_src;
+inet_pton(AF_INET6, nc->nw_dst, &ip6_dst);
+inet_pton(AF_INET6, nc->nw_src, &ip6_src);
+
+compose_nd_ns(&packet, nc->eth_src, &ip6_src, &ip6_dst);
+} else {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(&rl, "Unknown ovn neighbour type.");
+return;
+}
 
 /* Compose actions. */
 uint64_t ofpacts_stub[4096 / 8];
@@ -975,7 +994,8 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
 inet_ntop(AF_INET, &headers->nw_dst, ip_s, sizeof ip_s);
 ovs_strlcpy(nc->nw_src, ip_s, sizeof nc->nw_src);
 } else {
-/* ND codes. */
+inet_ntop(AF_INET6, &headers->ipv6_dst, ip_s, sizeof ip_s);
+ovs_strlcpy(nc->nw_src, ip_s, sizeof nc->nw_src);
 }
 }
 }
@@ -1433,6 +1453,71 @@ reload_metadata(struct ofpbuf *ofpacts, const struct 
match *md)
 }
 
 static void
+pinctrl_handle_nd_ns(const struct flow *ip_flow, const struct match *md,
+ struct ofpbuf *userdata)
+{
+/* This ac

[ovs-dev] [PATCH 1/3] ovn: Don't learn from unrequested ARP replies.

[nickcooper-zhangtonghao]

The the logical routers check only the "arp.op == 2" for ARP replies
and then use ARP replies to populate the logical router's ARP table.
If we continue to send ARP replies, which have different "arp.spa" and
"arp.sha", to logical router, the MAC_Binding table will continue to
increase. That may reduce system performance and cause instability
and crashes. This patches may fix this bug. IPv6 defines an NUD mechanism
that can help determine quickly whether neighbors have disconnected or
gone down. The OVN uses the same mechanism for both IPv4 and IPv6.
According to Section 7.3.2 in RFC 2461, A Neighbor Cache entry should
be in one of five states: INCOMPLETE, REACHABLE, STALE, DELAY, PROBE.
The OVN supports only INCOMPLETE, REACHABLE, PROBE in this patch.

You can find more information in RFC 2461 section 7.3.2.

* When logical router receive an ARP reply packet, we should check the
packet's "arp.tpa" and "arp.spa".

* The logical routers use a cache to store the ARP requests and set their
state INCOMPLETE. Only when logical routers get a corresponding ARP reply,
will the "ovn-controller" update the cache and set their state NUD_REACHABLE.

* More than ReachableTime milliseconds have elapsed since the last positive
confirmation was received that the forward path was functioning properly.
If no reachability confirmation is received within NEIGHBOUR_REACHABLE_TIME
seconds, send a Neighbor Solicitation and change the state to PROBE.

* According to Section 4.6 in RFC 5944, we should update the MAC address of 
existing
entries in the case of gratuitous ARP.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/controller/ovn-controller.c |   2 +-
 ovn/controller/pinctrl.c| 271 ++--
 ovn/controller/pinctrl.h|   2 +-
 ovn/lib/automake.mk |   1 +
 ovn/northd/ovn-northd.c |  35 --
 5 files changed, 205 insertions(+), 106 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 66a364f..b59550f 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -477,7 +477,7 @@ main(int argc, char *argv[])
 
 if (br_int) {
 ofctrl_wait();
-pinctrl_wait(&ctx);
+pinctrl_wait();
 }
 ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
 ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 2737467..177d0d4 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -38,6 +38,7 @@
 #include "ovn/actions.h"
 #include "ovn/lib/logical-fields.h"
 #include "ovn/lib/ovn-dhcp.h"
+#include "ovn/lib/ovn-neighbour.h"
 #include "ovn/lib/ovn-util.h"
 #include "poll-loop.h"
 #include "rconn.h"
@@ -57,13 +58,17 @@ static unsigned int conn_seq_no;
 static void pinctrl_handle_put_mac_binding(const struct flow *md,
const struct flow *headers,
bool is_arp);
-static void init_put_mac_bindings(void);
-static void destroy_put_mac_bindings(void);
+static void init_neighbour_caches(void);
+static void destroy_neighbour_caches(void);
 static void run_put_mac_bindings(struct controller_ctx *,
  const struct lport_index *lports);
-static void wait_put_mac_bindings(struct controller_ctx *);
-static void flush_put_mac_bindings(void);
+static void run_update_state_neighbour_cache(void);
 
+static void flush_neighbour_caches(void);
+static void pinctrl_handle_neighbour_cache(uint32_t dp_key,
+   uint32_t port_key,
+   const char *ip_s,
+   bool is_arp);
 static void init_send_garps(void);
 static void destroy_send_garps(void);
 static void send_garp_wait(void);
@@ -84,7 +89,7 @@ pinctrl_init(void)
 {
 swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
 conn_seq_no = 0;
-init_put_mac_bindings();
+init_neighbour_caches();
 init_send_garps();
 }
 
@@ -185,6 +190,17 @@ pinctrl_handle_arp(const struct flow *ip_flow, const 
struct match *md,
 enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version);
 queue_msg(ofputil_encode_packet_out(&po, proto));
 
+/* The logical routers use a cache to store the ARP requests and
+ * set their state NUD_INCOMPLETE. Only when logical routers get
+ * a corresponding ARP reply, will the 'ovn-controller' update
+ * the cache and set their state NUD_REACHABLE. */
+char ip_s[INET_ADDRSTRLEN];
+uint32_t dp_key = ntohll(md->flow.metadata);
+uint32_t port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
+
+inet_ntop(AF_INET, &ip_flow->nw_dst, ip_s, sizeo

[ovs-dev] [PATCH 3/3] ovn: update manpage, ovn-sb.xml

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-sb.xml | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index e119249..b6f022d 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1207,6 +1207,45 @@
   
 
 
+
+  nd_ns { action; ... };
+
+
+
+  
+Temporarily replaces the IPv6 packet being processed by an IPv6
+neighbor solicitation (NS) packet and executes each nested
+action on the NS packet.  Actions following the
+nd_ns action, if any, apply to the original,
+unmodified packet.
+  
+
+  
+The NS packet that this action operates on is initialized based on
+the IPv6 packet being processed, as follows. These are default
+values that the nested actions will probably want to change:
+  
+
+  
+eth.dst copied from nd.sll
+eth.type = 0x86dd
+ip6.dst copied from ip6.src
+ip6.src copied from nd.target
+icmp6.type = 135 (Neighbor Solicitation)
+nd.target unchanged
+nd.sll unchanged
+  
+
+  
+The NS packet has the same VLAN header, if any, as the IPv6 packet
+it replaces.
+  
+
+  
+Prerequisite: ip6
+  
+
+
 get_nd(P, A);
 
 
-- 
1.8.3.1

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

[ovs-dev] Load balancer creating method.

[nickcooper-zhangtonghao]

Hi all,
Can we create the “lb" from the “ovn-nbctl” command line?
manipulate directly the database?

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

Re: [ovs-dev] Load balancer creating method.

[nickcooper-zhangtonghao]

Thanks for your tips.

Why not provide the command line to create “lb”, such as “ovn-nbctl add-lb 
SWITCH other-aguments” ?

> On Aug 26, 2016, at 12:24 PM, Guru Shetty  wrote:
> 
> 
> Examples are in tests/system-ovn.at <http://system-ovn.at/>
> 
>> On Aug 25, 2016, at 6:21 PM, nickcooper-zhangtonghao 
>>  wrote:
>> 
>> 
>> Hi all,
>> Can we create the “lb" from the “ovn-nbctl” command line?
>> manipulate directly the database?

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

[ovs-dev] [PATCH 1/3] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 ++
 ovn/utilities/ovn-nbctl.c | 163 +-
 3 files changed, 171 insertions(+), 3 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 456ae98..87ce15f 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.3.1",
-"cksum": "1921908091 9353",
+"cksum": "360001470 9398",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -92,6 +92,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+"name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
@@ -102,7 +103,7 @@
 "external_ids": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}}},
-"isRoot": true},
+"isRoot": false},
 "ACL": {
 "columns": {
 "priority": {"type": {"key": {"type": "integer",
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 5719e74..c3666f5 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -660,6 +660,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This must be unique among all load 
balancers.
+  This name has no special meaning or purpose other than to provide 
convenience
+  for human interaction with the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index d6d64ea..fc02918 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -331,6 +331,13 @@ ACL commands:\n\
 remove ACLs from SWITCH\n\
   acl-list SWITCH   print ACLs for SWITCH\n\
 \n\
+LB commands:\n\
+  lb-add SWITCH LB VIP[:PORT] IP[:PORT]... [PROTOCOL]\n\
+add a load-balancer to SWITCH\n\
+  lb-del SWITCH [LB]\n\
+remove load-balancers from SWITCH\n\
+  lb-list SWITCH [LB]   print load-balancers for SWITCH\n\
+\n\
 Logical switch port commands:\n\
   lsp-add SWITCH PORT   add logical port PORT on SWITCH\n\
   lsp-add SWITCH PORT PARENT TAG\n\
@@ -1315,7 +1322,154 @@ nbctl_acl_del(struct ctl_context *ctx)
 }
 }
 }
-
+
+static void
+nbctl_lb_add(struct ctl_context *ctx)
+{
+const struct nbrec_logical_switch *ls;
+const char *lb_name = ctx->argv[2];
+const char *lb_vip = ctx->argv[3];
+const char *lb_ips = ctx->argv[4];
+
+ls = ls_by_name_or_uuid(ctx, ctx->argv[1], true);
+
+const char *lb_proto;
+if (ctx->argc == 5) {
+/* default protocol.*/
+lb_proto = "tcp";
+} else {
+lb_proto = ctx->argv[5];
+/* Validate protocol. */
+if (strcmp(lb_proto, "tcp") && strcmp(lb_proto, "udp")) {
+ctl_fatal("%s: protocol must be one of \"tcp\", \"udp\".", 
lb_proto);
+}
+}
+
+bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+for (int i = 0; i < ls->n_load_balancer; i++) {
+const struct nbrec_load_balancer *lb
+= ls->load_balancer[i];
+
+if (strcmp(lb->name, lb_name)) {
+continue;
+}
+
+if (!may_exist) {
+ctl_fatal("%s: a load balancer with this name already exists", 
lb_name);
+}
+
+nbrec_logical_switch_verify_load_balancer(ls);
+nbrec_load_balancer_verify_protocol(lb);
+nbrec_load_balancer_verify_vips(lb);
+nbrec_load_balancer_set_protocol(lb, lb_proto);
+
+struct smap vips = SMAP_INITIALIZER(&vips);
+smap_add(&vips, lb_vip, lb_ips);
+nbrec_load_balancer_set_vips(lb, &vips);
+smap_destroy(&vips);
+
+return;
+}
+
+/* Create the load balancer. */
+struct nbrec_load_balancer *lb = nbrec_load_balancer_insert(ctx->txn);
+nbrec_load_balancer_set_name(lb, lb_name);
+nbrec_load_balancer_set_protocol(lb, lb_proto);
+
+struct smap vips = SMAP_INITIALIZER(&vips);
+smap_add(&vips, lb_vip, lb_ips);
+nbrec_load_b

[ovs-dev] [PATCH 0/3] Add LB commands.

[nickcooper-zhangtonghao]

This patches provide the command line to create load-balancer and add
the unit tests.

nickcooper-zhangtonghao (3):
  vn-nbctl: Add LB commands.
  tests: Add LB commands unit tests.
  ovn-nbctl: Update manpage.

 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 ++
 ovn/utilities/ovn-nbctl.8.xml |  35 +
 ovn/utilities/ovn-nbctl.c | 163 +-
 tests/ovn-nbctl.at|  44 
 5 files changed, 250 insertions(+), 3 deletions(-)

-- 
1.8.3.1




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

[ovs-dev] [PATCH 3/3] ovn-nbctl: Update manpage.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index d44f039..fa908b7 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -102,6 +102,41 @@
   
 
 
+Logical Switch LB Commands
+
+  [--may-exist] lb-add switch 
lb vip ips [protocol]
+  
+Adds the specified LB to switch.  We should assign 
lb
+a virtual IPv4 address (and an optional port number with : as a 
separator)
+and the corresponding endpoint IPv4 addresses (and optional port 
numbers
+with : as separators) separated by commas.  The optional argument 
protocol
+must be either tcp or udp.  This argument is 
useful
+when a port number is provided as part of the vip.  If the 
protocol
+is unspecified and a port number is provided as part of vip,
+OVN assumes the protocol to be tcp.  It is an 
error if a load-balancer
+named lb already exists, unless --may-exist is 
specified.
+The following example adds a load-balancer with protocol 
udp:
+
+  lb-add ls0 lb0 30.0.0.10:80 
192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del switch 
[lb]
+  
+Deletes LBs from switch.  If only
+switch is supplied, all the LBs from the logical
+switch are deleted.  If lb is also specified,
+then the lb will be deleted only from the logical switch.
+It is an error if lb does not exist, unless 
--if-exists
+is specified.
+  
+
+  lb-list switch
+  
+Lists the LBs on switch.
+  
+
+
 Logical Switch Port Commands
 
   [--may-exist] lsp-add switch 
port
-- 
1.8.3.1



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

Re: [ovs-dev] [PATCH 0/3] Add LB commands.

[nickcooper-zhangtonghao]

Thanks for your review, I will submit the v2 patches soon.

> On Aug 30, 2016, at 1:14 AM, Guru Shetty  wrote:
> 
> 
> On 28 August 2016 at 02:05, nickcooper-zhangtonghao 
>  <mailto:nickcooper-zhangtong...@opencloud.tech>> wrote:
> This patches provide the command line to create load-balancer and add
> the unit tests.
> 
> Thank you for working on this. I have a few higher level comments.
> 1. I do not see the 2nd patch in the series. Neither in the mailing list or 
> in the patchwork.
> 2. You should merge the 3 patches into one in your next revision. Related 
> code change, man page change and unit test should be in a single commit.
> 3. I want the ability to have load-balancers created independently. i..e I 
> don't want to wait till I add it to a switch. This helps in creating a 
> load-balancer and adding it to multiple switches. Also, I have code sent out 
> for review to add the load-balancer to the gateway router too. So I suggest 
> waiting till that gets in to see what is a nice way to add ovn-nbctl commands.
> 4. I think the system tests will fail with this patch series. Please run: 
> make check-kernel TESTSUITEFLAGS="-k ovn"

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

[ovs-dev] [PATCH v2] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Add a name column for the load balancer.  This must be unique among
all load balancers.  This name has no special meaning or purpose
other than to provide convenience for human interaction with the
ovn-nb database.  This patch provides the command line to
create load-balancer and add the unit tests.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 ++
 ovn/utilities/ovn-nbctl.8.xml |  35 +
 ovn/utilities/ovn-nbctl.c | 163 +-
 tests/ovn-nbctl.at|  44 
 tests/system-ovn.at   |  13 ++--
 6 files changed, 255 insertions(+), 11 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 456ae98..87ce15f 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.3.1",
-"cksum": "1921908091 9353",
+"cksum": "360001470 9398",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -92,6 +92,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+"name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
@@ -102,7 +103,7 @@
 "external_ids": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}}},
-"isRoot": true},
+"isRoot": false},
 "ACL": {
 "columns": {
 "priority": {"type": {"key": {"type": "integer",
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 5719e74..c3666f5 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -660,6 +660,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This must be unique among all load 
balancers.
+  This name has no special meaning or purpose other than to provide 
convenience
+  for human interaction with the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index d44f039..fa908b7 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -102,6 +102,41 @@
   
 
 
+Logical Switch LB Commands
+
+  [--may-exist] lb-add switch 
lb vip ips [protocol]
+  
+Adds the specified LB to switch.  We should assign 
lb
+a virtual IPv4 address (and an optional port number with : as a 
separator)
+and the corresponding endpoint IPv4 addresses (and optional port 
numbers
+with : as separators) separated by commas.  The optional argument 
protocol
+must be either tcp or udp.  This argument is 
useful
+when a port number is provided as part of the vip.  If the 
protocol
+is unspecified and a port number is provided as part of vip,
+OVN assumes the protocol to be tcp.  It is an 
error if a load-balancer
+named lb already exists, unless --may-exist is 
specified.
+The following example adds a load-balancer with protocol 
udp:
+
+  lb-add ls0 lb0 30.0.0.10:80 
192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del switch 
[lb]
+  
+Deletes LBs from switch.  If only
+switch is supplied, all the LBs from the logical
+switch are deleted.  If lb is also specified,
+then the lb will be deleted only from the logical switch.
+It is an error if lb does not exist, unless 
--if-exists
+is specified.
+  
+
+  lb-list switch
+  
+Lists the LBs on switch.
+  
+
+
 Logical Switch Port Commands
 
   [--may-exist] lsp-add switch 
port
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index d6d64ea..fc02918 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -331,6 +331,13 @@ ACL commands:\n\
 remove ACLs from SWITCH\n\
   acl-list SWITCH   print ACLs for SWITCH\n\
 \n\
+LB commands:\n\
+  lb-add SWITCH LB VIP[:PORT] IP[:PORT]... [PROTOCOL]\n\
+add a load-balancer to SWITCH\n\
+  lb-del SWITCH [LB]\n\
+remove load-balancers from SWITCH\n\
+  lb-list SWITCH [LB]   print load-balancers for SWITCH\n

Re: [ovs-dev] [PATCH 3/3] ovn-nbctl: Update manpage.

[nickcooper-zhangtonghao]

Thanks for your tips. I have merged the 3 patches into one.

You can review it. http://patchwork.ozlabs.org/patch/663981/ 
<http://patchwork.ozlabs.org/patch/663981/>


> On Aug 31, 2016, at 6:06 AM, Ben Pfaff  wrote:
> 
> On Sun, Aug 28, 2016 at 02:05:18AM -0700, nickcooper-zhangtonghao wrote:
>> Signed-off-by: nickcooper-zhangtonghao 
>> > <mailto:nickcooper-zhangtong...@opencloud.tech>>
> 
> Code changes and its documentation should be in the same patch.
> 
> Thanks,
> 
> Ben.

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

Re: [ovs-dev] [PATCH v2] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Sorry for missing one of your points in the previous review.

I will submit v3 patch to support that function.

> On Aug 31, 2016, at 11:11 PM, Guru Shetty  wrote:
> 
> On 29 August 2016 at 20:12, nickcooper-zhangtonghao 
>  <mailto:nickcooper-zhangtong...@opencloud.tech>> wrote:
> Add a name column for the load balancer.  This must be unique among
> all load balancers.  This name has no special meaning or purpose
> other than to provide convenience for human interaction with the
> ovn-nb database.  This patch provides the command line to
> create load-balancer and add the unit tests.
> I think you missed one of my points in the previous review. I want to be able 
> to create the load balancer independently. 
>  i..e I don't want to wait till I add it to a switch. This helps in creating 
> a load-balancer and adding it to multiple switches. Also, I have code sent 
> out for review to add the load-balancer to the gateway router too. So the 
> commands should work with that too.
> 
> So with your patch, can I create a load balancer independently and add it to 
> multiple switches?

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

[ovs-dev] [PATCH v3] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it
to multiple switches. Add a name column for the load balancer.
With --add-duplicate, the command really creates a new load balancer
with a duplicate name. This name has no special meaning or
purpose other than to provide convenience for human interaction
with the ovn-nb database. This patch also provides the unit tests.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   3 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml |  52 
 ovn/utilities/ovn-nbctl.c | 275 +-
 tests/ovn-nbctl.at|  71 +++
 5 files changed, 405 insertions(+), 2 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 456ae98..d935475 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.3.1",
-"cksum": "1921908091 9353",
+"cksum": "440042936 9397",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -92,6 +92,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+"name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 5719e74..186abf8 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -660,6 +660,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index d44f039..297157c 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -102,6 +102,58 @@
   
 
 
+Logical Switch LB Commands
+
+  [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
+  
+Creates a new load balancer named lb.  We should assign
+lb a virtual IPv4 address (and an optional port number
+with : as a separator) and the corresponding endpoint IPv4 addresses
+(and optional port numbers with : as separators) separated by commas.
+The optional argument protocol must be either 
tcp
+or udp.  This argument is useful when a port number is
+provided as part of the vip.  If the protocol
+is unspecified and a port number is provided as part of vip,
+OVN assumes the protocol to be tcp.  It is an
+error if a load balancer named lb already exists, unless
+--may-exist is specified.  With 
--add-duplicate,
+the command really creates a new load balancer with a duplicate name.
+The following example adds a load balancer with protocol 
udp:
+
+  lb-add lb0 30.0.0.10:80 
192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del lb
+  
+Deletes lb.  It is an error if lb does not exist,
+unless --if-exists is specified.
+  
+
+  lb-list [switch] [lb]
+  
+Lists the LBs.  If switch is supplied, all the LBs from
+the logical switch are listed. If lb is also specified,
+then the lb will be listed only from the logical switch.
+  
+
+  [--may-exist] lb-append-to 
switch lb
+  
+Adds the specified lb to switch.
+It is an error if a load balancer named lb already exists
+in the switch, unless --may-exist is specified.
+  
+
+  [--if-exists] lb-remove-from 
switch [lb]
+  
+Deletes lb from switch.  If only 
switch is supplied,
+all the LBs from the logical switch are deleted.  If lb is 
also specified,
+then the lb will be deleted only from the logical switch.
+It is an error if lb does not exist in the 
switch,
+unless --if-exists is specified.
+  
+
+
 Logical Switch Port Commands
 
   [--may-exist] lsp-add switch 
port
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index d6d64ea..135d71e 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -331,6 +331,18 @@ ACL commands:\n\
 remove ACLs from SWITCH\n\
   acl-list SWITCH   print ACLs for SWITCH\n\
 \n\
+LB commands:\n\
+  lb-add LB VIP[:PORT] IP[:PORT]... [PROTOCOL]\n\
+   

Re: [ovs-dev] [PATCH v3] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

> On Sep 1, 2016, at 11:04 PM, Guru Shetty  wrote:
> 
> +AT_SETUP([ovn-nbctl - LBs])
> +OVN_NBCTL_TEST_START
> +
> +dnl Add two LBs
> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80  
> 192.168.10.10:80 ,192.168.10.20:80 
> ])
> +AT_CHECK([ovn-nbctl lb-add lb1 30.0.0.10:80  
> 192.168.10.10:80 ,192.168.10.20:80 
>  udp])
> +AT_CHECK([ovn-nbctl lb-list | ${PERL} $srcdir/uuidfilt.pl 
> ], [0], [dnl
> +UUID   LB PROTOVIP   
>IPs
> +<0>   lb0tcp  30.0.0.10:80  
> 192.168.10.10:80 ,192.168.10.20:80 
> 
> +<1>   lb1udp  30.0.0.10:80  
> 192.168.10.10:80 ,192.168.10.20:80 
> 
> +])
> +
> +dnl Update the lb1
> +AT_CHECK([ovn-nbctl --may-exist lb-add lb1 30.0.0.10:8080 
>  192.168.10.10:80 
> ,192.168.10.20:80  tcp])
> 
> A single load balancer can have multiple vips. So it would be nice if 
> 'lb-add' does not replace an existing different vip, but rather add the new 
> vip. If the vip already exists, it should replace it with the newly provided 
> "ips".  The "protocol" applies to all the vips in a load-balancer.   
> (I have only looked at the documentation and unit tests to start with. )

Thanks for your review, I will submit v4 patch soon.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v4] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it
to multiple switches. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   3 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml |  57 
 ovn/utilities/ovn-nbctl.c | 307 +-
 tests/ovn-nbctl.at|  97 +
 5 files changed, 468 insertions(+), 2 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index 456ae98..d935475 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.3.1",
-"cksum": "1921908091 9353",
+"cksum": "440042936 9397",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -92,6 +92,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+"name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 5719e74..186abf8 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -660,6 +660,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index d44f039..cf685a5 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -102,6 +102,63 @@
   
 
 
+Logical Switch LB Commands
+
+  [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
+  
+Creates a new load balancer named lb with the 
vip or
+adds the vip to lb.  A single load balancer can
+have multiple vips.  We should assign lb a virtual IPv4 
address
+(and an optional port number with : as a separator) and the 
corresponding
+endpoint IPv4 addresses (and optional port numbers with : as 
separators)
+separated by commas.  The optional argument protocol must be
+either tcp or udp.  This argument is useful 
when
+a port number is provided as part of the vip.  If the 
protocol
+is unspecified and a port number is provided as part of vip,
+OVN assumes the protocol to be tcp.  It is an
+error if the vip has been included in the load balancer 
named
+lb, unless --may-exist is specified.
+With --add-duplicate, the command really creates a new 
load
+balancer with a duplicate name.  The following example adds a load 
balancer
+with protocol udp:
+
+  lb-add lb0 30.0.0.10:80 
192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del lb 
[vip]
+  
+Deletes lb or the vip from lb.
+If vip is supplied, the vip will be deleted
+only from the lb.  If only lb is supplied,
+the lb will be deleted.  It is an error if vip
+does not be included in lb, unless --if-exists 
is specified.
+  
+
+  lb-list [switch] [lb]
+  
+Lists the LBs.  If switch is supplied, all the LBs from
+the logical switch are listed. If lb is also specified,
+then the lb will be listed only from the logical switch.
+  
+
+  [--may-exist] lb-append-to 
switch lb
+  
+Adds the specified lb to switch.
+It is an error if a load balancer named lb already exists
+in the switch, unless --may-exist is specified.
+  
+
+  [--if-exists] lb-remove-from 
switch [lb]
+  
+Deletes lb from switch.  If only 
switch is supplied,
+all the LBs from the logical switch are deleted.  If lb is 
also specified,
+then the lb will be deleted only from the logical switch.
+It is an error if lb does not exist in the 
switch,
+unless --if-exists is specified.
+  
+
+
 Logical Switch Port Commands
 
   [--may-exist] lsp-add switch 
port
diff --git a/ovn/utilities/ovn-nbc

[ovs-dev] [PATCH v5] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it to multiple
switches or routers. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   3 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml |  86 
 ovn/utilities/ovn-nbctl.c | 471 +-
 tests/ovn-nbctl.at| 147 +
 5 files changed, 711 insertions(+), 2 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index b7e70aa..7772ad2 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.3.3",
-"cksum": "2442952958 9945",
+"cksum": "1191768021 9975",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -97,6 +97,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+   "name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 9a8bdbd..2ebae29 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -676,6 +676,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 76cf97e..84a74ae 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -400,6 +400,92 @@
   
 
 
+Logical Switch/Router LB Commands
+
+  [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
+  
+Creates a new load balancer named lb with the 
vip or
+adds the vip to lb.  A single load balancer can
+have multiple vips.  We should assign lb a virtual IPv4 
address
+(and an optional port number with : as a separator) and the 
corresponding
+endpoint IPv4 addresses (and optional port numbers with : as 
separators)
+separated by commas.  The optional argument protocol must be
+either tcp or udp.  This argument is useful 
when
+a port number is provided as part of the vip.  If the 
protocol
+is unspecified and a port number is provided as part of vip,
+OVN assumes the protocol to be tcp.  It is an
+error if the vip has been included in the load balancer 
named
+lb, unless --may-exist is specified.
+With --add-duplicate, the command really creates a new 
load
+balancer with a duplicate name.  The following example adds a load 
balancer
+with protocol udp:
+
+  lb-add lb0 30.0.0.10:80 
192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del lb 
[vip]
+  
+Deletes lb or the vip from lb.
+If vip is supplied, the vip will be deleted
+only from the lb.  If only lb is supplied,
+the lb will be deleted.  It is an error if vip
+does not be included in lb, unless --if-exists 
is specified.
+  
+
+  lb-list [lb]
+  
+Lists the LBs.  If lb is also specified, then the 
lb will be listed.
+  
+
+  [--may-exist] ls-lb-add switch 
lb
+  
+Adds the specified lb to switch.
+It is an error if a load balancer named lb already exists
+in the switch, unless --may-exist is specified.
+  
+
+  [--if-exists] ls-lb-del switch 
[lb]
+  
+Deletes lb from switch.  If only 
switch is supplied,
+all the LBs from the logical switch are deleted.  If lb is 
also specified,
+then the lb will be deleted only from the logical switch.
+It is an error if lb does not exist in the 
switch,
+unless --if-exists is specified.
+  
+
+  ls-lb-list switch [lb]
+  
+Lists the LBs.  If switch is supplied, all the LBs from
+the logical switch are listed. If lb is also specified,
+then the lb will be listed only from the logical switch.
+ 

Re: [ovs-dev] [PATCH v4] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Yes, I have submitted the v5 patch which support for gateway router.
Thank you so much for your reviews.

> On Sep 10, 2016, at 5:54 AM, Guru Shetty  wrote:
> 
> On 1 September 2016 at 23:47, nickcooper-zhangtonghao 
>  <mailto:nickcooper-zhangtong...@opencloud.tech>> wrote:
> This patch provides the command line to create a load balancer.
> You can create a load balancer independently and add it
> to multiple switches. A single load balancer can have multiple vips.
> Add a name column for the load balancer. With --add-duplicate,
> the command really creates a new load balancer with a duplicate name.
> This name has no special meaning or purpose other than to provide
> convenience for human interaction with the ovn-nb database.
> This patch also provides the unit tests and the documentation.
> 
> Signed-off-by: nickcooper-zhangtonghao 
> 
> 
> Sorry for the delay on this. I was waiting for the load balancer addition to 
> gateway routers. Now that has been merged, would you please mind re-spinning 
> this with the feature to add it to a router too?

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

[ovs-dev] [PATCH v6] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it to multiple
switches or routers. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   3 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml | 109 +
 ovn/utilities/ovn-nbctl.c | 514 +-
 tests/ovn-nbctl.at| 147 
 5 files changed, 777 insertions(+), 2 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index b7e70aa..7772ad2 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
 "version": "5.3.3",
-"cksum": "2442952958 9945",
+"cksum": "1191768021 9975",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -97,6 +97,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+   "name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 9a8bdbd..2ebae29 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -676,6 +676,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 76cf97e..bdccc23 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -400,6 +400,115 @@
   
 
 
+Load Balancer Commands
+
+  [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
+  
+
+ Creates a new load balancer named lb with the provided
+ vip and ips or adds the vip to
+ an existing lb.  vip should be a
+ virtual IPv4 address (or an IPv4 address and a port number with
+ : as a separator).  Examples for vip are
+ 192.168.1.4 and 192.168.1.5:8080.
+ ips should be comma separated IPv4 endpoints (or comma
+ separated IPv4 addresses and port numbers with : as a
+ separator).  Examples for ips are 10.0.0.1,10.0.0.2
+ or 20.0.0.10:8800,20.0.0.11:8800.
+
+
+
+ The optional argument protocol must be either
+ tcp or udp.  This argument is useful when
+ a port number is provided as part of the vip.  If the
+ protocol is unspecified and a port number is provided as
+ part of the vip, OVN assumes the protocol to
+ be tcp.
+
+
+
+ It is an error if the vip already exists in the load
+ balancer named lb, unless --may-exist is
+ specified.  With --add-duplicate, the command really
+ creates a new load balancer with a duplicate name.
+
+
+
+ The following example adds a load balancer.
+
+
+
+ lb-add lb0 30.0.0.10:80
+ 192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del lb 
[vip]
+  
+Deletes lb or the vip from lb.
+If vip is supplied, only the vip will be
+deleted from the lb.  If only the lb is supplied,
+the lb will be deleted.  It is an error if vip
+does not already exist in lb, unless
+--if-exists is specified.
+  
+
+  lb-list [lb]
+  
+Lists the LBs.  If lb is also specified, then only the
+specified lb will be listed.
+  
+
+  [--may-exist] ls-lb-add switch 
lb
+  
+Adds the specified lb to switch.
+It is an error if a load balancer named lb already exists
+in the switch, unless --may-exist is specified.
+  
+
+  [--if-exists] ls-lb-del switch 
[lb]
+  
+Removes lb from switch.  If only
+switch is supplied, all the LBs from the logical switch are
+removed.  If lb is also specified, then only the
+lb will be removed from the logical switch.
+It is an error if lb does not exist in the
+   

Re: [ovs-dev] [PATCH v6] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

> On Sep 19, 2016, at 10:40 PM, Guru Shetty  wrote:
> 
> On 19 September 2016 at 05:16, nickcooper-zhangtonghao 
>  <mailto:nickcooper-zhangtong...@opencloud.tech>> wrote:
> This patch provides the command line to create a load balancer.
> You can create a load balancer independently and add it to multiple
> switches or routers. A single load balancer can have multiple vips.
> Add a name column for the load balancer. With --add-duplicate,
> the command really creates a new load balancer with a duplicate name.
> This name has no special meaning or purpose other than to provide
> convenience for human interaction with the ovn-nb database.
> This patch also provides the unit tests and the documentation.
> 
> Signed-off-by: nickcooper-zhangtonghao 
> 
> 
> I get the following compilation error:
> 
> ovn/utilities/ovn-nbctl.c: In function ‘nbctl_lb_list_router’:
> ovn/utilities/ovn-nbctl.c:1493:13: error: format not a string literal and no 
> format arguments [-Werror=format-security]
>  smap_add_format(lbs, ds_cstr(&key), ds_cstr(&val));
>  ^
> ovn/utilities/ovn-nbctl.c: In function ‘nbctl_lb_list_switch’:
> ovn/utilities/ovn-nbctl.c:1540:13: error: format not a string literal and no 
> format arguments [-Werror=format-security]
>  smap_add_format(lbs, ds_cstr(&key), ds_cstr(&val));
>  ^
> ovn/utilities/ovn-nbctl.c: In function ‘nbctl_lb_list_all’:
> ovn/utilities/ovn-nbctl.c:1584:13: error: format not a string literal and no 
> format arguments [-Werror=format-security]
>  smap_add_format(lbs, ds_cstr(&key), ds_cstr(&val));
>  ^
> cc1: all warnings being treated as errors
> make[2]: *** [ovn/utilities/ovn-nbctl.o] Error 1
> make[2]: *** Waiting for unfinished jobs
> make[2]: Leaving directory `/root/git/openvswitch'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/root/git/openvswitch'
> make: *** [all] Error 2
>  
> Can you please add to the unit test the scenario where we have multiple vips 
> in a single load balancer and lb-list it.. Also add the case wherein the vip 
> is just an IP address. Also, please update the version number in schema to 
> 5.3.4 (the rationale is explained in 'man ovs-vswitchd.conf.db'. Search for 
> 'db_version'.) 

I do not get the compilation error above. I submitted the v7 patch which 
updates the test case. Thanks for your review.




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

[ovs-dev] [PATCH v7] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it to multiple
switches or routers. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml | 109 +
 ovn/utilities/ovn-nbctl.c | 514 +-
 tests/ovn-nbctl.at| 167 ++
 5 files changed, 798 insertions(+), 3 deletions(-)

diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index b7e70aa..5f2f2bf 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "5.3.3",
-"cksum": "2442952958 9945",
+"version": "5.3.4",
+"cksum": "1155817817 9975",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -97,6 +97,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+   "name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index c45a444..b7690d0 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -676,6 +676,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 76cf97e..bdccc23 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -400,6 +400,115 @@
   
 
 
+Load Balancer Commands
+
+  [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
+  
+
+ Creates a new load balancer named lb with the provided
+ vip and ips or adds the vip to
+ an existing lb.  vip should be a
+ virtual IPv4 address (or an IPv4 address and a port number with
+ : as a separator).  Examples for vip are
+ 192.168.1.4 and 192.168.1.5:8080.
+ ips should be comma separated IPv4 endpoints (or comma
+ separated IPv4 addresses and port numbers with : as a
+ separator).  Examples for ips are 10.0.0.1,10.0.0.2
+ or 20.0.0.10:8800,20.0.0.11:8800.
+
+
+
+ The optional argument protocol must be either
+ tcp or udp.  This argument is useful when
+ a port number is provided as part of the vip.  If the
+ protocol is unspecified and a port number is provided as
+ part of the vip, OVN assumes the protocol to
+ be tcp.
+
+
+
+ It is an error if the vip already exists in the load
+ balancer named lb, unless --may-exist is
+ specified.  With --add-duplicate, the command really
+ creates a new load balancer with a duplicate name.
+
+
+
+ The following example adds a load balancer.
+
+
+
+ lb-add lb0 30.0.0.10:80
+ 192.168.10.10:80,192.168.10.20:80,192.168.10.30:80 udp
+
+  
+
+  [--if-exists] lb-del lb 
[vip]
+  
+Deletes lb or the vip from lb.
+If vip is supplied, only the vip will be
+deleted from the lb.  If only the lb is supplied,
+the lb will be deleted.  It is an error if vip
+does not already exist in lb, unless
+--if-exists is specified.
+  
+
+  lb-list [lb]
+  
+Lists the LBs.  If lb is also specified, then only the
+specified lb will be listed.
+  
+
+  [--may-exist] ls-lb-add switch 
lb
+  
+Adds the specified lb to switch.
+It is an error if a load balancer named lb already exists
+in the switch, unless --may-exist is specified.
+  
+
+  [--if-exists] ls-lb-del switch 
[lb]
+  
+Removes lb from switch.  If only
+switch is supplied, all the LBs from the logical switch are
+removed.  If lb is also specified, then only the
+lb will be removed from the logical switch.

Re: [ovs-dev] [PATCH v7] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

> On Sep 20, 2016, at 4:22 AM, Guru Shetty  wrote:
> 
> 
> On 19 September 2016 at 09:38, nickcooper-zhangtonghao 
>  <mailto:nickcooper-zhangtong...@opencloud.tech>> wrote:
> This patch provides the command line to create a load balancer.
> You can create a load balancer independently and add it to multiple
> switches or routers. A single load balancer can have multiple vips.
> Add a name column for the load balancer. With --add-duplicate,
> the command really creates a new load balancer with a duplicate name.
> This name has no special meaning or purpose other than to provide
> convenience for human interaction with the ovn-nb database.
> This patch also provides the unit tests and the documentation.
> 
> Signed-off-by: nickcooper-zhangtonghao 
> 
> 
> You should run your ./configure with ---enable-Werror option and you will not 
> miss any warnings as they are treated as errors.
> 
> For the case wherein the LB VIP is just a IP address, you should not 
> automatically give it a protocol.  The code does not look at protocol when 
> there are no L4 ports (as explained in ovn-nb documentation).  There are a 
> few lines in ovn-nbctl that exceed 78 characters. You can add the following 
> incremental to fix them (along with the compilation error). The incremental 
> does not fix the problem of automatic protocol allocation when no port is 
> given.


The 'ovn-nbctl lb-add’ will check the vip and ips. The protocol will be 
unnecessary when there are no L4 ports.
The ips should not contain port number if the vip does not contains a port 
number. each ip of ips should contain a
port number if the vip contains a port number. There are 16 characters for LB 
name. The v8 patch updates the test case.

Unfortunately， I can’t get the compilation error, ./configure —enable-Werror
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v8] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it to multiple
switches or routers. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/packets.c |  17 ++
 lib/packets.h |  10 +
 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml | 109 
 ovn/utilities/ovn-nbctl.c | 588 +-
 tests/ovn-nbctl.at| 228 
 7 files changed, 960 insertions(+), 3 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index e4c29d5..a8b1654 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -427,6 +427,23 @@ ip_parse(const char *s, ovs_be32 *ip)
 return inet_pton(AF_INET, s, ip) == 1;
 }
 
+/* Parses string 's', which must be an IP address with a port number.
+ * Stores the IP address into '*ip' and port number to '*port'. */
+char * OVS_WARN_UNUSED_RESULT
+ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
+{
+int n = 0;
+if (!ovs_scan_len(s, &n, IP_PORT_SCAN_FMT,
+IP_PORT_SCAN_ARGS(ip, port))) {
+return xasprintf("%s: invalid IP address or port number", s);
+}
+
+if (s[n]) {
+return xasprintf("%s: invalid IP address or port number", s);
+}
+return NULL;
+}
+
 /* Parses string 's', which must be an IP address with an optional netmask or
  * CIDR prefix length.  Stores the IP address into '*ip', netmask into '*mask',
  * (255.255.255.255, if 's' lacks a netmask), and number of scanned characters
diff --git a/lib/packets.h b/lib/packets.h
index dcfcd04..21bd35c 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -537,6 +537,14 @@ mpls_lse_to_bos(ovs_be32 mpls_lse)
 &((uint8_t *) ip)[2],   \
 &((uint8_t *) ip)[3]
 
+#define IP_PORT_SCAN_FMT "%"SCNu8".%"SCNu8".%"SCNu8".%"SCNu8":%"SCNu16
+#define IP_PORT_SCAN_ARGS(ip, port)\
+((void) (ovs_be32) *(ip), &((uint8_t *) ip)[0]),\
+&((uint8_t *) ip)[1],   \
+&((uint8_t *) ip)[2],   \
+&((uint8_t *) ip)[3],   \
+((void) (ovs_be16) *(port), (uint16_t *) port)
+
 /* Returns true if 'netmask' is a CIDR netmask, that is, if it consists of N
  * high-order 1-bits and 32-N low-order 0-bits. */
 static inline bool
@@ -558,6 +566,8 @@ ip_is_local_multicast(ovs_be32 ip)
 int ip_count_cidr_bits(ovs_be32 netmask);
 void ip_format_masked(ovs_be32 ip, ovs_be32 mask, struct ds *);
 bool ip_parse(const char *s, ovs_be32 *ip);
+char *ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
+OVS_WARN_UNUSED_RESULT;
 char *ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask)
 OVS_WARN_UNUSED_RESULT;
 char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index b7e70aa..5f2f2bf 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "5.3.3",
-"cksum": "2442952958 9945",
+"version": "5.3.4",
+"cksum": "1155817817 9975",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -97,6 +97,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+   "name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index c45a444..b7690d0 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -676,6 +676,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 76cf97e.

[ovs-dev] [PATCH v9] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it to multiple
switches or routers. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/packets.c |  18 ++
 lib/packets.h |  10 +
 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml | 105 +
 ovn/utilities/ovn-nbctl.c | 480 +-
 tests/ovn-nbctl.at| 218 +++
 7 files changed, 839 insertions(+), 3 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index e4c29d5..11bb587 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -427,6 +427,24 @@ ip_parse(const char *s, ovs_be32 *ip)
 return inet_pton(AF_INET, s, ip) == 1;
 }
 
+/* Parses string 's', which must be an IP address with a port number
+ * with ":" as a separator (e.g.: 192.168.1.2:80).
+ * Stores the IP address into '*ip' and port number to '*port'. */
+char * OVS_WARN_UNUSED_RESULT
+ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
+{
+int n = 0;
+if (!ovs_scan_len(s, &n, IP_PORT_SCAN_FMT,
+IP_PORT_SCAN_ARGS(ip, port))) {
+return xasprintf("%s: invalid IP address or port number", s);
+}
+
+if (s[n]) {
+return xasprintf("%s: invalid IP address or port number", s);
+}
+return NULL;
+}
+
 /* Parses string 's', which must be an IP address with an optional netmask or
  * CIDR prefix length.  Stores the IP address into '*ip', netmask into '*mask',
  * (255.255.255.255, if 's' lacks a netmask), and number of scanned characters
diff --git a/lib/packets.h b/lib/packets.h
index dcfcd04..21bd35c 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -537,6 +537,14 @@ mpls_lse_to_bos(ovs_be32 mpls_lse)
 &((uint8_t *) ip)[2],   \
 &((uint8_t *) ip)[3]
 
+#define IP_PORT_SCAN_FMT "%"SCNu8".%"SCNu8".%"SCNu8".%"SCNu8":%"SCNu16
+#define IP_PORT_SCAN_ARGS(ip, port)\
+((void) (ovs_be32) *(ip), &((uint8_t *) ip)[0]),\
+&((uint8_t *) ip)[1],   \
+&((uint8_t *) ip)[2],   \
+&((uint8_t *) ip)[3],   \
+((void) (ovs_be16) *(port), (uint16_t *) port)
+
 /* Returns true if 'netmask' is a CIDR netmask, that is, if it consists of N
  * high-order 1-bits and 32-N low-order 0-bits. */
 static inline bool
@@ -558,6 +566,8 @@ ip_is_local_multicast(ovs_be32 ip)
 int ip_count_cidr_bits(ovs_be32 netmask);
 void ip_format_masked(ovs_be32 ip, ovs_be32 mask, struct ds *);
 bool ip_parse(const char *s, ovs_be32 *ip);
+char *ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
+OVS_WARN_UNUSED_RESULT;
 char *ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask)
 OVS_WARN_UNUSED_RESULT;
 char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index b7e70aa..5f2f2bf 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "5.3.3",
-"cksum": "2442952958 9945",
+"version": "5.3.4",
+"cksum": "1155817817 9975",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -97,6 +97,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+   "name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index c45a444..b7690d0 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -676,6 +676,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/

Re: [ovs-dev] [PATCH v9] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Thanks for your tips, I fixed the memory leak(via free(ips)).
The patch follows the style of the original code. So the 'const' is not used 
with
'may_exist', 'add_duplicate' and 'must_exist'. You can find that in files, such 
as
'utilities/ovs-vsctl.c', 'ovn/utilities/ovn-sbctl.c' and 
'ovn/utilities/ovn-nbctl.c'.
If necessary, I will submit an individual patch. 



> On Sep 30, 2016, at 5:30 AM, Flavio Fernandes  wrote:
> 
> comments/suggestions inline.
> 
> Also, I'd think it would be good to run with valgrind [1] to see if there is 
> any error codepath
> that may be causing leaks.
> 
> -- flaviof

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

[ovs-dev] [PATCH v10] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create a load balancer.
You can create a load balancer independently and add it to multiple
switches or routers. A single load balancer can have multiple vips.
Add a name column for the load balancer. With --add-duplicate,
the command really creates a new load balancer with a duplicate name.
This name has no special meaning or purpose other than to provide
convenience for human interaction with the ovn-nb database.
This patch also provides the unit tests and the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/packets.c |  18 ++
 lib/packets.h |  10 +
 ovn/ovn-nb.ovsschema  |   5 +-
 ovn/ovn-nb.xml|   6 +
 ovn/utilities/ovn-nbctl.8.xml | 105 +
 ovn/utilities/ovn-nbctl.c | 486 +-
 tests/ovn-nbctl.at| 229 
 7 files changed, 856 insertions(+), 3 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index e4c29d5..11bb587 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -427,6 +427,24 @@ ip_parse(const char *s, ovs_be32 *ip)
 return inet_pton(AF_INET, s, ip) == 1;
 }
 
+/* Parses string 's', which must be an IP address with a port number
+ * with ":" as a separator (e.g.: 192.168.1.2:80).
+ * Stores the IP address into '*ip' and port number to '*port'. */
+char * OVS_WARN_UNUSED_RESULT
+ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
+{
+int n = 0;
+if (!ovs_scan_len(s, &n, IP_PORT_SCAN_FMT,
+IP_PORT_SCAN_ARGS(ip, port))) {
+return xasprintf("%s: invalid IP address or port number", s);
+}
+
+if (s[n]) {
+return xasprintf("%s: invalid IP address or port number", s);
+}
+return NULL;
+}
+
 /* Parses string 's', which must be an IP address with an optional netmask or
  * CIDR prefix length.  Stores the IP address into '*ip', netmask into '*mask',
  * (255.255.255.255, if 's' lacks a netmask), and number of scanned characters
diff --git a/lib/packets.h b/lib/packets.h
index dcfcd04..21bd35c 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -537,6 +537,14 @@ mpls_lse_to_bos(ovs_be32 mpls_lse)
 &((uint8_t *) ip)[2],   \
 &((uint8_t *) ip)[3]
 
+#define IP_PORT_SCAN_FMT "%"SCNu8".%"SCNu8".%"SCNu8".%"SCNu8":%"SCNu16
+#define IP_PORT_SCAN_ARGS(ip, port)\
+((void) (ovs_be32) *(ip), &((uint8_t *) ip)[0]),\
+&((uint8_t *) ip)[1],   \
+&((uint8_t *) ip)[2],   \
+&((uint8_t *) ip)[3],   \
+((void) (ovs_be16) *(port), (uint16_t *) port)
+
 /* Returns true if 'netmask' is a CIDR netmask, that is, if it consists of N
  * high-order 1-bits and 32-N low-order 0-bits. */
 static inline bool
@@ -558,6 +566,8 @@ ip_is_local_multicast(ovs_be32 ip)
 int ip_count_cidr_bits(ovs_be32 netmask);
 void ip_format_masked(ovs_be32 ip, ovs_be32 mask, struct ds *);
 bool ip_parse(const char *s, ovs_be32 *ip);
+char *ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
+OVS_WARN_UNUSED_RESULT;
 char *ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask)
 OVS_WARN_UNUSED_RESULT;
 char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
index b7e70aa..5f2f2bf 100644
--- a/ovn/ovn-nb.ovsschema
+++ b/ovn/ovn-nb.ovsschema
@@ -1,7 +1,7 @@
 {
 "name": "OVN_Northbound",
-"version": "5.3.3",
-"cksum": "2442952958 9945",
+"version": "5.3.4",
+"cksum": "1155817817 9975",
 "tables": {
 "NB_Global": {
 "columns": {
@@ -97,6 +97,7 @@
 "isRoot": true},
 "Load_Balancer": {
 "columns": {
+   "name": {"type": "string"},
 "vips": {
 "type": {"key": "string", "value": "string",
  "min": 0, "max": "unlimited"}},
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index c45a444..b7690d0 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -676,6 +676,12 @@
   Each row represents one load balancer.
 
 
+
+  A name for the load balancer.  This name has no special meaning or
+  purpose other than to provide convenience for human interaction with
+  the ovn-nb database.
+
+
 
   
 A map of virtual IPv4 addresses (and an optional port number with
diff --git a/ovn/

[ovs-dev] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

I have submitted the v10 patch, which can parse the ips
(e.g. ",,,192.168.10.1,,,” “,,,192.168.10.1,,,192.168.10.2,,,192.168.10.3,,,”). 
This patch
fixes the memory leak.

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

[ovs-dev] [PATCH 2/2] Fix memory leak in nbctl_lrp_add.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 563c6ec..71cffda 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1810,6 +1810,8 @@ nbctl_lrp_add(struct ctl_context *ctx)
 sset_add_array(&orig_networks, lrp->networks, lrp->n_networks);
 
 if (!sset_equals(&orig_networks, &new_networks)) {
+sset_destroy(&orig_networks);
+sset_destroy(&new_networks);
 ctl_fatal("%s: port already exists with different network",
   lrp_name);
 }
-- 
1.8.3.1




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

[ovs-dev] [PATCH 1/2] ovn-nbctl: check the length of MAC address

[nickcooper-zhangtonghao]

The command "ovn-nbctl lrp-add" should not set the MAC address
which length is invalid to logical router port. This patch also
updates the ovn-nbctl tests.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c | 7 ++-
 tests/ovn-nbctl.at| 7 +++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 2148665..563c6ec 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1836,7 +1836,12 @@ nbctl_lrp_add(struct ctl_context *ctx)
 }
 
 struct eth_addr ea;
-if (!ovs_scan(mac, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+int n = 0;
+if (!ovs_scan_len(mac, &n, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+ctl_fatal("%s: invalid mac address %s", lrp_name, mac);
+}
+
+if (mac[n]) {
 ctl_fatal("%s: invalid mac address %s", lrp_name, mac);
 }
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 241e6d3..3f459a8 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -301,6 +301,13 @@ AT_SETUP([ovn-nbctl - basic logical router port commands])
 OVN_NBCTL_TEST_START
 
 AT_CHECK([ovn-nbctl lr-add lr0])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02 192.168.1.1/24], [1], [],
+  [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02
+])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03:04 192.168.1.1/24], 
[1], [],
+  [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02:03:04
+])
+
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
 
 AT_CHECK([ovn-nbctl show lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
-- 
1.8.3.1



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

[ovs-dev] [PATCH 2/3] Fix memory leak in nbctl_lr_route_add.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c | 11 ++-
 tests/ovn-nbctl.at| 16 
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 7de8057..179c9eb 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1574,17 +1574,22 @@ nbctl_lr_route_add(struct ctl_context *ctx)
 
 next_hop = normalize_prefix_str(ctx->argv[3]);
 if (!next_hop) {
+free(prefix);
 ctl_fatal("bad next hop argument: %s", ctx->argv[3]);
 }
 
 if (strchr(prefix, '.')) {
 ovs_be32 hop_ipv4;
 if (!ip_parse(ctx->argv[3], &hop_ipv4)) {
+free(prefix);
+free(next_hop);
 ctl_fatal("bad IPv4 nexthop argument: %s", ctx->argv[3]);
 }
 } else {
 struct in6_addr hop_ipv6;
 if (!ipv6_parse(ctx->argv[3], &hop_ipv6)) {
+free(prefix);
+free(next_hop);
 ctl_fatal("bad IPv6 nexthop argument: %s", ctx->argv[3]);
 }
 }
@@ -1607,8 +1612,12 @@ nbctl_lr_route_add(struct ctl_context *ctx)
 }
 
 if (!may_exist) {
+char errm[60];
+snprintf(errm, sizeof errm, "duplicate prefix: %s", prefix);
+free(prefix);
+free(next_hop);
 free(rt_prefix);
-ctl_fatal("duplicate prefix: %s", prefix);
+ctl_fatal("%s", errm);
 }
 
 /* Update the next hop for an existing route. */
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 3f459a8..1a10194 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -411,6 +411,22 @@ dnl Add overlapping route with 10.0.0.1/24
 AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1], [1], [],
   [ovn-nbctl: duplicate prefix: 10.0.0.0/24
 ])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111a/24 11.0.0.1], [1], [],
+  [ovn-nbctl: bad prefix argument: 10.0.0.111a/24
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24a 11.0.0.1], [1], [],
+  [ovn-nbctl: bad prefix argument: 10.0.0.111/24a
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1a], [1], [],
+  [ovn-nbctl: bad next hop argument: 11.0.0.1a
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1/24], [1], [],
+  [ovn-nbctl: bad IPv4 nexthop argument: 11.0.0.1/24
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::1/64], 
[1], [],
+  [ovn-nbctl: bad IPv6 nexthop argument: 2001:0db8:0:f103::1/64
+])
+
 AT_CHECK([ovn-nbctl --may-exist lr-route-add lr0 10.0.0.111/24 11.0.0.1])
 
 AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
-- 
1.8.3.1



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

[ovs-dev] [PATCH 1/3] remove unreachable code.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 71cffda..7de8057 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1113,7 +1113,6 @@ nbctl_lsp_set_dhcpv4_options(struct ctl_context *ctx)
 if (error){
 free(error);
 ctl_fatal("DHCP options cidr '%s' is not IPv4", dhcp_opt->cidr);
-return;
 }
 }
 nbrec_logical_switch_port_set_dhcpv4_options(lsp, dhcp_opt);
@@ -1419,7 +1418,6 @@ nbctl_dhcp_options_create(struct ctl_context *ctx)
 if (error) {
 free(error);
 ctl_fatal("Invalid cidr format '%s'", ctx->argv[1]);
-return;
 }
 }
 
-- 
1.8.3.1




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

[ovs-dev] [PATCH 3/3] Fix memory leak in nbctl_lr_route_del.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 179c9eb..3f31047 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1699,7 +1699,10 @@ nbctl_lr_route_del(struct ctl_context *ctx)
 }
 
 if (!shash_find(&ctx->options, "--if-exists")) {
-ctl_fatal("no matching prefix: %s", prefix);
+char errm[60];
+snprintf(errm, sizeof errm, "no matching prefix: %s", prefix);
+free(prefix);
+ctl_fatal("%s", errm);
 }
 free(prefix);
 }
-- 
1.8.3.1




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

[ovs-dev] [PATCH 4/4] ovn-nbctl: Improve ovn-nbctl manpage

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 7cd515f..70798dc 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -173,10 +173,17 @@
 address.  Each address should be either an
 Ethernet address or an Ethernet address followed by IP addresses
 (separated by space and quoted to form a single command-line
-argument).  The special form unknown is also valid.
+argument).  The special form unknown is valid.
+When an OVN logical switch processes a unicast Ethernet frame
+whose destination MAC address is not in any logical port's addresses
+column, it delivers it to the port (or ports) whose addresses columns
+include unknown.  The special form dynamic is also valid.
+Use this keyword to make ovn-northd generate a globally unique MAC
+address and choose an unused IPv4 address with the logical port's
+subnet and store them in the port's dynamic_addresses column.
 Multiple Ethernet addresses or Ethernet+IPs combinations may be set.
-If no address argument is given, port will have
-no addresses associated with it.
+If no address argument is given, port will
+have no addresses associated with it.
   
 
   lsp-get-addresses port
-- 
1.8.3.1



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

[ovs-dev] [PATCH 2/4] ovn-nbctl: Check the length of MAC address

[nickcooper-zhangtonghao]

The command "ovn-nbctl lrp-add" should not set the MAC address
which length is invalid to logical router port. This patch
updates the eth_addr_from_string() to check trailing characters.
We should use the ovs_scan() to check the "addresses" owned by
the logical port, instead of eth_addr_from_string(). This patch
also updates the ovn-nbctl tests.

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/packets.c | 15 +--
 ovn/northd/ovn-northd.c   | 11 +++
 ovn/utilities/ovn-nbctl.c |  2 +-
 tests/ovn-nbctl.at|  7 +++
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index 11bb587..f661c34 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -142,16 +142,19 @@ eth_addr_is_reserved(const struct eth_addr ea)
 
 /* Attempts to parse 's' as an Ethernet address.  If successful, stores the
  * address in 'ea' and returns true, otherwise zeros 'ea' and returns
- * false.  */
+ * false.  This function checks trailing characters. */
 bool
 eth_addr_from_string(const char *s, struct eth_addr *ea)
 {
-if (ovs_scan(s, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(*ea))) {
-return true;
-} else {
-*ea = eth_addr_zero;
-return false;
+int n = 0;
+if (ovs_scan_len(s, &n, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(*ea))) {
+if (!s[n]) {
+return true;
+}
 }
+
+*ea = eth_addr_zero;
+return false;
 }
 
 /* Fills 'b' with a Reverse ARP packet with Ethernet source address 'eth_src'.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index eeeb41d..42b5423 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2912,9 +2912,12 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 }
 
 for (size_t i = 0; i < op->nbsp->n_addresses; i++) {
+/* Addresses are owned by the logical port.
+ * Ethernet address followed by zero or more IPv4
+ * or IPv6 addresses (or both). */
 struct eth_addr mac;
-
-if (eth_addr_from_string(op->nbsp->addresses[i], &mac)) {
+if (ovs_scan(op->nbsp->addresses[i],
+ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
 ds_clear(&match);
 ds_put_format(&match, "eth.dst == "ETH_ADDR_FMT,
   ETH_ADDR_ARGS(mac));
@@ -2930,8 +2933,8 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
*ports,
 }
 } else if (!strcmp(op->nbsp->addresses[i], "dynamic")) {
 if (!op->nbsp->dynamic_addresses
-|| !eth_addr_from_string(op->nbsp->dynamic_addresses,
-&mac)) {
+|| !ovs_scan(op->nbsp->dynamic_addresses,
+ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
 continue;
 }
 ds_clear(&match);
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 7643241..453ff72 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2299,7 +2299,7 @@ nbctl_lrp_add(struct ctl_context *ctx)
 }
 
 struct eth_addr ea;
-if (!ovs_scan(mac, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+if (!eth_addr_from_string(mac, &ea)) {
 ctl_fatal("%s: invalid mac address %s", lrp_name, mac);
 }
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index d8331f8..115d781 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -530,6 +530,13 @@ AT_SETUP([ovn-nbctl - basic logical router port commands])
 OVN_NBCTL_TEST_START
 
 AT_CHECK([ovn-nbctl lr-add lr0])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02 192.168.1.1/24], [1], [],
+  [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02
+])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03:04 192.168.1.1/24], 
[1], [],
+  [ovn-nbctl: lrp0: invalid mac address 00:00:00:01:02:03:04
+])
+
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24])
 
 AT_CHECK([ovn-nbctl show lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
-- 
1.8.3.1



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

[ovs-dev] [PATCH 1/4] ovn: make ipam tests more reliable

[nickcooper-zhangtonghao]

Fixes: 8639f9be("ovn-northd, tests: Adding IPAM to ovn-northd.")
The IPAM tests began to fail occasionally. Adding --wait=sb
to commands triggering address allocation eliminated these failures.

Signed-off-by: nickcooper-zhangtonghao 
---
 tests/ovn.at | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 2b193dd..73f1694 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4543,8 +4543,8 @@ ovn_start
 # Add a port to a switch that does not have a subnet set, then set the
 # subnet which should result in an address being allocated for the port.
 ovn-nbctl ls-add sw0
-ovn-nbctl lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
-ovn-nbctl add Logical-Switch sw0 other_config subnet=192.168.1.0/24
+ovn-nbctl --wait=sb lsp-add sw0 p0 -- lsp-set-addresses p0 dynamic
+ovn-nbctl --wait=sb add Logical-Switch sw0 other_config subnet=192.168.1.0/24
 AT_CHECK([ovn-nbctl get Logical-Switch-Port p0 dynamic_addresses], [0],
 ["0a:00:00:00:00:01 192.168.1.2"
 ])
@@ -4584,7 +4584,7 @@ AT_CHECK([ovn-nbctl get Logical-Switch-Port p9 
dynamic_addresses], [0],
 # Trying similar tests with a second switch. MAC addresses should be unique
 # across both switches but IP's only need to be unique within the same switch.
 ovn-nbctl ls-add sw1
-ovn-nbctl lsp-add sw1 p10 -- lsp-set-addresses p10 dynamic
+ovn-nbctl --wait=sb lsp-add sw1 p10 -- lsp-set-addresses p10 dynamic
 ovn-nbctl --wait=sb add Logical-Switch sw1 other_config subnet=192.168.1.0/24
 AT_CHECK([ovn-nbctl get Logical-Switch-Port p10 dynamic_addresses], [0],
  ["0a:00:00:00:00:0b 192.168.1.2"
@@ -4700,8 +4700,8 @@ ovn_start
 ovn-nbctl lr-add R1
 
 # Test for a ping using dynamically allocated addresses.
-ovn-nbctl ls-add foo -- add Logical_Switch foo other_config 
subnet=192.168.1.0/24
-ovn-nbctl ls-add alice -- add Logical_Switch alice other_config 
subnet=192.168.2.0/24
+ovn-nbctl --wait=sb ls-add foo -- add Logical_Switch foo other_config 
subnet=192.168.1.0/24
+ovn-nbctl --wait=sb ls-add alice -- add Logical_Switch alice other_config 
subnet=192.168.2.0/24
 
 # Connect foo to R1
 ovn-nbctl lrp-add R1 foo 00:00:00:01:02:03 192.168.1.1/24
@@ -4714,21 +4714,21 @@ ovn-nbctl lsp-add alice rp-alice -- set 
Logical_Switch_Port rp-alice type=router
   options:router-port=alice addresses=\"00:00:00:01:02:04\"
 
 # Create logical port foo1 in foo
-ovn-nbctl lsp-add foo foo1 \
+ovn-nbctl --wait=sb lsp-add foo foo1 \
 -- lsp-set-addresses foo1 "dynamic"
 AT_CHECK([ovn-nbctl get Logical-Switch-Port foo1 dynamic_addresses], [0],
  ["0a:00:00:00:00:01 192.168.1.2"
 ])
 
 # Create logical port alice1 in alice
-ovn-nbctl lsp-add alice alice1 \
+ovn-nbctl --wait=sb lsp-add alice alice1 \
 -- lsp-set-addresses alice1 "dynamic"
 AT_CHECK([ovn-nbctl get Logical-Switch-Port alice1 dynamic_addresses], [0],
  ["0a:00:00:00:00:02 192.168.2.2"
 ])
 
 # Create logical port foo2 in foo
-ovn-nbctl lsp-add foo foo2 \
+ovn-nbctl --wait=sb lsp-add foo foo2 \
 -- lsp-set-addresses foo2 "dynamic"
 AT_CHECK([ovn-nbctl get Logical-Switch-Port foo2 dynamic_addresses], [0],
  ["0a:00:00:00:00:03 192.168.1.3"
-- 
1.8.3.1



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

[ovs-dev] [PATCH 3/4] ovn-nbctl: Fix memory leak in nbctl_lr_route_add

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.c |  6 ++
 tests/ovn-nbctl.at| 16 
 2 files changed, 22 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 453ff72..572370f 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -2039,17 +2039,22 @@ nbctl_lr_route_add(struct ctl_context *ctx)
 
 next_hop = normalize_prefix_str(ctx->argv[3]);
 if (!next_hop) {
+free(prefix);
 ctl_fatal("bad next hop argument: %s", ctx->argv[3]);
 }
 
 if (strchr(prefix, '.')) {
 ovs_be32 hop_ipv4;
 if (!ip_parse(ctx->argv[3], &hop_ipv4)) {
+free(prefix);
+free(next_hop);
 ctl_fatal("bad IPv4 nexthop argument: %s", ctx->argv[3]);
 }
 } else {
 struct in6_addr hop_ipv6;
 if (!ipv6_parse(ctx->argv[3], &hop_ipv6)) {
+free(prefix);
+free(next_hop);
 ctl_fatal("bad IPv6 nexthop argument: %s", ctx->argv[3]);
 }
 }
@@ -2072,6 +2077,7 @@ nbctl_lr_route_add(struct ctl_context *ctx)
 }
 
 if (!may_exist) {
+free(next_hop);
 free(rt_prefix);
 ctl_fatal("duplicate prefix: %s", prefix);
 }
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 115d781..af00dad 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -640,6 +640,22 @@ dnl Add overlapping route with 10.0.0.1/24
 AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1], [1], [],
   [ovn-nbctl: duplicate prefix: 10.0.0.0/24
 ])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111a/24 11.0.0.1], [1], [],
+  [ovn-nbctl: bad prefix argument: 10.0.0.111a/24
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24a 11.0.0.1], [1], [],
+  [ovn-nbctl: bad prefix argument: 10.0.0.111/24a
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1a], [1], [],
+  [ovn-nbctl: bad next hop argument: 11.0.0.1a
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 10.0.0.111/24 11.0.0.1/24], [1], [],
+  [ovn-nbctl: bad IPv4 nexthop argument: 11.0.0.1/24
+])
+AT_CHECK([ovn-nbctl lr-route-add lr0 2001:0db8:1::/64 2001:0db8:0:f103::1/64], 
[1], [],
+  [ovn-nbctl: bad IPv6 nexthop argument: 2001:0db8:0:f103::1/64
+])
+
 AT_CHECK([ovn-nbctl --may-exist lr-route-add lr0 10.0.0.111/24 11.0.0.1])
 
 AT_CHECK([ovn-nbctl lr-route-list lr0], [0], [dnl
-- 
1.8.3.1



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

Re: [ovs-dev] [PATCH v10] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Thanks very much.

> On Oct 4, 2016, at 5:12 AM, Guru Shetty  wrote:
> 
> I applied the patch. For future, when you version, please have some sort of 
> versioning information that tells the difference between the versions below 
> the "---" line.

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

Re: [ovs-dev] [PATCH 1/4] ovn: make ipam tests more reliable

[nickcooper-zhangtonghao]

Good idea. I have submitted v2 patch.
> On Oct 6, 2016, at 8:30 AM, Ben Pfaff  wrote:
> 
> Thanks for working on this.
> 
> When there are multiple ovn-nbctl calls in a row, it's ordinarily only
> necessary to add --wait to the last call.  Can you make that change?

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

Re: [ovs-dev] [PATCH 1/4] ovn: make ipam tests more reliable

[nickcooper-zhangtonghao]

Yes, I submitted the v2 patch. I can not see also it in
http://patchwork.ozlabs.org/project/openvswitch/list/ 


There may an error in patchwork. I will send directly the patch to you.
 
> On Oct 6, 2016, at 11:34 PM, Ben Pfaff  wrote:
> 
> Are you sure?  I do not see it.

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

Re: [ovs-dev] [PATCH v10] ovn-nbctl: Add LB commands.

[nickcooper-zhangtonghao]

Good idea. I am interested in that. The patch will be submitted. Thanks.

> On Oct 8, 2016, at 2:09 AM, Guru Shetty  wrote:
> 
> On 5 October 2016 at 04:25, nickcooper-zhangtonghao 
>  <mailto:nickcooper-zhangtong...@opencloud.tech>> wrote:
> Thanks very much.
> 
> If it is of interest,  something similar can be done for NAT too.

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

[ovs-dev] [PATCH 2/2] ovn-nbctl: Fix manpage formatting typo.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index be802da..993e1c4 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -412,7 +412,7 @@
 
 
 
-  It is an error if prefix is specified and there
+  It is an error if prefix is specified and there
   is no matching route entry, unless --if-exists is
   specified.
 
-- 
1.8.3.1




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

[ovs-dev] [PATCH 1/2] ovn-nbctl: Add NAT commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create NAT rules
on logical router.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml |  66 +++
 ovn/utilities/ovn-nbctl.c | 185 ++
 tests/ovn-nbctl.at| 114 ++
 3 files changed, 365 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 2cbd6e0..be802da 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -424,6 +424,72 @@
   
 
 
+NAT Commands
+
+
+  [--may-exist] lr-nat-add router 
type external_ip logical_ip
+  
+
+  Adds the specified NAT to router.
+  The type must be one of snat,
+  dnat, or dnat_and_snat.
+  The external_ip is an IPv4 address.
+  The logical_ip is an IPv4 network (e.g 192.168.1.0/24)
+  or an IPv4 address.
+
+
+  When type is dnat, the externally
+  visible IP address external_ip is DNATted to the
+  IP address logical_ip in the logical space.
+
+
+  When type is snat, IP packets with their
+  source IP address that either matches the IP address in
+  logical_ip or is in the network provided by
+  logical_ip is SNATed into the IP address in
+  external_ip.
+
+
+  When type is dnat_and_snat,
+  the externally visible IP address external_ip
+  is DNATted to the IP address logical_ip in
+  the logical space.  In addition, IP packets with the source
+  IP address that matches logical_ip is SNATed into
+  the IP address in external_ip.
+
+
+  It is an error if a NAT already exists,
+  unless --may-exist is specified.
+
+  
+
+  [--if-exists] lr-nat-del router 
[type [ip]]
+  
+
+  Deletes NATs from router.  If only router
+  is supplied, all the NATs from the logical router are
+  deleted.  If type is also specified, then all the
+  NATs that match the type will be deleted from the logical
+  router.  If all the fields are given, then a single NAT rule
+  that matches all the fields will be deleted.  When type
+  is snat, the ip should be logical_ip.
+  When type is dnat or
+  dnat_and_snat, the ip shoud be external_ip.
+
+
+
+  It is an error if ip is specified and there
+  is no matching NAT entry, unless --if-exists is
+  specified.
+
+  
+
+  lr-nat-list router
+  
+Lists the NATs on router.
+  
+
+
 Load Balancer Commands
 
   [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index ad2d2f8..916dedb 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -384,6 +384,13 @@ Route commands:\n\
 remove routes from ROUTER\n\
   lr-route-list ROUTER  print routes for ROUTER\n\
 \n\
+NAT commands:\n\
+  lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP\n\
+add a NAT to ROUTER\n\
+  lr-nat-del ROUTER [TYPE [IP]]\n\
+remove NATs from ROUTER\n\
+  lr-nat-list ROUTERprint NATs for ROUTER\n\
+\n\
 LB commands:\n\
   lb-add LB VIP[:PORT] IP[:PORT]... [PROTOCOL]\n\
 create a load-balancer or add a VIP to an\n\
@@ -2165,6 +2172,177 @@ nbctl_lr_route_del(struct ctl_context *ctx)
 }
 free(prefix);
 }
+
+static void
+nbctl_lr_nat_add(struct ctl_context *ctx)
+{
+const struct nbrec_logical_router *lr;
+const char *nat_type = ctx->argv[2];
+const char *external_ip = ctx->argv[3];
+const char *logical_ip = ctx->argv[4];
+char *new_logical_ip = NULL;
+
+lr = lr_by_name_or_uuid(ctx, ctx->argv[1], true);
+
+if (strcmp(nat_type, "dnat") && strcmp(nat_type, "snat")
+&& strcmp(nat_type, "dnat_and_snat")) {
+ctl_fatal("%s: type must be one of \"dnat\", \"snat\" and "
+"\"dnat_and_snat\".", nat_type);
+}
+
+ovs_be32 ipv4 = 0;
+unsigned int plen;
+if (!ip_parse(external_ip, &ipv4)) {
+ctl_fatal("%s: should be an IPv4 address.", external_ip);
+}
+
+if (strcmp("snat", nat_type)) {
+if (!ip_parse(logical_ip, &ipv4)) {
+ctl_fatal("%s: should be an IPv4 address.", logical_ip);
+}
+new_logical_ip = xstrdup(logical_ip);
+} else {
+char *error = ip_parse_cidr(logical_ip, &ipv4, &plen);
+if (error) {
+free(error);
+ctl_fatal("%s: should be an IPv4 address or network.",
+

[ovs-dev] [PATCH 3/4] ovn-nbctl: Fix manpage formatting typo.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index be802da..993e1c4 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -412,7 +412,7 @@
 
 
 
-  It is an error if prefix is specified and there
+  It is an error if prefix is specified and there
   is no matching route entry, unless --if-exists is
   specified.
 
-- 
1.8.3.1




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

[ovs-dev] [PATCH 2/4] ovn-nbctl: Add NAT commands.

[nickcooper-zhangtonghao]

This patch provides the command line to create NAT rules
on logical router.

Signed-off-by: nickcooper-zhangtonghao 
---
 ovn/utilities/ovn-nbctl.8.xml |  66 ++
 ovn/utilities/ovn-nbctl.c | 203 ++
 tests/ovn-nbctl.at| 130 +++
 3 files changed, 399 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 2cbd6e0..be802da 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -424,6 +424,72 @@
   
 
 
+NAT Commands
+
+
+  [--may-exist] lr-nat-add router 
type external_ip logical_ip
+  
+
+  Adds the specified NAT to router.
+  The type must be one of snat,
+  dnat, or dnat_and_snat.
+  The external_ip is an IPv4 address.
+  The logical_ip is an IPv4 network (e.g 192.168.1.0/24)
+  or an IPv4 address.
+
+
+  When type is dnat, the externally
+  visible IP address external_ip is DNATted to the
+  IP address logical_ip in the logical space.
+
+
+  When type is snat, IP packets with their
+  source IP address that either matches the IP address in
+  logical_ip or is in the network provided by
+  logical_ip is SNATed into the IP address in
+  external_ip.
+
+
+  When type is dnat_and_snat,
+  the externally visible IP address external_ip
+  is DNATted to the IP address logical_ip in
+  the logical space.  In addition, IP packets with the source
+  IP address that matches logical_ip is SNATed into
+  the IP address in external_ip.
+
+
+  It is an error if a NAT already exists,
+  unless --may-exist is specified.
+
+  
+
+  [--if-exists] lr-nat-del router 
[type [ip]]
+  
+
+  Deletes NATs from router.  If only router
+  is supplied, all the NATs from the logical router are
+  deleted.  If type is also specified, then all the
+  NATs that match the type will be deleted from the logical
+  router.  If all the fields are given, then a single NAT rule
+  that matches all the fields will be deleted.  When type
+  is snat, the ip should be logical_ip.
+  When type is dnat or
+  dnat_and_snat, the ip shoud be external_ip.
+
+
+
+  It is an error if ip is specified and there
+  is no matching NAT entry, unless --if-exists is
+  specified.
+
+  
+
+  lr-nat-list router
+  
+Lists the NATs on router.
+  
+
+
 Load Balancer Commands
 
   [--may-exist | --add-duplicate] 
lb-add lb vip ips 
[protocol]
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index df1c405..abe9bcc 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -383,6 +383,13 @@ Route commands:\n\
 remove routes from ROUTER\n\
   lr-route-list ROUTER  print routes for ROUTER\n\
 \n\
+NAT commands:\n\
+  lr-nat-add ROUTER TYPE EXTERNAL_IP LOGICAL_IP\n\
+add a NAT to ROUTER\n\
+  lr-nat-del ROUTER [TYPE [IP]]\n\
+remove NATs from ROUTER\n\
+  lr-nat-list ROUTERprint NATs for ROUTER\n\
+\n\
 LB commands:\n\
   lb-add LB VIP[:PORT] IP[:PORT]... [PROTOCOL]\n\
 create a load-balancer or add a VIP to an\n\
@@ -2164,6 +2171,195 @@ nbctl_lr_route_del(struct ctl_context *ctx)
 }
 free(prefix);
 }
+
+static void
+nbctl_lr_nat_add(struct ctl_context *ctx)
+{
+const struct nbrec_logical_router *lr;
+const char *nat_type = ctx->argv[2];
+const char *external_ip = ctx->argv[3];
+const char *logical_ip = ctx->argv[4];
+char *new_logical_ip = NULL;
+
+lr = lr_by_name_or_uuid(ctx, ctx->argv[1], true);
+
+if (strcmp(nat_type, "dnat") && strcmp(nat_type, "snat")
+&& strcmp(nat_type, "dnat_and_snat")) {
+ctl_fatal("%s: type must be one of \"dnat\", \"snat\" and "
+"\"dnat_and_snat\".", nat_type);
+}
+
+ovs_be32 ipv4 = 0;
+ovs_be16 port = 0;
+unsigned int plen;
+if (!ip_parse(external_ip, &ipv4)) {
+if (!strcmp(nat_type, "dnat")) {
+char *error = ip_parse_port(external_ip, &ipv4, &port);
+if (error || !port) {
+free(error);
+ctl_fatal("%s: should be an IPv4 address (and an "
+  "optional port number).", external_ip);
+}
+} else {
+ctl_fatal("%s: should be an IPv4 address.", external_ip);
+}
+}
+
+if (strcmp("snat", nat_type)) {

[ovs-dev] [PATCH 4/4] ovs: optimize 'ip_parse_port' function.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 lib/packets.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lib/packets.c b/lib/packets.c
index 990c407..1d2d452 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -436,15 +436,12 @@ char * OVS_WARN_UNUSED_RESULT
 ip_parse_port(const char *s, ovs_be32 *ip, ovs_be16 *port)
 {
 int n = 0;
-if (!ovs_scan_len(s, &n, IP_PORT_SCAN_FMT,
-IP_PORT_SCAN_ARGS(ip, port))) {
-return xasprintf("%s: invalid IP address or port number", s);
+if (ovs_scan(s, IP_PORT_SCAN_FMT"%n", IP_PORT_SCAN_ARGS(ip, port), &n)
+&& !s[n]) {
+return NULL;
 }
 
-if (s[n]) {
-return xasprintf("%s: invalid IP address or port number", s);
-}
-return NULL;
+return xasprintf("%s: invalid IP address or port number", s);
 }
 
 /* Parses string 's', which must be an IP address with an optional netmask or
-- 
1.8.3.1



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

[ovs-dev] [CudaMailTagged] [PATCH 1/4] ovn: Support DNAT with port.

[nickcooper-zhangtonghao]

When the type of NAT is dnat, the externally visible IP address
and port number can be DNATted to the IP address and port number
in the logical space. Adds a protocol column for the NAT. Valid
protocols  are  tcp  or  udp.  This column is useful when a port
number is provided as part of the external_ip column and the type
is set as dnat. If this column is empty and a port number is
provided as part of external_ip column when the type is set as dnat,
OVN assumes the protocol to be tcp.

For example :
ovn-nbctl -- --id=@nat create nat type="dnat" protocol="tcp" \
logical_ip="192.168.1.2\:8080" external_ip="30.0.0.2\:80" \
-- add logical_router R1 nat @nat

This patch improves the "tests/test-l7.py" which can support the
"--port" argument. This patch also provides the unit tests and
the documentation.

Signed-off-by: nickcooper-zhangtonghao 
---
 include/ovn/actions.h   |   1 +
 ovn/lib/actions.c   |  13 +++-
 ovn/northd/ovn-northd.8.xml |  10 +++
 ovn/northd/ovn-northd.c | 149 ++--
 ovn/ovn-nb.ovsschema|   8 ++-
 ovn/ovn-nb.xml  |  22 +--
 tests/system-ovn.at | 113 +
 tests/test-l7.py|  13 +++-
 8 files changed, 287 insertions(+), 42 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 0bf6145..ce8aca5 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -170,6 +170,7 @@ struct ovnact_ct_commit {
 struct ovnact_ct_nat {
 struct ovnact ovnact;
 ovs_be32 ip;
+uint16_t port;
 uint8_t ltable; /* Logical table ID of next table. */
 };
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index df526c0..90c19f3 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -697,13 +697,21 @@ parse_ct_nat(struct action_context *ctx, const char *name,
 
 if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
 if (ctx->lexer->token.type != LEX_T_INTEGER
-|| ctx->lexer->token.format != LEX_F_IPV4) {
+|| mf_subvalue_width(&ctx->lexer->token.value) > 32) {
 lexer_syntax_error(ctx->lexer, "expecting IPv4 address");
 return;
 }
 cn->ip = ctx->lexer->token.value.ipv4;
 lexer_get(ctx->lexer);
 
+/* Parse optional port. */
+uint16_t port = 0;
+if (lexer_match(ctx->lexer, LEX_T_COLON)
+&& !action_parse_port(ctx, &port)) {
+return;
+}
+cn->port = port;
+
 if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
 return;
 }
@@ -779,6 +787,9 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
 if (snat) {
 nat->flags |= NX_NAT_F_SRC;
 } else {
+if (cn->port) {
+nat->range.proto.min = cn->port;
+}
 nat->flags |= NX_NAT_F_DST;
 }
 }
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index df53d4c..f141462 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1191,6 +1191,16 @@ icmp4 {
 
   
 For each configuration in the OVN Northbound database, that asks
+to change the destination IP address A with a port
+P1 of protocol P to IP address B
+and port P2, a priority-100 flow matches ip &&
+ip4.dst == A && P &&
+P.dst == P1 with an action
+flags.loopback = 1; ct_dnat(B:P2);.
+  
+
+  
+For each configuration in the OVN Northbound database, that asks
 to change the destination IP address of a packet from A to
 B, a priority-100 flow matches ip &&
 ip4.dst == A with an action
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index ad0739c..03bd9e4 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2252,7 +2252,7 @@ build_pre_acls(struct ovn_datapath *od, struct hmap 
*lflows)
  * 'ip_address'.  The caller must free() the memory allocated for
  * 'ip_address'. */
 static void
-ip_address_and_port_from_lb_key(const char *key, char **ip_address,
+ip_address_and_port(const char *key, char **ip_address,
 uint16_t *port)
 {
 char *ip_str, *start, *next;
@@ -2262,8 +2262,6 @@ ip_address_and_port_from_lb_key(const char *key, char 
**ip_address,
 next = start = xstrdup(key);
 ip_str = strsep(&next, ":");
 if (!ip_str || !ip_str[0]) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-VLOG_WARN_RL(&rl, "bad ip address for load balancer key %s", key);
 free(start);
 return;
 }
@@ -2271,8 +2269,6 @@ ip_address_and_port_from_lb_key(const char *key, char 
**ip_address,
 ovs_be32 ip, mask;
 cha

[ovs-dev] [PATCH] ovn: Fix url in OVN-Tutorial.md.

[nickcooper-zhangtonghao]

Signed-off-by: nickcooper-zhangtonghao 
---
 tutorial/OVN-Tutorial.md | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tutorial/OVN-Tutorial.md b/tutorial/OVN-Tutorial.md
index 129e918..3779558 100644
--- a/tutorial/OVN-Tutorial.md
+++ b/tutorial/OVN-Tutorial.md
@@ -1005,10 +1005,10 @@ and `lport2`.
 
[env1packet1]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/packet1.sh
 
[env1packet2]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/packet2.sh
 
[env1thirdport]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/add-third-port.sh
-[env1unknownports]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env1/add-unknown-ports.sh
-[env1securityport]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env1/add-security-ip-ports.sh
-[env1packet3]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env1/packet3.sh
-[env1packet4]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env1/packet4.sh
+[env1unknownports]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/add-unknown-ports.sh
+[env1securityport]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/add-security-ip-ports.sh
+[env1packet3]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/packet3.sh
+[env1packet4]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env1/packet4.sh
 
[env2setup]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env2/setup.sh
 
[env2packet1]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env2/packet1.sh
 
[env2packet2]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env2/packet2.sh
@@ -1029,8 +1029,8 @@ and `lport2`.
 
[env7contports]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env7/add-container-ports.sh
 
[env7packet1]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env7/packet1.sh
 
[env7packet2]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env7/packet2.sh
-[env8setup]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env8/setup.sh
-[env8packet1]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env8/packet1.sh
-[env8packet2]:https://github.com/nickcooper-zhangtonghao/ovs/blob/master/tutorial/ovn/env8/packet2.sh
+[env8setup]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env8/setup.sh
+[env8packet1]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env8/packet1.sh
+[env8packet2]:https://github.com/openvswitch/ovs/blob/master/tutorial/ovn/env8/packet2.sh
 
[openstack-ovn-acl-blog]:http://blog.russellbryant.net/2015/10/22/openstack-security-groups-using-ovn-acls/
 
[openvswitch-docker]:http://openvswitch.org/support/dist-docs/INSTALL.Docker.rst.txt
-- 
1.8.3.1



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