On 3.4.2014 15:35, Jan Cholasta wrote:
On 2.4.2014 14:07, Martin Basti wrote:
Patch 30:

+        if isinstance(labels, str):
+            if not labels:
+                raise ValueError('empty string')
+        elif isinstance(labels, unicode):
+            if not labels:
+                raise ValueError('empty string')

It might be nicer to:

+        if isinstance(labels, basestring) and not labels:
+            raise ValueError('empty string')
+        if isinstance(labels, str):
+        elif isinstance(labels, unicode):
Is it expected that you can't create root name?

I would like to imitate dns-python semantics as much as possible.

In [2]: dns.name.from_text("")
Out[2]: <DNS name .>

In [4]: dns.name.Name([])
Out[4]: <DNS name @>

In [5]: dns.name.Name([""])
Out[5]: <DNS name .>

I would like to see more descriptive error texts if you insist on current semantics.


+    @staticmethod
+    def get_root():
+        return DNSName(dns.name.root)
+    @staticmethod
+    def get_origin_sign():
+        return DNSName(u'@')
+    @staticmethod
+    def get_rev_zone():
+        return DNSName(u'in-addr.arpa.')
+    @staticmethod
+    def get_ip6_rev_zone():
+        return DNSName(u'ip6.arpa.')

I think you should either drop the "get_" prefix from the name, or (even
better) make these global constants.

I would shorten "origin_sign" to just "sign".
Sign of what? Decay? :-) I don't think that sign is descriptive enough, I would personally stick with origin_sign.

Can you please use tuples of str objects (i.e. what dns.name.Name uses
internally) instead of unicode objects for the initialization? I think it
should be the preferred style of initializing DNSName objects (DN objects do
the same).
Please don't forget to consult dns-python3 and try to make migration from dns-python to dns-python3 easy.


+    def ToASCII(self, omit_final_dot=False):
+        return super(DNSName,
+    def ToUnicode(self, omit_final_dot=False):
+        return super(DNSName,

What was the reason for the unusual naming again? I would prefer PEP-8
compatible names (e.g. "to_ascii" and "to_unicode"), but if the current names
absolutely have to stay, please add a comment with explanation.

I don't like the "omit_final_dot" flag. IMHO it should be dropped and whether
the result includes a final dot or not should depend solely on whether the
name is absolute or relative. You can still use e.g.
"name.derelativize(root).ToUnicode()" to drop the final dot, which is more
Generally, I agree. We can get rid of the relative/absolute name madness by using final dot everywhere.


+    def is_ip_reverse(self):
Please use is_ip4_reverse() instead. That name will always remind developer not to forgot to IPv6 :-)

+        if self.is_subdomain(self.get_rev_zone()):
+            return True
+        return False

Patch 31:
+                except dns.name.NameTooLong:
+ raise ValueError(_('domain name cannot be longer than 255 characters'))

Personally, I would say '255 bytes' instead of '255 characters'. Characters are tricky when IDN is involved etc.

Patch freeipa-mbasti-0036-DNSName-conversion-in-ipaldap.patch:

@@ -255,6 +256,10 @@ class IPASimpleLDAPObject(object):
         'managedtemplate': DN,
         'managedbase':     DN,
         'originscope':     DN,
+        'idnsname':        DNSName,
+        'idnssoamname':    DNSName,
+        'idnssoarname':    DNSName,
+        'dnszoneidnsname': DNSName,

Maybe you can add following attributes too:

Patch 38:


+_dns_zone_record = DNSName(u'@')

You know you already defined a constant for this in patch 30, right?
It seems that both constants are unnecessary because IMHO
is more readable and you don't need to dig into code to find out what the cryptic name means.


+def _hostname_validator_idn(ugettext, value):
+    assert isinstance(value, DNSName)
+    req_len = 2
+    if value.is_absolute():
+        req_len = 3
+    if len(value.labels) < req_len:
+        return _('invalid domain-name: not fully qualified')
This test is not correct because FQDN = an absolute name.

This code tries to guess if the name is FQDN or not but we can directly use is_absolute() test.

 def is_forward_record(zone, str_address):
     addr = netaddr.IPAddress(str_address)
@@ -461,6 +444,7 @@ def is_forward_record(zone, str_address):

 def add_forward_record(zone, name, str_address):
     addr = netaddr.IPAddress(str_address)
         if addr.version == 4:
             api.Command['dnsrecord_add'](zone, name, arecord=str_address)

Personally, I think that 'forward' is confusing (in this context). Could you rename *_forward_record() functions to *_ipaddr_record(), please? (Maybe in a separate patch...)

def add_records_for_host(host, domain, ip_addresses, add_forward=True, add_reverse=True):
+    assert isinstance(host, DNSName)
+    assert isinstance(domain, DNSName)

The same applies to all places with:
+    assert isinstance(value, DNSName)
scattered all over the patch.

Is this really necessary? I thought that Python usually uses

It seems like leftovers from development phase. (Maybe it is not the case for IPA framework, correct me if I'm wrong :-)

Patch 39:


-        Str('hostname',
-            _hostname_validator,
-            normalizer=_normalize_hostname,
+        DNSNameParam('hostname',
+            #RFC 2317 section 5.2 -- don't have to be FQDN

It seems you are changing behavior here. Such things belong in separate patches.
Note: This is (mostly) related to classless reverse zones.

* Records data are always returned as string
* Attributes idnsname, idnssoamname, idnssoarname are returned as DNSName, with
  option --raw as string
* option --raw returns all IDN domains punycoded

I have mentioned

above. Their are also pure DNS names ...
     def get_name_in_zone(self, zone, hostname):
-        be added to a zone
+        be added to a zone. Returns '@' when the hostname is directly in the 

I would replace "in the zone" with "at zone apex". Technically, all records with suffix = zone are "in" the zone.

+idnzone1_ip = u''
+revidnzone1 = u'15.168.192.in-addr.arpa.'

Please use addresses from 172.16/12 prefix so IPA tests don't consume more than one prefix.

More prefixes we use => higher probability that it will clash with somebody's environment.

I'm looking forward to see this is IPA! Good work!

Petr^2 Spacek

Freeipa-devel mailing list

Reply via email to