Ben Pfaff <[email protected]> wrote on 06/02/2016 06:35:25 PM:
> From: Ben Pfaff <[email protected]>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: [email protected]
> 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 <[email protected]>
>
> 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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
