On 2.4.2013 17:17, Adam Tkac wrote:
On Fri, Mar 22, 2013 at 01:03:12PM +0100, Petr Spacek wrote:
Hello,

this patch set separates master zones (idnsZone objectClass) from
forward zones (idnsForwardZone objectClass). Support for forward
zones in idnsZone objectClass is still present to ease upgrades.

See each commit message for all the gory details.

Since patches are non-trivial, I will review them "per partes" (i.e. each patch
in separate mail). Please check my comments below.

Regards, Adam

 From d0c598ea7e9c02a1ec786c6f1c596ae1be7ac1e2 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Fri, 22 Mar 2013 12:17:07 +0100
Subject: [PATCH] Add helper functions for generic iteration over RBT.

https://fedorahosted.org/bind-dyndb-ldap/ticket/99

Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
  src/rbt_helper.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  src/rbt_helper.h |  29 +++++++++++
  2 files changed, 179 insertions(+)
  create mode 100644 src/rbt_helper.c
  create mode 100644 src/rbt_helper.h

diff --git a/src/rbt_helper.c b/src/rbt_helper.c
new file mode 100644
index 
0000000000000000000000000000000000000000..70ab06134694e36a6ae049284d506bbf5bc3a977
--- /dev/null
+++ b/src/rbt_helper.c
@@ -0,0 +1,150 @@
+#include <dns/rbt.h>
+
+#include "rbt_helper.h"
+
+#define LDAPDB_RBTITER_MAGIC ISC_MAGIC('L', 'D', 'P', 'I')
+
+/**
+ * Copy the RBT node name, i.e. copies the name pointed to by RBT iterator.
+ *
+ * @param[in]  iter     Initialized RBT iterator.
+ * @param[out] nodename Target dns_name suitable for rbt_fullnamefromnode() 
call.
+ *
+ * @pre Nodename has pre-allocated storage space.
+ *
+ * @retval ISC_R_SUCCESS   Actual name was copied to nodename.
+ * @retval ISC_R_NOTFOUND  Iterator doesn't point to any node.
+ * @retval DNS_R_EMPTYNAME Iterator points to name without assigned data,
+ *                         nodename is unchanged.
+ * @retval others          Errors from dns_name_concatenate() and others.
+ *
+ */
+static isc_result_t
+rbt_iter_getnodename(rbt_iterator_t *iter, dns_name_t *nodename) {
+       isc_result_t result;
+       dns_rbtnode_t *node = NULL;
+
+       REQUIRE(iter != NULL);
+       REQUIRE(nodename != NULL);
+       REQUIRE(ISC_MAGIC_VALID(iter, LDAPDB_RBTITER_MAGIC));
+
+       CHECK(dns_rbtnodechain_current(&iter->chain, NULL, NULL, &node));
+       if (node->data == NULL)
+               return DNS_R_EMPTYNAME;
+
+       CHECK(dns_rbt_fullnamefromnode(node, nodename));
+       result = ISC_R_SUCCESS;
+
+cleanup:
+       return result;
+}
+
+/**
+ * Initialize RBT iterator, lock RBT and copy name of the first node with
+ * non-NULL data. Empty RBT nodes (with data == NULL) are ignored.
+ *
+ * RBT remains locked after iterator initialization. RBT has to be
+ * unlocked by reaching end of iteration or explicit rbt_iter_stop() call.
+ *
+ * @param[in,out] rwlock   guard for RBT, will be read-locked
+ * @param[out]    iter     iterator structure, will be initialized
+ * @param[out]    nodename dns_name with pre-allocated storage
+ *
+ * @pre Nodename has pre-allocated storage space.
+ *
+ * @retval ISC_R_SUCCESS   Node with non-NULL data found,
+ *                         RBT is in locked state, iterator is valid,
+ *                         nodename holds copy of actual RBT node name.
+ * @retval ISC_R_NOTFOUND  Node with non-NULL data is not present,
+ *                         RBT is in unlocked state, iterator is invalid.
+ * @retval others          Any error from rbt_iter_getnodename() and
+ *                         rbt_iter_next().
+ */
+isc_result_t
+rbt_iter_first(isc_mem_t *mctx, dns_rbt_t *rbt, isc_rwlock_t *rwlock,
+              rbt_iterator_t *iter, dns_name_t *nodename) {
+
+       isc_result_t result;
+
+       REQUIRE(rbt != NULL);
+       REQUIRE(rwlock != NULL);
+       REQUIRE(iter != NULL);
+
+       ZERO_PTR(iter);
+
+       isc_mem_attach(mctx, &iter->mctx);
+       dns_rbtnodechain_init(&iter->chain, mctx);
+       iter->rbt = rbt;
+       iter->rwlock = rwlock;
+       iter->locktype = isc_rwlocktype_read;
+       iter->magic = LDAPDB_RBTITER_MAGIC;
+
+       RWLOCK(iter->rwlock, iter->locktype);
+
+       result = dns_rbtnodechain_first(&iter->chain, rbt, NULL, NULL);
+       if (result != DNS_R_NEWORIGIN) {
+               rbt_iter_stop(iter);
+               return result;

I would substitute those two lines with "goto cleanup;".

+       }
+
+       result = rbt_iter_getnodename(iter, nodename);
+       if (result == DNS_R_EMPTYNAME)
+               result = rbt_iter_next(iter, nodename);
+       if (result == ISC_R_NOMORE)
+               result = ISC_R_NOTFOUND;

In my opinion this function should leave rbt in locked state only when it
returns ISC_R_SUCCESS. All other cases should unlock the tree. I recommend to
add this statement:

cleanup:
        if (result != ISC_R_SUCCESS)
                rbt_iter_stop(iter);

+
+       return result;
+}
+
+/**
+ * Copy name of the next non-empty node in RBT.
+ *
+ * @param[in]  iter      valid iterator
+ * @param[out] nodename  dns_name with pre-allocated storage
+ *
+ * @pre Nodename has pre-allocated storage space.
+ *
+ * @retval ISC_R_SUCCESS Nodename holds independent copy of RBT node name,
+ *                       RBT is in locked state.
+ * @retval ISC_R_NOMORE  Iteration ended, RBT is in unlocked state,
+ *                       iterator is no longer valid.
+ * @retval others        Errors from dns_name_concatenate() and others.
+ */
+isc_result_t
+rbt_iter_next(rbt_iterator_t *iter, dns_name_t *nodename) {
+       isc_result_t result;
+
+       REQUIRE(iter != NULL);
+       REQUIRE(ISC_MAGIC_VALID(iter, LDAPDB_RBTITER_MAGIC));
+       REQUIRE(iter->locktype != isc_rwlocktype_none);
+
+       do {
+               result = dns_rbtnodechain_next(&iter->chain, NULL, NULL);
+               if (result != ISC_R_SUCCESS && result != DNS_R_NEWORIGIN)
+                       goto cleanup;
+
+               result = rbt_iter_getnodename(iter, nodename);
+       } while (result == DNS_R_EMPTYNAME);
+
+cleanup:
+       if (result != ISC_R_SUCCESS)
+               rbt_iter_stop(iter);

I think rbt_iter_stop() shouldn't be called here. Imagine this piece of code:

CHECK(rbt_iter_first(..));
for (..) {
        CHECK(isc_mem_alloc(..));
        CHECK(rbt_iter_next(..));
}

cleanup:
        if (rbt_iter_first_was_called)
                // Now you should call rbt_iter_stop() only when isc_mem_alloc 
fails
                // but not when rbt_iter_next() fails. However how do you 
figure out
                // which function failed?

Due to this reason, I recommend to drop rbt_iter_stop() call from the
rbt_iter_next().

+
+       return result;
+}
+
+/**
+ * Stop RBT iteration and unlock RBT.
+ */
+void
+rbt_iter_stop(rbt_iterator_t *iter) {
+       REQUIRE(iter != NULL);
+       REQUIRE(ISC_MAGIC_VALID(iter, LDAPDB_RBTITER_MAGIC));
+
+       if (iter->locktype != isc_rwlocktype_none)
+               isc_rwlock_unlock(iter->rwlock, iter->locktype);
+
+       dns_rbtnodechain_invalidate(&iter->chain);
+       isc_mem_detach(&(iter->mctx));
+       ZERO_PTR(iter);
+}
diff --git a/src/rbt_helper.h b/src/rbt_helper.h
new file mode 100644
index 
0000000000000000000000000000000000000000..9c9bcd202cafafb39a3018bbafff6bbd3c9189a9
--- /dev/null
+++ b/src/rbt_helper.h
@@ -0,0 +1,29 @@
+#ifndef _LD_RBT_HELPER_H_
+#define _LD_RBT_HELPER_H_
+
+#include <isc/rwlock.h>
+#include <dns/rbt.h>
+#include "util.h"
+
+struct rbt_iterator {
+       unsigned int            magic;
+       isc_mem_t               *mctx;
+       dns_rbt_t               *rbt;
+       isc_rwlock_t            *rwlock;
+       isc_rwlocktype_t        locktype;
+       dns_rbtnodechain_t      chain;
+};

There is no reason to have struct rbt_iterator exported in header if I read
patchset correctly. Please move it into rbt_helper.c and leave only typedef
below here.

+
+typedef struct rbt_iterator    rbt_iterator_t;
+
+isc_result_t
+rbt_iter_first(isc_mem_t *mctx, dns_rbt_t *rbt, isc_rwlock_t *rwlock,
+              rbt_iterator_t *iter, dns_name_t *nodename);
+
+isc_result_t
+rbt_iter_next(rbt_iterator_t *iter, dns_name_t *nodename);
+
+void
+rbt_iter_stop(rbt_iterator_t *iter);
+
+#endif /* !_LD_RBT_HELPER_H_ */
--
1.7.11.7

Discussion on IRC revealed that interface with 'rbt_iterator_t **iter' (instead of 'rbt_iterator_t *iter') will be cleaner and safer. I will prepare separate patch with this change.

--
Petr^2 Spacek

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

Reply via email to