[{"id":1764539,"web_url":"http://patchwork.ozlabs.org/comment/1764539/","msgid":"","list_archive_url":null,"date":"2017-09-07T06:59:55","subject":"Re: [ovs-dev] [PATCH] bridge: Fix controller status update to\n\tpassive connections","submitter":{"id":67727,"url":"http://patchwork.ozlabs.org/api/people/67727/","name":"Joe Stringer","email":"joe@ovn.org"},"content":"On 6 September 2017 at 15:24, Andy Zhou wrote:\n> The bug can cause ovs-vswitchd to crash (due to assert) when it is\n> set up with a passive controller connection. Since only active\n> connections are kept, the passive connection status update should be\n> ignored and not trigger asserts.\n>\n\nFixes: 85c55772a453 (\"bridge: Fix controller status update\")\n\n> Reported-by: Josh Bailey \n> Signed-off-by: Andy Zhou \n> ---\n\nThis bug was reported on v2.8.0, so will need backport to branch-2.8.\n\n> AUTHORS.rst | 1 +\n> vswitchd/bridge.c | 12 +++++++-----\n> 2 files changed, 8 insertions(+), 5 deletions(-)\n>\n> diff --git a/AUTHORS.rst b/AUTHORS.rst\n> index cb0cd91b21ba..e304609b2b27 100644\n> --- a/AUTHORS.rst\n> +++ b/AUTHORS.rst\n> @@ -389,6 +389,7 @@ Ben Basler bbasler@nicira.com\n> Bhargava Shastry bshastry@sec.t-labs.tu-berlin.de\n> Bob Ball bob.ball@citrix.com\n> Brad Hall brad@nicira.com\n> +Brailey Josh josh@faucet.nz\n> Brandon Heller brandonh@stanford.edu\n> Brendan Kelley bkelley@nicira.com\n> Brent Salisbury brent.salisbury@gmail.com\n> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c\n> index a8cbae78cb23..00f86182c820 100644\n> --- a/vswitchd/bridge.c\n> +++ b/vswitchd/bridge.c\n> @@ -2720,11 +2720,13 @@ refresh_controller_status(void)\n> struct ofproto_controller_info *cinfo =\n> shash_find_data(&info, cfg->target);\n>\n> - ovs_assert(cinfo);\n> - ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);\n> - const char *role = ofp12_controller_role_to_str(cinfo->role);\n> - ovsrec_controller_set_role(cfg, role);\n> - ovsrec_controller_set_status(cfg, &cinfo->pairs);\n> + /* cinfo is NULL when 'cfg->target' is a passive connection. */\n> + if (cinfo) {\n> + ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);\n> + const char *role = ofp12_controller_role_to_str(cinfo->role);\n> + ovsrec_controller_set_role(cfg, role);\n> + ovsrec_controller_set_status(cfg, &cinfo->pairs);\n> + }\n\nPrior to commit 85c55772a453f, the following lines were in the alternative case:\n ovsrec_controller_set_is_connected(cfg, false);\n ovsrec_controller_set_role(cfg, NULL);\n ovsrec_controller_set_status(cfg, NULL);\n\nShould we update the records in the else case here like was previously done?","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnrtx28P0z9sCZ\n\tfor ;\n\tThu, 7 Sep 2017 17:00:24 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 6FADE901;\n\tThu, 7 Sep 2017 07:00:21 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id C9F398FF\n\tfor ; Thu, 7 Sep 2017 07:00:19 +0000 (UTC)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id F4217D3\n\tfor ; Thu, 7 Sep 2017 07:00:18 +0000 (UTC)","from mail-lf0-f46.google.com (mail-lf0-f46.google.com\n\t[209.85.215.46]) (Authenticated sender: joe@ovn.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 9E520C5A67\n\tfor ; Thu, 7 Sep 2017 09:00:17 +0200 (CEST)","by mail-lf0-f46.google.com with SMTP id q132so22326737lfe.5\n\tfor ; Thu, 07 Sep 2017 00:00:17 -0700 (PDT)","by 10.46.33.226 with HTTP; Wed, 6 Sep 2017 23:59:55 -0700 (PDT)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"209.85.215.46","X-Gm-Message-State":"AHPjjUhMbLUbYVxQBWgVDzqiENa89ktC7WWfNaQaaiguBk2tlcLGukGW\n\txxg9gbieoHCFrdCYNJVwvxe0t0WHYA==","X-Google-Smtp-Source":"ADKCNb5eoCSowgL+ScaKM0gDm7YkACrMTMDM+ZIM32z4+Ufn1wwS2eD7VYAwd03qJWlrOFHp3yBL6aB53dbe4wnI7fU=","X-Received":"by 10.46.22.13 with SMTP id w13mr580992ljd.1.1504767616791; Thu,\n\t07 Sep 2017 00:00:16 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1504736647-15435-1-git-send-email-azhou@ovn.org>","References":"<1504736647-15435-1-git-send-email-azhou@ovn.org>","From":"Joe Stringer ","Date":"Wed, 6 Sep 2017 23:59:55 -0700","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Andy Zhou ","X-Spam-Status":"No, score=-0.2 required=5.0 tests=RCVD_IN_DNSWL_LOW,\n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"ovs dev ","Subject":"Re: [ovs-dev] [PATCH] bridge: Fix controller status update to\n\tpassive connections","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1764886,"web_url":"http://patchwork.ozlabs.org/comment/1764886/","msgid":"","list_archive_url":null,"date":"2017-09-07T18:35:44","subject":"Re: [ovs-dev] [PATCH] bridge: Fix controller status update to\n\tpassive connections","submitter":{"id":67699,"url":"http://patchwork.ozlabs.org/api/people/67699/","name":"Andy Zhou","email":"azhou@ovn.org"},"content":"On Wed, Sep 6, 2017 at 11:59 PM, Joe Stringer wrote:\n> On 6 September 2017 at 15:24, Andy Zhou wrote:\n>> The bug can cause ovs-vswitchd to crash (due to assert) when it is\n>> set up with a passive controller connection. Since only active\n>> connections are kept, the passive connection status update should be\n>> ignored and not trigger asserts.\n>>\n>\n> Fixes: 85c55772a453 (\"bridge: Fix controller status update\")\n\nThanks. Will add to the commit message.\n>\n>> Reported-by: Josh Bailey \n>> Signed-off-by: Andy Zhou \n>> ---\n>\n> This bug was reported on v2.8.0, so will need backport to branch-2.8.\n\nWill keep this in mind when pushing.\n>\n>> AUTHORS.rst | 1 +\n>> vswitchd/bridge.c | 12 +++++++-----\n>> 2 files changed, 8 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/AUTHORS.rst b/AUTHORS.rst\n>> index cb0cd91b21ba..e304609b2b27 100644\n>> --- a/AUTHORS.rst\n>> +++ b/AUTHORS.rst\n>> @@ -389,6 +389,7 @@ Ben Basler bbasler@nicira.com\n>> Bhargava Shastry bshastry@sec.t-labs.tu-berlin.de\n>> Bob Ball bob.ball@citrix.com\n>> Brad Hall brad@nicira.com\n>> +Brailey Josh josh@faucet.nz\n>> Brandon Heller brandonh@stanford.edu\n>> Brendan Kelley bkelley@nicira.com\n>> Brent Salisbury brent.salisbury@gmail.com\n>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c\n>> index a8cbae78cb23..00f86182c820 100644\n>> --- a/vswitchd/bridge.c\n>> +++ b/vswitchd/bridge.c\n>> @@ -2720,11 +2720,13 @@ refresh_controller_status(void)\n>> struct ofproto_controller_info *cinfo =\n>> shash_find_data(&info, cfg->target);\n>>\n>> - ovs_assert(cinfo);\n>> - ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);\n>> - const char *role = ofp12_controller_role_to_str(cinfo->role);\n>> - ovsrec_controller_set_role(cfg, role);\n>> - ovsrec_controller_set_status(cfg, &cinfo->pairs);\n>> + /* cinfo is NULL when 'cfg->target' is a passive connection. */\n>> + if (cinfo) {\n>> + ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);\n>> + const char *role = ofp12_controller_role_to_str(cinfo->role);\n>> + ovsrec_controller_set_role(cfg, role);\n>> + ovsrec_controller_set_status(cfg, &cinfo->pairs);\n>> + }\n>\n> Prior to commit 85c55772a453f, the following lines were in the alternative case:\n> ovsrec_controller_set_is_connected(cfg, false);\n> ovsrec_controller_set_role(cfg, NULL);\n> ovsrec_controller_set_status(cfg, NULL);\n>\n> Should we update the records in the else case here like was previously done?\n\nI don't think it is necessary. Those are the default values and we are\nnot changing them.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xp8L95SPcz9sNd\n\tfor ;\n\tFri, 8 Sep 2017 04:36:32 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 05365BC4;\n\tThu, 7 Sep 2017 18:36:30 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 8A383BC2\n\tfor ; Thu, 7 Sep 2017 18:36:28 +0000 (UTC)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id C53C9D3\n\tfor ; Thu, 7 Sep 2017 18:36:27 +0000 (UTC)","from mail-pg0-f50.google.com (mail-pg0-f50.google.com\n\t[74.125.83.50]) (Authenticated sender: azhou@ovn.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 30D9DFB8A9\n\tfor ; Thu, 7 Sep 2017 20:36:25 +0200 (CEST)","by mail-pg0-f50.google.com with SMTP id t3so955082pgt.0\n\tfor ; Thu, 07 Sep 2017 11:36:25 -0700 (PDT)","by 10.100.192.14 with HTTP; Thu, 7 Sep 2017 11:35:44 -0700 (PDT)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"74.125.83.50","X-Gm-Message-State":"AHPjjUj4B21/v3Xdrn8WGx7YS97NyhUyXhEaUvVuPQ4+W9lVZwzGtkSC\n\tp8dn+UlTzBZ7iMlIWjZecMEH61SEtR5g4RM/CAI=","X-Google-Smtp-Source":"ADKCNb4izBUBoIilQeCxh00Nfs86/VajXc/oFVm4tKemhZe9lISZSW8PtzYJqymQjEU8edzKcfiS3JVCq1yxZJac1SA=","X-Received":"by 10.99.95.131 with SMTP id t125mr299035pgb.172.1504809384458; \n\tThu, 07 Sep 2017 11:36:24 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"","References":"<1504736647-15435-1-git-send-email-azhou@ovn.org>\n\t","From":"Andy Zhou ","Date":"Thu, 7 Sep 2017 11:35:44 -0700","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Joe Stringer ","X-Spam-Status":"No, score=-0.2 required=5.0 tests=RCVD_IN_DNSWL_LOW,\n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"ovs dev ","Subject":"Re: [ovs-dev] [PATCH] bridge: Fix controller status update to\n\tpassive connections","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}}]