On Wed, Sep 6, 2017 at 11:59 PM, Joe Stringer <[email protected]> wrote: > On 6 September 2017 at 15:24, Andy Zhou <[email protected]> wrote: >> The bug can cause ovs-vswitchd to crash (due to assert) when it is >> set up with a passive controller connection. Since only active >> connections are kept, the passive connection status update should be >> ignored and not trigger asserts. >> > > Fixes: 85c55772a453 ("bridge: Fix controller status update")
Thanks. Will add to the commit message. > >> Reported-by: Josh Bailey <[email protected]> >> Signed-off-by: Andy Zhou <[email protected]> >> --- > > This bug was reported on v2.8.0, so will need backport to branch-2.8. Will keep this in mind when pushing. > >> AUTHORS.rst | 1 + >> vswitchd/bridge.c | 12 +++++++----- >> 2 files changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/AUTHORS.rst b/AUTHORS.rst >> index cb0cd91b21ba..e304609b2b27 100644 >> --- a/AUTHORS.rst >> +++ b/AUTHORS.rst >> @@ -389,6 +389,7 @@ Ben Basler [email protected] >> Bhargava Shastry [email protected] >> Bob Ball [email protected] >> Brad Hall [email protected] >> +Brailey Josh [email protected] >> Brandon Heller [email protected] >> Brendan Kelley [email protected] >> Brent Salisbury [email protected] >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> index a8cbae78cb23..00f86182c820 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -2720,11 +2720,13 @@ refresh_controller_status(void) >> struct ofproto_controller_info *cinfo = >> shash_find_data(&info, cfg->target); >> >> - ovs_assert(cinfo); >> - ovsrec_controller_set_is_connected(cfg, cinfo->is_connected); >> - const char *role = ofp12_controller_role_to_str(cinfo->role); >> - ovsrec_controller_set_role(cfg, role); >> - ovsrec_controller_set_status(cfg, &cinfo->pairs); >> + /* cinfo is NULL when 'cfg->target' is a passive connection. */ >> + if (cinfo) { >> + ovsrec_controller_set_is_connected(cfg, >> cinfo->is_connected); >> + const char *role = >> ofp12_controller_role_to_str(cinfo->role); >> + ovsrec_controller_set_role(cfg, role); >> + ovsrec_controller_set_status(cfg, &cinfo->pairs); >> + } > > Prior to commit 85c55772a453f, the following lines were in the alternative > case: > ovsrec_controller_set_is_connected(cfg, false); > ovsrec_controller_set_role(cfg, NULL); > ovsrec_controller_set_status(cfg, NULL); > > Should we update the records in the else case here like was previously done? I don't think it is necessary. Those are the default values and we are not changing them. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
