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