> Because everywhere else in the file {read,write}_lock_bh() is used
> instead of {read,write}_lock(), so I'm assuming that _bh is required
> but I really don't know why.
maybe.
> - locking inside ipv6_add_addr() is simpler and more linear but
> semantically wrong because you're unable to tell the user why his
> "ip addr add" failed. E.g. you answer ENOBUFS instead of EEXIST.
We don't want to create duplicate address in any case.
ipv6_add_addr() IS right place.
And, we can return error code by using IS_ERR() etc.
I'll fix this.
> - your ipv6_chk_same_addr() does a useless check for (dev != NULL)
>
> > +static
> > +int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev)
> > +{
> > + struct inet6_ifaddr * ifp;
> > + u8 hash = ipv6_addr_hash(addr);
> > +
> > + read_lock_bh(&addrconf_hash_lock);
> > + for(ifp = inet6_addr_lst[hash]; ifp; ifp=ifp->lst_next) {
> > + if (ipv6_addr_cmp(&ifp->addr, addr) == 0) {
> > + if (dev != NULL && ifp->idev->dev == dev)
> > break;
> > }
>
> your never "break" if dev == NULL, so you could return 0 before
> even acquiring the lock.
It is not a problem because dev is always non-NULL.
However, it should be dev == NULL || ifp->idev->dev == dev.
Thanks.
(I don't understand what you mean by "you could return 0
before even acquiring the lock.")
-- Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org> GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/