On Thu, May 22, 2014 at 9:36 AM, Ben Pfaff <[email protected]> wrote:
> The test added in this commit would have caught the bug fixed by commit
> 96be8de595150 (bridge: When ports disappear from a datapath, add them
> back.). With that commit reverted, the new test fails.
>
> Signed-off-by: Ben Pfaff <[email protected]>
> Acked-by: Gurucharan Shetty <[email protected]>
> ---
> v1->v2: Rebase, fixing a lot of conflicts.
looks good.
>
> lib/dpif-netdev.c | 63
> +++++++++++++++++++++++++++++++++++++++-------------
> tests/automake.mk | 1 +
> tests/bridge.at | 38 +++++++++++++++++++++++++++++++
> tests/testsuite.at | 3 ++-
> 4 files changed, 89 insertions(+), 16 deletions(-)
> create mode 100644 tests/bridge.at
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index df62912..91c83d6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -325,7 +325,7 @@ static void dp_netdev_flow_flush(struct dp_netdev *);
> static int do_add_port(struct dp_netdev *dp, const char *devname,
> const char *type, odp_port_t port_no)
> OVS_REQUIRES(dp->port_mutex);
> -static int do_del_port(struct dp_netdev *dp, odp_port_t port_no)
> +static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *)
> OVS_REQUIRES(dp->port_mutex);
> static void dp_netdev_destroy_all_queues(struct dp_netdev *dp)
> OVS_REQ_WRLOCK(dp->queue_rwlock);
> @@ -548,7 +548,7 @@ dp_netdev_free(struct dp_netdev *dp)
> dp_netdev_flow_flush(dp);
> ovs_mutex_lock(&dp->port_mutex);
> CMAP_FOR_EACH (port, node, &cursor, &dp->ports) {
> - do_del_port(dp, port->port_no);
> + do_del_port(dp, port);
> }
> ovs_mutex_unlock(&dp->port_mutex);
>
> @@ -762,7 +762,16 @@ dpif_netdev_port_del(struct dpif *dpif, odp_port_t
> port_no)
> int error;
>
> ovs_mutex_lock(&dp->port_mutex);
> - error = port_no == ODPP_LOCAL ? EINVAL : do_del_port(dp, port_no);
> + if (port_no == ODPP_LOCAL) {
> + error = EINVAL;
> + } else {
> + struct dp_netdev_port *port;
> +
> + error = get_port_by_number(dp, port_no, &port);
> + if (!error) {
> + do_del_port(dp, port);
> + }
> + }
> ovs_mutex_unlock(&dp->port_mutex);
>
> return error;
> @@ -851,26 +860,17 @@ get_port_by_name(struct dp_netdev *dp,
> return ENOENT;
> }
>
> -static int
> -do_del_port(struct dp_netdev *dp, odp_port_t port_no)
> +static void
> +do_del_port(struct dp_netdev *dp, struct dp_netdev_port *port)
> OVS_REQUIRES(dp->port_mutex)
> {
> - struct dp_netdev_port *port;
> - int error;
> -
> - error = get_port_by_number(dp, port_no, &port);
> - if (error) {
> - return error;
> - }
> -
> - cmap_remove(&dp->ports, &port->node, hash_odp_port(port_no));
> + cmap_remove(&dp->ports, &port->node, hash_odp_port(port->port_no));
> seq_change(dp->port_seq);
> if (netdev_is_pmd(port->netdev)) {
> dp_netdev_reload_pmd_threads(dp);
> }
>
> port_unref(port);
> - return 0;
> }
>
> static void
> @@ -2292,6 +2292,37 @@ exit:
> }
>
> static void
> +dpif_dummy_delete_port(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[], void *aux OVS_UNUSED)
> +{
> + struct dp_netdev_port *port;
> + struct dp_netdev *dp;
> +
> + ovs_mutex_lock(&dp_netdev_mutex);
> + dp = shash_find_data(&dp_netdevs, argv[1]);
> + if (!dp || !dpif_netdev_class_is_dummy(dp->class)) {
> + ovs_mutex_unlock(&dp_netdev_mutex);
> + unixctl_command_reply_error(conn, "unknown datapath or not a dummy");
> + return;
> + }
> + ovs_refcount_ref(&dp->ref_cnt);
> + ovs_mutex_unlock(&dp_netdev_mutex);
> +
> + ovs_mutex_lock(&dp->port_mutex);
> + if (get_port_by_name(dp, argv[2], &port)) {
> + unixctl_command_reply_error(conn, "unknown port");
> + } else if (port->port_no == ODPP_LOCAL) {
> + unixctl_command_reply_error(conn, "can't delete local port");
> + } else {
> + do_del_port(dp, port);
> + unixctl_command_reply(conn, NULL);
> + }
> + ovs_mutex_unlock(&dp->port_mutex);
> +
> + dp_netdev_unref(dp);
> +}
> +
> +static void
> dpif_dummy_register__(const char *type)
> {
> struct dpif_class *class;
> @@ -2324,4 +2355,6 @@ dpif_dummy_register(bool override)
> unixctl_command_register("dpif-dummy/change-port-number",
> "DP PORT NEW-NUMBER",
> 3, 3, dpif_dummy_change_port_number, NULL);
> + unixctl_command_register("dpif-dummy/delete-port", "DP PORT",
> + 2, 2, dpif_dummy_delete_port, NULL);
> }
> diff --git a/tests/automake.mk b/tests/automake.mk
> index a858148..9354fad 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -39,6 +39,7 @@ TESTSUITE_AT = \
> tests/ovs-vswitchd.at \
> tests/dpif-netdev.at \
> tests/ofproto-dpif.at \
> + tests/bridge.at \
> tests/vlan-splinters.at \
> tests/ofproto-macros.at \
> tests/ofproto.at \
> diff --git a/tests/bridge.at b/tests/bridge.at
> new file mode 100644
> index 0000000..817931f
> --- /dev/null
> +++ b/tests/bridge.at
> @@ -0,0 +1,38 @@
> +AT_BANNER([bridge])
> +
> +dnl When a port disappears from a datapath, e.g. because an admin used
> +dnl "ovs-dpctl del-port", the bridge code should be resilient enough to
> +dnl notice and add it back the next time we reconfigure. A prior version
> +dnl of the code failed to do this, so this test guards against regression.
> +AT_SETUP([bridge - ports that disappear get added back])
> +OVS_VSWITCHD_START
> +
> +# Add some ports and make sure that they show up in the datapath.
> +ADD_OF_PORTS([br0], 1, 2)
> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
> +dummy@ovs-dummy: hit:0 missed:0
> + br0:
> + br0 65534/100: (dummy)
> + p1 1/1: (dummy)
> + p2 2/2: (dummy)
> +])
> +
> +# Delete p1 from the datapath as if by "ovs-dpctl del-port"
> +# and check that it disappeared.
> +AT_CHECK([ovs-appctl dpif-dummy/delete-port ovs-dummy p1])
> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
> +dummy@ovs-dummy: hit:0 missed:0
> + br0:
> + br0 65534/100: (dummy)
> + p2 2/2: (dummy)
> +])
> +
> +# Force reconfiguration and make sure that p1 got added back.
> +AT_CHECK([ovs-vsctl del-port p2])
> +AT_CHECK([ovs-appctl dpif/show], [0], [dnl
> +dummy@ovs-dummy: hit:0 missed:0
> + br0:
> + br0 65534/100: (dummy)
> + p1 1/1: (dummy)
> +])
> +AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 1911ac6..d7e5a7d 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -1,6 +1,6 @@
> AT_INIT
>
> -AT_COPYRIGHT([Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> +AT_COPYRIGHT([Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>
> Licensed under the Apache License, Version 2.0 (the "License");
> you may not use this file except in compliance with the License.
> @@ -147,6 +147,7 @@ m4_include([tests/ovs-vswitchd.at])
> m4_include([tests/ofproto.at])
> m4_include([tests/dpif-netdev.at])
> m4_include([tests/ofproto-dpif.at])
> +m4_include([tests/bridge.at])
> m4_include([tests/vlan-splinters.at])
> m4_include([tests/ovsdb.at])
> m4_include([tests/ovs-vsctl.at])
> --
> 1.7.10.4
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
