3
\$\begingroup\$

I have tried the code for finding the class of the IP address the user inputs, and printing the network and the host bits in the IP address. Could someone please review this approach and provide suggestions. Thanks:).

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<sys/socket.h>
#include<netinet/in.h>
#include<arpa/inet.h>
#include<error.h>
#include<errno.h>
#define size 32
int bin[size];
void print(int class, int nb, int hb);
void convert_to_binary(uint32_t decimal);

void print(int class, int nb, int hb) 
{
    int i = 0;

    printf("The Network bits are:\n");
    for(i = class; i < (nb + class); i++) {
        printf("%d ", bin[i]);
    }
    printf("\n");
    printf("The host bits are:\n");
    for(i = (class + nb); i < size; i++) {
        printf("%d ", bin[i]);
    }
}

void convert_to_binary(uint32_t decimal)
{

    int index = 0;
    int temp = 0;
    int i = 0;

    while (decimal > 0) {
        temp = decimal % 2;
        bin[index++] = temp;
        decimal = decimal / 2;
    }
    for (i = 0; i < size / 2; i++) {
        temp = bin[i];
        bin[i] = bin[size - i - 1];
        bin[size - i - 1] = temp;
    }

}

int main(int argc, char **argv)
{
    struct in_addr addr;
    uint32_t result;
    char *IP;
    int ret = 0;    
    int value = 0;

    if (argc != 2)
        error(1, errno, "Too many or few arguments\n");
    IP = argv[1];
        ret = inet_aton(IP, &addr);
    if (ret == 0)
        error(1, errno, "Invalid IP- address provided\n");
    printf("The address in the structure is network byte order:(Big- Endian) %X %d\n", addr, addr);
    result = ntohl(addr.s_addr);
    convert_to_binary(result);
    if (bin[0] == 0) {
        printf("Class - A\n");
        print(1, 7, 24);
    } else if (bin[0] == 1 && bin[1] == 0) {
        printf("Class - B\n");
        print(2, 14, 24);
    } else if(bin[0] == 1 && bin[1] == 1 && bin[2] == 0) {
        printf("Class - C\n");
        print(3, 21, 8);
    } else {
        printf("It does not belong to any of the class\n");
    }   
}
```
\$\endgroup\$
  • \$\begingroup\$ There is actually a minor bug. for class B, print(2, 14, 24); should be print(2, 14, 16); Since the third parameter is ignored, this bug doesn't matter. \$\endgroup\$ – David G. May 31 at 10:42
3
\$\begingroup\$

Here are some things that may help you improve your code.

Eliminate unused variables

The parameter hb in the print() function and value in main() are unused variables and should be omitted from the program.

Fix your formatting

There are inconsistent spaces at the beginning of lines and inconsistent indentation. Being consistent helps others read and understand your code.

Use only required #includes

The code has a number of #includes that are not needed. In this code, for instance, nothing from <string.h> is used, so that header can and should be omitted. Only include files that are actually needed. This makes the code easier to understand and maintain and also may slightly speed up compiling.

Understand what standards don't guarantee

A knowledgeable reader of this code (and possibly the compiler) is likely to be puzzled by this line of code:

printf("The address in the structure is network byte order:(Big- Endian) %X %d\n", addr, addr);

The reason is that "%X" expects an unsigned int and "%d" expects an int but what you're passing is struct in_addr. The man page tells us that the relevant defintion for that is as follows:

typedef uint32_t in_addr_t;

struct in_addr {
   in_addr_t s_addr;
};

This means that on machines where int is not the same size as uint32_t, there is a potential for trouble. Specifically, if the code is compiled for a machine with a 16-bit int, this won't give the results you intend. Better would be to have the compiler check to make sure that this doesn't happen. One way to do this is to add these lines to the program:

#include <limits.h>
#if UINT_MAX == 65535
#error "This program requires at least a 32-bit unigned int."
#endif

Note that sizeof can't be used for the preprocessor, which is why we use the definition of UINT_MAX. This isn't foolproof, since there are real machines using 24-bit words, but catches the more common case of a 16-bit. To be completely sure, we can't really use the preprocessor. We could do this:

if (sizeof(int) < 4) {
    printf("This program requires at least 32-bit int, but yours has a %ld-bit int\n", sizeof(int) * 8);
    return 1;
}

Then instead of passing addr to your printf, pass addr.s_addr instead.

Try to write portable code

Things that are guaranteed by the C standard are absolutely portable to conforming compilers on all platforms. Things that are specified by POSIX standards are portable to the subset of machines that conform to POSIX. Your life will be easier if you aim for those. Things that are compiler extensions, such as error are not necessarily portable to other machines. For that reason, I'd suggest that instead of using error in this program (which is a GNU extension), you could use the form shown above and use printf and return. It's also worth noting that inet_aton is not actually in the POSIX standard, but is widely used (and much better than the poorly defined but standard inet_addr). However, it may have some surprising attributes. For example, it will interpret a single number as an IP address. If we give the argument 3232236027 it will interpret this as the address 192.168.1.251. If that's what you want, that's fine. If not, you might consider instead using inet_pton (which is POSIX):

ret = inet_pton(AF_INET, IP, &addr);

Eliminate global variables where practical

The code declares and uses a global variable, bin. Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them. That way the linkage is explicit and may be altered more easily if needed. In this case, you could define bin within main like this:

int bin[32];

And then call print like this:

printf("Class - A\n");
print(1, 7, bin, 32);

Eliminate complexity where practical

The convert_to_binary() function is rather complex internally. I'd recommend simplifying it or eliminating it entirely. Here's one way to simplify it:

void convert_to_binary(uint32_t decimal, int *bin, size_t size)
{
    bin += size - 1;
    for ( ; size; --size) {
        *bin-- = decimal & 1u;
        decimal >>= 1;
    }
}

Rethink function interfaces

The essential piece of work done in the program is to identify the address class of the passed IPv4 address. I'd put that into a function like this:

char address_class(uint32_t addr) {
    const struct class_mask {
        uint32_t mask;
        uint32_t classbits;
        char designator;
    } class_mask[3] = {
        { 0x80000000u, 0x00000000u, 'A' },
        { 0xc0000000u, 0x80000000u, 'B' },
        { 0xe0000000u, 0xc0000000u, 'C' },
    };
    for (unsigned i=0; i < 3; ++i) {
        if ((addr & class_mask[i].mask) == class_mask[i].classbits) {
            return class_mask[i].designator;
        }
    }
    return '?';
}

With a small change, we could actually turn this into the print function. If we do so, the entire program then looks like this:

#include <stdio.h>
#include <arpa/inet.h>

char print_classification(uint32_t addr) {
    const struct class_mask {
        uint32_t mask;
        uint32_t classbits;
        uint32_t netmask;
        uint32_t hostmask;
        char designator;
    } class_mask[3] = {
        { 0x80000000u, 0x00000000u, 0x40000000u, 0x00800000u, 'A' },
        { 0xc0000000u, 0x80000000u, 0x20000000u, 0x00008000u, 'B' },
        { 0xe0000000u, 0xc0000000u, 0x10000000u, 0x00000080u, 'C' },
    };
    for (unsigned i=0; i < 3; ++i) {
        if ((addr & class_mask[i].mask) == class_mask[i].classbits) {
            printf("class = %c\nThe network bits are:\n", class_mask[i].designator);
            for (uint32_t mask = class_mask[i].netmask; mask; mask >>= 1) {
                if (mask == class_mask[i].hostmask) {
                    printf("\nThe host bits are:\n");
                }
                printf("%c ", addr & mask ? '1' : '0');
            }
            printf("\n");
            return class_mask[i].designator;
        }
    }
    printf("Unknown address class\n");
    return '?';
}

int main(int argc, char **argv)
{
    struct in_addr addr;

    if (sizeof(int) < 4) {
        printf("This program requires at least 32-bit int, but yours has a %ld-bit int\n", sizeof(int) * 8);
        return 1;
    }
    if (argc != 2) {
        printf("Usage: %s ipv4addr\nwhere ipv4addr is of the form xxx.xxx.xxx.xxx\n", argv[0]);
        return 1;
    }
    if (inet_pton(AF_INET, argv[1], &addr) == 0) {
        printf("Error: couldn't interpret %s as an IPv4 address\n", argv[1]);
    }   
    print_classification(ntohl(addr.s_addr));
}

Rethink the purpose of the program

Perhaps this was just a programming exercise, but it's probably worth pointing out that the use of IPv4 address classes has been obsolete since the introduction of CIDR in 1993.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You seem to think class B is /12 and class C is /16. This is wrong. Class B is /16 and class C is /24. OP had this right. The sizes you are using are the size of the private use blocks in the classes. \$\endgroup\$ – David G. May 31 at 10:29
  • \$\begingroup\$ @DavidG. You're right. Thank you! \$\endgroup\$ – Edward May 31 at 13:36

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.