Brian Haley <[EMAIL PROTECTED]> wrote on 10/10/2007 02:20:45 PM:
> David Stevens wrote:
> > What about just checking for 0 in the later test?
> >
> > if (val && __dev_get_by_index(val) == NULL) {
>
> We could fail the next check right before that though:
Right, the semantics there would be "if we have a bound
dev if, that's the only legal value here." Setting it to '0' in
that case doesn't really do anythng, anyway. But I don't care
about that semantic difference-- could even add "val &&" to the
bound_dev_if check.
What I don't like is that your "if" creates an identical
duplicate code path for the functional part of it. In this case
it's trivial (the asignment), but makes the code look more
complex than it really is. If v4 does it that way, I don't
like that either. :-)
I agree with it in general, and may not be worth the
trouble, but I'd personally prefer something like:
if (sk->sk_type == SOCK_STREAM)
goto e_inval;
if (val && sk->sk_bound_dev_if && sk->sk_bound_dev_if != val)
goto e_inval;
if (val && __dev_get_by_index(val) != NULL) {
retv = -ENODEV;
break;
}
[at this point all validity checks are done and we're following
one code path to do the work; each check is easily
identifiable.]
np->mcast_oif = val;
retv = 0;
break;
Or maybe:
if (sk->sk_type == SOCK_STREAM)
goto e_inval;
if (val) {
if (sk->sk_bound_dev_if && sk->sk_bound_dev_if != val)
goto e_inval;
if (__dev_get_by_index(val != NULL) {
retv = -ENODEV;
break;
}
}
np->mcast_oif = val;
retv = 0;
break;
But anyway, I made the comment; I think some form of it
should go in. :-) If you like the original better, that's
ok with me, too.
+-DLS
-
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
