Setting IPv4/IPv6 address and port to a sockaddr_storage structure

Question

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?

Answer 1

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);
}

Answer 2

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.