On Mon, Sep 24, 2012 at 03:21:23PM +0200, Petr Spacek wrote:
> On 09/24/2012 03:09 PM, Adam Tkac wrote:
> >On Mon, Sep 17, 2012 at 02:55:06PM +0200, Petr Spacek wrote:
> >>Hello,
> >>
> >>this patch adds missing notification to DNS slaves if zone serial
> >>number modification was detected.
> >
> >Hi,
> >
> >please check my comment below.
> >
> >> From eb8d7fc0c02e03b9c7c90e497225536c449fab1c Mon Sep 17 00:00:00 2001
> >>From: Petr Spacek <pspa...@redhat.com>
> >>Date: Mon, 17 Sep 2012 14:29:45 +0200
> >>Subject: [PATCH] Notify DNS slaves if zone serial number modification was
> >>  detected.
> >>
> >>Signed-off-by: Petr Spacek <pspa...@redhat.com>
> >>---
> >>  src/ldap_helper.c | 22 ++++++++++++++--------
> >>  1 file changed, 14 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> >>index 
> >>658b960f50b461fa602edf51e955f3bdd4769e1d..d22e8714df57edaad4cf45e6cc26ec0dbbd59108
> >> 100644
> >>--- a/src/ldap_helper.c
> >>+++ b/src/ldap_helper.c
> >>@@ -1035,9 +1035,10 @@ ldap_parse_zoneentry(ldap_entry_t *entry, 
> >>ldap_instance_t *inst)
> >>    isc_task_t *task = inst->task;
> >>    isc_uint32_t ldap_serial;
> >>    isc_uint32_t zr_serial; /* SOA serial value from in-memory zone 
> >> register */
> >>-   unsigned char ldap_digest[RDLIST_DIGESTLENGTH];
> >>+   unsigned char ldap_digest[RDLIST_DIGESTLENGTH] = {0};
> >>    unsigned char *zr_digest = NULL;
> >>    ldapdb_rdatalist_t rdatalist;
> >>+   isc_boolean_t zone_dynamic = ISC_FALSE;
> >>
> >>    REQUIRE(entry != NULL);
> >>    REQUIRE(inst != NULL);
> >>@@ -1131,13 +1132,13 @@ ldap_parse_zoneentry(ldap_entry_t *entry, 
> >>ldap_instance_t *inst)
> >>            && result != DNS_R_DYNAMIC && result != DNS_R_CONTINUE)
> >>            goto cleanup;
> >>
> >>+   zone_dynamic = (result == DNS_R_DYNAMIC);
> >>    result = ISC_R_SUCCESS;
> >>
> >>    /* initialize serial in zone register and always increment serial
> >>     * for a new zone (typically after BIND start)
> >>     * - the zone was possibly changed in meanwhile */
> >>    if (publish) {
> >>-           memset(ldap_digest, 0, RDLIST_DIGESTLENGTH);
> >>            CHECK(ldap_get_zone_serial(inst, &name, &ldap_serial));
> >>            CHECK(zr_set_zone_serial_digest(inst->zone_register, &name, 
> >> ldap_serial,
> >>                            ldap_digest));
> >>@@ -1154,23 +1155,28 @@ ldap_parse_zoneentry(ldap_entry_t *entry, 
> >>ldap_instance_t *inst)
> >>     * 3c) The old and new serials are same: autoincrement only if something
> >>     *     else was changed.
> >>     */
> >>+   CHECK(ldap_get_zone_serial(inst, &name, &ldap_serial));
> >>+   CHECK(zr_get_zone_serial_digest(inst->zone_register, &name, &zr_serial,
> >>+                   &zr_digest));
> >>    if (inst->serial_autoincrement) {
> >>-           CHECK(ldap_get_zone_serial(inst, &name, &ldap_serial));
> >>            CHECK(ldapdb_rdatalist_get(inst->mctx, inst, &name,
> >>                            &name, &rdatalist));
> >>            CHECK(rdatalist_digest(inst->mctx, &rdatalist, ldap_digest));
> >>
> >>-           CHECK(zr_get_zone_serial_digest(inst->zone_register, &name, 
> >>&zr_serial,
> >>-                           &zr_digest));
> >>            if (ldap_serial == zr_serial) {
> >>                    /* serials are same - increment only if something was 
> >> changed */
> >>                    if (memcmp(zr_digest, ldap_digest, RDLIST_DIGESTLENGTH) 
> >> != 0)
> >>                            CHECK(soa_serial_increment(inst->mctx, inst, 
> >> &name));
> >>-           } else { /* serial in LDAP was changed - update zone register */
> >>-                   CHECK(zr_set_zone_serial_digest(inst->zone_register, 
> >>&name,
> >>-                                   ldap_serial, ldap_digest));
> >>            }
> >>    }
> >>+   if (ldap_serial != zr_serial) {
> >>+           /* serial in LDAP was changed - update zone register */
> >>+           CHECK(zr_set_zone_serial_digest(inst->zone_register, &name,
> >>+                           ldap_serial, ldap_digest));
> >>+
> >>+           if (zone_dynamic)
> >>+                   dns_zone_notify(zone);
> >
> >This if() doesn't seem fully correct to me. Although in most FreeIPA 
> >scenarios
> >it will work, consider situation when someone disables DDNS updates for zone 
> >and
> >modifies records in LDAP. In this case zone_dynamic is FALSE but SOA serial
> >changes.
> >
> >I would recommend to simpy send notify every time when serial changes.
> 
> Two lines above start of the patch is following code:
> 
> result = dns_zone_load(zone);
>       if (result != ISC_R_SUCCESS && result != DNS_R_UPTODATE
>               && result != DNS_R_DYNAMIC && result != DNS_R_CONTINUE)
>               goto cleanup;
> 
>       zone_dynamic = (result == DNS_R_DYNAMIC);
> 
> For non-dynamic zones "dns_zone_load()" sends notifies. Zone_dynamic
> condition only prevents doubled dns_zone_notify() calls for
> non-dynamic zones. I can remove this condition if doubled calls are
> not problem.

Ah, good point. Then ack for the original version.

> Petr^2 Spacek
> 
> >
> >Regards, Adam
> >
> >>+   }
> >>
> >>  cleanup:
> >>    if (unlock)
> >>--
> >>1.7.11.4
> >>
> >
> >
> 

-- 
Adam Tkac, Red Hat, Inc.

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to