From: Robert Olsson <[EMAIL PROTECTED]>
Date: Thu, 22 Dec 2005 10:51:46 +0100
> @@ -219,28 +230,27 @@
> if (rta[RTA_FLOW-1])
> memcpy(&new_r->r_tclassid, RTA_DATA(rta[RTA_FLOW-1]), 4);
> #endif
> -
> - rp = &fib_rules;
> + r = (struct fib_rule *) fib_rules.first;
> if (!new_r->r_preference) {
> - r = fib_rules;
> - if (r && (r = r->r_next) != NULL) {
> - rp = &fib_rules->r_next;
> + if (r && r->hlist.next != NULL) {
> + r = (struct fib_rule *) r->hlist.next;
> if (r->r_preference)
> new_r->r_preference = r->r_preference - 1;
> }
> }
I don't think this is right.
fib_rules.first is a pointer to the hlist_node (within the fib_rule)
not the fib_rule itself.
> - for (r=fib_rules; r; r=r->r_next) {
> + hlist_for_each_entry(r, node, &fib_rules, hlist) {
> if (r->r_ifindex == dev->ifindex) {
> - write_lock_bh(&fib_rules_lock);
> + preempt_disable();
> r->r_ifindex = -1;
> - write_unlock_bh(&fib_rules_lock);
> + preempt_enable();
> }
> }
What is the preempt_disable() actually protecting here?
It's a simple assignment to -1, and since the disable
occurs inside the ifindex test, it is not protecting
that either.
> - for (r=fib_rules; r; r=r->r_next) {
> + hlist_for_each_entry(r, node, &fib_rules, hlist) {
> if (r->r_ifindex == -1 && strcmp(dev->name, r->r_ifname) == 0) {
> - write_lock_bh(&fib_rules_lock);
> + preempt_disable();
> r->r_ifindex = dev->ifindex;
> - write_unlock_bh(&fib_rules_lock);
> + preempt_enable();
> }
> }
Same situation here.
Otherwise, it looks OK :-)
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
