Merge pull request python-zeroconf#75 from stephenrauch/Fix-name-change

stephenrauch · web-flow · commit 136dce985fd6 · 2016-07-12T18:33:54.000-07:00
Fix for python-zeroconf#29
diff --git a/README.rst b/README.rst
@@ -78,7 +78,7 @@ The easiest way to install python-zeroconf is using pip::
 How do I use it?
 ================
 
-Here's an example:
+Here's an example of browsing for a service:
 
 .. code-block:: python
 
@@ -122,6 +122,21 @@ See examples directory for more.
 Changelog
 =========
 
+0.17.7.dev
+------
+
+* Better Handling of DNS Incoming Packets parsing exceptions
+* Many exceptions will now log a warning the first time they are seen
+* Catch and log sendto() errors
+* Fix/Implement duplicate name change
+* Greatly improve handling of oversized packets including:
+
+  - Implement name compression per RFC1035
+  - Limit size of generated packets to 9000 bytes as per RFC6762
+  - Better handle over sized incoming packets
+
+* Increased test coverage to 94%
+
 0.17.6
 ------
 
diff --git a/setup.py b/setup.py
@@ -45,6 +45,7 @@
         'Programming Language :: Python :: 3',
         'Programming Language :: Python :: 3.3',
         'Programming Language :: Python :: 3.4',
+        'Programming Language :: Python :: 3.5',
         'Programming Language :: Python :: Implementation :: CPython',
         'Programming Language :: Python :: Implementation :: PyPy',
     ],
diff --git a/test_zeroconf.py b/test_zeroconf.py
@@ -91,6 +91,10 @@ def test_service_info_dunder(self):
         assert not info != info
         repr(info)
 
+    def test_dns_outgoing_repr(self):
+        dns_outgoing = r.DNSOutgoing(r._FLAGS_QR_QUERY)
+        repr(dns_outgoing)
+
 
 class PacketGeneration(unittest.TestCase):
 
@@ -216,6 +220,15 @@ def test_lots_of_names(self):
         # instantiate a zeroconf instance
         zc = Zeroconf(interfaces=['127.0.0.1'])
 
+        # create a bunch of servers
+        type_ = "_my-service._tcp.local."
+        name = 'a wonderful service'
+        server_count = 300
+        self.generate_many_hosts(zc, type_, name, server_count)
+
+        # verify that name changing works
+        self.verify_name_change(zc, type_, name, server_count)
+
         # we are going to monkey patch the zeroconf send to check packet sizes
         old_send = zc.send
 
@@ -233,24 +246,14 @@ def send(out, addr=r._MDNS_ADDR, port=r._MDNS_PORT):
         # monkey patch the zeroconf send
         zc.send = send
 
-        # create a bunch of servers
-        type_ = "_my-service._tcp.local."
-        server_count = 300
-        records_per_server = 2
-        for i in range(int(server_count / 10)):
-            self.generate_many_hosts(zc, type_, 10)
-            sleep_count = 0
-            while sleep_count < 100 and server_count * records_per_server > len(
-                    zc.cache.entries_with_name(type_)):
-                sleep_count += 1
-                time.sleep(0.01)
-
         # dummy service callback
         def on_service_state_change(zeroconf, service_type, state_change, name):
             pass
 
-        # start a browser and run for a bit
+        # start a browser
         browser = ServiceBrowser(zc, type_, [on_service_state_change])
+
+        # wait until the browse request packet has maxed out in size
         sleep_count = 0
         while sleep_count < 100 and \
                 longest_packet[0] < r._MAX_MSG_ABSOLUTE - 100:
@@ -265,7 +268,7 @@ def on_service_state_change(zeroconf, service_type, state_change, name):
                            sleep_count, longest_packet[0])
 
         # now the browser has sent at least one request, verify the size
-        assert longest_packet[0] < r._MAX_MSG_ABSOLUTE
+        assert longest_packet[0] <= r._MAX_MSG_ABSOLUTE
         assert longest_packet[0] >= r._MAX_MSG_ABSOLUTE - 100
 
         # mock zeroconf's logger warning() and debug()
@@ -333,11 +336,35 @@ def on_service_state_change(zeroconf, service_type, state_change, name):
         mocked_log_warn.stop()
         mocked_log_debug.stop()
 
-    def generate_many_hosts(self, zc, type_, number_hosts):
-        import random
-        for i in range(number_hosts):
-            name = hex(random.randint(0, 1 << 32 - 1))
-            self.generate_host(zc, 'a wonderful service' + name[1:], type_)
+    def verify_name_change(self, zc, type_, name, number_hosts):
+        desc = {'path': '/~paulsm/'}
+        info_service = ServiceInfo(
+            type_, '%s.%s' % (name, type_), socket.inet_aton("10.0.1.2"),
+            80, 0, 0, desc, "ash-2.local.")
+
+        # verify name conflict
+        self.assertRaises(
+            r.NonUniqueNameException,
+            zc.register_service, info_service)
+
+        zc.register_service(info_service, allow_name_change=True)
+        assert info_service.name.split('.')[0] == '%s-%d' % (
+            name, number_hosts + 1)
+
+    def generate_many_hosts(self, zc, type_, name, number_hosts):
+        records_per_server = 2
+        block_size = 25
+        number_hosts = int(((number_hosts - 1) / block_size + 1)) * block_size
+        for i in range(1, number_hosts + 1):
+            next_name = name if i == 1 else '%s-%d' % (name, i)
+            self.generate_host(zc, next_name, type_)
+            if i % block_size == 0:
+                sleep_count = 0
+                while sleep_count < 40 and \
+                        i * records_per_server > len(
+                            zc.cache.entries_with_name(type_)):
+                    sleep_count += 1
+                    time.sleep(0.05)
 
     @staticmethod
     def generate_host(zc, host_name, type_):
@@ -356,7 +383,9 @@ def generate_host(zc, host_name, type_):
 class Framework(unittest.TestCase):
 
     def test_launch_and_close(self):
-        rv = r.Zeroconf(interfaces=['127.0.0.1'])
+        rv = r.Zeroconf(interfaces=r.InterfaceChoice.All)
+        rv.close()
+        rv = r.Zeroconf(interfaces=r.InterfaceChoice.Default)
         rv.close()
 
 
diff --git a/zeroconf.py b/zeroconf.py
@@ -217,10 +217,12 @@ def service_type_name(type_):
     """
     if not (type_.endswith('._tcp.local.') or type_.endswith('._udp.local.')):
         raise BadTypeInNameException(
-            "Type must end with '._tcp.local.' or '._udp.local.'")
+            "Type '%s' must end with '._tcp.local.' or '._udp.local.'" %
+            type_)
 
     if type_.startswith('.'):
-        raise BadTypeInNameException("Type must not start with '.'")
+        raise BadTypeInNameException(
+            "Type '%s' must not start with '.'" % type_)
 
     remaining = type_[:-len('._tcp.local.')].split('.')
     name = remaining.pop()
@@ -264,7 +266,7 @@ def service_type_name(type_):
 
     if len(remaining) > 1:
         raise BadTypeInNameException(
-            "Unexpected characters '%s.' in %s" % (
+            "Unexpected characters '%s.' in '%s'" % (
                 '.'.join(remaining[1:]), type_))
 
     if remaining:
@@ -765,6 +767,16 @@ def __init__(self, flags, multicast=True):
         self.authorities = []
         self.additionals = []
 
+    def __repr__(self):
+        return '<DNSOutgoing:{%s}>' % ', '.join([
+            'multicast=%s' % self.multicast,
+            'flags=%s' % self.flags,
+            'questions=%s' % self.questions,
+            'answers=%s' % self.answers,
+            'authorities=%s' % self.authorities,
+            'additionals=%s' % self.additionals,
+        ])
+
     class State(enum.Enum):
         init = 0
         finished = 1
@@ -789,7 +801,41 @@ def add_authorative_answer(self, record):
         self.authorities.append(record)
 
     def add_additional_answer(self, record):
-        """Adds an additional answer"""
+        """ Adds an additional answer
+
+        From: RFC 6763, DNS-Based Service Discovery, February 2013
+
+        12.  DNS Additional Record Generation
+
+           DNS has an efficiency feature whereby a DNS server may place
+           additional records in the additional section of the DNS message.
+           These additional records are records that the client did not
+           explicitly request, but the server has reasonable grounds to expect
+           that the client might request them shortly, so including them can
+           save the client from having to issue additional queries.
+
+           This section recommends which additional records SHOULD be generated
+           to improve network efficiency, for both Unicast and Multicast DNS-SD
+           responses.
+
+        12.1.  PTR Records
+
+           When including a DNS-SD Service Instance Enumeration or Selective
+           Instance Enumeration (subtype) PTR record in a response packet, the
+           server/responder SHOULD include the following additional records:
+
+           o  The SRV record(s) named in the PTR rdata.
+           o  The TXT record(s) named in the PTR rdata.
+           o  All address records (type "A" and "AAAA") named in the SRV rdata.
+
+        12.2.  SRV Records
+
+           When including an SRV record in a response packet, the
+           server/responder SHOULD include the following additional records:
+
+           o  All address records (type "A" and "AAAA") named in the SRV rdata.
+
+        """
         self.additionals.append(record)
 
     def pack(self, format_, value):
@@ -995,10 +1041,18 @@ def get_by_details(self, name, type_, class_):
     def entries_with_name(self, name):
         """Returns a list of entries whose key matches the name."""
         try:
-            return self.cache[name]
+            return self.cache[name.lower()]
         except KeyError:
             return []
 
+    def current_entry_with_name_and_alias(self, name, alias):
+        now = current_time_millis()
+        for record in self.entries_with_name(name):
+            if (record.type == _TYPE_PTR and
+                    not record.is_expired(now) and
+                    record.alias == alias):
+                return record
+
     def entries(self):
         """Returns a list of all entries"""
         if not self.cache:
@@ -1689,12 +1743,12 @@ def remove_all_service_listeners(self):
         for listener in [k for k in self.browsers]:
             self.remove_service_listener(listener)
 
-    def register_service(self, info, ttl=_DNS_TTL):
+    def register_service(self, info, ttl=_DNS_TTL, allow_name_change=False):
         """Registers service information to the network with a default TTL
         of 60 seconds.  Zeroconf will then respond to requests for
         information for that service.  The name of the service may be
         changed if needed to make it unique on the network."""
-        self.check_service(info)
+        self.check_service(info, allow_name_change)
         self.services[info.name.lower()] = info
         if info.type in self.servicetypes:
             self.servicetypes[info.type] += 1
@@ -1789,28 +1843,42 @@ def unregister_all_services(self):
                 i += 1
                 next_time += _UNREGISTER_TIME
 
-    def check_service(self, info):
+    def check_service(self, info, allow_name_change):
         """Checks the network for a unique service name, modifying the
         ServiceInfo passed in if it is not unique."""
+        service_name = service_type_name(info.name)
+
+        # This asserts breaks on the current subtype based tests
+        # need to make subtypes a first class citizen
+        #          assert service_name == info.type
+        #  instead try:
+        assert service_name == '.'.join(info.type.split('.')[-4:])
+        instance_name = info.name[:-len(service_name) - 1]
+        next_instance_number = 2
+
         now = current_time_millis()
         next_time = now
         i = 0
         while i < 3:
-            for record in self.cache.entries_with_name(info.type):
-                if (record.type == _TYPE_PTR and
-                        not record.is_expired(now) and
-                        record.alias == info.name):
-                    if info.name.find('.') < 0:
-                        info.name = '%s.[%s:%s].%s' % (
-                            info.name, info.address, info.port, info.type)
-
-                        self.check_service(info)
-                        return
+            # check for a name conflict
+            while self.cache.current_entry_with_name_and_alias(
+                    info.type, info.name):
+                if not allow_name_change:
                     raise NonUniqueNameException
+
+                # change the name and look for a conflict
+                info.name = '%s-%s.%s' % (
+                    instance_name, next_instance_number, info.type)
+                next_instance_number += 1
+                service_type_name(info.name)
+                next_time = now
+                i = 0
+
             if now < next_time:
                 self.wait(next_time - now)
                 now = current_time_millis()
                 continue
+
             out = DNSOutgoing(_FLAGS_QR_QUERY | _FLAGS_AA)
             self.debug = out
             out.add_question(DNSQuestion(info.type, _TYPE_PTR, _CLASS_IN))
@@ -1874,7 +1942,7 @@ def handle_query(self, msg, addr, port):
         # Support unicast client responses
         #
         if port != _MDNS_PORT:
-            out = DNSOutgoing(_FLAGS_QR_RESPONSE | _FLAGS_AA, False)
+            out = DNSOutgoing(_FLAGS_QR_RESPONSE | _FLAGS_AA, multicast=False)
             for question in msg.questions:
                 out.add_question(question)
 
