On Fri, Aug 09, 2013 at 04:13:22PM -0700, Andy Zhou wrote:
> Thanks for the update. the code is now easier to reason, especially with
> Clang's lock annotation.
>
> It looks good over all. Some minor comments inline.
>
> Acked-by: Andy Zhou <[email protected]>
> > @@ -359,22 +426,38 @@ netdev_get_tunnel_config(const struct netdev *netdev)
> >
> > static void
> > netdev_unref(struct netdev *dev)
> > + OVS_RELEASES(netdev_mutex)
> > {
> > ovs_assert(dev->ref_cnt);
> > if (!--dev->ref_cnt) {
> > + const struct netdev_class *class = dev->netdev_class;
> > + struct netdev_registered_class *rc;
> > + int old_ref_cnt;
> > +
> > dev->netdev_class->destruct(dev);
> >
> > shash_delete(&netdev_shash, dev->node);
> > free(dev->name);
> > dev->netdev_class->dealloc(dev);
> > + ovs_mutex_unlock(&netdev_mutex);
> > +
> > + ovs_rwlock_rdlock(&netdev_class_rwlock);
> >
> Is the class pointer still safe if dev is deallocated?
It ought to be because we know that dev had a ref on the class. We're
about to drop that ref.
> > + rc = netdev_lookup_class(class->type);
> >
> Should we check for rc value? May be assert if it is NULL?
I usually figure that a segfault is as good as an assert in a case like
this.
Thanks a lot for the review.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
