On Fri, 2014-04-04 at 15:46 +0200, Martin Basti wrote: > On Fri, 2014-04-04 at 12:59 +0200, Petr Spacek wrote: > > On 3.4.2014 15:35, Jan Cholasta wrote: > > > On 2.4.2014 14:07, Martin Basti wrote: > > > Patch 30: > > > 2) > > > > > > + 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 : dns.name.from_text("") > > Out: <DNS name .> > > > > In : dns.name.Name() > > Out: <DNS name @> > > > > In : dns.name.Name([""]) > > Out: <DNS name .> > > Cos we need to keep if user added domain with/without zone, we use I mean end dot not zone. > origin=None, and there is allowed to add only '.' as domain name => root > In : a = dns.name.from_unicode(u'.', origin=None) > > In : a.labels > Out: ('',) > > That is more clearly to user add . as root domain, instead of empty > string > > > I would like to see more descriptive error texts if you insist on current > > semantics. > > > > > 4) > > > > > > + @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. > I agree with origin_sing, or zone_record, or origin, not 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. > I will take look at dns-python3. > > > > 6) > > > > > > + def ToASCII(self, omit_final_dot=False): > > > + return super(DNSName, > > > self).to_text(omit_final_dot=omit_final_dot).decode('ascii') > > > + > > > + def ToUnicode(self, omit_final_dot=False): > > > + return super(DNSName, > > > self).to_unicode(omit_final_dot=omit_final_dot).decode('utf-8') > > > > > > 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 > > > explanatory. > > Generally, I agree. We can get rid of the relative/absolute name madness by > > using final dot everywhere. > Ok I will remove it. > > > > 8) > > > > > > + def is_ip_reverse(self): > > Please use is_ip4_reverse() instead. That name will always remind developer > > not to forgot to IPv6 :-) > As you wish. > > > + if self.is_subdomain(self.get_rev_zone()): > > > + return True > > > + return False > > > > > > > > > Patch 31: > > 2b) > > + 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. > Okay, bytes then. I will reword it. > > > > > > 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: > > CNAMERecord > > DNAMERecord > > MDRecord > > NSRecord > > PTRRecord > > > I'm not sure with that. Now all record data are returned as string, only > domain names and domain related attributes in zone are returned as > DNSName. It could be mess if half of records be returned as DNSName and > second as string. > > > > > > > Patch 38: > > > > > > 1) > > > > > > +_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 > > DNSName(u'@') > > is more readable and you don't need to dig into code to find out what the > > cryptic name means. > I like constants :-). I'm not sure if making new object, when in the most > cases want only to compare, is good idea. > > > > 4) > > > > > > +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. > I know, but in this IPA case is FQDN something like 'example.domain', not > 'domain.' . > I kept behavior of original function. > > > > > 4b) > > 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) > > + > > try: > > 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...) > I was confused too, maybe later, now I'm almost lost in all these > changes. > > > > 4c) > > 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 > > http://en.wikipedia.org/wiki/Duck_typing > > > > It seems like leftovers from development phase. (Maybe it is not the case > > for > > IPA framework, correct me if I'm wrong :-) > I used in development phase, but it should be clear that these > functions/methods dont work with unicode, some of them are used in other > plugins (include 'private' functions) > > > > Patch 39: > > > > > > 1) > > > > > > - 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. > > > freeipa-mbasti-0040-Modified-record-and-zone-class-to-support-IDN.patch > > > * 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 > > CNAMERecord > > DNAMERecord > > MDRecord > > NSRecord > > PTRRecord > > > > above. Their are also pure DNS names ... > As I wrote above, it could be mayhem. > > > > def get_name_in_zone(self, zone, hostname): > > [snip] > > > - be added to a zone > > > + be added to a zone. Returns '@' when the hostname is directly in > > > the zone > > > > I would replace "in the zone" with "at zone apex". Technically, all records > > with suffix = zone are "in" the zone. > I would use zone record instead. Is it okay? > > > > > > freeipa-mbasti-0046-DNS-new-tests.patch > > +idnzone1_ip = u'192.168.11.1' > > [snip] > > +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. > Thank you for make it clear for me. I though the private address is enough. > > > > > I'm looking forward to see this is IPA! Good work! > > > > Thank you for your review.
-- Martin^2 Basti _______________________________________________ Freeipa-devel mailing list Freeipaemail@example.com https://www.redhat.com/mailman/listinfo/freeipa-devel