Krdytkyu

I've written a small Python function to check whether a string is a valid IPv4 subnet mask.

It works but maybe it's too convoluted and terse? Or maybe it could be improved / optimised / rewritten in a cleverer way than just to check 1's in a string?..

IsIPv4Address is another function of mine (EDIT 1: added it into the question).
I specifically don't want to use any libraries / external functions as this is rather a study in Python :)

def IsIPv4Address(ipAdd):

    """Validate an IPv4 address"""

    octets = ipAdd.split(".")

    if len(octets) != 4:

        return False

    for octet in octets:

        if not IsInteger(octet):

            return False

        if int(octet) > 255 or int(octet) < 0:

            return False



    return True





def IsIPv4Mask(ipMask):

    """Validate an IPv4 subnet mask"""



    # Each mask looks like an IPv4 address and must pass the checks

    if not IsIPv4Address(ipMask):

        return False



    ipMaskBinary = ""

    ipMaskBinary = ipMaskBinary.join([bin(int(oct))[2:] for oct in ipMask.split(".")])



    isBitZero = ipMask[0] == "0"

    for bit in ipMaskBinary[1:]:

        if bit == "1" and isBitZero:

                return False



        if bit == "0":

            isBitZero = True



    return True

As recommended in PEP 8, the official style guide, function names should be lower_case_with_underscores unless you have a good reason for making an exception.

Functions like this are perfect places to write doctests, to explain how the function should behave while providing unit tests at the same time.

The IsIPv4Address() function is easy to implement. I would take advantage of the all() function with a generator expression and a double-ended inequality.

def is_ipv4_address(dotquad):

    """

    Validate an IPv4 address in dotted-quad notation.



    >>> is_ipv4_address("1.2.3.4")

    True

    >>> is_ipv4_address("127.0.0.1/8")

    False

    >>> is_ipv4_address("1.2.3.4.5")

    False

    >>> is_ipv4_address("1.2.3")

    False

    >>> is_ipv4_address("1.2.3.256")

    False

    >>> is_ipv4_address("1.2.3.-4")

    False

    >>> is_ipv4_address("fe80::")

    False

    """

    octets = dotquad.split(".")

    return len(octets) == 4 and 

           all(o.isdigit() and 0 <= int(o) < 256 for o in octets)

The netmask verifier is trickier to write well. You have chosen to use bin() to stringify the 32-bit number. I'd avoid using strings for something that can be done arithmetically, since string manipulation is relatively inefficient.

Furthermore, I'd suggest that instead of writing just a validation function, you may as well write a function to tell you the length of the netmask, since it's nearly the same amount of work, but you can get more useful information that way.

def ipv4_mask_len(dotquad):

    """

    Finds the number of bits set in the netmask.



    >>> ipv4_mask_len("255.255.255.0")

    24

    >>> ipv4_mask_len("0.0.0.0")

    0

    >>> ipv4_mask_len("255.255.255.255")

    32

    >>> ipv4_mask_len("127.0.0.0")

    Traceback (most recent call last):

      ...

    ValueError: Invalid netmask: 127.0.0.0

    """

    if not is_ipv4_address(dotquad):

        raise ValueError("Invalid netmask: {0}".format(dotquad))

    a, b, c, d = (int(octet) for octet in dotquad.split("."))

    mask = a << 24 | b << 16 | c << 8 | d



    if mask == 0:

        return 0



    # Count the number of consecutive 0 bits at the right.

    # https://wiki.python.org/moin/BitManipulation#lowestSet.28.29

    m = mask & -mask

    right0bits = -1

    while m:

        m >>= 1

        right0bits += 1



    # Verify that all the bits to the left are 1's

    if mask | ((1 << right0bits) - 1) != 0xffffffff:

        raise ValueError("Invalid netmask: {0}".format(dotquad))

    return 32 - right0bits

IsIPv4Address should pass a value of 3 for maxsplit on your split call.

Most of that function (after the len check) can be condensed to:

return all(IsInteger(o) and 0 <= o < 256 for o in octets)

Your IsIPv4Mask should probably be done in a very different manner - rather than string-ifying the octets to binary text, it should convert the words to a single 32-bit integer (as is done everywhere else), for efficiency. At that point, write a loop that

• checks the current LSB


• shifts the integer by 1


• loops until the integer is equal to 0