Ben Pfaff <b...@ovn.org> wrote on 04/11/2016 06:48:16 PM:
> From: Ben Pfaff <b...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 04/11/2016 06:48 PM
> Subject: Re: [ovs-dev, v4] Dynamically reconnect ovn-controller if
> ovn-remote value changes
>
> On Tue, Apr 05, 2016 at 10:22:48AM -0500, Ryan Moats wrote:
> > From: RYAN D. MOATS <rmo...@us.ibm.com>
> >
> > Allows for auto detection and reconnect if the ovn-remote needs
> > to change. Ovn-controller test case updated to include testing
> > this code.
> >
> > Signed-off-by: RYAN D. MOATS <rmo...@us.ibm.com>
>
> Thanks for the patch! I think that this is much closer to being ready.
>
> Please remove the blank line here:
>
> > +/* Changes the remote and creates a new session. */
> > +
Done
> > +void
> > +ovsdb_idl_set_remote(struct ovsdb_idl *idl, const char *remote,
> > + bool retry)
> > +{
> > + if (idl) {
> > + ovs_assert(!idl->txn);
>
> Why do you want to clear the contents of the IDL, by calling
> ovsdb_idl_clear()? This will cause the client to see an empty database
> until the new session successfully connects and retrieves a snapshot.
> Without this call, the IDL's snapshot should atomically (from the
> viewpoint of the client) replace the previous IDL contents.
This comes from my having experience with more "classic" DBs where one
doesn't want to risk leaving stale information around (this experience
will come up again when talking about change tracking). So removed.
> > + ovsdb_idl_clear(idl);
> > + idl->session = jsonrpc_session_open(remote, retry);
> > + idl->state_seqno = UINT_MAX;
>
> I also don't think it's wise to set these to NULL. For one thing it's a
> memory leak, also ovsdb_idl_run() should free these and set them to NULL
> when it detects that the session has reconnected (via
> jsonrpc_session_get_seqno()):
I misread the original code and thought these were additional pointers
to memory that was managed elsewhere. These are unnecessary and have
been removed.
> No space before ( here, please:
>
> > + free (ovnsb_remote);
> > + ovnsb_remote = new_ovnsb_remote;
> > + ovsdb_idl_set_remote(ovnsb_idl_loop.idl, ovnsb_remote,
true);
> > + } else {
>
> Also here:
> > + free (new_ovnsb_remote);
> > + }
Done. New patch to appear soon...
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
