On Thu, Jul 28, 2016 at 10:34:12AM +0800, nickcooper-zhangtonghao wrote: > > > On Jul 28, 2016, at 5:01 AM, Ben Pfaff <b...@ovn.org> 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 <g...@ovn.org> > >> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/075524.html > >> Signed-off-by: Ben Pfaff <b...@ovn.org> > >> Acked-by: Gurucharan Shetty <g...@ovn.org> > >> Signed-off-by: nickcooper-zhangtonghao > >> <nickcooper-zhangtong...@opencloud.tech> > >> --- > >> 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
OK, I finally understand. I applied this to master as the following. --8<--------------------------cut here-------------------------->8-- From: nickcooper-zhangtonghao <nickcooper-zhangtong...@opencloud.tech> Date: Fri, 12 Aug 2016 13:39:25 -0700 Subject: [PATCH] ovn-northd: Only warn about peer as switch port when it really is one. At the end of join_logical_ports(), some ovn_ports might not have been bound as logical switch ports or logical router ports, but the code assumed that they were and gave a confusing warning when the assumption was violated. Reported-by: Gurucharan Shetty <g...@ovn.org> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/075524.html Acked-by: Gurucharan Shetty <g...@ovn.org> Signed-off-by: nickcooper-zhangtonghao <nickcooper-zhangtong...@opencloud.tech> Signed-off-by: Ben Pfaff <b...@ovn.org> --- 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 861f872..8c7e80c 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1117,7 +1117,7 @@ join_logical_ports(struct northd_context *ctx, if (peer) { 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 -- 2.1.3 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
