10
\$\begingroup\$

I have written the following code for doing a reverse dns lookup. I'm not sure if there are any errors in it. Please have a look:

#include <stdio.h> //for printf()
#include <stdlib.h> //for exit()
#include <arpa/inet.h> //for inet_pton()
#include <netdb.h> // for NI_MAXHOST, getnameinfo() and gai_strerror()
#include <errno.h> // for errno
#include <string.h> // for strerror()

int main(int argc, char** argv) {
  if(argc<2) {
    printf("\n%s [IP]\n",argv[0]);
    printf("For e.g. %s 10.32.129.77\n",argv[0]);
    exit(-1);
  }
  struct sockaddr_in sa;
  int res = inet_pton(AF_INET, argv[1] , &sa.sin_addr);
  switch(res) {
    case 0: printf("\nInput address is not a valid IPv4 address.\n");
    case -1: if(res == -1)
               printf("\nError(%s)\n",strerror(errno));
             int n_res = inet_pton(AF_INET6, argv[1] , &sa.sin_addr);
             switch(n_res) {
               case 0: printf("\nInput address is not a valid IPv6 address.\n");
               case -1: if(n_res == -1)
                          printf("\nError(%s)\n",strerror(errno));
                        exit(-1);
               case 1: sa.sin_family = AF_INET6;
             }
    case 1: sa.sin_family = AF_INET;
  }

  printf("\nsa.sin_addr.s_addr[%d]\n",sa.sin_addr.s_addr);

  char node[NI_MAXHOST];
  memset(node,0,NI_MAXHOST);
  res = getnameinfo((struct sockaddr*)&sa, sizeof(sa), node, sizeof(node), NULL, 0, 0);
  if (res) {
    printf("%s\n", gai_strerror(res));
    exit(1);
  }

  printf("\nIP[%s]\n",argv[1]);
  printf("HOSTNAME[%s]\n", node);

  return 0;
}
\$\endgroup\$
10
\$\begingroup\$

It's generally best to divide your code into separate sections (possibly separate functions) for argument handling and actual computation.

Errors should be printed to standard error rather than standard output. Also, prefer small integer values for exit status (and since we're in main(), we can use simple return rather than exit() - note the useful EXIT_FAILURE macro for the return value).

It's worth documenting case fallthroughs. This one is especially suspect:

               case 1: sa.sin_family = AF_INET6;
             }
    case 1: sa.sin_family = AF_INET;
  }

After we store AF_INET6, it's immediately overwritten with AF_INET - is that really intended?

This fallthrough would be clearer as two independent cases:

    case 0:
        printf("\nInput address is not a valid IPv6 address.\n");
        /* fallthrough */
    case -1:
        if (n_res == -1)
            printf("\nError(%s)\n",strerror(errno));
        exit(-1);

Compare:

    case 0:
        fprintf(stderr, "\nInput address is not a valid IPv6 address.\n");
        return 1;
    case -1:
        fprintf(stderr, "\nError(%s)\n", strerror(errno));
        return 1;

This line seems to be no use to a user:

printf("\nsa.sin_addr.s_addr[%d]\n",sa.sin_addr.s_addr);

We check for argv less than 2, but we neither complain about nor use any excess arguments.

There's no need to null out the node storage, as getnameinfo() will either fail (in which case we'll never access it) or write a valid string.

Minor (grammar): don't use "for" with "e.g." - that reads like, "for for example".


When I tried running the program, I found it wouldn't work at all with IPv6 addresses, because sockaddr_in is too small for IPv6 addresses. I had to totally rewrite with a union of address types:

#include <arpa/inet.h> //for inet_pton()
#include <netdb.h> // for NI_MAXHOST, getnameinfo() and gai_strerror()

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static int convert4(struct sockaddr_in *sa, const char *name)
{
    return inet_pton(sa->sin_family = AF_INET, name, &sa->sin_addr);
}

static int convert6(struct sockaddr_in6 *sa, const char *name)
{
    return inet_pton(sa->sin6_family = AF_INET6, name, &sa->sin6_addr);
}


int main(int argc, char** argv)
{
    if (argc != 2) {
        fprintf(stderr, "Usage: %s [IP]\nE.g. %s 10.32.129.77\n", argv[0], argv[0]);
        return EXIT_FAILURE;
    }

    union {
        struct sockaddr         sa;
        struct sockaddr_in      s4;
        struct sockaddr_in6     s6;
        struct sockaddr_storage ss;
    } addr;

    if (convert4(&addr.s4, argv[1]) != 1 && convert6(&addr.s6, argv[1]) != 1) {
        fprintf(stderr, "%s: not a valid IP address.\n", argv[1]);
        return EXIT_FAILURE;
    }

    char node[NI_MAXHOST];
    int res = getnameinfo(&addr.sa, sizeof addr, node, sizeof node, NULL, 0, NI_NAMEREQD);
    if (res) {
        fprintf(stderr, "%s: %s\n", argv[1], gai_strerror(res));
        return EXIT_FAILURE;
    }

    puts(node);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for your corrections and suggestions. Btw what's the purpose of including sockaddr_storage in the union? \$\endgroup\$ – Vishal Sharma Jan 15 at 12:29
  • \$\begingroup\$ struct sockaddr_storage isn't required for this program (I just copied wholesale from How to fill sockaddr_storage?), but just helps demonstrate that the union is large enough for any address type. You can safely remove it. \$\endgroup\$ – Toby Speight Jan 15 at 12:33
8
\$\begingroup\$

Use getaddrinfo()

getnameinfo() has a counterpart: getaddrinfo(). Prefer to use that instead of having to call inet_pton() multiple times. You can force it to only allow numerical IP addresses as input. Here is how it would work:

struct addrinfo *ai;
struct addrinfo hints = {
    .ai_flags = AI_NUMERICHOST,
    .ai_family = AF_UNSPEC,
};

int res = getaddrinfo(argv[1], NULL, &hints, &ai);
if (res) {
    fprintf(stderr, "%s: %s\n", argv[1], gai_strerror(res));
    return 1;
}

char node[NI_MAXHOST];
res = getnameinfo(ai->ai_addr, ai->ai_addrlen, node, sizeof node, NULL, 0, NI_NAMEREQD);
freeaddrinfo(ai);

if (res) {
    fprintf(stderr, "%s: %s\n", argv[1], gai_strerror(res));
    exit(1);
}
\$\endgroup\$
5
\$\begingroup\$

Your code uses an inconsistent style for its spacing. Sometimes you write if (cond) and sometimes if(cond). You should pick either style and stick to it.

I am assuming that you prefer a condensed writing style because in several cases you don't even leave a space after a comma. In that case, you should consistently apply this condensed style, and your code should look like:

#include<errno.h>
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<arpa/inet.h>
#include<netdb.h>
int main(int c,char**v){if(c<2){printf("\n%s [IP]\n",v[0]);printf("For e.g. %"
"s 10.32.129.77\n",v[0]);exit(-1);}struct sockaddr_in sa;int res=inet_pton(
AF_INET,v[1],&sa.sin_addr);switch(res){case 0:printf("\nInput address is not a"
" valid IPv4 address.\n");case-1:if(res==-1)printf("\nError(%s)\n",strerror(
errno));int n_res=inet_pton(AF_INET6,v[1],&sa.sin_addr);switch(n_res){case 0:
printf("\nInput address is not a valid IPv6 address.\n");case-1:if(n_res==-1)
printf("\nError(%s)\n",strerror(errno));exit(-1);case 1:sa.sin_family=AF_INET6;
}case 1:sa.sin_family=AF_INET;}printf("\nsa.sin_addr.s_addr[%d]\n",sa.sin_addr.
s_addr);char node[NI_MAXHOST];memset(node,0,NI_MAXHOST);res=getnameinfo((struct
sockaddr*)&sa,sizeof sa,node,sizeof node,NULL,0,0);if(res){printf("%s\n",
gai_strerror(res));exit(1);}printf("\nIP[%s]\n",v[1]);printf("HOSTNAME[%s]\n",
node);return 0;}

This code style is unreadable for anyone. There is a fun competition that encourages this code style, but other than that, it's useless.

On the other hand, when you take an arbitrary IDE (integrated development environment) and tell it to format your code (it's often just a single keyboard shortcut), it will come up with something like this:

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <arpa/inet.h>
#include <netdb.h>

int main(int argc, char **argv)
{
    if (argc < 2) {
        printf("\n%s [IP]\n", argv[0]);
        printf("For e.g. %s 10.32.129.77\n", argv[0]);
        exit(-1);
    }

    struct sockaddr_in sa;
    int res = inet_pton(AF_INET, argv[1], &sa.sin_addr);
    switch (res) {
    case 0:
        printf("\nInput address is not a valid IPv4 address.\n");
    case -1:
        if (res == -1)
            printf("\nError(%s)\n", strerror(errno));

        int n_res = inet_pton(AF_INET6, argv[1], &sa.sin_addr);
        switch (n_res) {
        case 0:
            printf("\nInput address is not a valid IPv6 address.\n");
        case -1:
            if (n_res == -1)
                printf("\nError(%s)\n", strerror(errno));
            exit(-1);
        case 1:
            sa.sin_family = AF_INET6;
        }
    case 1:
        sa.sin_family = AF_INET;
    }
    printf("\nsa.sin_addr.s_addr[%d]\n", sa.sin_addr.s_addr);

    char node[NI_MAXHOST];
    memset(node, 0, NI_MAXHOST);
    res = getnameinfo((struct sockaddr *)&sa, sizeof sa, node, sizeof node, NULL, 0, 0);
    if (res) {
        printf("%s\n", gai_strerror(res));
        exit(1);
    }

    printf("\nIP[%s]\n", argv[1]);
    printf("HOSTNAME[%s]\n", node);

    return 0;
}

I manually added some empty lines to split the code into logical sections, to give the reader a time to breathe. This style is much more common and can be read easily by any C programmer.

The remaining issues have already been covered in the other answers.

\$\endgroup\$
  • \$\begingroup\$ Your application is not consistent: it should be #include<errno.h> and int main(int c,char**v) (note the removed space). (trying to pretend I'm not joking) \$\endgroup\$ – L. F. Jan 17 at 4:13
  • \$\begingroup\$ Thanks, I fixed the inconsistencies. \$\endgroup\$ – Roland Illig Jan 17 at 6:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.