[Freeipa-devel] [PATCH 0134] Make RBT iterators more resilient.
Adam Tkac
atkac at redhat.com
Tue Apr 2 17:13:55 UTC 2013
On Tue, Apr 02, 2013 at 06:49:53PM +0200, Petr Spacek wrote:
> Hello,
>
> Make RBT iterators more resilient.
>
> This patch implements more resilient interface for RBT iterators, as
> I promised in thread about patches 123-126.
>
> Now multiple calls to rbt_iter_stop() with the same argument do not hurt.
Ack
> From 9ee8cb1b9be0db6ca1530b43e96547b130181519 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at redhat.com>
> Date: Tue, 2 Apr 2013 18:46:48 +0200
> Subject: [PATCH] Make RBT iterators more resilient.
>
> Signed-off-by: Petr Spacek <pspacek at redhat.com>
> ---
> src/fwd_register.c | 2 +-
> src/fwd_register.h | 2 +-
> src/ldap_helper.c | 4 +++-
> src/rbt_helper.c | 66 ++++++++++++++++++++++++++++++++++++-----------------
> src/rbt_helper.h | 15 +++---------
> src/zone_register.c | 4 ++--
> src/zone_register.h | 2 +-
> 7 files changed, 56 insertions(+), 39 deletions(-)
>
> diff --git a/src/fwd_register.c b/src/fwd_register.c
> index c663b25909b0e393421c49950d1f29a1352cfe6c..81eaac5b66ff66890935e7e6a94138c5e854332d 100644
> --- a/src/fwd_register.c
> +++ b/src/fwd_register.c
> @@ -146,7 +146,7 @@ cleanup:
> }
>
> isc_result_t
> -fwdr_rbt_iter_init(fwd_register_t *fwdr, rbt_iterator_t *iter,
> +fwdr_rbt_iter_init(fwd_register_t *fwdr, rbt_iterator_t **iter,
> dns_name_t *nodename) {
> if (fwdr->rbt == NULL)
> return ISC_R_NOTFOUND;
> diff --git a/src/fwd_register.h b/src/fwd_register.h
> index 0bee3cba82d1deca1aa2fce235be118d076332f0..5fb96c0eb9b07e7374f4591d9cc166714abc23bd 100644
> --- a/src/fwd_register.h
> +++ b/src/fwd_register.h
> @@ -29,7 +29,7 @@ isc_result_t
> fwdr_zone_ispresent(fwd_register_t *fwdr, dns_name_t *name);
>
> isc_result_t
> -fwdr_rbt_iter_init(fwd_register_t *fwdr, rbt_iterator_t *iter,
> +fwdr_rbt_iter_init(fwd_register_t *fwdr, rbt_iterator_t **iter,
> dns_name_t *nodename);
>
> #endif /* !_LD_FWD_REGISTER_H_ */
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 72456228ba9d223d239f34ae88d63192e0ffbbb4..99d67724a61304a2f39a0d3fa9391ce35f12b72f 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1549,7 +1549,7 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst, isc_boolean_t delete_only)
>
> /* Walk through master zone register and remove all zones which
> * disappeared from LDAP. */
> - rbt_iterator_t iter;
> + rbt_iterator_t *iter = NULL;
> char name_txt[DNS_NAME_FORMATSIZE];
> DECLARE_BUFFERED_NAME(registered_name);
> DECLARE_BUFFERED_NAME(ldap_name);
> @@ -1588,6 +1588,7 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst, isc_boolean_t delete_only)
> /* Walk through forward zone register and remove all zones which
> * disappeared from LDAP. */
> INIT_BUFFERED_NAME(registered_name);
> + iter = NULL;
> result = fwdr_rbt_iter_init(ldap_inst->fwd_register, &iter, ®istered_name);
> while (result == ISC_R_SUCCESS) {
> void *data = NULL;
> @@ -1625,6 +1626,7 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst, isc_boolean_t delete_only)
> goto cleanup;
>
> cleanup:
> + rbt_iter_stop(&iter);
> if (master_rbt != NULL)
> dns_rbt_destroy(&master_rbt);
> if (forward_rbt != NULL)
> diff --git a/src/rbt_helper.c b/src/rbt_helper.c
> index 70ab06134694e36a6ae049284d506bbf5bc3a977..ab37e3c754d06c1b49e389e2e85a5340d4317db2 100644
> --- a/src/rbt_helper.c
> +++ b/src/rbt_helper.c
> @@ -4,6 +4,16 @@
>
> #define LDAPDB_RBTITER_MAGIC ISC_MAGIC('L', 'D', 'P', 'I')
>
> +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;
> +};
> +
> +
> /**
> * Copy the RBT node name, i.e. copies the name pointed to by RBT iterator.
> *
> @@ -47,7 +57,7 @@ cleanup:
> * 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] iterp iterator structure, will be initialized
> * @param[out] nodename dns_name with pre-allocated storage
> *
> * @pre Nodename has pre-allocated storage space.
> @@ -62,14 +72,16 @@ cleanup:
> */
> 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) {
> + rbt_iterator_t **iterp, dns_name_t *nodename) {
>
> isc_result_t result;
> + rbt_iterator_t *iter = NULL;
>
> REQUIRE(rbt != NULL);
> REQUIRE(rwlock != NULL);
> - REQUIRE(iter != NULL);
> + REQUIRE(iterp != NULL && *iterp == NULL);
>
> + CHECKED_MEM_GET_PTR(mctx, iter);
> ZERO_PTR(iter);
>
> isc_mem_attach(mctx, &iter->mctx);
> @@ -82,69 +94,81 @@ rbt_iter_first(isc_mem_t *mctx, dns_rbt_t *rbt, isc_rwlock_t *rwlock,
> 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;
> - }
> + if (result != DNS_R_NEWORIGIN)
> + goto cleanup;
>
> result = rbt_iter_getnodename(iter, nodename);
> if (result == DNS_R_EMPTYNAME)
> - result = rbt_iter_next(iter, nodename);
> + result = rbt_iter_next(&iter, nodename);
> if (result == ISC_R_NOMORE)
> result = ISC_R_NOTFOUND;
>
> +cleanup:
> + if (result == ISC_R_SUCCESS)
> + *iterp = iter;
> + else
> + rbt_iter_stop(&iter);
> +
> return result;
> }
>
> /**
> * Copy name of the next non-empty node in RBT.
> *
> - * @param[in] iter valid iterator
> + * @param[in] iterp 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.
> + * RBT is in unlocked state, iterator is no longer valid.
> */
> isc_result_t
> -rbt_iter_next(rbt_iterator_t *iter, dns_name_t *nodename) {
> +rbt_iter_next(rbt_iterator_t **iterp, 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);
> + REQUIRE(iterp != NULL && *iterp != NULL);
> + REQUIRE(ISC_MAGIC_VALID(*iterp, LDAPDB_RBTITER_MAGIC));
> + REQUIRE((*iterp)->locktype != isc_rwlocktype_none);
>
> do {
> - result = dns_rbtnodechain_next(&iter->chain, NULL, NULL);
> + result = dns_rbtnodechain_next(&(*iterp)->chain, NULL, NULL);
> if (result != ISC_R_SUCCESS && result != DNS_R_NEWORIGIN)
> goto cleanup;
>
> - result = rbt_iter_getnodename(iter, nodename);
> + result = rbt_iter_getnodename(*iterp, nodename);
> } while (result == DNS_R_EMPTYNAME);
>
> cleanup:
> if (result != ISC_R_SUCCESS)
> - rbt_iter_stop(iter);
> + rbt_iter_stop(iterp);
>
> return result;
> }
>
> /**
> * Stop RBT iteration and unlock RBT.
> + * @param[in] iterp valid iterator or NULL
> */
> void
> -rbt_iter_stop(rbt_iterator_t *iter) {
> - REQUIRE(iter != NULL);
> +rbt_iter_stop(rbt_iterator_t **iterp) {
> + rbt_iterator_t *iter;
> +
> + REQUIRE(iterp != NULL);
> + iter = *iterp;
> +
> + if (iter == NULL)
> + return;
> +
> REQUIRE(ISC_MAGIC_VALID(iter, LDAPDB_RBTITER_MAGIC));
> -
> + iter->magic = 0;
> 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);
> + MEM_PUT_AND_DETACH(*iterp);
> }
> diff --git a/src/rbt_helper.h b/src/rbt_helper.h
> index 9c9bcd202cafafb39a3018bbafff6bbd3c9189a9..98aef20f217c68b74e497f91e549081ea3160225 100644
> --- a/src/rbt_helper.h
> +++ b/src/rbt_helper.h
> @@ -5,25 +5,16 @@
> #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;
> -};
> -
> 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);
> + rbt_iterator_t **iter, dns_name_t *nodename);
>
> isc_result_t
> -rbt_iter_next(rbt_iterator_t *iter, dns_name_t *nodename);
> +rbt_iter_next(rbt_iterator_t **iter, dns_name_t *nodename);
>
> void
> -rbt_iter_stop(rbt_iterator_t *iter);
> +rbt_iter_stop(rbt_iterator_t **iter);
>
> #endif /* !_LD_RBT_HELPER_H_ */
> diff --git a/src/zone_register.c b/src/zone_register.c
> index 949219ffc2e2adb40943709a3420014e672c88e6..dd2e78098cef9e3fac181c0aa38d311e26a76a74 100644
> --- a/src/zone_register.c
> +++ b/src/zone_register.c
> @@ -91,7 +91,7 @@ static const setting_t zone_settings[] = {
> };
>
> isc_result_t
> -zr_rbt_iter_init(zone_register_t *zr, rbt_iterator_t *iter,
> +zr_rbt_iter_init(zone_register_t *zr, rbt_iterator_t **iter,
> dns_name_t *nodename) {
> if (zr->rbt == NULL)
> return ISC_R_NOTFOUND;
> @@ -160,7 +160,7 @@ zr_destroy(zone_register_t **zrp)
> {
> DECLARE_BUFFERED_NAME(name);
> zone_register_t *zr;
> - rbt_iterator_t iter;
> + rbt_iterator_t *iter = NULL;
> isc_result_t result;
>
> if (zrp == NULL || *zrp == NULL)
> diff --git a/src/zone_register.h b/src/zone_register.h
> index 7ef40f7abdf4ed17cb32414907b5b7283456bb22..091b3d53aba168f54ac60b0c9e86e8fff3347950 100644
> --- a/src/zone_register.h
> +++ b/src/zone_register.h
> @@ -58,7 +58,7 @@ isc_result_t
> zr_get_zone_settings(zone_register_t *zr, dns_name_t *name, settings_set_t **set);
>
> isc_result_t
> -zr_rbt_iter_init(zone_register_t *zr, rbt_iterator_t *iter,
> +zr_rbt_iter_init(zone_register_t *zr, rbt_iterator_t **iter,
> dns_name_t *nodename);
>
> dns_rbt_t *
> --
> 1.7.11.7
>
--
Adam Tkac, Red Hat, Inc.
More information about the Freeipa-devel
mailing list