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 > -- Adam Tkac, Red Hat, Inc. _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel