Bugzilla – Bug 943
Add a SO_BROADCAST socket option
Last modified: 2010-07-19 10:21:07 EDT
Created attachment 919 [details] patch Real world sockets do not allow applications to send datagrams to a broadcast destination address unless the socket has been configured to allow broadcast via setsockopt with SO_BROADCAST. This patch adds such an option to the socket via SetAllowBroadcast and GetAllowBroadcast methods. The option is not properly enforced yet in any of the NS-3 sockets, but we should probably try to implement it in UDP sockets at least (I would have, but it does not sound trivial). I have implemented in a new socket type I am working on called RealUdpSocket, which translates NS-3 socket API calls into real unix sockets.
In principle I have no problems with the concept; I like to align with sockets when people have use cases for it. I think it is applicable to Ipv4 datagram sockets only. For stream sockets or for IPv6 we could either return -1 and set EINVAL, or else (keeping the return void as you have done) just unconditionally FATAL_ERROR ("Invalid for this type of socket"), and always return "false" for the Get() method. Plus, as you mention, enforce it.
(In reply to comment #1) > In principle I have no problems with the concept; I like to align with sockets > when people have use cases for it. > > I think it is applicable to Ipv4 datagram sockets only. For stream sockets or > for IPv6 we could either return -1 and set EINVAL, or else (keeping the return > void as you have done) just unconditionally FATAL_ERROR ("Invalid for this type > of socket"), and always return "false" for the Get() method. Agreed regarding IPv6. I checked olsrd, it does not seem to use the SO_BROADCAST option for the IPv6 sockets, only for the IPv4 ones. About returning void or an error code, the choice of returning void I made based on the only other socket option method that I saw available: BindToNetDevice(). However, setsockopt does return an int (0=ok, -1=error), so I think it makes more sense to return some kind of error indicator. In the case of the Socket class, we seem to be using the same kind of 0 or -1 int return for signalling errors... > > Plus, as you mention, enforce it. This might require changes deep in the bowels of IPv4, as it is not easy to determine immediately whether a destination address is broadcast or not, you have to look at the available interfaces in the host. SendTo is the most problematic one: doing the above iteration of host interfaces on a per-packet basis is going to be slow without some sort of cache based optimization, and adding a cache also wastes memory... really not the kind of decision I like to make (lose-lose) :-/
Created attachment 923 [details] patch to enforce the AllowBroadcast socket option for UDP This additional patch enforces the AllowBroadcast option for UDP sockets (sorry, not possible to distinguish between IPv4 and IPv6 atm). TCP sockets do not allow broadcast. All the other sockets report that they allow broadcast.
comparing with ssh://code@code.nsnam.org/repos/ns-3-dev searching for changes changeset: 6437:c11291f51d57 user: Gustavo J. A. M. Carneiro <gjc@inescporto.pt> date: Tue Jun 15 18:29:45 2010 +0100 summary: Bug 943 - Add a SO_BROADCAST socket option changeset: 6438:96f612b05035 user: Gustavo J. A. M. Carneiro <gjc@inescporto.pt> date: Tue Jun 15 18:32:04 2010 +0100 summary: Fix OLSR socket usage: use the new SetAllowBroadcast socket option; Bind to interface bcast address instead of local address, use SendTo instead of Send. This is how things have to work with real world sockets. changeset: 6439:958a299f1100 user: Gustavo J. A. M. Carneiro <gjc@inescporto.pt> date: Fri Jun 18 17:18:59 2010 +0100 summary: Enforce the AllowBroadcast socket option for UDP sockets changeset: 6440:38a995eb219e user: Gustavo J. A. M. Carneiro <gjc@inescporto.pt> date: Mon Jul 12 11:44:47 2010 +0100 summary: Update python bindings changeset: 6441:f555caf874dc tag: tip user: Gustavo J. A. M. Carneiro <gjc@inescporto.pt> date: Mon Jul 12 11:45:15 2010 +0100 summary: Fix regression tests after bug 943 changes
Reopening... In repairing the failing regression test tonight, it seemed to me that this is not quite correct: /** * \brief Query whether broadcast datagram transmissions are allowed * * This method corresponds to using getsockopt() SO_BROADCAST of * real network or BSD sockets. * * \returns true if broadcast is allowed, false otherwise */ virtual bool GetAllowBroadcast () const = 0; The way this is implemented is that it returns true if the socket is Udp, regardless of whether SetAllowBroadcast(true) was called or not. However, getsockopt(SO_BROADCAST) will return the state of the broadcast flag. If we want to make the implementation match the doxygen and match getsockopt(), we should return the state of the underlying broadcast flag. If we do that, we will be lacking a way to tell whether the underlying socket is broadcast capable. I see three possible solutions here. 1) add to class Socket virtual void GetSockType (enum Socket::SocketType& stype) = 0; and check stype against SOCK_DGRAM before trying to SetAllowBroadcast(). 2) make this change to SetAllowBroadcast: * \param allowBroadcast Whether broadcast is allowed + * \return true if operation succeeds */ - virtual void SetAllowBroadcast (bool allowBroadcast) = 0; + virtual bool SetAllowBroadcast (bool allowBroadcast) = 0; 3) keep SetAllowBroadcast returning void, but set SocketErrno I would vote for 2) above to solve this particular problem.
(In reply to comment #5) > Reopening... > > In repairing the failing regression test tonight, it seemed to me that this is > not quite correct: > > /** > * \brief Query whether broadcast datagram transmissions are allowed > * > * This method corresponds to using getsockopt() SO_BROADCAST of > * real network or BSD sockets. > * > * \returns true if broadcast is allowed, false otherwise > */ > virtual bool GetAllowBroadcast () const = 0; > > The way this is implemented is that it returns true if the socket is Udp, > regardless of whether SetAllowBroadcast(true) was called or not. However, > getsockopt(SO_BROADCAST) will return the state of the broadcast flag. > > If we want to make the implementation match the doxygen and match getsockopt(), > we should return the state of the underlying broadcast flag. Sorry about that, an obvious oversight. > > If we do that, we will be lacking a way to tell whether the underlying socket > is broadcast capable. I see three possible solutions here. > > 1) add to class Socket > > virtual void GetSockType (enum Socket::SocketType& stype) = 0; > > and check stype against SOCK_DGRAM before trying to SetAllowBroadcast(). > > 2) make this change to SetAllowBroadcast: > > * \param allowBroadcast Whether broadcast is allowed > + * \return true if operation succeeds > */ > - virtual void SetAllowBroadcast (bool allowBroadcast) = 0; > + virtual bool SetAllowBroadcast (bool allowBroadcast) = 0; > > 3) keep SetAllowBroadcast returning void, but set SocketErrno > > I would vote for 2) above to solve this particular problem. Yes. 2) sounds good. But then, in addition we should never have fatal errors if trying to enable broadcast for a socket that doesn't support broadcast.
> > Yes. 2) sounds good. But then, in addition we should never have fatal errors if > trying to enable broadcast for a socket that doesn't support broadcast. Agree.
Created attachment 935 [details] patch to SetAllowBroadcast return bool (minus python bindings rescan)
comparing with ssh://code@code.nsnam.org/repos/ns-3-dev searching for changes changeset: 6448:184a509cc71d user: Gustavo J. A. M. Carneiro <gjc@inescporto.pt> date: Mon Jul 19 12:17:51 2010 +0100 summary: Still Bug 943: fix UdpSocketImpl::GetAllowBroadcast, let Socket::SetAllowBroadcast return a bool indicating success/failure, instead of a fatal error.