Proposal to improve the resolve function

[hSaria]

Looking at the is_ipv4_address method, it seems it uses a somewhat simple method to test for an IPv4 address. Testing 999.999.999.999 would evaluate to True and can lead to resolve assuming that the address is valid.

If I may suggest a solution, you could skip the entirety of the manual work and rely solely on socket.getaddrinfo which will return the address immediately if it does not require resolution:

I'm using time.time() here to show whether the resolution involves a DNS server or not, but I did check with tcpdump.

>>> import socket
>>> import time
>>> 
>>> def resolve(host):
...     checkpoint = time.time()
...     try:
...             print(socket.getaddrinfo(host, port=None)[0][4][0])
...     except OSError:
...             print('failed to resolve')
...     print('took', time.time() - checkpoint)
... 
>>> resolve('9.9.9.9')
9.9.9.9
took 0.00821995735168457
>>> 
>>> resolve ('666.666.666.666')
failed to resolve
took 0.031622886657714844

This will make it so is_ipv4_address and is_ipv6_address are no longer required as the tests are performed by socket.

In addition, there is no way to specify a preferred address family and IPv4 is a forced-defaulted. When an OS has IPv4 and IPv6 routing (i.e. default route over both families), it will prefer IPv6 when calling socket.getaddrinfo by default as it is the newer version.

As such, I would suggest that the resolve function have a keyword argument that defaults to None, but can be set to either 4 or 6 to force a specific address family.

For example:

def resolve(name, address_family=None):
    if address_family == 4:
        # Force IPv4-only
        address_family = socket.AF_INET
    elif address_family == 6:
        # Force IPv6-only
        address_family = socket.AF_INET6
    else:
        # This lets the OS pick which is best (i.e. IPv6 if it has connectivity)
        address_family = None

    try:
        return socket.getaddrinfo(
            host=name,
            port=None,
            family=address_family,
        )[0][4][0]
    except OSError:
        raise NameLookupError(name)

P.S. Great work on the library

[ValentinBELYN]

Hi @hSaria 👋

Thank you for using icmplib! The function used to detect the type of IP address is intentionally simple. It has been designed according to the needs of the library and according to its performance. If you want an example that I'm not looking to verify the correctness of the parameters take a look at the is_ipv6_address function 😄 Generally, the verification of the values is done upstream by the developer when reading its application configuration file or when it asks the user to enter a value.

I agree with you on the second point. Having an option to indicate a specific IP address family could make the function more generic and useful for other purposes. I will implement this for the next version.

However, passing the family=None parameter to the getaddrinfo function is not a good idea. By doing this, you perform two DNS lookups (for A and AAAA records). In this case, the function sends unnecessary packets over the network and degrades the performance of the function. It is better to make two getaddrinfo separately as currently and don't do the second lookup if an IPv4 address has already been found.