[Freeipa-devel] [PATCH 0123-0126] Separate master and forward zones (add idnsForwardZone object class)
Adam Tkac
atkac at redhat.com
Tue Apr 2 15:17:10 UTC 2013
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 <pspacek at 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 <pspacek at 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.
More information about the Freeipa-devel
mailing list