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. */
> +
> +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.
> + 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()):
> + idl->request_id = NULL;
> + idl->schema = NULL;
> + }
> +}
> +
> /* Destroys 'idl' and all of the data structures that it manages. */
> void
> ovsdb_idl_destroy(struct ovsdb_idl *idl)
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index e2e2a5e..bad2dc6 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -51,6 +51,7 @@ struct ovsdb_idl *ovsdb_idl_create(const char *remote,
> const struct ovsdb_idl_class *,
> bool monitor_everything_by_default,
> bool retry);
> +void ovsdb_idl_set_remote(struct ovsdb_idl *, const char *, bool);
> void ovsdb_idl_destroy(struct ovsdb_idl *);
>
> void ovsdb_idl_run(struct ovsdb_idl *);
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
> index 6027011..2ee4589 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -302,6 +302,16 @@ main(int argc, char *argv[])
> /* Main loop. */
> exiting = false;
> while (!exiting) {
> + /* Check OVN SB database. */
> + char *new_ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
> + if (strcmp(ovnsb_remote, new_ovnsb_remote)) {
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);
> + }
> +
> struct controller_ctx ctx = {
> .ovs_idl = ovs_idl_loop.idl,
> .ovs_idl_txn = ovsdb_idl_loop_run(&ovs_idl_loop),
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
