| Message ID | 20171026180901.19267-1-mmichels@redhat.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [ovs-dev] OVN: Don't let peers be set to "<error>" on port bindings. | expand |
On Thu, Oct 26, 2017 at 01:09:01PM -0500, Mark Michelson wrote: > There are a couple of places in ovn-northd that set the "peer" option on > certain ports to "<error>" in certain cases. In every case where a peer is > looked up on a port binding, the code performs a NULL check in order to > ensure a peer exists. None check for the "<error>" string. They assume that the > presence of a peer string means a peer is defined and all is well. > > In the past (OVS 2.6 series), this sometimes led to patch ports being created > in ovs that had names like "patch-ro-to-<error>". This particular problem > resolved itself in OVS 2.7 since such patch ports were no longer automatically > created. However, by naming the peer "<error>" the seeds are still sown for > similar issues to occur. > > The solution this patch suggests is to no longer set the "peer" option > on a port binding to "<error>". Instead, if no peer can be set, then we > set no peer. Since other code is already equipped to deal with this, > this poses no problem. > > Signed-off-by: Mark Michelson <mmichels@redhat.com> Thanks for the improvement! I applied this to master.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 2db238073..ec981541e 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1924,8 +1924,9 @@ ovn_port_update_sbrec(struct northd_context *ctx, } smap_add(&new, "distributed-port", op->nbrp->name); } else { - const char *peer = op->peer ? op->peer->key : "<error>"; - smap_add(&new, "peer", peer); + if (op->peer) { + smap_add(&new, "peer", op->peer->key); + } if (chassis_name) { smap_add(&new, "l3gateway-chassis", chassis_name); } @@ -1978,16 +1979,20 @@ ovn_port_update_sbrec(struct northd_context *ctx, sbrec_port_binding_set_type(op->sb, "patch"); } - const char *router_port = smap_get_def(&op->nbsp->options, - "router-port", "<error>"); - struct smap new; - smap_init(&new); - smap_add(&new, "peer", router_port); - if (chassis) { - smap_add(&new, "l3gateway-chassis", chassis); + const char *router_port = smap_get(&op->nbsp->options, + "router-port"); + if (router_port || chassis) { + struct smap new; + smap_init(&new); + if (router_port) { + smap_add(&new, "peer", router_port); + } + if (chassis) { + smap_add(&new, "l3gateway-chassis", chassis); + } + sbrec_port_binding_set_options(op->sb, &new); + smap_destroy(&new); } - sbrec_port_binding_set_options(op->sb, &new); - smap_destroy(&new); const char *nat_addresses = smap_get(&op->nbsp->options, "nat-addresses");
There are a couple of places in ovn-northd that set the "peer" option on certain ports to "<error>" in certain cases. In every case where a peer is looked up on a port binding, the code performs a NULL check in order to ensure a peer exists. None check for the "<error>" string. They assume that the presence of a peer string means a peer is defined and all is well. In the past (OVS 2.6 series), this sometimes led to patch ports being created in ovs that had names like "patch-ro-to-<error>". This particular problem resolved itself in OVS 2.7 since such patch ports were no longer automatically created. However, by naming the peer "<error>" the seeds are still sown for similar issues to occur. The solution this patch suggests is to no longer set the "peer" option on a port binding to "<error>". Instead, if no peer can be set, then we set no peer. Since other code is already equipped to deal with this, this poses no problem. Signed-off-by: Mark Michelson <mmichels@redhat.com> --- ovn/northd/ovn-northd.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)