[ovs-dev] [patch_v1] ovn: Remove unreferenced patched datapaths

[Darrell Ball]

On Thu, Jul 7, 2016 at 1:17 PM, Russell Bryant <russell at ovn.org> wrote:

>
>
> On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu998 at 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 at 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 at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>
>
> --
> Russell Bryant
>