3
\$\begingroup\$

For my Networking class, we learned about subnetting and subnet masks.

I decided to write a little "calculator" (a generous term) that when given n-many network bits, generates a 32-bit subnet mask, then displays it as octets, and as binary with the leading 0s removed. Example main:

int main() {
    int nNetworkBits = 26;
    BitField mask = generateSubnetMask(nNetworkBits);

    printf("%u\n", mask);  // Prints 4294967232
    printf("\n");

    printOctets(mask);  // Prints 255.255.255.192
    printf("\n");

    printBinary(mask);  // Prints 11111111111111111111111111000000
    printf("\n");
}

I'm pretty terrible at C, so I'd like comments on anything and everything that is worth mentioning. I figured out the math/bitshifting as I was writing the code, so if I'm doing anything "roundabout", I'd definitely like to know about it.

The only thing that I don't need advice on is my "abuse" of printf. I know I should be accumulating a string in a buffer then returning that buffer instead of printing directly from the functions. That's a pain in C though and detracted from the main problem that I was trying to solve, so I decided to go for the quick and dirty way.

#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>


#define TOTAL_BITS 32
#define N_OCTETS ((TOTAL_BITS / 8))

typedef uint32_t BitField;


BitField generateOctetMask(int octetI) {
    return (BitField)0xFF << (octetI * 8);
}

BitField generateSubnetMask(int nNetworkBits) {
    int nHostBits = TOTAL_BITS - nNetworkBits;
    return ~0 << nHostBits;  // TODO: Cast 0 to BitField?
}

void printOctets(BitField maskAddress) {
    for (int octetN = N_OCTETS - 1; octetN >= 0; octetN--) {
        BitField mask = generateOctetMask(octetN);
        printf("%d", (maskAddress & mask) >> (octetN * 8));

        if (octetN > 0) {
            printf(".");
        }
    }
}

void printBinary(BitField n) {
    bool areDropping = true;  // So we can drop leading 0s.

    for (BitField mask = 1u << 31; mask > 0; mask >>= 1) {
        bool isSet = n & mask;

        if (isSet) {
            areDropping = false;
            printf("1");

        } else if (!areDropping) {
            printf("0");
        }
    }
}

int main() {
    int nNetworkBits = 26;
    BitField mask = generateSubnetMask(nNetworkBits);

    printf("%u\n", mask);  // Prints 4294967232
    printf("\n");

    printOctets(mask);  // Prints 255.255.255.192
    printf("\n");

    printBinary(mask);  // Prints 11111111111111111111111111000000
    printf("\n");
}
\$\endgroup\$
4
\$\begingroup\$

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

Prefer const to #define

For constants such as TOTAL_BITS, it's generally better to use a named const value rather than a #define. The primary reason is that the const definition enforces type checking, while the macro does not. However, see the next suggestion.

Use sizeof for portability

The code currently contains these lines:

#define TOTAL_BITS 32
#define N_OCTETS ((TOTAL_BITS / 8))

typedef uint32_t BitField;

However, I think I'd define everything instead as const values all derived from the type. That is, I'd suggest this:

typedef uint32_t BitField;
const int BitFieldOctets = sizeof(BitField);
const int BitFieldBits = 8 * BitFieldOctets;

Think carefully about signed vs. unsigned

If generateSubnetMask is passed a negative number, what should it do? I'd suggest that the passed number should be unsigned instead of int and that the value could be checked to make sure it does not exceed the naximum number of bits.

Be careful when shifting negative values

By default, constants are int, so this code:

return ~0 << nHostBits;

Should probably instead be written like this:

return ~0u << nHostBits;

Avoid pointless recalculations

The code has this function:

void printOctets(BitField maskAddress) {
    for (int octetN = BitFieldOctets - 1; octetN >= 0; octetN--) {
        BitField mask = generateOctetMask(octetN);
        printf("%d", (maskAddress & mask) >> (octetN * 8));

        if (octetN > 0) {
            printf(".");
        }
    }
}

This is more complex than it needs to be. First, the mask can be generated just once for the high byte and then shifted, eliminating a number of calculations that don't need to be done:

void printOctets(BitField maskAddress) {
    BitField mask = generateOctetMask(BitFieldOctets - 1);
    for (unsigned i=BitFieldOctets; i; --i) {
        printf("%d", (maskAddress & mask) >> (BitFieldBits - 8));
        maskAddress <<= 8;
        if (i != 1) {
            printf(".");
        }
    }
}

This works by keeping the mask in place over the most significant byte and then shifting the address to the left each iteration. An alternative would be to use a union as the BitField type to allow access to each byte very simply. That approach, however, requires that you account for the endian-ness of your machine, which this code does not.

Consider restructuring for simplicity and clarity

The printBinary code is currently like this:

void printBinary(BitField n) {
    bool areDropping = true;  // So we can drop leading 0s.

    for (BitField mask = 1u << 31; mask > 0; mask >>= 1) {
        bool isSet = n & mask;

        if (isSet) {
            areDropping = false;
            printf("1");

        } else if (!areDropping) {
            printf("0");
        }
    }
}

Instead, I'd break this into two different loops: the first to skip leading zeroes and the second to actually print the bits. Here's one way to do that:

void printBinary(BitField n) {
    BitField mask = 1u << (BitFieldBits - 1);
    // skip leading zeroes
    for (; mask && !(mask & n); mask >>= 1);
    // print all other bits
    for (; mask; mask >>= 1) {
        putchar(n & mask ? '1' : '0');
    }
}

Note that here I'm using the named constants rather than the "magic number" 31.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good stuff, thank you. \$\endgroup\$ – Carcigenicate Jan 20 at 22:44

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.