-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/30/2013 01:42 PM, Lukas Slebodnik wrote: > On (30/04/13 17:51), Jakub Hrozek wrote: >> On Mon, Apr 22, 2013 at 02:04:03PM +0200, Pavel Březina wrote: >>> On 04/17/2013 09:03 PM, Jakub Hrozek wrote: >>>> Hi, >>>> >>>> the attached patches implement a couple of new dynamic DNS >>>> options. The AD dyndns code will be just a wrapper around >>>> these options. >>>> >>>> [PATCH 1/5] dyndns: new option dyndns_refresh_interval This >>>> new options adds the possibility of updating the DNS entries >>>> periodically regardless if they have changed or not. This >>>> feature will be useful mainly in AD environments where the >>>> Windows clients periodically update their DNS records. >>>> >>>> There is one place (in IPA dyndns code in this patch but also >>>> in AD code later on) that I wanted to discuss specifically. >>>> It may happen that the periodic update would trigger going >>>> online in which case the online callback would fire and >>>> another dyndns update would be invoked as an online callback. >>>> To prevent a race between these two updates, there is an >>>> interval, currently hardcoded to 60 seconds that would just >>>> make the next update quit without doing anything. Ideas on >>>> how to fix the problem without a hardcoded timeout are >>>> welcome. >>>> >>>> [PATCH 2/5] resolver: Return PTR record as string Having the >>>> possibility to format a PTR record based on an A/AAAA record >>>> is a requirement to update the PTR records. Includes a unit >>>> test. >>>> >>>> [PATCH 3/5] dyndns: New option dyndns_update_ptr >>>> https://fedorahosted.org/sssd/ticket/1832 >>>> >>>> While some servers, such as FreeIPA allow the PTR record to >>>> be synchronized when the forward record is updated, other >>>> servers, including Active Directory, require that the PTR >>>> record is synchronized manually. >>>> >>>> This patch adds a new option, dyndns_update_ptr that >>>> automatically generates appropriate DNS update message for >>>> updating the reverse zone. >>>> >>>> The PTR update is performed separately from the forward >>>> record update mostly because the current IPA dyndns code >>>> allows the zone to be specified in the message, so another >>>> zone must be updated using another message. >>>> >>>> This option is off by default in the IPA provider. >>>> >>>> [PATCH 4/5] dyndns: new option dyndns_use_tcp >>>> https://fedorahosted.org/sssd/ticket/1831 >>>> >>>> Adds a new option that can be used to force nsupdate to only >>>> use TCP to communicate with the DNS server. >>>> >>>> [PATCH 5/5] dyndns: new option dyndns_auth This options is >>>> mostly provided for future expansion. Currently it is >>>> undocumented and both IPA and AD dynamic DNS updates default >>>> to GSS-TSIG. Allowed values are GSS-TSIG and none. >>> >>> Hi, good job. I have just few comments inline. >>> >>>> 0001-dyndns-new-option-dyndns_refresh_interval.patch >>>> >>> >>>> <varlistentry> + >>>> <term>dyndns_refresh_interval (integer)</term> + >>>> <listitem> + <para> + >>>> How often should the back end perform periodic DNS update in >>>> + addition to the automatic update >>>> performed when the back end + >>>> becomes online. >>> >>> goes online sounds better to me. >> >> OK, fixed. >> >>> >>>> + This option is optional and >>>> applicable only when dyndns_update + >>>> is true. + </para> + >>>> <para> + Default: 0 (disabled) + >>>> </para> + </listitem> + >>>> </varlistentry> >>> >>>> +void ipa_dyndns_timer(void *pvt) +{ + struct ipa_options >>>> *ctx = talloc_get_type(pvt, struct ipa_options); + struct >>>> sdap_id_ctx *sdap_ctx = ctx->id_ctx->sdap_id_ctx; + struct >>>> tevent_req *req; + struct ipa_dyndns_timer_ctx >>>> *timer_ctx; + errno_t ret; + + timer_ctx = >>>> talloc_zero(ctx, struct ipa_dyndns_timer_ctx); + if >>>> (timer_ctx == NULL) { + /* Not much we can do */ >>> >>> I agree there is not much we can do, but we can report it at >>> least. :-) >> >> Added a DEBUG message. >> >>> >>>> >>>> 0002-resolver-Return-PTR-record-as-string.patch +char * >>>> +resolv_get_string_ptr_address(TALLOC_CTX *mem_ctx, + >>>> int family, uint8_t *address) +{ + char *straddr; + >>>> static char hex_digits[] = { + '0', '1', '2', >>>> '3', '4', '5', '6', '7', + '8', '9', 'a', 'b', >>>> 'c', 'd', 'e', 'f' + }; + + if (family == AF_INET6) { + >>>> char *cp; + int i; + + straddr = >>>> talloc_zero_array(mem_ctx, char, 128); + if (!straddr) >>>> { + return NULL; + } + + cp = >>>> straddr; + for (i = 15; i >= 0; i--) { + >>>> *cp++ = hex_digits[address[i] & 0x0f]; + *cp++ >>>> = '.'; + *cp++ = hex_digits[(address[i] >> 4) >>>> & 0x0f]; + *cp++ = '.'; + } >>> >>> I'm not opposed this, but wouldn't it be better to use >>> sprintf("%02x") instead of this bit wise magic? Something >>> like: >>> >>> char hexbyte[3]; snprintf(hexbyte, 2, "%02x", address[i]); >>> talloc_asprintf(straddr, "%c.%c.", hexbyte[1], hexbyte[0]); >> >> OK, that's more readable. >> >>> >>>> + strcpy(cp, "ip6.arpa."); + } else if (family == >>>> AF_INET) { + straddr = talloc_asprintf(mem_ctx, + >>>> "%u.%u.%u.%u.in-addr.arpa.", + >>>> (address[3] & 0xff), + >>>> (address[2] & 0xff), + >>>> (address[1] & 0xff), + >>>> (address[0] & 0xff)); >>> >>> address is already uint8_t so I believe applying 0xff is not >>> necessary. >> >> Removed the mask. >> >>> >>>> + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("Unknown >>>> address family\n")); + return NULL; + } + + >>>> return straddr; +} + >>>> >>>> 0003-dyndns-New-option-dyndns_update_ptr.patch >>> >>> nsupdate_msg_add_common() should also take update_msg as >>> parameter as do other add functions. >>> >> >> No, the purpose is to create the update message, not add contents >> to existing one. I renamed the function to >> nsupdate_msg_create_common. >> >>> You are leaking update_msg in be_nsupdate_create_msg() and >>> be_nsupdate_create_ptr_msg(). If one of the _add function or >>> talloc_asprintf_append() fails, you have to free the original >>> pointer. >>> >>> I know it is all allocated on state so it will be freed later >>> anyway, but still... >>> >>> Using tmp_ctx and "done" pattern would be much better in these >>> functions. >>> >> >> OK, I went with tmp_ctx. >> >>> Also please rename be_nsupdate_create_msg() to >>> be_nsupdate_create_fwd_msg() to make it similar to >>> be_nsupdate_create_ptr_msg(). >>> >> >> Renamed. >> >>> Patch 4: I think dyndns_force_tcp would be a better name for >>> the option. >>> >> >> At first I didn't agree, but you're right that "force" more >> closely describes that without this option, nsupdate might still >> opt for TCP internally. >> >>> Patch 5: Originally, I wanted to write that you shouldn't spend >>> time on this patch, but I kinda like the new way of creating >>> nsupdate args. So kudos :-) >> >> Patch 5 is the same. > > I want to comment only 2nd patch. > > You introduced compile time warnings: > > src/resolv/async_resolv.c: In function > ‘resolv_get_string_ptr_address’: src/resolv/async_resolv.c:1433:9: > warning: zero-length gnu_printf format string > [-Wformat-zero-length] src/resolv/async_resolv.c:1424:17: warning: > unused variable ‘hex_digits’ [-Wunused-variable] > > > > >> From fda33381912699bfdcf255bfe2b7794a66218a4f Mon Sep 17 00:00:00 >> 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 30 Apr >> 2013 16:40:00 +0200 Subject: [PATCH 2/5] resolver: Return PTR >> record as string >> >> This is a requirement to update the PTR records. Includes a unit >> test. --- src/resolv/async_resolv.c | 40 ++++++++++++++++++ >> src/resolv/async_resolv.h | 4 ++ src/tests/resolv-tests.c | >> 104 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files >> changed, 148 insertions(+) >> >> diff --git a/src/resolv/async_resolv.c >> b/src/resolv/async_resolv.c index >> 60d9e05bfa61f1490263a3defa21cab1c709c5ac..b56a68da79fd25b775da8e19702999b1c3cfb57b >> 100644 --- a/src/resolv/async_resolv.c +++ >> b/src/resolv/async_resolv.c @@ -1416,6 +1416,46 @@ >> resolv_get_string_address_index(TALLOC_CTX *mem_ctx, return >> address; } >> >> +char * +resolv_get_string_ptr_address(TALLOC_CTX *mem_ctx, + >> int family, uint8_t *address) +{ + char *straddr; + static >> char hex_digits[] = { + '0', '1', '2', '3', '4', >> '5', '6', '7', + '8', '9', 'a', 'b', 'c', 'd', >> 'e', 'f' + }; + + if (family == AF_INET6) { + int >> i; + char hexbyte[3]; + + straddr = >> talloc_asprintf(mem_ctx, ""); + if (!straddr) { + >> return NULL; + } + + for (i = 15; i >= 0; i--) { + >> snprintf(hexbyte, 3, "%02x", address[i]); > ^^^^^^ You call function snprintf 16 times, only because Pavel > don't like bit operators. It is useless. > > Please use first version of this patch and also warnings disappear > with previous version. >
I'm going to contradict this, actually. Given that this is a computationally-rare event, I'd say that the readability gains are well-worth the slight loss in performance. Let's not optimize unnecessarily. This is *not* going to be a bottleneck. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlGABdsACgkQeiVVYja6o6MRVwCffHrLFz6tnwnWSd1BaP2yIE9Q +1wAniATxuEUNwGRpnJk1W9JJA3x23YD =uGWS -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
