On 7 July 2016 at 14:15, Darrell Ball <dlu...@gmail.com> wrote: > On Thu, Jul 7, 2016 at 1:17 PM, Russell Bryant <russ...@ovn.org> wrote: > > > > > > > On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu...@gmail.com> wrote: > > > >> Patched datapaths that are no longer referenced should be removed from > >> the patched_datapaths map; otherwise incorrect state references for a > >> patched datapath may be used and also datapaths that are absent will be > >> interpreted as present. > >> > >> Signed-off-by: Darrell Ball <dlu...@gmail.com> > >> > > > > What failure was observed to find this issue? > > > > This is caught by code reading as I would like to rely on the correctness > of this > internal datastructure in future. > > > > > Could we add a test case that would have caught it? > > > > I tested this with instrumented code and a contrived test. > Since this relates to internal datastructure management, I don't intend to > add a test case > for this. > > When testcases are added for OVN NAT code, which relies on this > datastructure internal fields, I might add a delta > on top of those if those tests are lacking. If I also add a feature that > depends on this datastructure, I will a > test for it as I normally do... >
Fwiw, there are a couple of unit tests that currently add gateway routers (but don't test deleting them) e.g: https://github.com/openvswitch/ovs/blob/master/tests/ovn.at#L2940 > > > > > > > >> --- > >> ovn/controller/ovn-controller.h | 3 ++- > >> ovn/controller/patch.c | 23 ++++++++++++++++++++--- > >> 2 files changed, 22 insertions(+), 4 deletions(-) > >> > >> diff --git a/ovn/controller/ovn-controller.h > >> b/ovn/controller/ovn-controller.h > >> index 6a021a0..8816940 100644 > >> --- a/ovn/controller/ovn-controller.h > >> +++ b/ovn/controller/ovn-controller.h > >> @@ -51,8 +51,9 @@ struct local_datapath *get_local_datapath(const struct > >> hmap *, > >> * with at least one logical patch port binding. */ > >> struct patched_datapath { > >> struct hmap_node hmap_node; > >> - bool local; /* 'True' if the datapath is for gateway router. */ > >> const struct sbrec_port_binding *port_binding; > >> + bool local; /* 'True' if the datapath is for gateway router. */ > >> + bool stale; > >> }; > >> > >> struct patched_datapath *get_patched_datapath(const struct hmap *, > >> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c > >> index 589529e..a701db2 100644 > >> --- a/ovn/controller/patch.c > >> +++ b/ovn/controller/patch.c > >> @@ -252,12 +252,14 @@ static void > >> add_patched_datapath(struct hmap *patched_datapaths, > >> const struct sbrec_port_binding *binding_rec, bool > >> local) > >> { > >> - if (get_patched_datapath(patched_datapaths, > >> - binding_rec->datapath->tunnel_key)) { > >> + struct patched_datapath * pd; > >> + if (pd = get_patched_datapath(patched_datapaths, > >> + binding_rec->datapath->tunnel_key)) { > >> > > > > Can you move the assignment outside the if condition? > > > > CodingStyle.md says "Avoid assignments inside "if" and "while" > conditions." > > > > > alright > > > > > > > > >> + pd->stale = false; > >> return; > >> } > >> > >> - struct patched_datapath *pd = xzalloc(sizeof *pd); > >> + pd = xzalloc(sizeof *pd); > >> pd->local = local; > >> pd->port_binding = binding_rec; > >> hmap_insert(patched_datapaths, &pd->hmap_node, > >> @@ -300,6 +302,13 @@ add_logical_patch_ports(struct controller_ctx *ctx, > >> } > >> > >> const struct sbrec_port_binding *binding; > >> + > >> + /* Mark all patched datapaths as stale for later cleanup check */ > >> + struct patched_datapath *pd; > >> + HMAP_FOR_EACH (pd, hmap_node, patched_datapaths) { > >> + pd->stale = true; > >> + } > >> + > >> SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) { > >> bool local_port = false; > >> if (!strcmp(binding->type, "gateway")) { > >> @@ -332,6 +341,14 @@ add_logical_patch_ports(struct controller_ctx *ctx, > >> } > >> } > >> } > >> + /* Clean up stale patched datapaths. */ > >> + struct patched_datapath *pd_cur_node, *pd_next_node; > >> + HMAP_FOR_EACH_SAFE (pd_cur_node, pd_next_node, hmap_node, > >> patched_datapaths) { > >> + if (pd_cur_node->stale == true) { > >> + hmap_remove(patched_datapaths, &pd_cur_node->hmap_node); > >> + free(pd_cur_node); > >> + } > >> + } > >> } > >> > >> void > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> http://openvswitch.org/mailman/listinfo/dev > >> > > > > > > > > -- > > Russell Bryant > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
