| Message ID | 1504736647-15435-1-git-send-email-azhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] bridge: Fix controller status update to passive connections | expand |
On 6 September 2017 at 15:24, Andy Zhou <azhou@ovn.org> 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") > Reported-by: Josh Bailey <josh@faucet.nz> > Signed-off-by: Andy Zhou <azhou@ovn.org> > --- This bug was reported on v2.8.0, so will need backport to branch-2.8. > 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 bbasler@nicira.com > Bhargava Shastry bshastry@sec.t-labs.tu-berlin.de > Bob Ball bob.ball@citrix.com > Brad Hall brad@nicira.com > +Brailey Josh josh@faucet.nz > Brandon Heller brandonh@stanford.edu > Brendan Kelley bkelley@nicira.com > Brent Salisbury brent.salisbury@gmail.com > 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?
On Wed, Sep 6, 2017 at 11:59 PM, Joe Stringer <joe@ovn.org> wrote: > On 6 September 2017 at 15:24, Andy Zhou <azhou@ovn.org> 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 <josh@faucet.nz> >> Signed-off-by: Andy Zhou <azhou@ovn.org> >> --- > > 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 bbasler@nicira.com >> Bhargava Shastry bshastry@sec.t-labs.tu-berlin.de >> Bob Ball bob.ball@citrix.com >> Brad Hall brad@nicira.com >> +Brailey Josh josh@faucet.nz >> Brandon Heller brandonh@stanford.edu >> Brendan Kelley bkelley@nicira.com >> Brent Salisbury brent.salisbury@gmail.com >> 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.
diff --git a/AUTHORS.rst b/AUTHORS.rst index cb0cd91b21ba..e304609b2b27 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -389,6 +389,7 @@ Ben Basler bbasler@nicira.com Bhargava Shastry bshastry@sec.t-labs.tu-berlin.de Bob Ball bob.ball@citrix.com Brad Hall brad@nicira.com +Brailey Josh josh@faucet.nz Brandon Heller brandonh@stanford.edu Brendan Kelley bkelley@nicira.com Brent Salisbury brent.salisbury@gmail.com 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); + } } ofproto_free_ofproto_controller_info(&info);
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. Reported-by: Josh Bailey <josh@faucet.nz> Signed-off-by: Andy Zhou <azhou@ovn.org> --- AUTHORS.rst | 1 + vswitchd/bridge.c | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-)