[ovs-dev] [PATCH] bridge.c: prevent controller connects while flow-restore-wait

Ben Pfaff blp at ovn.org
Thu Oct 25 17:03:09 UTC 2018


On Wed, Oct 24, 2018 at 03:56:00PM -0700, Zak Whittington wrote:
> When force-reload-kmod is used, it shows an error when reinstalling
> tlvs during "Restoring saved flows" step:
> OFPT_ERROR (xid=0x4): NXTTMFC_ALREADY_MAPPED
> 
> This is caused by a race condition between the restore script,
> which calls ofctl, and the connected controllers both adding back
> the same TLVs.
> 
> The restore script already sets flow-restore-wait to true while
> doing flow restoration, and sets it back to false after it is
> done, and this patch utilizes that fact to prevent the TLV race.
> It does this by preventing vswitchd from connecting to
> controllers in the controller table while it is in a
> flow-restore-wait state.
> 
> With this patch, when bridge_configure_remotes() calls
> bridge_get_controllers(), it first checks if flow-restore-wait
> has been set, and if so, it ignores any controllers in the
> controller database and sets n_controllers to 0.
> 
> This solution does preserve the management service controller
> which is added via bridge_ofproto_controller_for_mgmt() after
> checking whether we should call bridge_get_controllers()
> (and thus n_controllers is properly set to 1, etc)
> 
> VMware-BZ: 2195377
> Signed-off-by: Zak Whittington <zwhitt.vmware at gmail.com>
> ---
>  vswitchd/bridge.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 706a07c..de43302 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -3608,7 +3608,8 @@ bridge_configure_remotes(struct bridge *br,
>          ofproto_set_extra_in_band_remotes(br->ofproto, managers, n_managers);
>      }
>  
> -    n_controllers = bridge_get_controllers(br, &controllers);
> +    n_controllers = ofproto_get_flow_restore_wait()? 0 :
> +                    bridge_get_controllers(br, &controllers);
>  
>      ocs = xmalloc((n_controllers + 1) * sizeof *ocs);
>      n_ocs = 0;

Thanks for the patch.

Would you mind editing this a little bit to better match the common
coding style?  coding-style.rst has a few principles that apply here:

    Put one space on each side of infix binary and ternary operators:

    ::

        * / %
        + -
        << >>
        < <= > >=
        == !=
        &
        ^
        |
        &&
        ||
        ?:
        = += -= *= /= %= &= ^= |= <<= >>=

...

    Break long lines before the ternary operators ? and :, rather than after
    them, e.g.

    ::

        return (out_port != VIGP_CONTROL_PATH
                ? alpheus_output_port(dp, skb, out_port)
                : alpheus_output_control(dp, skb, fwd_save_skb(skb),
                                         VIGR_ACTION));

...

    Do parenthesize a subexpression that must be split across more than
    one line, e.g.::

        *idxp = ((l1_idx << PORT_ARRAY_L1_SHIFT) |
                 (l2_idx << PORT_ARRAY_L2_SHIFT) |
                 (l3_idx << PORT_ARRAY_L3_SHIFT));

Thanks,

Ben.


More information about the dev mailing list