[ovs-dev,ovs-dev,v17,1/5] Change encaps_run to work incrementally

On Sun, May 22, 2016 at 04:36:18PM -0500, Ryan Moats wrote:
> As a side effect, tunnel context is persisted.
> 
> Signed-off-by: Ryan Moats <rmoats@us.ibm.com>

Thanks for the new version!  I hardly have any comments this time so I
think we're really close ;-)

There appears to be a memory leak in the process_full_encaps case.  At
least, I think that this loop should free(old_hash_node):
+        HMAP_FOR_EACH_POP(old_hash_node, node, &keep_tunnel_hmap_by_uuid) {
+            ;
+        }
+        hmap_destroy(&keep_tunnel_hmap_by_uuid);
If it does not need to free the hash node, then I do not think that the
loop does anything useful, because it's OK to destroy an hmap that
still contains nodes.

Please start comments with a capital letter and end them with a period,
e.g. in these cases:
+                /* lookup the tunnel by row uuid and remove it */
+    /* track the southbound idl */

check_and_update_tunnel() tries to use ovsrec_interface_set_name() on an
existing tunnel, but the server will reject this because the 'name'
column in the Interface table is immutable.

When check_and_update_tunnel() fails to find a tunnel, should it try to
create it?  Alternatively, should it warn that it does not exist?

Thanks,

Ben.

P.S. Here are some style fixes I suggest folding in:

--8<--------------------------cut here-------------------------->8--

Ben Pfaff <blp@ovn.org> wrote on 06/02/2016 06:35:25 PM:

> From: Ben Pfaff <blp@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 06/02/2016 06:35 PM
> Subject: Re: [ovs-dev,v17,1/5] Change encaps_run to work incrementally
>
> On Sun, May 22, 2016 at 04:36:18PM -0500, Ryan Moats wrote:
> > As a side effect, tunnel context is persisted.
> >
> > Signed-off-by: Ryan Moats <rmoats@us.ibm.com>
>
> Thanks for the new version!  I hardly have any comments this time so I
> think we're really close ;-)
>
> There appears to be a memory leak in the process_full_encaps case.  At
> least, I think that this loop should free(old_hash_node):
> +        HMAP_FOR_EACH_POP(old_hash_node, node,
&keep_tunnel_hmap_by_uuid) {
> +            ;
> +        }
> +        hmap_destroy(&keep_tunnel_hmap_by_uuid);
> If it does not need to free the hash node, then I do not think that the
> loop does anything useful, because it's OK to destroy an hmap that
> still contains nodes.

I will revisit this, because it is a potential anti-pattern across the
patch sets...

>
> Please start comments with a capital letter and end them with a period,
> e.g. in these cases:
> +                /* lookup the tunnel by row uuid and remove it */
> +    /* track the southbound idl */

Ack...

> check_and_update_tunnel() tries to use ovsrec_interface_set_name() on an
> existing tunnel, but the server will reject this because the 'name'
> column in the Interface table is immutable.

So noted

> When check_and_update_tunnel() fails to find a tunnel, should it try to
> create it?  Alternatively, should it warn that it does not exist?

I've gone back and forth on this one in my head for a bit now - when I
go through and make all the needed changes, I'll revisit and do one or the
other...

>
> Thanks,
>
> Ben.
>
> P.S. Here are some style fixes I suggest folding in:
>
> --8<--------------------------cut here-------------------------->8--
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index 226d0a1..7fa55ed 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -129,7 +129,7 @@ port_lookup_by_uuid(struct hmap *hmap_p, const
> struct uuid *uuid)
>      HMAP_FOR_EACH_WITH_HASH (answer, uuid_node, uuid_hash(uuid),
>                               hmap_p) {
>          if (uuid_equals(uuid, answer->uuid)) {
> -            return(answer);
> +            return answer;
>          }
>      }
>      return NULL;
> @@ -142,7 +142,7 @@ port_lookup_by_port(const struct ovsrec_port *port)
>      HMAP_FOR_EACH_WITH_HASH (answer, node, port_hash_rec(port),
>                               &tc.tunnel_hmap) {
>          if (port == answer->port) {
> -            return(answer);
> +            return answer;
>          }
>      }
>      return NULL;
> @@ -288,10 +288,11 @@ check_and_add_tunnel(const struct
> sbrec_chassis *chassis_rec,
>  }
>
>  static void
> -check_and_update_tunnel(const struct sbrec_chassis *chassis_rec) {
> +check_and_update_tunnel(const struct sbrec_chassis *chassis_rec)
> +{
>      struct port_hash_node *port_node;
> -    if ((port_node =port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
> -                                        &chassis_rec->header_.uuid))) {
> +    if ((port_node = port_lookup_by_uuid(&tc.tunnel_hmap_by_uuid,
> +                                         &chassis_rec->header_.uuid))) {
>          const struct sbrec_encap *encap = preferred_encap(chassis_rec);
>          const struct ovsrec_port *port = port_node->port;
>          const struct ovsrec_interface *iface = port->interfaces[0];
> @@ -390,7 +391,7 @@ encaps_run(struct controller_ctx *ctx, const
> struct ovsrec_bridge *br_int,
>                              &old_hash_node->uuid_node);
>              }
>          }
> -        HMAP_FOR_EACH_POP(old_hash_node, node,
&keep_tunnel_hmap_by_uuid) {
> +        HMAP_FOR_EACH_POP (old_hash_node, node,
&keep_tunnel_hmap_by_uuid) {
>              ;
>          }
>          hmap_destroy(&keep_tunnel_hmap_by_uuid);
> @@ -416,14 +417,10 @@ encaps_run(struct controller_ctx *ctx, const
> struct ovsrec_bridge *br_int,
>                                  &port_hash->uuid_node);
>                      free(port_hash);
>                  }
> -                continue;
> -            }
> -            if (!is_new) {
> -                check_and_update_tunnel(chassis_rec);
> -                continue;
> -            } else {
> +            } else if (is_new) {
>                  check_and_add_tunnel(chassis_rec, chassis_id);
> -                continue;
> +            } else {
> +                check_and_update_tunnel(chassis_rec);
>              }
>          }
>      }
>

so noted...

Ryan