[Xorp-hackers] Patch to FEA to support multiple xorp instances on Linux.

[Bruce Simpson]

Hi Ben,

Thanks for your FEA patch. Unfortunately, I don't believe that we can 
take this patch as-is just at the moment. A good deal of reworking will 
be required.

Ben Greear wrote:
> Attached is a patch for just my FEA changes (and a few related bits to 
> make that
> function properly).

A few technical comments:

 * Setting SO_REUSEADDR unconditionally on UDP socket binds should not 
be needed across all operating systems.
    BSD-derived networking stacks, for example, should support 
SO_REUSEPORT; and binding the socket on such systems is a no-no (the 
laddr and faddr part of the tuple should not be bound, this breaks the 
kernel's lookup and traffic will not be received).

 * It would be helpful if the Linux specific behaviour, which apparently 
requires SO_REUSEADDR, could be contained to the FEA. You should be able 
to call comm_set_reuseaddr() conditionally from within the relevant FEA 
code, instead of performing it in libcomm.

    * For an example, look at the 
IoTcpUdpSocket::udp_open_bind_broadcast() method in the FEA. This also 
contains an example of how to deal with SO_BINDTODEVICE in a portable way.

    * I'm concerned that as we plan to move to Boost.ASIO in the future, 
that having platform hacks in libcomm is going to pollute the code, and 
make it difficult to transition. I noticed that Boost.ASIO's socket 
support is lacking in a few places, and we will have to push changes 
back to them incrementally; preserving the existing code flow in the FEA 
will make this much easier.

 * Using a socket per interface is a good idea in principle; not all 
systems support the BSD behaviour of requesting IP_RECVIF information as 
an out-of-band control message with recvmsg().

 * Also, as libcomm is a C API, code which invokes it shouldn't be 
relying on C++ booleans being casted to int values; please use explicit 
integer values for libcomm calls.

 * As discussed elsewhere, we can't take changes which specify the 
device name explicitly in the APIs just yet, as this is likely to break 
portability. However, some refinements could be made here; please read on.

 * I notice that the patch makes changes to multicast socket behaviour.
    What would be ideal is a change which conditionally uses the RFC 
3678 (Socket Interface Extensions for Multicast) APIs, as these are 
supported by up-to-date operating systems, and do allow multicast groups 
to be joined by specifying the link explicitly (which is the correct 
behaviour), not the protocol address of the link.
    We also need to support RFC 3678 for dealing with unnumbered 
interfaces, and IPv6, in a forwards compatible way.

 * We do need to parse RTA attributes in the Netlink socket support, to 
be able to deal with features such as ECMP. A good starting point would 
be to rework this change, and keep it simple, but more general.

 * Have you seen any alignment errors with the reinterpret_cast<> used 
herein? A test run of compiling for an embedded target would be really 
useful.
    I want to avoid introducing alignment constraint violations into the 
code; some of the offending code has been cleaned up, however, this 
remains in a few places.

 * The method to leave all multicast groups on a link, in the FEA, is a 
useful addition.

 * Can you elaborate further on the nature of the race conditions you 
seem to be experiencing with the FEA on Linux (based on your code 
comments) ?

Style comments:
 * Try to preserve whitespace and indentation style wherever possible.
 * Please don't use camelCase, this is a style bug.
 * Block comments should be /* ... */ wherever possible.
 * Try to avoid // at the end of a block, unless the nesting is 
non-obvious from indentation.

I appreciate this may be frustrating, given you have clearly put a lot 
of hard work into your changes, but I need to ensure that any patch 
which makes such wide-ranging changes to the system, preserves code 
style, portability, and correctness.

When writing code like this, I find it very helpful to have a copy of 
UNIX Network Programming handy:
    http://www.kohala.com/start/unpv12e.html
...it points out the differences between the main implementations, and 
has a number of useful illustrations which show how the APIs fit 
together. It is no substitute for experience, however, it is an 
excellent desk reference and tutorial book.

Thanks for the SCons RTA_TABLE change, this has already been checked 
into SVN, and I hope it helps minimize diffs between your own fork, and 
XORP SVN.

best regards,
BMS