Thx, all look good to me~
On Wed, Jul 29, 2015 at 9:02 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Jul 28, 2015 at 05:48:50PM -0700, Alex Wang wrote:
> > On Tue, Jul 28, 2015 at 4:40 PM, Justin Pettit <jpet...@nicira.com>
> wrote:
> > > > --- a/ovn/controller/binding.c
> > > > +++ b/ovn/controller/binding.c
> > > > @@ -91,7 +91,9 @@ binding_run(struct controller_ctx *ctx, const
> struct
> > > ovsrec_bridge *br_int,
> > > >
> > > > sset_init(&lports);
> > > > sset_init(&all_lports);
> > > > - get_local_iface_ids(br_int, &lports);
> > > > + if (br_int) {
> > > > + get_local_iface_ids(br_int, &lports);
> > > > + }
> > >
> >
> > Could we add a comment here? If I understand correctly, if br_int is
> NULL,
> > we will reset all binding table entries with chassis column referring to
> > this
> > HV chassis.
>
> Fair enough, I folded this in:
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index b6b345e..849dddf0 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -93,6 +93,9 @@ binding_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
> sset_init(&all_lports);
> if (br_int) {
> get_local_iface_ids(br_int, &lports);
> + } else {
> + /* We have no integration bridge, therefore no local logical
> ports.
> + * We'll remove our chassis from all port binding records below.
> */
> }
> sset_clone(&all_lports, &lports);
>
> > > > sset_clone(&all_lports, &lports);
> > > >
> > > > ovsdb_idl_txn_add_comment(ctx->ovnsb_idl_txn,
> > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> > > > index 7aa523f..f150246 100644
> > > > --- a/ovn/controller/encaps.c
> > > > +++ b/ovn/controller/encaps.c
> > > > @@ -231,7 +231,7 @@ void
> > > > encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge
> > > *br_int,
> > > > const char *chassis_id)
> > > > {
> > > > - if (!ctx->ovs_idl_txn) {
> > > > + if (!ctx->ovs_idl_txn || !br_int) {
> > > > return;
> > > > }
> > > >
> > >
> >
> >
> > I think we also need to do this for the encaps_cleanup(). If br_int is
> > NULL,
> > simply return true.~
>
> Good spotting, thanks! I fixed that:
>
> diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> index f150246..5ae9267 100644
> --- a/ovn/controller/encaps.c
> +++ b/ovn/controller/encaps.c
> @@ -298,6 +298,10 @@ encaps_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
> bool
> encaps_cleanup(struct controller_ctx *ctx, const struct ovsrec_bridge
> *br_int)
> {
> + if (!br_int) {
> + return true;
> + }
> +
> /* Delete all the OVS-created tunnels from the integration bridge. */
> struct ovsrec_port **ports
> = xmalloc(sizeof *br_int->ports * br_int->n_ports);
>
> > > > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> > > > index c548645..b1c421c 100644
> > > > --- a/ovn/controller/ofctrl.c
> > > > +++ b/ovn/controller/ofctrl.c
> > > > @@ -92,13 +92,17 @@ ofctrl_init(void)
> > > > void
> > > > ofctrl_run(const struct ovsrec_bridge *br_int, struct hmap
> *flow_table)
> > > > {
> > > > - char *target;
> > > > - target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int->name);
> > > > - if (strcmp(target, rconn_get_target(swconn))) {
> > > > - VLOG_INFO("%s: connecting to switch", target);
> > > > - rconn_connect(swconn, target, target);
> > > > + if (br_int) {
> > > > + char *target;
> > > > + target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> > > br_int->name);
> > > > + if (strcmp(target, rconn_get_target(swconn))) {
> > > > + VLOG_INFO("%s: connecting to switch", target);
> > > > + rconn_connect(swconn, target, target);
> > > > + }
> > > > + free(target);
> > > > + } else {
> > > > + rconn_disconnect(swconn);
> > > > }
> >
> > Curious about the use case when ofctrl_run() is called with br_int being
> > NULL... And we just close the rconn? And what if someone call it when
> > swconn is still NULL?
>
> We disconnect the rconn in that case, but disconnecting isn't the same
> as destroying; it still exists and can be reconnected later. swconn
> never becomes NULL.
>
> > > > - free(target);
> > > >
> > > > rconn_run(swconn);
>
> Thanks a lot for the comments!
>
> Ben
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
