Gur Stavi wrote:
If we don't care about opening up fanout groups to ETH_P_NONE, then patch v2 seems sufficient. If explicitly blocking this, the ENXIO return can be added, but ideally without touching the other lines.
I don't think that allowing ETH_P_NONE is relevant. In my opinion the 2 options that should be considered to fail fanout_add are:
- Testing proto == 0
- Testing proto == 0 || ifindex == -1
The only corner case that is caught by [2] and missed by [1] is the "unlisted" case during do_bind. It is such a rare case that probably no one will ever encounter bind "unlisted" followed by FANOUT_ADD. And this is not a dangerous corner case that leads to system crash.
However, being a purist, I see the major goal of code review to promote correctness by identifying corner cases while improving style is a secondary priority. Since we did identify this corner case in our discussion I think we should still use [2]. I don't consider the code complex. In fact, to me, the ifindex clause is a more understandable direct reason for failure than the proto which is indirect. Having the ifindex clause helps figuring out the proto clause.
It's interesting that the unlisted fix does not return ENODEV, but returns success and leaves the socket in an unbound state, equivalent to binding to ETH_P_NONE and ifindex 0. This seems surprising behavior to the caller.
On rereading that, I still do not see a purpose of special ifindex -1.