8
\$\begingroup\$

I am writing a wrapper library for network functionality in C and wanted to let the user let the user initialize a struct sockaddr_storage with a format string.

Format ist like: "proto:host:port/servicename" Example: "tcp:localhost:8080", "udp:8.8.8.8:dns"

Possible Protocols Strings: tcp, tcp4, tcp6, udp, udp4, udp6

Some questions I'v got now.

  • Am I using to many comments?
  • Am I using to much register variables?
  • Is this code readable?
  • Do you see parts of the code that I can reduce?

I get the following timing: - see my main function

2:2001::124:2144:153:80 - 0.000163 ms
3:172.217.16.195:80 - 0.000135 ms
0:127.0.0.1:8080 - 0.000013 ms
0:127.129.41.24:463 - 0.000005 ms

Why is the time going rapidly down?

Code:

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
#include <errno.h>
#include <assert.h>
#include <unistd.h>
#include <time.h>

#include <netinet/ip.h>
#include <arpa/inet.h>
#include <netdb.h>

typedef enum {tcp, tcp4, tcp6, udp, udp4, udp6} network_id;

typedef struct Addr {
    network_id proto;       /* protocol (can be): tcp, tcp4, tcp6, udp, udp4, udp6, _unix (unix), unixgram, unixpacket */
    char addr[40];          /* string containng either ipv4 or ipv6 address */
    int port;               /* port number */
} Addr;


#ifndef strncpy_s
int strncpy_s(char *dest, size_t destsz, const char *src, size_t n)
{
    if(dest == NULL)
        return -1;

    while(destsz-- && n-- && *src != '\0')
        *dest++ = *src++;
    *dest = '\0';

    return 0;
}
#endif

/* compare functions that compared until delimiter is found either in str1 or in str2 */
int compare_until_delim(const char *str1, const char *str2, char delim)
{
    /* *str1 != delim xor *str2 == 0 and *str2 != delim xor *str2 == 0 */
    while((!(*str1 != delim) != !(*str1 == '\0')) && (!(*str2 != delim) != !(*str2 == '\0'))) {
        if(*str1 != *str2)
            return false;
        str1++;
        str2++;
    }

    if(*str1 != *str2)
        return false;

    return true;
}

/* protocol table containing supported protocol for socket configuration */
const char *protocol_table[] = 
{
    "tcp:",
    "tcp4:",
    "tcp6:",
    "udp:",
    "udp4:",
    "upd6:",
};

/* simple compare until dilimiter check for the protocol table above */
int check_protocol(const char *str)
{
    int i, table_size;
    table_size = sizeof(protocol_table) / sizeof(*protocol_table);

    for(i = 0; i < table_size ; i++) {
        if(compare_until_delim(protocol_table[i], str, ':'))
            return i;
    }
    return -1; /* not found */
}
 
/** ipstr_to_addr - initializes struct Addr and struct sockaddr_in by string containing specific ip address format
 *  return@int:         (0 on success, -2 = inet_pton error, -1 = parsing error at start index 0, 1...n = error at index)
 *  param@ipstr:        format string like format "proto:host:service" 
 *  param@addr:         struct Addr (internal use) (contains printable addr infos, except protocol) if NULL only the _sockaddr_storage
 *  param@_sockaddr_in: struct sockaddr_in, will be setup regarding to the format string
*/
int ipstr_to_addr(const char *ipstr, Addr *addr, struct sockaddr_storage *_sockaddr_storage, int *socktype)
{

    assert(_sockaddr_storage != NULL);

    register const char *start_ip, *end_ipv6;
    register const char *service_start;
    register char *domain_p;
    struct Addr local;


    char domain_buffer[256]; /* will temporary hold ipv4 and domain format to convert later from */
    bool ipv6;
    int proto, port, err;

    if(addr == NULL)
        addr = &local;

    domain_p = domain_buffer;
    /* set service start to beginning of ipstr and use it as a seperator to service/port */
    service_start = ipstr;
    ipv6 = false;
    err = 0;
    
    /* get internal protocol id  */
    proto = check_protocol(ipstr);
    if(proto == -1) {
        err = -1;
        goto cleanup_error;
    }

    service_start = service_start + strlen(protocol_table[proto]);

    /* if first char after protocol is [ check for ] to say it is an ipv6 address */
    if(*service_start == '[') {
        /* go latest until null byte */
        start_ip = service_start + 1;
        while(*service_start != '\0') {
            /* if enclosure bracket ] was found set ipv6 true and increment service_start */
            if(*service_start == ']') {
                end_ipv6 = service_start;
                ipv6 = true;
                service_start++;
                break;
            }
            service_start++;
        }
        /* service_start should point to ':' in format string right before service/port */
    }

    /* ip not ipv6 assume ipv4 or domain name */
    if(ipv6 == false) {
        /* go latest until null byte */
        while(*service_start != '\0') {
            /* service_start points to seperator ':', increment domain_p first and set to null byte */
            if(*service_start == ':') {
                *domain_p = '\0';
                break;
            }
            /* write a copy to domain_buffer */
            *domain_p++ = *service_start++;
        }
        /* service_start should point to ':' in format string right before service/port too */
    }

    /* if not ':' we should be at the end of the string which leads to incomplete format, return position of error */
    if(*service_start != ':') {
        err = service_start - ipstr;
        goto cleanup_error;
    }

    service_start++; /* increase by one to get the real service start position */
    port = atoi(service_start);   /* try to get port number after ':' */
    /* if port port was set to 0, try get port by name */
    if(port == 0) {
        /* NOTE: maybe use getservent_r for thread safety */
        struct servent *service = getservbyname(service_start, NULL);
        if(service == NULL) {
            err = service_start - ipstr;
            goto cleanup_error;
        }
        port = htons(service->s_port); /* convert to host byte order */
    }

     /*********************/
    /* setup struct Addr */
    addr->proto = proto;
    addr->port = port;

    if(ipv6) {
        ipstr++;
        strncpy_s(addr->addr, sizeof(addr->addr), start_ip, end_ipv6 - start_ip);
    }
    else {
        ///* add check for ipv6 protocol later
        struct hostent *entry = gethostbyname(domain_buffer);
        strncpy(addr->addr, inet_ntoa(*(struct in_addr*)entry->h_addr_list[0]), sizeof(addr->addr));
    }

    if(ipv6 && addr->proto == tcp)
        addr->proto = tcp6;
    else if(ipv6 && addr->proto == udp)
        addr->proto = udp6;

     /****************************/
    /* setup struct sockaddr_in */
    switch(addr->proto) {
        case tcp6:
        case udp6:
            ((struct sockaddr_in6*)_sockaddr_storage)->sin6_family = AF_INET6;
            break;
        case tcp:   /* NOTE: add later should only be usable for listener if calls like 'tcp::port' */
        case udp:   /* NOTE: same as tcp */ 
        case tcp4:
        case udp4:
            ((struct sockaddr_in*)_sockaddr_storage)->sin_family = AF_INET;
            break;
        default:
            err = -3; /* if this happens and internal error occured because this function should check strict and returns if something not parsed correctly*/
            goto cleanup_error;
            break;
    }
    

    /* set port */
    ((struct sockaddr_in*)_sockaddr_storage)->sin_port = htons(addr->port);
    
    if(ipv6) {
        if(inet_pton(((struct sockaddr_in6*)_sockaddr_storage)->sin6_family, addr->addr, &((struct sockaddr_in6*)_sockaddr_storage)->sin6_addr) != 1) {
            err = -2;
            goto cleanup_error;
        }
    }
    else { /* ipv4 */
        if(inet_pton(((struct sockaddr_in*)_sockaddr_storage)->sin_family, addr->addr, &((struct sockaddr_in*)_sockaddr_storage)->sin_addr) != 1) {
            err = -2;
            goto cleanup_error;
        }
    }

    /* set correct socket type */
    if(addr->proto >= tcp && addr->proto <= tcp6)
        *socktype = SOCK_STREAM; /* socket is tcp socket */
    else if(addr->proto >= udp && addr->proto <= udp6)
        *socktype = SOCK_DGRAM;  /* socket is udp socket */

    return err; /* error is 0 (success) */

    /* if some error occure you land here handle the struct Addr properly */
    cleanup_error: {
        *addr->addr = '\0';
        addr->port = -1;
        addr->proto = -1;
        return err;
    }
}

int main(void)
{
    Addr test;
    struct sockaddr_storage addr;
    int socktype;

    clock_t clock_start, clock_end;

    clock_start = clock();
    printf("%d\n", ipstr_to_addr("tcp:[2001::124:2144:153]:http", &test, &addr, &socktype));
    clock_end = clock();
    printf("%d:%s:%d - ", test.proto, test.addr, test.port);
    printf("%f ms\n", (double) (clock_end - clock_start) / CLOCKS_PER_SEC);
    
    clock_start = clock();
    ipstr_to_addr("udp:www.google.de:http", &test, &addr, &socktype);
    clock_end = clock();
    printf("%d:%s:%d - ", test.proto, test.addr, test.port);
    printf("%f ms\n", (double) (clock_end - clock_start) / CLOCKS_PER_SEC);

    clock_start = clock();
    ipstr_to_addr("tcp:localhost:8080", &test, &addr, &socktype);
    clock_end = clock();
    printf("%d:%s:%d - ", test.proto, test.addr, test.port);
    printf("%f ms\n", (double) (clock_end - clock_start) / CLOCKS_PER_SEC);


    clock_start = clock();
    ipstr_to_addr("tcp:127.129.41.24:463", &test, &addr, &socktype);
    clock_end = clock();
    printf("%d:%s:%d - ", test.proto, test.addr, test.port);
    printf("%f ms\n", (double) (clock_end - clock_start) / CLOCKS_PER_SEC);

    /* open listener with netcat and run this */

    int sock = socket(addr.ss_family, SOCK_STREAM, 0);
    if(sock == -1) {
        perror("socket error");
        exit(1);
    }

    if(connect(sock, (struct sockaddr*) &addr, sizeof addr)) {
        perror("connection failed");
        exit(1);
    }

    send(sock, "Hallo Meister\n", 15, 0);

    close(sock);

}
```
\$\endgroup\$
0

3 Answers 3

Reset to default
10
\$\begingroup\$

Am I using too many comments?

No.

Am I using too much register variables?

Yes. The register keyword is completely obsolete these days. It was intended as an optimisation hint to the compiler, which the compiler can shamelessly and freely ignore.

A declaration of an identifier for an A declaration of an identifier for an object with storage-class specifier register suggests that access to the object be as fast as possible. The extent to which such suggestions are effective is implementation-defined.

Most compiler optimizers can do a better job at optimizing code, and you should leave the keyword where it belongs — in the past.

Eliminate magic numbers:

typedef struct Addr {
    network_id proto;       /* protocol (can be): tcp, tcp4, tcp6, udp, udp4, udp6, _unix (unix), unixgram, unixpacket */
    char addr[40];          /* string containng either ipv4 or ipv6 address */
    int port;               /* port number */
} Addr;

Instead of that magic number 40, code could use INET_ADDRSTRLEN and INET6_ADDRSTRLEN defined in <netinet/in.h>

INET_ADDRSTRLEN -- storage for an IPv4 address

INET6_ADDRSTRLEN -- storage for an IPv6 address

Similarly, in:

char domain_buffer[256]

256 should be a named constant.

#define DOMAIN_BUFSIZ 256
char domain_buffer[DOMAIN_BUFSIZ];

It eases maintainability; requires just one change to the constant instead of all occurrences of the number.

Is this code readable?

Short-term memory and the field of vision are small:

ipstr_to_addr() is too long, complex, and hurts readability. Consider breaking it down into 3-4 (or more) smaller functions.

For instance, this:

/* set correct socket type */
    if(addr->proto >= tcp && addr->proto <= tcp6)
        *socktype = SOCK_STREAM; /* socket is tcp socket */
    else if(addr->proto >= udp && addr->proto <= udp6)
        *socktype = SOCK_DGRAM;  /* socket is udp socket */

deserves to be a separate set_socktype() function. So does this:

if(ipv6) {
        if(inet_pton(((struct sockaddr_in6*)_sockaddr_storage)->sin6_family, addr->addr, &((struct sockaddr_in6*)_sockaddr_storage)->sin6_addr) != 1) {
            err = -2;
            goto cleanup_error;
        }
    }
    else { /* ipv4 */
        if(inet_pton(((struct sockaddr_in*)_sockaddr_storage)->sin_family, addr->addr, &((struct sockaddr_in*)_sockaddr_storage)->sin_addr) != 1) {
            err = -2;
            goto cleanup_error;
        }
    }

Do you see parts of the code that I can reduce?

int compare_until_delim(const char *str1, const char *str2, char delim)
{
    /* *str1 != delim xor *str2 == 0 and *str2 != delim xor *str2 == 0 */
    while((!(*str1 != delim) != !(*str1 == '\0')) && (!(*str2 != delim) != !(*str2 == '\0'))) {
        if(*str1 != *str2)
            return false;
        str1++;
        str2++;
    }

    if(*str1 != *str2)
        return false;

    return true;
}

can be shortened to just:

bool compare_until_delim(const char *str1, const char *str2, char delim)
{
    /* *str1 != delim xor *str2 == 0 and *str2 != delim xor *str2 == 0 */
    while((!(*str1 != delim) != !(*str1 == '\0')) && (!(*str2 != delim) != !(*str2 == '\0'))) {
        if(*str1++ != *str2++) {
            return false;
        }
    }
    return *str1 == *str2;
}

with no loss of readability.

NB that I changed the return type from int to bool because the function returns true / false. Use the correct types regardless of any implicit conversions.

And:

int check_protocol(const char *str)
{
    int i, table_size;
    table_size = sizeof(protocol_table) / sizeof(*protocol_table);

    for(i = 0; i < table_size ; i++) {
        if(compare_until_delim(protocol_table[i], str, ':'))
            return i;
    }
    return -1; /* not found */
}

can be shortened to:

int check_protocol(const char *str)
{
    for (size_t i = 0; i < sizeof(protocol_table) / sizeof (*protocol_table); i++) {
        if(compare_until_delim(protocol_table[i], str, ':'))
            return i;
    }
    return -1; /* not found */
}

Use a consistent style:

While I do prefer the shorter one, use a consistent style throughout the code:

while((!(*str1 != delim)

// if(dest == NULL)
if (!dest) 

// if(ipv6 == false)
if (!ipv6)

Simplify:

// while((!(*str1 != delim) ...

while ((*str1 == delim) ...  /* -- @Toby */

atoi() lacks error checking:

port = atoi(service_start);

errno is not set on error so there is no way to distinguish between 0 as an error and as the converted value. No checks for overflow or underflow are done. Only base-10 input can be converted. It is recommended to instead use the strtol() and strtoul() family of functions in new programs.

Limit the scope of variables and only declare where needed:

//int i, table_size;
//    table_size = sizeof(protocol_table) / sizeof(*protocol_table);
//    for(i = 0; i < table_size ; i++) {
//        ...
//    }

size_t table_size = sizeof(protocol_table) / sizeof(*protocol_table);

// Use unsigned types for array and loop indexing.
// sizeof() also returns size_t.
for (size_t i = 0; i < table_size; i++) {
    ...
}

Don't define more variables than you need:

We can forego table_size, as it's not used anywhere else in the function.

for (size_t i = 0; i < sizeof(protocol_table) / sizeof(*protocol_table); i++) {
     ...
}

Or if that is too long for your liking, here's a handy function-like macro:

#define ARRAY_CARDINALITY(x) (sizeof(x) / sizeof((x)[0]))

for (size_t i = 0; ARRAY_CARDINALITY (protocol_table); i++) {
    ...
}

Specify internal linkage:

Functions have external linkage by default in C. But you are not exporting anything from this single translation unit, so specify internal linkage for all functions except main():

// int check_protocol(const char *str)

static int check_protocol (const char *str)

Braces not required:

//cleanup_error: {
//        *addr->addr = '\0';
//        addr->port = -1;
//        addr->proto = -1;
//        return err;
//    }

/* Following the convention that macros and goto labels are in all caps.
*  This makes it easier to spot.
*/
CLEANUP_ERROR:
*addr->addr = '\0'; /* Note that `->` binds more tightly than `*. */
addr->port = -1;
addr->proto = -1;

return -1;

Implementation doesn't match documentation:

typedef struct Addr {
    network_id proto; /* protocol (can be): tcp, tcp4, tcp6, udp, udp4, udp6, _unix (unix), unixgram, unixpacket */
    char addr[40];    /* string containng either ipv4 or ipv6 address */
    int port;         /* port number */
} Addr;

I see 9 protocols in the above comment. But, there are only 6 in protocol_table.

Prefer returning from main():

Use named constants EXIT_SUCCESS and EXIT_FAILURE as return values for main():

//if(sock == -1) {
//        perror("socket error");
//        exit(1);
//}

if (sock == -1) {
    perror ("socket:");
    return EXIT_FAILURE;
}

TCP is a byte stream:

send() and recv() doesn't necessarily send and receive all bytes with a single call.

send() returns the number of bytes actually sent out—this might be less than the number you told it to send! See, sometimes you tell it to send a whole gob of data and it just can't handle it. It'll fire off as much of the data as it can, and trust you to send the rest later. Remember, if the value returned by send() doesn't match the value in len, it's up to you to send the rest of the string. The good news is this: if the packet is small (less than 1K or so) it will probably manage to send the whole thing all in one go. Again, -1 is returned on error, and errno is set to the error number.

— Beej's guide to Network Programming

The convention is to call it in a loop until all bytes are sent, and check its return value.

Upon a successful return, send() shall return the number of bytes sent. Otherwise, it returns -1 to indicate a failure, or 0 to indicate a closed connection.

The return value of strncpy_s is discarded:

// strncpy_s(addr->addr, sizeof(addr->addr), start_ip, end_ipv6 - start_ip);

/* Add */
if (strncpy_s (addr->addr, sizeof addr->addr, start_ip, end_ipv6 - start_ip) == -1) {
    complain();
}

Why return a value when you're just going to ignore it?

Similarly, ipstr_to_addr is documented to return an error code, but its return value is discarded.

/** ipstr_to_addr - initializes struct Addr and struct sockaddr_in by string containing specific ip address format
 *  return@int:         (0 on success, -2 = inet_pton error, -1 = parsing error at start index 0, 1...n = error at index)

Use braces around if/else/for..:

It's generally considered a good practice to use braces even if it's just a single statement.

//if(ipv6 && addr->proto == tcp)
//        addr->proto = tcp6;
//    else if(ipv6 && addr->proto == udp)
//        addr->proto = udp6;

if(ipv6 && addr->proto == tcp) {
    addr->proto = tcp6;
}
else if(ipv6 && addr->proto == udp) {
    addr->proto = udp6;
}

Consider what happens when you add another statement before the else and forgot to add braces? The first one would be rightly bounded to the if statement, but the second? It wouldn't remain conditional at all. This could lead to some nasty bugs. See, for instance, Apple's SSL/TSL bug.

gethostbyname is an obsolete function:

The gethostbyname*(), gethostbyaddr*(), herror(), and hstrerror() functions are obsolete. Applications should use getaddrinfo(3), getnameinfo(3), and gai_strerror(3) instead.

strncpy_s risks invoking undefined behavior:

while(destsz-- && n-- && *src != '\0')
        *dest++ = *src++;

You did not check whether src was a valid pointer. Your code would invoke undefined behavior if src was a NULL pointer.

\$\endgroup\$
9
  • 2
    \$\begingroup\$ It's true that almost all optimizing compilers do produce more optimal code that human coders, but there are times when optimization can't be used. I agree the use of register is generally obsolete, it can help in very rare cases, but this isn't one of them. \$\endgroup\$ Commented Feb 24, 2023 at 13:12
  • \$\begingroup\$ All those !(a != b) simplify to a == b, too. \$\endgroup\$ Commented Feb 24, 2023 at 13:32
  • \$\begingroup\$ @TobySpeight I thought I need a logical xor but it must not overflow str1 and str2 so would'nt I need to check for while(*str1 != delim && (*str1 != '\0' && *str2 != '\0') then compare if *str1 != *str2 and break if true and then return true if *str1 == *str2 else false? because thats what the functions is for. i agree to your first shortening if we check str1 AND str2 for NULL otherwise if the delimiter in str1 is behind the length of str2 it will overflow? \$\endgroup\$ Commented Feb 24, 2023 at 14:15
  • \$\begingroup\$ but I dont agree to your simplification while(*str1 == delim) because it will only be true if the string is filled only with the delim or am I completly wrong? \$\endgroup\$ Commented Feb 24, 2023 at 14:17
  • 1
    \$\begingroup\$ Simpler is to just switch pointers to a handy null char when we see the delimiter: const char end[] = "\0\0"; while (*s1 || *s2) { if (*s1 == delim) s1 = end; if (*s2 == delim) s2 = end; if (*s1++ != *s2++) return false; } return true; \$\endgroup\$ Commented Feb 24, 2023 at 14:51
10
\$\begingroup\$

Use getaddrinfo()

Instead of using inet_ntoa(), getservbyent() and similar low-level functions, use getaddrinfo() instead. It will parse address and ports, both either specified numerically or by name. The only thing that's left for you to do is split the input on colons, and to parse the protocol name. The latter can then be supplied to getaddrinfo() via the hint parameter, where you can specify if you want IPv4, IPv6, and whether you want TCP or UDP.

One name can result in multiple addresses

If you are passing in a hostname instead of a numeric address, the hostname lookup can result in multiple addresses being returned. They could also be a mix of IPv4 and IPv6. You can of course forbid that (pass AI_NUMERICHOST as a flag to getaddrinfo()), or allow it, but in the latter case you might want to return a struct addrinfo* instead of a struct sockaddr.

\$\endgroup\$
7
  • \$\begingroup\$ Very nice this looks like more convenient way so I will add this. \$\endgroup\$ Commented Feb 25, 2023 at 1:57
  • \$\begingroup\$ Okay I implemented a version of this function that return a struct addrinfo but I can not use it in my library if compiled with c89 right? Should I use an include guard to exclude it if c89 is used? \$\endgroup\$ Commented Feb 25, 2023 at 15:03
  • 1
    \$\begingroup\$ You need to #define _POSIX_C_SOURCE 200112L to enable support for getaddrinfo(), unless some other header you are including already does that. But then it should compile fine with a C89-compliant compiler. \$\endgroup\$ Commented Feb 25, 2023 at 15:30
  • \$\begingroup\$ Compiled with the C89 flag or with a C89 compiler? Why would you want to do either of them? \$\endgroup\$ Commented Feb 25, 2023 at 15:44
  • 3
    \$\begingroup\$ @mortytheshorty You need such a #define for many POSIX functions. However, by default GCC will compile in gnuXX mode, where XX is a year number, which automatically adds such definitions (or ones with similar effect) in order to make your life easier. If you compile with a more strict -std=cXX, then you start needing those. \$\endgroup\$ Commented Feb 25, 2023 at 15:55
3
\$\begingroup\$

Candidate compare_until_delim() simplification

/* Compare function that compares strings until delimiter 
   is found either in str1 or in str2 */
bool compare_until_delim(const char *str1, const char *str2, char delim)
  while (*str1 == *str2 && *str1 != delim && *str1) {
    str1++; 
    str2++; 
  }
  return *str1 == *str2;
}

Avoid different signatures

I recommend that code uses exactly the C one or maybe the VS one.

// OP
int strncpy_s(char *, size_t, const char *, size_t)`

// C spec
errno_t strncpy_s(char * restrict, rsize_t, const char * restrict, rsize_t);

// VS 2022
errno_t strncpy_s(char *, size_t, const char *, size_t);

BTW: Writing a fully compatible strncpy_s() with portable code is hard given the "Copying shall not take place between objects that overlap." requirement.

\$\endgroup\$
2
  • \$\begingroup\$ Well I am trying to do this right now although I don't really need it anymore because as I mentioned in the comments above. I write the host part while parsing it into the buffer. But as you said hard I wanted to try to implement it like here here but I am still failing. \$\endgroup\$ Commented Feb 25, 2023 at 19:28
  • \$\begingroup\$ are you sure? if I click it it works. en.cppreference.com/w/c/string/byte/strncpy \$\endgroup\$ Commented Feb 25, 2023 at 19:30

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.