On 6.5.2014 14:41, Tomas Hozza wrote:
----- Original Message -----
Hello,
This patch set attempts to move ldap_parse_master_zoneentry() a little bit
closer to sane code.
It is preparation for
https://fedorahosted.org/bind-dyndb-ldap/ticket/56
--
Petr^2 Spacek
Patches look good.
ACK.
ACKing of version 2 of the patch 242 will follow. The patch 243 introduced new
compilation
warning that Peter is aware of. Unfortunately we are unable to find the root
cause of it,
so leaving it as is for now...
I managed to find & fix one problem (see new version of the patch 243) but GCC
still complains.
../../src/ldap_helper.c: In function 'update_zone':
../../src/ldap_helper.c:2334:34: error: 'data_changed' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
if (sync_state == sync_finished && data_changed == ISC_TRUE)
^
../../src/ldap_helper.c:2218:16: note: 'data_changed' was declared here
isc_boolean_t data_changed;
On my machine with gcc-4.8.2-7.fc20.x86_64 this happens only with -O2.
I'm not able to reproduce this with clang-3.4-6.fc20.x86_64 but it is no so
surprising - Clang didn't catch even the first case (fixed by patch version 2).
Any hint what is wrong or how to refactor code will be appreciated! ;-)
--
Petr^2 Spacek
From 237d116de60d7ada5d6be84c9d58a52d5e306f90 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Thu, 17 Apr 2014 19:57:48 +0200
Subject: [PATCH] Refactor zone apex synchronization and serial maintenance.
ldap_parse_master_zoneentry() is way too long and unmanageable.
https://fedorahosted.org/bind-dyndb-ldap/ticket/56
Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
src/ldap_helper.c | 230 ++++++++++++++++++++++++++++++------------------------
1 file changed, 129 insertions(+), 101 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index d94bb57fdd6e5e0e43a978d7aaba471c62014eb9..7374948a3b283155035aea33fa0da62e8beae95d 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1896,49 +1896,153 @@ cleanup:
return result;
}
+/**
+ * Synchronize internal RBTDB with master zone object in LDAP and update serial
+ * as necessary.
+ *
+ * @param[in] new_zone Is the RBTDB empty? (I.e. even without SOA record.)
+ * @param[in] version LDAP DB opened for reading and writing.
+ * @param[out] diff Initialized diff. It will be filled with differences
+ * between RBTDB and LDAP object + SOA serial update.
+ * @param[out] new_serial SOA serial after update;
+ * valid if ldap_writeback = ISC_TRUE.
+ * @param[out] ldap_writeback SOA serial was updated.
+ * @param[out] data_changed Other data were updated.
+ *
+ */
+static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
+zone_sync_apex(const ldap_instance_t * const inst,
+ ldap_entry_t * const entry, dns_name_t name,
+ const sync_state_t sync_state, const isc_boolean_t new_zone,
+ dns_db_t * const ldapdb, dns_db_t * const rbtdb,
+ dns_dbversion_t * const version, dns_diff_t * const diff,
+ isc_uint32_t * const new_serial,
+ isc_boolean_t * const ldap_writeback,
+ isc_boolean_t * const data_changed) {
+ isc_result_t result;
+ const char *fake_mname = NULL;
+ ldapdb_rdatalist_t rdatalist;
+ dns_rdatasetiter_t *rbt_rds_iterator = NULL;
+ /* RBTDB's origin node cannot be detached until the node is non-empty.
+ * This is workaround for ISC-Bug #35080. */
+ dns_dbnode_t *node = NULL;
+ dns_difftuple_t *soa_tuple = NULL;
+ isc_boolean_t soa_tuple_alloc = ISC_FALSE;
+ isc_uint32_t curr_serial;
+
+ INIT_LIST(rdatalist);
+ CHECK(setting_get_str("fake_mname", inst->local_settings,
+ &fake_mname));
+ CHECK(ldap_parse_rrentry(inst->mctx, entry, &name, fake_mname,
+ &rdatalist));
+
+ CHECK(dns_db_getoriginnode(rbtdb, &node));
+ result = dns_db_allrdatasets(rbtdb, node, version, 0,
+ &rbt_rds_iterator);
+ if (result == ISC_R_SUCCESS) {
+ CHECK(diff_ldap_rbtdb(inst->mctx, &name, &rdatalist,
+ rbt_rds_iterator, diff));
+ dns_rdatasetiter_destroy(&rbt_rds_iterator);
+ } else if (result != ISC_R_NOTFOUND)
+ goto cleanup;
+
+ /* New zone doesn't have serial defined yet. */
+ if (new_zone != ISC_TRUE)
+ CHECK(dns_db_getsoaserial(rbtdb, version, &curr_serial));
+
+ /* Detect if SOA serial is affected by the update or not.
+ * Always bump serial in case of re-synchronization. */
+ CHECK(diff_analyze_serial(diff, &soa_tuple, data_changed));
+ if (new_zone == ISC_TRUE || *data_changed == ISC_TRUE ||
+ sync_state != sync_finished) {
+ if (soa_tuple == NULL) {
+ /* The diff doesn't contain new SOA serial
+ * => generate new serial and write it back to LDAP. */
+ *ldap_writeback = ISC_TRUE;
+ soa_tuple_alloc = ISC_TRUE;
+ CHECK(dns_db_createsoatuple(ldapdb, version, inst->mctx,
+ DNS_DIFFOP_DEL, &soa_tuple));
+ dns_diff_appendminimal(diff, &soa_tuple);
+ CHECK(dns_db_createsoatuple(ldapdb, version, inst->mctx,
+ DNS_DIFFOP_ADD, &soa_tuple));
+ CHECK(update_soa_serial(dns_updatemethod_unixtime,
+ soa_tuple, new_serial));
+ dns_diff_appendminimal(diff, &soa_tuple);
+ } else if (new_zone == ISC_TRUE || sync_state != sync_finished ||
+ isc_serial_le(dns_soa_getserial(&soa_tuple->rdata),
+ curr_serial)) {
+ /* The diff tries to send SOA serial back!
+ * => generate new serial and write it back to LDAP.
+ * Force serial update if we are adding a new zone. */
+ *ldap_writeback = ISC_TRUE;
+ CHECK(update_soa_serial(dns_updatemethod_unixtime,
+ soa_tuple, new_serial));
+ } else {
+ /* The diff contains new serial already
+ * => do nothing. */
+ *ldap_writeback = ISC_FALSE;
+ }
+
+ } else {/* if (data_changed == ISC_FALSE) */
+ *ldap_writeback = ISC_FALSE;
+ if (soa_tuple == NULL) {
+ /* The diff is empty => do nothing. */
+ INSIST(EMPTY(diff->tuples));
+ } else if (isc_serial_le(dns_soa_getserial(&soa_tuple->rdata),
+ curr_serial)) {
+ /* Attempt to move serial backwards without any data
+ * => ignore it. */
+ dns_diff_clear(diff);
+ }/* else:
+ * The diff contains new serial already
+ * => do nothing. */
+ }
+
+ /* New zone has to have at least SOA record and NS record. */
+ if (new_zone == ISC_TRUE
+ && (*data_changed == ISC_FALSE || soa_tuple == NULL))
+ result = DNS_R_BADZONE;
+
+cleanup:
+ if (soa_tuple_alloc == ISC_TRUE && soa_tuple != NULL)
+ dns_difftuple_free(&soa_tuple);
+ if (node != NULL)
+ dns_db_detachnode(rbtdb, &node);
+ if (rbt_rds_iterator != NULL)
+ dns_rdatasetiter_destroy(&rbt_rds_iterator);
+ ldapdb_rdatalist_destroy(inst->mctx, &rdatalist);
+ return result;
+}
+
/* Parse the master zone entry */
static isc_result_t ATTR_NONNULLS ATTR_CHECKRESULT
ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst,
isc_task_t *task)
{
const char *dn;
dns_name_t name;
dns_zone_t *raw = NULL;
- dns_zone_t *zone_raw = NULL;
isc_result_t result;
isc_boolean_t unlock = ISC_FALSE;
isc_boolean_t new_zone = ISC_FALSE;
- isc_boolean_t configured = ISC_FALSE;
- ldapdb_rdatalist_t rdatalist;
settings_set_t *zone_settings = NULL;
- const char *fake_mname = NULL;
- isc_boolean_t data_changed;
isc_boolean_t ldap_writeback;
- isc_uint32_t curr_serial;
+ isc_boolean_t data_changed;
isc_uint32_t new_serial;
dns_db_t *rbtdb = NULL;
dns_db_t *ldapdb = NULL;
dns_diff_t diff;
dns_dbversion_t *version = NULL;
- /* RBTDB's origin node cannot be detached until the node is non-empty.
- * This is workaround for possible bug in bind-9.9.3-P2. */
- dns_dbnode_t *node = NULL;
- dns_rdatasetiter_t *rbt_rds_iterator = NULL;
- dns_difftuple_t *soa_tuple = NULL;
- isc_boolean_t soa_tuple_alloc = ISC_FALSE;
-
sync_state_t sync_state;
dns_journal_t *journal = NULL;
char *journal_filename = NULL;
+ REQUIRE(entry != NULL);
+ REQUIRE(inst != NULL);
+
dns_diff_init(inst->mctx, &diff);
-
- REQUIRE(entry != NULL);
- REQUIRE(inst != NULL);
-
dns_name_init(&name, NULL);
- INIT_LIST(rdatalist);
/* Derive the dns name of the zone from the DN. */
dn = entry->dn;
@@ -1988,73 +2092,12 @@ ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst,
CHECK(publish_zone(task, inst, raw));
/* synchronize zone origin with LDAP */
- CHECK(setting_get_str("fake_mname", inst->local_settings,
- &fake_mname));
- CHECK(ldap_parse_rrentry(inst->mctx, entry, &name, fake_mname,
- &rdatalist));
-
CHECK(zr_get_zone_dbs(inst->zone_register, &name, &ldapdb, &rbtdb));
CHECK(dns_db_newversion(ldapdb, &version));
- CHECK(dns_db_getoriginnode(rbtdb, &node));
- result = dns_db_allrdatasets(rbtdb, node, version, 0,
- &rbt_rds_iterator);
- if (result == ISC_R_SUCCESS) {
- CHECK(diff_ldap_rbtdb(inst->mctx, &name, &rdatalist,
- rbt_rds_iterator, &diff));
- dns_rdatasetiter_destroy(&rbt_rds_iterator);
- } else if (result != ISC_R_NOTFOUND)
- goto cleanup;
-
- /* New zone doesn't have serial defined yet. */
- if (new_zone == ISC_FALSE)
- CHECK(dns_db_getsoaserial(rbtdb, version, &curr_serial));
-
- /* Detect if SOA serial is affected by the update or not.
- * Always bump serial in case of re-synchronization. */
- CHECK(diff_analyze_serial(&diff, &soa_tuple, &data_changed));
- if (data_changed == ISC_TRUE || sync_state != sync_finished) {
- if (soa_tuple == NULL) {
- /* The diff doesn't contain new SOA serial
- * => generate new serial and write it back to LDAP. */
- ldap_writeback = ISC_TRUE;
- soa_tuple_alloc = ISC_TRUE;
- CHECK(dns_db_createsoatuple(ldapdb, version, inst->mctx,
- DNS_DIFFOP_DEL, &soa_tuple));
- dns_diff_appendminimal(&diff, &soa_tuple);
- CHECK(dns_db_createsoatuple(ldapdb, version, inst->mctx,
- DNS_DIFFOP_ADD, &soa_tuple));
- CHECK(update_soa_serial(dns_updatemethod_unixtime,
- soa_tuple, &new_serial));
- dns_diff_appendminimal(&diff, &soa_tuple);
- } else if (new_zone == ISC_TRUE || sync_state != sync_finished ||
- isc_serial_le(dns_soa_getserial(&soa_tuple->rdata),
- curr_serial)) {
- /* The diff tries to send SOA serial back!
- * => generate new serial and write it back to LDAP.
- * Force serial update if we are adding a new zone. */
- ldap_writeback = ISC_TRUE;
- CHECK(update_soa_serial(dns_updatemethod_unixtime,
- soa_tuple, &new_serial));
- } else {
- /* The diff contains new serial already
- * => do nothing. */
- ldap_writeback = ISC_FALSE;
- }
-
- } else {/* if (data_changed == ISC_FALSE) */
- ldap_writeback = ISC_FALSE;
- if (soa_tuple == NULL) {
- /* The diff is empty => do nothing. */
- INSIST(EMPTY(diff.tuples));
- } else if (isc_serial_le(dns_soa_getserial(&soa_tuple->rdata),
- curr_serial)) {
- /* Attempt to move serial backwards without any data
- * => ignore it. */
- dns_diff_clear(&diff);
- }/* else:
- * The diff contains new serial already
- * => do nothing. */
- }
+ CHECK(zone_sync_apex(inst, entry, name, sync_state, new_zone,
+ ldapdb, rbtdb, version,
+ &diff, &new_serial, &ldap_writeback,
+ &data_changed));
#if RBTDB_DEBUG >= 2
dns_diff_print(&diff, stdout);
@@ -2074,11 +2117,7 @@ ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst,
if (!EMPTY(diff.tuples)) {
if (sync_state == sync_finished && new_zone == ISC_FALSE) {
/* write the transaction to journal */
- dns_zone_getraw(raw, &zone_raw);
- if (zone_raw == NULL)
- journal_filename = dns_zone_getjournal(raw);
- else
- journal_filename = dns_zone_getjournal(zone_raw);
+ journal_filename = dns_zone_getjournal(raw);
CHECK(dns_journal_open(inst->mctx, journal_filename,
DNS_JOURNAL_CREATE, &journal));
CHECK(dns_journal_write_transaction(journal, &diff));
@@ -2089,32 +2128,22 @@ ldap_parse_master_zoneentry(ldap_entry_t *entry, ldap_instance_t *inst,
dns_db_closeversion(ldapdb, &version, ISC_TRUE);
}
- /* Make sure that zone has at least SOA record. */
- if (new_zone == ISC_FALSE
- || (data_changed == ISC_TRUE && soa_tuple != NULL))
- configured = ISC_TRUE;
-
/* Do zone load only if the initial LDAP synchronization is done. */
if (sync_state == sync_finished && data_changed == ISC_TRUE)
CHECK(load_zone(raw));
cleanup:
dns_diff_clear(&diff);
- if (soa_tuple_alloc == ISC_TRUE && soa_tuple != NULL)
- dns_difftuple_free(&soa_tuple);
- if (rbt_rds_iterator != NULL)
- dns_rdatasetiter_destroy(&rbt_rds_iterator);
- if (node != NULL)
- dns_db_detachnode(rbtdb, &node);
if (rbtdb != NULL && version != NULL)
dns_db_closeversion(ldapdb, &version, ISC_FALSE); /* rollback */
if (rbtdb != NULL)
dns_db_detach(&rbtdb);
if (journal != NULL)
dns_journal_destroy(&journal);
if (ldapdb != NULL)
dns_db_detach(&ldapdb);
- if (new_zone && !configured) { /* Failure in ACL parsing or so. */
+ if (new_zone && result != ISC_R_SUCCESS) {
+ /* Failure in ACL parsing or so. */
log_error_r("zone '%s': publishing failed, rolling back due to",
entry->dn);
result = ldap_delete_zone2(inst, &name, ISC_TRUE, ISC_FALSE);
@@ -2127,7 +2156,6 @@ cleanup:
dns_name_free(&name, inst->mctx);
if (raw != NULL)
dns_zone_detach(&raw);
- ldapdb_rdatalist_destroy(inst->mctx, &rdatalist);
return result;
}
--
1.9.0
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
