Looks Good.
Ethan
On Mon, Jun 6, 2011 at 12:41, Ben Pfaff <b...@nicira.com> wrote:
> I believe that this actually fixes the race described in the comments,
> whereas I'm pretty sure that the old way still left a race window.
> ---
> vswitchd/ovs-brcompatd.c | 68 +++++++++++++--------------------------------
> 1 files changed, 20 insertions(+), 48 deletions(-)
>
> diff --git a/vswitchd/ovs-brcompatd.c b/vswitchd/ovs-brcompatd.c
> index 52cd93f..458aead 100644
> --- a/vswitchd/ovs-brcompatd.c
> +++ b/vswitchd/ovs-brcompatd.c
> @@ -1096,6 +1096,26 @@ brc_recv_update(struct ovsdb_idl *idl)
> goto error;
> }
>
> + /* Service all pending network device notifications before executing the
> + * command. This is very important to avoid a race in a scenario like
> the
> + * following, which is what happens with XenServer Tools version 5.0.0
> + * during boot of a Windows VM:
> + *
> + * 1. Create tap1.0 and vif1.0.
> + * 2. Delete tap1.0.
> + * 3. Delete vif1.0.
> + * 4. Re-create vif1.0.
> + *
> + * We must process the network device notification from step 3 before we
> + * process the brctl command from step 4. If we process them in the
> + * reverse order, then step 4 completes as a no-op but step 3 then
> deletes
> + * the port that was just added.
> + *
> + * (XenServer Tools 5.5.0 does not exhibit this behavior, and neither
> does
> + * a VM without Tools installed at all.)
> + */
> + rtnetlink_link_notifier_run();
> +
> switch (genlmsghdr->cmd) {
> case BRC_GENL_C_DP_ADD:
> handle_bridge_cmd(idl, ovs, buffer, true);
> @@ -1167,54 +1187,6 @@ netdev_changed_cb(const struct rtnetlink_link_change
> *change, void *idl_)
> return;
> }
>
> - if (netdev_exists(port_name)) {
> - /* A network device by that name exists even though the kernel
> - * told us it had disappeared. Probably, what happened was
> - * this:
> - *
> - * 1. Device destroyed.
> - * 2. Notification sent to us.
> - * 3. New device created with same name as old one.
> - * 4. ovs-brcompatd notified, removes device from bridge.
> - *
> - * There's no a priori reason that in this situation that the
> - * new device with the same name should remain in the bridge;
> - * on the contrary, that would be unexpected. *But* there is
> - * one important situation where, if we do this, bad things
> - * happen. This is the case of XenServer Tools version 5.0.0,
> - * which on boot of a Windows VM cause something like this to
> - * happen on the Xen host:
> - *
> - * i. Create tap1.0 and vif1.0.
> - * ii. Delete tap1.0.
> - * iii. Delete vif1.0.
> - * iv. Re-create vif1.0.
> - *
> - * (XenServer Tools 5.5.0 does not exhibit this behavior, and
> - * neither does a VM without Tools installed at all.)
> - *
> - * Steps iii and iv happen within a few seconds of each other.
> - * Step iv causes /etc/xensource/scripts/vif to run, which in
> - * turn calls ovs-cfg-mod to add the new device to the bridge.
> - * If step iv happens after step 4 (in our first list of
> - * steps), then all is well, but if it happens between 3 and 4
> - * (which can easily happen if ovs-brcompatd has to wait to
> - * lock the configuration file), then we will remove the new
> - * incarnation from the bridge instead of the old one!
> - *
> - * So, to avoid this problem, we do nothing here. This is
> - * strictly incorrect except for this one particular case, and
> - * perhaps that will bite us someday. If that happens, then we
> - * will have to somehow track network devices by ifindex, since
> - * a new device will have a new ifindex even if it has the same
> - * name as an old device.
> - */
> - VLOG_INFO("kernel reported network device %s removed but "
> - "a device by that name exists (XS Tools 5.0.0?)",
> - port_name);
> - return;
> - }
> -
> VLOG_INFO("network device %s destroyed, removing from bridge %s",
> port_name, br_name);
>
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
