9

I'm porting a IPv4 application to a AF-independent codebase(it should work with IPv4 and IPv6). Now I'm using sockaddr_storage wherever I can, however now I have to set(populate) a sockaddr_storage. But I don't know what the correct way is. The previous code was:

// defined in data_socket.h
struct sockaddr_in laddr;

Now there is this function that sets sin_addr and sin_port:

void DataSocket::SetLocalAddr(const char *addr, const int port)
{
    this->laddr.sin_port = htons(port);
    if(addr != NULL)
        this->laddr.sin_addr.s_addr = inet_addr(addr);
    else
        this->laddr.sin_addr.s_addr = inet_addr("0.0.0.0");
}

As you see this is the old style(uses IPv4).

Now my changes are below. First I've changed sockaddr_in to sockaddr_storage

// defined in data_socket.h
struct sockaddr_storage laddr;

Then I changed the code above to support IPv4 and IPv6:

void DataSocket::SetLocalAddr(const char *addr, const int port)
{ 
switch (this->GetAddrFamily(addr)) {
    case AF_INET:
        (struct sockaddr_in *) this->laddr.sin_port = htons(port);
        if(addr != NULL)
            inet_pton(AF_INET, addr, (struct sockaddr_in *) this->laddr.sin_addr);
        else
            inet_pton(AF_INET, "0.0.0.0", (struct sockaddr_in *) this->laddr.sin_addr);
        break;

    case AF_INET6:
        (struct sockaddr_in6 *) this->laddr.sin6_port = htons(port);
        if(addr != NULL)
            inet_pton(AF_INET6, addr, (struct sockaddr_in6 *) this->laddr.sin6_addr);
        else
            inet_pton(AF_INET6, "0:0:0:0:0:0:0:0", (struct sockaddr_in6 *) this->laddr.sin6_addr);
        break;

    default:
        return NULL;

}

}

Where GetAddrFamily() is:

int DataSocket::GetAddrFamily(const char *addr)
{
    struct addrinfo hints, *res;
    int status, result;

    memset(&hints, 0, sizeof(hints));
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;

    if ((status = getaddrinfo(addr, 0, &hints, &res)) != 0)
    {
        fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(status));
        return false;
    }

    result = res->ai_family; // This might be AF_INET, AF_INET6,etc..
    freeaddrinfo(res); // We're done with res, free it up

    return result;
}

It seems my way is to complex. Is this the correct way to do this? Because I've changed sockaddr_in to sockaddr_storage, actually I just want the reverse of this question: Getting IPV4 address from a sockaddr structure

I'm trying to find the best solution, for example here: http://www.kame.net/newsletter/19980604/ it says never use inet_ntop() and inet_pton(), however some others(like the Beej's network tutorial) says that inet_ntop() and inet_pton() should be used for IPv6 based application.

Is my way of implementation correct or should I change it?

Community
  • 1
  • 1
Fatih Arslan
  • 16,499
  • 9
  • 54
  • 55

2 Answers2

5

I highly recommend letting getaddrinfo do all the heavy lifting, eg.

void DataSocket::SetLocalAddr(const char *addr, const unsigned short int port)
{
    struct addrinfo hints, *res;
    int status;
    char port_buffer[6];

    sprintf(port_buffer, "%hu", port);

    memset(&hints, 0, sizeof(hints));
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    /* Setting AI_PASSIVE will give you a wildcard address if addr is NULL */
    hints.ai_flags = AI_NUMERICHOST | AI_NUMERICSERV | AI_PASSIVE;

    if ((status = getaddrinfo(addr, port_buffer, &hints, &res) != 0)
    {
        fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(status));
        return;
    }

    /* Note, we're taking the first valid address, there may be more than one */
    memcpy(&this->laddr, res->ai_addr, res->ai_addrlen);

    freeaddrinfo(res);
}
Hasturkun
  • 35,395
  • 6
  • 71
  • 104
  • 1
    Thanks for the code. But I'm confused with the `memcpy(&this->laddr, res->ai_addr, res->ai_addrlen);` line. The `res->ai_addr` is a sockaddr structure, I guess it gets automatically converted to `sockaddr_in` or `sockaddr_in6`? How it stored into `this->laddr`(because `this->laddr` is a `sockaddr_storage` structure)? – Fatih Arslan Jul 12 '12 at 15:11
  • And i guess I should use `port_buffer`instead of `port` in `getaddrinfo`, because `getaddrinfo()` excepted a const char *(aka string)? – Fatih Arslan Jul 12 '12 at 15:19
  • 2
    `res->ai_addr` points to some variant of `struct sockaddr`, either `sockaddr_in` or `sockaddr_in6`. There's no conversion going on, the structure is simply being copied as is. `struct sockaddr_storage` is of a size and layout allowing it to be cast into any of the `sockaddr` variants, all of which share a common header. re: passing a string as the port, I wouldn't bother, but it's your code. (also, fixed typo, getaddrinfo was meant to be the buffer) – Hasturkun Jul 12 '12 at 15:56
  • I asked because I will use the `this->laddr` structure in another function. However I don`t know which structure it is based on. I've initially set it as `sockaddr_storage`. Does it mean I have to check now `this->laddr.ss_family` (suppose it is `sockaddr_storage) and cast it to `sockaddr_in` or `sockaddr_in6` if I want to use this in other functions? Or should I check for `this->laddr.sa_family` (because it is a plain `sockaddr` structure)? – Fatih Arslan Jul 13 '12 at 08:12
  • @FatihArslan: Checking `laddr.ss_family` and checking `(struct sockaddr)laddr.sa_family` are identical. For the most part, you can just pass the `struct sockaddr_storage` around, casting it to `sockaddr` (or the _in/in6 variants) when needed. – Hasturkun Jul 15 '12 at 16:20
1

If I understand correctly you are casting this to the first member? don't do that, name the member.

I also would make it easier to read by introducing a {} scope and a local variable for both of the cases, something like:

{
 struct sockaddr_in * in4 = reinterpret_cast< struct sockaddr_in * >(&this->addr);
 in4->laddr.sin_port = htons(port);
 ... etc
} 

An since you are using C++ and not C for that, use C++ style casts. C-style casts in C++ are far to ambiguous.

Jens Gustedt
  • 76,821
  • 6
  • 102
  • 177