Bug 283

(In reply to comment #0)
> I want to implement getsockname in my code. I would like to add GetSockName in
> socket.h:
> 
> class Socket
> {
> public:
>   // pick the one you prefer.
>   int GetSockName (Address &address);
>   int GetSockName (Address *address);
> };
> 

What about:
  Address GetSockName (void) const;
which could be mapped to int getsockname(...); at the simu level?

1) how do I know the error status of that function call ?
2) so far, we have been consistent about our function style except for Recv: we use a BSD-like int error code as return value of all functions.

(In reply to comment #2)
> 1) how do I know the error status of that function call ?
> 2) so far, we have been consistent about our function style except for Recv: we
> use a BSD-like int error code as return value of all functions.
> 

OK, I would pick the &address variant, then.

Created attachment 235 [details]
patch by liu jian

Comment on attachment 235 [details]
patch by liu jian

>diff -r e0a429cb5811 src/internet-stack/tcp-socket-impl.cc
>--- a/src/internet-stack/tcp-socket-impl.cc	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/internet-stack/tcp-socket-impl.cc	Fri Aug 29 13:14:26 2008 +0800
>@@ -544,6 +544,15 @@
>       fromAddress = tag.GetAddress ();
>     }
>   return packet;
>+}
>+
>+int
>+TcpSocketImpl::GetSockName (Address &address) const
>+{
>+  NS_LOG_FUNCTION_NOARGS ();
>+
>+  address = InetSocketAddress(m_localAddress, m_localPort);

Did you check that the default value of m_localAddress is compatible with the value returned by this function when GetSockName is called before Bind ? If not, you have to check what linux does when getsockname is called before bind for tcp sockets and do the same thing here.

>+  return 0;
> }
> 
> void
>diff -r e0a429cb5811 src/internet-stack/tcp-socket-impl.h
>--- a/src/internet-stack/tcp-socket-impl.h	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/internet-stack/tcp-socket-impl.h	Fri Aug 29 13:14:26 2008 +0800
>@@ -93,6 +93,7 @@
>   virtual Ptr<Packet> Recv (uint32_t maxSize, uint32_t flags);
>   virtual Ptr<Packet> RecvFrom (uint32_t maxSize, uint32_t flags,
>     Address &fromAddress);
>+  virtual int GetSockName (Address &address) const; 
> 
> private:
>   friend class Tcp;
>diff -r e0a429cb5811 src/internet-stack/udp-socket-impl.cc
>--- a/src/internet-stack/udp-socket-impl.cc	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/internet-stack/udp-socket-impl.cc	Fri Aug 29 13:14:26 2008 +0800
>@@ -435,6 +435,14 @@
>   return packet;
> }
> 
>+int
>+UdpSocketImpl::GetSockName (Address &address) const
>+{
>+  NS_LOG_FUNCTION_NOARGS ();
>+  address = InetSocketAddress (m_endPoint->GetLocalAddress (), m_endPoint->GetLocalPort());

The above will fail if GetSockName is called before Bind is called which is legal. You need to check for m_endPoint != 0 and return a sane default. I don't know what the default is in linux but you can check that easily with a simple program which creates a socket and calls getsockname without calling bind.

>+  return 0;
>+}
>+
> void 
> UdpSocketImpl::ForwardUp (Ptr<Packet> packet, Ipv4Address ipv4, uint16_t port)
> {
>diff -r e0a429cb5811 src/internet-stack/udp-socket-impl.h
>--- a/src/internet-stack/udp-socket-impl.h	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/internet-stack/udp-socket-impl.h	Fri Aug 29 13:14:26 2008 +0800
>@@ -72,6 +72,7 @@
>   virtual Ptr<Packet> Recv (uint32_t maxSize, uint32_t flags);
>   virtual Ptr<Packet> RecvFrom (uint32_t maxSize, uint32_t flags,
>     Address &fromAddress);
>+  virtual int GetSockName (Address &address) const; 
> 
> private:
>   // Attributes set through UdpSocket base class 
>diff -r e0a429cb5811 src/node/packet-socket.cc
>--- a/src/node/packet-socket.cc	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/node/packet-socket.cc	Fri Aug 29 13:14:26 2008 +0800
>@@ -432,4 +432,26 @@
>   return packet;
> }
> 
>+int
>+PacketSocket::GetSockName (Address &address) const
>+{
>+  NS_LOG_FUNCTION_NOARGS ();
>+  PacketSocketAddress ad;
>+  
>+  ad.SetProtocol (m_protocol);
>+  if (m_isSingleDevice)
>+    {
>+      ad.SetSingleDevice (m_device);
>+    }
>+  else
>+    {
>+      ad.SetAllDevices ();
>+    }
>+  //not sure about this line

Yes, it is clearly wrong. GetSockName is supposed to return the address to which the socket is bound to, not the remote address. You need to return here what was set in DoBind. You also need to be careful about returning valid data if GetSockName is called _before_ Bind is called (yes, this is legal). So, you need to make sure that all the fields you read are initialized to a sane default in the PacketSocket constructor.

For the record, here is what linux does for packet sockets:

        sll->sll_family = AF_PACKET;
        sll->sll_ifindex = po->ifindex;
        sll->sll_protocol = po->num;
        dev = dev_get_by_index(sk->sk_net, po->ifindex);
        if (dev) {
                sll->sll_hatype = dev->type;
                sll->sll_halen = dev->addr_len;
                memcpy(sll->sll_addr, dev->dev_addr, dev->addr_len);
                dev_put(dev);
        } else {
                sll->sll_hatype = 0;    /* Bad: we have no ARPHRD_UNSPEC */
                sll->sll_halen = 0;
        }


>+  ad.SetPhysicalAddress (m_destAddr);
>+  address = ad;
>+  
>+  return 0;
>+}
>+
> }//namespace ns3
>diff -r e0a429cb5811 src/node/packet-socket.h
>--- a/src/node/packet-socket.h	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/node/packet-socket.h	Fri Aug 29 13:14:26 2008 +0800
>@@ -101,6 +101,7 @@
>   virtual Ptr<Packet> Recv (uint32_t maxSize, uint32_t flags);
>   virtual Ptr<Packet> RecvFrom (uint32_t maxSize, uint32_t flags,
>     Address &fromAddress);
>+  virtual int GetSockName (Address &address) const; 
> 
> private:
>   void ForwardUp (Ptr<NetDevice> device, Ptr<const Packet> packet, 
>diff -r e0a429cb5811 src/node/socket.h
>--- a/src/node/socket.h	Thu Aug 28 15:11:55 2008 -0700
>+++ b/src/node/socket.h	Fri Aug 29 13:14:26 2008 +0800
>@@ -493,6 +493,11 @@
>   int RecvFrom (uint8_t* buf, uint32_t size, uint32_t flags,
>                 Address &fromAddress);
>  
>+  /**
>+   * \returns the address name this socket is associated with.
>+   */
>+  virtual int GetSockName (Address &address) const = 0; 
>+ 
> protected:
>   void NotifyConnectionSucceeded (void);
>   void NotifyConnectionFailed (void);

Created attachment 237 [details]
Socket::GetSockName patch

thanks methieu's comments, i did some test calling getsockname before bind, the 
output sockaddr was same to the input.

(In reply to comment #6)
> Created an attachment (id=237) [details]
> Socket::GetSockName patch
> 
> thanks methieu's comments, i did some test calling getsockname before bind, the 
> output sockaddr was same to the input.

can you show me the code you used to test this ?

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=237) [details] [details]
> > Socket::GetSockName patch
> > 
> > thanks methieu's comments, i did some test calling getsockname before bind, the 
> > output sockaddr was same to the input.
> 
> can you show me the code you used to test this ?

And the reason I am asking this is that I wrote the following code and it shows that getsockname does not return the the input address: it returns ip=0.0.0.0, port=0, and family = AF_INET

#include <sys/socket.h>
#include <netinet/in.h>
#include <stdio.h>

int main (int argc, char *argv[])
{
  struct sockaddr_in name;
  socklen_t namelen;
  int fd = socket (AF_INET, SOCK_STREAM, 0);
  
  namelen = sizeof (struct sockaddr);
  name.sin_family = 0xff;
  name.sin_port = 0xffff;
  name.sin_addr.s_addr = 0xffffffff;
  int retval = getsockname (fd, (struct sockaddr *)&name, &namelen);

  if (name.sin_family != AF_INET)
    {
      printf ("family error\n");
    }
  printf ("port=%d\n", name.sin_port);
  printf ("ad=%d\n", name.sin_addr.s_addr);

  return 0;
}

i know what has happend after reading kernel code.
when getsockname() was called without bind(), linux kernel fill the struct sockaddr with the initial value(memset with zero), then it call move_addr_to_user() to fill the userspace sockaddr by the input namelen. 
so,
 if the input namelen  == 0, the output sockaddr not changed,
 if the input namelen == sizeof(sockaddr), the output sockaddr were all zero,
 if the input namelen (0, sizeof(sockaddr)), the output sockaddr partly zero, partly not changed. 

At the NS3 side, GetSockName return address value by 0, when called before bind(). 

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Created an attachment (id=237) [details] [details] [details]
> > > Socket::GetSockName patch
> > > 
> > > thanks methieu's comments, i did some test calling getsockname before bind, the 
> > > output sockaddr was same to the input.
> > 
> > can you show me the code you used to test this ?
> 
> And the reason I am asking this is that I wrote the following code and it shows
> that getsockname does not return the the input address: it returns ip=0.0.0.0,
> port=0, and family = AF_INET
> 
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <stdio.h>
> 
> int main (int argc, char *argv[])
> {
>   struct sockaddr_in name;
>   socklen_t namelen;
>   int fd = socket (AF_INET, SOCK_STREAM, 0);
> 
>   namelen = sizeof (struct sockaddr);
>   name.sin_family = 0xff;
>   name.sin_port = 0xffff;
>   name.sin_addr.s_addr = 0xffffffff;
>   int retval = getsockname (fd, (struct sockaddr *)&name, &namelen);
> 
>   if (name.sin_family != AF_INET)
>     {
>       printf ("family error\n");
>     }
>   printf ("port=%d\n", name.sin_port);
>   printf ("ad=%d\n", name.sin_addr.s_addr);
> 
>   return 0;
> }
> 

my test code:

int main()
{
    int sockfd, ret;
    struct sockaddr_in addr, addr2;
    socklen_t len = 0;
    
    if ((sockfd = socket(AF_INET,SOCK_STREAM,0)) == -1)
    {
        printf("socket creat error\n");
    }
    
    memset(&addr, 0, sizeof(addr));

    len = 6;//different value, different sockaddr output
    addr.sin_family = AF_INET;
    addr.sin_port = htons(8001);
    addr.sin_addr.s_addr = inet_addr("127.0.0.1");
    
    addr2.sin_port = 0xffff;
    addr2.sin_addr.s_addr = 0xffffffff;
    
#if 0    
    if (bind(sockfd, (struct sockaddr*)&addr, sizeof(addr)) == -1)
    {
        printf("bind error %d\n", errno);
    }
#endif

  ret = getsockname(sockfd, (struct sockaddr*)&addr2, &len);
    if (ret == -1)
    {
        printf("getsockname error %d\n", errno);
    }
    
    printf("addr.sin_family = %d, len = %d, addr->sin_port = %d, addr= %s\n",
     addr2.sin_family, len, htons(addr2.sin_port), inet_ntoa(addr2.sin_addr.s_addr));    
}

(In reply to comment #9)
> i know what has happend after reading kernel code.
> when getsockname() was called without bind(), linux kernel fill the struct
> sockaddr with the initial value(memset with zero), then it call
> move_addr_to_user() to fill the userspace sockaddr by the input namelen. 
> so,
>  if the input namelen  == 0, the output sockaddr not changed,
>  if the input namelen == sizeof(sockaddr), the output sockaddr were all zero,
>  if the input namelen (0, sizeof(sockaddr)), the output sockaddr partly zero,
> partly not changed. 
> 
> At the NS3 side, GetSockName return address value by 0, when called before
> bind(). 
> 

(In reply to comment #9)
> i know what has happend after reading kernel code.
> when getsockname() was called without bind(), linux kernel fill the struct
> sockaddr with the initial value(memset with zero), then it call
> move_addr_to_user() to fill the userspace sockaddr by the input namelen. 
> so,
>  if the input namelen  == 0, the output sockaddr not changed,
>  if the input namelen == sizeof(sockaddr), the output sockaddr were all zero,
>  if the input namelen (0, sizeof(sockaddr)), the output sockaddr partly zero,
> partly not changed. 

yes.

> 
> At the NS3 side, GetSockName return address value by 0, when called before
> bind(). 
> 

> > At the NS3 side, GetSockName return address value by 0, when called before
> > bind(). 

The problem with your latest patch is the same as before: GetSockName must do more than just return 0. It should also change the content of the Address variable to contain what the linux version returns, that is, an InetSocketAddress with the right initial values. 

i.e., for example: the right code for TcpSocketImpl:

// make sure local variables are initialized during construction
TcpSocketImpl::TcpSocketImpl ()
: m_localAddress (Ipv4Address::GetAny ()),
  m_localPort (0)
{}
int
TcpSocketImpl::GetSockName (Address &address)
{
  address = InetSocketAddress (m_localAddress, m_localPort);
  return 0;
}

Please, update your patch accordingly for UdpSocket and check that you do something similar for PacketSocket.

mathieu, i can not push an attachment here::internal error..
(i had sent you by mail) 

(In reply to comment #12)
> > > At the NS3 side, GetSockName return address value by 0, when called before
> > > bind(). 
> 
> The problem with your latest patch is the same as before: GetSockName must do
> more than just return 0. It should also change the content of the Address
> variable to contain what the linux version returns, that is, an
> InetSocketAddress with the right initial values. 
> 
> i.e., for example: the right code for TcpSocketImpl:
> 
> // make sure local variables are initialized during construction
> TcpSocketImpl::TcpSocketImpl ()
> : m_localAddress (Ipv4Address::GetAny ()),
>   m_localPort (0)
> {}
> int
> TcpSocketImpl::GetSockName (Address &address)
> {
>   address = InetSocketAddress (m_localAddress, m_localPort);
>   return 0;
> }
> 
> Please, update your patch accordingly for UdpSocket and check that you do
> something similar for PacketSocket.
> 

Created attachment 248 [details]
patch by liu

(In reply to comment #14)
> Created an attachment (id=248) [details]
> patch by liu
> 

latest patch looks good to me. Please, commit _after_ ns-3.2 is released.