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 <jhrozek(a)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.
+ straddr = talloc_asprintf_append(straddr,
"%c.%c.",
+ hexbyte[1], hexbyte[0]);
+ }
+ straddr = talloc_asprintf_append(straddr, "ip6.arpa.");
+ } else if (family == AF_INET) {
+ straddr = talloc_asprintf(mem_ctx,
+ "%u.%u.%u.%u.in-addr.arpa.",
+ (address[3]),
+ (address[2]),
+ (address[1]),
+ (address[0]));
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Unknown address family\n"));
+ return NULL;
+ }
+
+ return straddr;
+}