| Message ID | 1467855454-123558-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu998@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 <dlu998@gmail.com> > What failure was observed to find this issue? Could we add a test case that would have caught it? > --- > 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." > + 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 >
On Thu, Jul 7, 2016 at 1:17 PM, Russell Bryant <russell@ovn.org> wrote: > > > On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu998@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 <dlu998@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... > > >> --- >> 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 >
On 6 July 2016 at 18:37, Darrell Ball <dlu998@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 <dlu998@gmail.com> > So if I understand this correctly, currently we add things to patched_datapath but never remove when say a router/switch is deleted. I suppose, one could trigger deference problem if all we do is create a gateway router, connect it to some other switch or router and then delete gateway router. ovn-controller would then crash in update_ct_zones() as it will try to access port_binding record stored inside the stale patched_datapath. > --- > 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)) { > + 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 >
On 7 July 2016 at 14:15, Darrell Ball <dlu998@gmail.com> wrote: > On Thu, Jul 7, 2016 at 1:17 PM, Russell Bryant <russell@ovn.org> wrote: > > > > > > > On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu998@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 <dlu998@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 >
On Thu, Jul 7, 2016 at 3:45 PM, Guru Shetty <guru@ovn.org> wrote: > > > On 7 July 2016 at 14:15, Darrell Ball <dlu998@gmail.com> wrote: > >> On Thu, Jul 7, 2016 at 1:17 PM, Russell Bryant <russell@ovn.org> wrote: >> >> > >> > >> > On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu998@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 <dlu998@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 > I don't testcases for OVN NAT at all - did I miss them ? How can a feature be committed without testcases ? > > > >> >> >> > >> > >> >> --- >> >> 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 >> > >
> > > > I don't testcases for OVN NAT at all - did I miss them ? > How can a feature be committed without testcases ? > We currently do not have NAT in userspace datapath to test OVN NAT. I think this is the same case with OVN firewall. Both of them are currently dependent on OpenStack testing. We should probably do something about it. > > > > > > > > > > >> > >> > >> > > >> > > >> >> --- > >> >> 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 >
On Thu, Jul 7, 2016 at 3:38 PM, Guru Shetty <guru@ovn.org> wrote: > > > On 6 July 2016 at 18:37, Darrell Ball <dlu998@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 <dlu998@gmail.com> >> > > So if I understand this correctly, currently we add things to > patched_datapath but never remove when say a router/switch is deleted. I > suppose, one could trigger deference problem if all we do is create a > gateway router, connect it to some other switch or router and then delete > gateway router. ovn-controller would then crash in update_ct_zones() as it > will try to access port_binding record stored inside the stale > patched_datapath. > hmm: update_ct_zones() is obviously missing a NULL pointer check for logical ports, which is another issue. Perhaps, it would be good to test and add testcases before associated features are added rather than after the crash as we see several times recently. I noticed this datastructure issue in relation to some feature that I want to add. I can fix it while I'm here. > > >> --- >> 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)) { >> + 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 >> > >
On Thu, Jul 7, 2016 at 4:09 PM, Darrell Ball <dlu998@gmail.com> wrote: > > > On Thu, Jul 7, 2016 at 3:38 PM, Guru Shetty <guru@ovn.org> wrote: > >> >> >> On 6 July 2016 at 18:37, Darrell Ball <dlu998@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 <dlu998@gmail.com> >>> >> >> So if I understand this correctly, currently we add things to >> patched_datapath but never remove when say a router/switch is deleted. I >> suppose, one could trigger deference problem if all we do is create a >> gateway router, connect it to some other switch or router and then delete >> gateway router. ovn-controller would then crash in update_ct_zones() as it >> will try to access port_binding record stored inside the stale >> patched_datapath. >> > > hmm: > > update_ct_zones() is obviously missing a NULL pointer check for > > logical ports, which is another issue. > > Perhaps, it would be good to test and add testcases before > associated features are added rather than after the crash as we > see several times recently. > > I noticed this datastructure issue in relation to some feature that > I want to add. > > I can fix it while I'm here. > Guru and I discussed offline I will him look into the specific NAT support aspects separately from the general patched datapaths issues. > > > > >> >> >>> --- >>> 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)) { >>> + 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 >>> >> >> >
On Thu, Jul 7, 2016 at 3:55 PM, Guru Shetty <guru@ovn.org> wrote: > >> >> I don't testcases for OVN NAT at all - did I miss them ? >> How can a feature be committed without testcases ? >> > > We currently do not have NAT in userspace datapath to test OVN NAT. I > think this is the same case with OVN firewall. Both of them are currently > dependent on OpenStack testing. We should probably do something about it. > Its worrisome to have features that cannot be properly tested. I suggest to prioritize the test support. No testability -> No feature > > >> >> >> >> > >> > >> > >> >> >> >> >> >> > >> >> > >> >> >> --- >> >> >> 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 >> > >
> > > > Its worrisome to have features that cannot be properly tested. > I suggest to prioritize the test support. > No testability -> No feature > That is fair. I am working on adding OVN tests to system-traffic.at so that I can add NAT related tests. > > > > > > > > > >> > >> > >> > >> > > >> > > >> > > >> >> > >> >> > >> >> > > >> >> > > >> >> >> --- > >> >> >> 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 > >> > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
On Thu, Jul 7, 2016 at 11:42 PM, Guru Shetty <guru@ovn.org> wrote: > That is fair. I am working on adding OVN tests to system-traffic.at so > that > I can add NAT related tests. > This will also be covered by OpenStack CI once that integration is completed since we run full end-to-end test cases, including the use of the kernel datapath.
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)) { + 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
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 <dlu998@gmail.com> --- ovn/controller/ovn-controller.h | 3 ++- ovn/controller/patch.c | 23 ++++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-)