On 6.5.2014 22:11, Lukas Slebodnik wrote:
On (06/05/14 17:15), Petr Spacek wrote:
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.
The same problem with -01,-Os,-O2 or -O3
I doubt it is false possibive, because I could reproduce it even with
gcc-4.9.0-1.fc21.x86_64
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! ;-)
I think it can be some kind of optimization in function zone_sync_apex.
You can try to debug this function with plugin "-O2"-build :-)
The warning can be suppresed with initialising variable before the 1st CHECK.
It will not work if you try to initialize later.
Yesterday I have discussed this with jkratoch. We weren't able to find out
case where would initialization in ldap_parse_master_zoneentry() cause any
problem so I have added initialization there.
--
Petr^2 Spacek
From f79be59896bbb51cb19b8f438a7d23ead81d1a28 Mon Sep 17 00:00:00 2001
From: Petr Spacek <[email protected]>
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 <[email protected]>
---
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..4f4d42243af6e42ae08d4f62bf21eacae581e760 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_FALSE; /* GCC */
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.3
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
