Looks good to me. Thanks.
--Justin
On Apr 23, 2012, at 9:16 AM, Ben Pfaff wrote:
> update_port() can delete the port for which it is called, if the underlying
> network device has been destroyed, so HMAP_FOR_EACH is unsafe in
> ofproto_run().
>
> Less obviously, update_port() can delete unrelated ports. For example,
> suppose that initially device A is port 1 and device B is port 2. If
> update_port("A") runs just after this, then it will ofport_remove() both
> ports, then ofport_install() A as the new port 2.
>
> So this commit first assembles a list of ports to update, then updates them
> in a separate loop.
>
> Without this commit, running "ovs-dpctl del-dp" while ovs-vswitchd is
> running consistently causes a crash for me within a few seconds.
>
> Signed-off-by: Ben Pfaff <[email protected]>
> ---
> ofproto/ofproto.c | 18 ++++++++++++++++--
> 1 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index f934306..cb46d26 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1027,8 +1027,9 @@ process_port_change(struct ofproto *ofproto, int error,
> char *devname)
> int
> ofproto_run(struct ofproto *p)
> {
> + struct sset changed_netdevs;
> + const char *changed_netdev;
> struct ofport *ofport;
> - char *devname;
> int error;
>
> error = p->ofproto_class->run(p);
> @@ -1037,18 +1038,31 @@ ofproto_run(struct ofproto *p)
> }
>
> if (p->ofproto_class->port_poll) {
> + char *devname;
> +
> while ((error = p->ofproto_class->port_poll(p, &devname)) != EAGAIN) {
> process_port_change(p, error, devname);
> }
> }
>
> + /* Update OpenFlow port status for any port whose netdev has changed.
> + *
> + * Refreshing a given 'ofport' can cause an arbitrary ofport to be
> + * destroyed, so it's not safe to update ports directly from the
> + * HMAP_FOR_EACH loop, or even to use HMAP_FOR_EACH_SAFE. Instead, we
> + * need this two-phase approach. */
> + sset_init(&changed_netdevs);
> HMAP_FOR_EACH (ofport, hmap_node, &p->ports) {
> unsigned int change_seq = netdev_change_seq(ofport->netdev);
> if (ofport->change_seq != change_seq) {
> ofport->change_seq = change_seq;
> - update_port(p, netdev_get_name(ofport->netdev));
> + sset_add(&changed_netdevs, netdev_get_name(ofport->netdev));
> }
> }
> + SSET_FOR_EACH (changed_netdev, &changed_netdevs) {
> + update_port(p, changed_netdev);
> + }
> + sset_destroy(&changed_netdevs);
>
> switch (p->state) {
> case S_OPENFLOW:
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
