[Freeipa-devel] [PATCH 0020] Separate LDAP result from LDAP connection, fix deadlock.
Adam Tkac
atkac at redhat.com
Fri May 11 10:26:55 UTC 2012
On Mon, May 07, 2012 at 02:49:07PM +0200, Petr Spacek wrote:
> Hello,
>
> this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/66:
> Plugin deadlocks during new zone load when connections == 1.
>
> It fixes structural problem, when LDAP query result was tied with
> LDAP connection up. It wasn't possible to release connection and
> work with query result after that.
> Described deadlock is consequence of this problematic design.
>
> Now LDAP connection is separated from LDAP result. Next planed patch
> will avoid "manual" connection management, so possibility of
> deadlock should be next to zero.
>
> Petr^2 Spacek
Hello Peter,
good work, please check my comments below.
Regards, Adam
> From 8ee1fd607531ef71369e99c9228456baea45b65d Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at redhat.com>
> Date: Mon, 7 May 2012 12:51:09 +0200
> Subject: [PATCH] Separate LDAP result from LDAP connection, fix deadlock.
> https://fedorahosted.org/bind-dyndb-ldap/ticket/66
> Signed-off-by: Petr Spacek <pspacek at redhat.com>
>
> ---
> src/ldap_helper.c | 191 ++++++++++++++++++++++++++++++-----------------------
> 1 files changed, 109 insertions(+), 82 deletions(-)
>
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 304c37296f8f3a428c4c72b45fe4154b2c9e954c..298d8e64cc9f6a0bafac6408f199276ac0e45f0d 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -108,6 +108,7 @@
> * must acquire the semaphore and the lock.
> */
>
> +typedef struct ldap_qresult ldap_qresult_t;
> typedef struct ldap_connection ldap_connection_t;
> typedef struct ldap_pool ldap_pool_t;
> typedef struct ldap_auth_pair ldap_auth_pair_t;
> @@ -188,28 +189,28 @@ struct ldap_connection {
> ld_string_t *query_string;
>
> LDAP *handle;
> - LDAPMessage *result;
> LDAPControl *serverctrls[2]; /* psearch/NULL or NULL/NULL */
> int msgid;
>
> /* Parsing. */
> isc_lex_t *lex;
> isc_buffer_t rdata_target;
> unsigned char *rdata_target_mem;
>
> - /* Cache. */
> - ldap_entrylist_t ldap_entries;
> -
> /* For reconnection logic. */
> isc_time_t next_reconnect;
> unsigned int tries;
> +};
>
> - /* Temporary stuff. */
> - LDAPMessage *entry;
> - BerElement *ber;
> - char *attribute;
> - char **values;
> - char *dn;
> +/**
> + * Result from single LDAP query.
> + */
> +struct ldap_qresult {
> + isc_mem_t *mctx;
> + LDAPMessage *result;
> +
> + /* Cache. */
> + ldap_entrylist_t ldap_entries;
> };
>
> /*
> @@ -271,9 +272,10 @@ static int handle_connection_error(ldap_instance_t *ldap_inst,
> ldap_connection_t *ldap_conn, isc_boolean_t force,
> isc_result_t *result);
> static isc_result_t ldap_query(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
> - const char *base,
> - int scope, char **attrs, int attrsonly, const char *filter, ...);
> -static void ldap_query_free(ldap_connection_t *ldap_conn);
> + ldap_qresult_t **ldap_qresult, const char *base, int scope, char **attrs,
When naming "**param" output parameters, convention is to append "p" (i.e.
**ldap_qresultp) in this case.
> + int attrsonly, const char *filter, ...);
> +static void ldap_query_free(ldap_qresult_t **ldap_qresult);
Ditto.
> +
>
> /* Functions for writing to LDAP. */
> static isc_result_t ldap_modify_do(ldap_connection_t *ldap_conn, const char *dn,
> @@ -1095,6 +1097,8 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
> {
> isc_result_t result = ISC_R_SUCCESS;
> ldap_connection_t *ldap_conn = NULL;
> + ldap_qresult_t *ldap_config_qresult = NULL;
> + ldap_qresult_t *ldap_zones_qresult = NULL;
> int zone_count = 0;
> ldap_entry_t *entry;
> dns_rbt_t *rbt = NULL;
> @@ -1119,31 +1123,31 @@ refresh_zones_from_ldap(ldap_instance_t *ldap_inst)
>
> log_debug(2, "refreshing list of zones for %s", ldap_inst->db_name);
>
> + /* Query configuration and zones from LDAP and release LDAP connection
I think "Query for configuration..." is more appropriate here.
> + * before processing them. It prevents deadlock in situations where
> + * ldap_parse_zoneentry() requests another connection. */
> CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
> -
> - CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(ldap_inst->base),
> + CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_config_qresult, str_buf(ldap_inst->base),
> LDAP_SCOPE_SUBTREE, config_attrs, 0,
> "(objectClass=idnsConfigObject)"));
> -
> - for (entry = HEAD(ldap_conn->ldap_entries);
> - entry != NULL;
> - entry = NEXT(entry, link)) {
> - CHECK(ldap_parse_configentry(entry, ldap_inst));
> - }
> -
> - ldap_query_free(ldap_conn);
> -
> - CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(ldap_inst->base),
> + CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_zones_qresult, str_buf(ldap_inst->base),
> LDAP_SCOPE_SUBTREE, zone_attrs, 0,
> "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
> + ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
> +
> + for (entry = HEAD(ldap_config_qresult->ldap_entries);
> + entry != NULL;
> + entry = NEXT(entry, link)) {
> + CHECK(ldap_parse_configentry(entry, ldap_inst));
> + }
>
> /*
> * Create RB-tree with all zones stored in LDAP for cross check
> * with registered zones in plugin.
> */
> CHECK(dns_rbt_create(ldap_inst->mctx, NULL, NULL, &rbt));
>
> - for (entry = HEAD(ldap_conn->ldap_entries);
> + for (entry = HEAD(ldap_zones_qresult->ldap_entries);
> entry != NULL;
> entry = NEXT(entry, link)) {
>
> @@ -1232,6 +1236,8 @@ cleanup:
> if (invalidate_nodechain)
> dns_rbtnodechain_invalidate(&chain);
>
> + ldap_query_free(&ldap_config_qresult);
> + ldap_query_free(&ldap_zones_qresult);
> ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
>
> log_debug(2, "finished refreshing list of zones");
> @@ -1387,6 +1393,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
> {
> isc_result_t result;
> ldap_connection_t *ldap_conn = NULL;
> + ldap_qresult_t *ldap_qresult = NULL;
> ldap_entry_t *entry;
> ld_string_t *string = NULL;
> ldapdb_node_t *node;
> @@ -1403,15 +1410,15 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
> CHECK(str_new(mctx, &string));
> CHECK(dnsname_to_dn(ldap_inst->zone_register, name, string));
>
> - CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(string),
> + CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_qresult, str_buf(string),
> LDAP_SCOPE_SUBTREE, NULL, 0, "(objectClass=idnsRecord)"));
>
> - if (EMPTY(ldap_conn->ldap_entries)) {
> + if (EMPTY(ldap_qresult->ldap_entries)) {
> result = ISC_R_NOTFOUND;
> goto cleanup;
> }
>
> - for (entry = HEAD(ldap_conn->ldap_entries);
> + for (entry = HEAD(ldap_qresult->ldap_entries);
> entry != NULL;
> entry = NEXT(entry, link)) {
> node = NULL;
> @@ -1444,6 +1451,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
> result = ISC_R_SUCCESS;
>
> cleanup:
> + ldap_query_free(&ldap_qresult);
> ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
> str_destroy(&string);
>
> @@ -1456,6 +1464,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
> {
> isc_result_t result;
> ldap_connection_t *ldap_conn = NULL;
> + ldap_qresult_t *ldap_qresult = NULL;
> ldap_entry_t *entry;
> ld_string_t *string = NULL;
> ldap_cache_t *cache;
> @@ -1478,15 +1487,15 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
> CHECK(dnsname_to_dn(ldap_inst->zone_register, name, string));
>
> CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
> - CHECK(ldap_query(ldap_inst, ldap_conn, str_buf(string),
> + CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_qresult, str_buf(string),
> LDAP_SCOPE_BASE, NULL, 0, "(objectClass=idnsRecord)"));
>
> - if (EMPTY(ldap_conn->ldap_entries)) {
> + if (EMPTY(ldap_qresult->ldap_entries)) {
> result = ISC_R_NOTFOUND;
> goto cleanup;
> }
>
> - for (entry = HEAD(ldap_conn->ldap_entries);
> + for (entry = HEAD(ldap_qresult->ldap_entries);
> entry != NULL;
> entry = NEXT(entry, link)) {
> if (ldap_parse_rrentry(mctx, entry, ldap_conn,
> @@ -1504,6 +1513,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
> result = ISC_R_NOTFOUND;
>
> cleanup:
> + ldap_query_free(&ldap_qresult);
> ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
> str_destroy(&string);
>
> @@ -1601,16 +1611,23 @@ cleanup:
> return result;
> }
>
> +/**
> + * @param ldap_conn A LDAP connection structure obtained via ldap_get_connection().
> + * @param ldap_qresult New ldap_qresult structure will be allocated and pointer
> + * to it will be returned through this parameter. The result
> + * has to be freed by caller via ldap_query_free().
> + */
> static isc_result_t
> ldap_query(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
> - const char *base, int scope, char **attrs,
> + ldap_qresult_t **ldap_qresult, const char *base, int scope, char **attrs,
s/ldap_qresult/ldap_qresultp/
> int attrsonly, const char *filter, ...)
> {
> va_list ap;
> isc_result_t result;
> int cnt;
>
> REQUIRE(ldap_conn != NULL);
> + REQUIRE(ldap_qresult != NULL && *ldap_qresult == NULL);
>
> va_start(ap, filter);
> str_vsprintf(ldap_conn->query_string, filter, ap);
> @@ -1629,62 +1646,59 @@ ldap_query(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
> return result;
> }
>
> + CHECKED_MEM_GET_PTR(ldap_inst->mctx, *ldap_qresult);
> + (*ldap_qresult)->mctx = ldap_inst->mctx;
> + INIT_LIST((*ldap_qresult)->ldap_entries);
This is not so good. When ldap_query() doesn't return success, it shouldn't
modify it's **ldap_qresultp parameter, as other functions do. Good way how to
handle this situations is (pseudocode):
function(**outputp)
{
*output;
REQUIRE(outputp != NULL && *outputp == NULL);
output = mem_get();
modify_output(output);
if (some_failure)
goto cleanup;
*outputp = output;
return success;
cleanup:
mem_put(output);
return failure;
}
> +
> do {
> int ret;
>
> ret = ldap_search_ext_s(ldap_conn->handle, base, scope,
> str_buf(ldap_conn->query_string),
> attrs, attrsonly, NULL, NULL, NULL,
> - LDAP_NO_LIMIT, &ldap_conn->result);
> + LDAP_NO_LIMIT, &((*ldap_qresult)->result));
> if (ret == 0) {
> ldap_conn->tries = 0;
> - cnt = ldap_count_entries(ldap_conn->handle, ldap_conn->result);
> + cnt = ldap_count_entries(ldap_conn->handle, (*ldap_qresult)->result);
> log_debug(2, "entry count: %d", cnt);
>
> result = ldap_entrylist_create(ldap_conn->mctx,
> ldap_conn->handle,
> - ldap_conn->result,
> - &ldap_conn->ldap_entries);
> + (*ldap_qresult)->result,
> + &(*ldap_qresult)->ldap_entries);
> if (result != ISC_R_SUCCESS) {
> log_error("failed to save LDAP query results");
> return result;
> }
>
> return ISC_R_SUCCESS;
> }
> } while (handle_connection_error(ldap_inst, ldap_conn, ISC_FALSE, &result));
>
> +cleanup:
Previously allocated ldap_qresult should be free()-ed here.
> return result;
> }
>
> static void
> -ldap_query_free(ldap_connection_t *ldap_conn)
> +ldap_query_free(ldap_qresult_t **ldap_qresult)
> {
> - if (ldap_conn == NULL)
> + ldap_qresult_t *qresult;
> + REQUIRE(ldap_qresult != NULL);
> +
> + qresult = *ldap_qresult;
> +
> + if (qresult == NULL)
> return;
>
> - if (ldap_conn->dn) {
> - ldap_memfree(ldap_conn->dn);
> - ldap_conn->dn = NULL;
> - }
> - if (ldap_conn->values) {
> - ldap_value_free(ldap_conn->values);
> - ldap_conn->values = NULL;
> - }
> - if (ldap_conn->attribute) {
> - ldap_memfree(ldap_conn->attribute);
> - ldap_conn->attribute = NULL;
> - }
> - if (ldap_conn->ber) {
> - ber_free(ldap_conn->ber, 0);
> - ldap_conn->ber = NULL;
> - }
> - if (ldap_conn->result) {
> - ldap_msgfree(ldap_conn->result);
> - ldap_conn->result = NULL;
> + if (qresult->result) {
> + ldap_msgfree(qresult->result);
> + qresult->result = NULL;
> }
>
> - ldap_entrylist_destroy(ldap_conn->mctx, &ldap_conn->ldap_entries);
> + ldap_entrylist_destroy(qresult->mctx, &qresult->ldap_entries);
> +
> + SAFE_MEM_PUT_PTR(qresult->mctx, qresult);
> + *ldap_qresult = NULL;
> }
>
> /* FIXME: Tested with SASL/GSSAPI/KRB5 only */
> @@ -2229,6 +2243,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> isc_result_t result;
> isc_mem_t *mctx = ldap_inst->mctx;
> ldap_connection_t *ldap_conn = NULL;
> + ldap_qresult_t *ldap_qresult = NULL;
> ld_string_t *owner_dn = NULL;
> LDAPMod *change[3] = { NULL };
> LDAPMod *change_ptr = NULL;
> @@ -2255,12 +2270,12 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> }
>
> CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn));
> - CHECK(ldap_query(ldap_inst, ldap_conn, zone_dn,
> + CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_qresult, zone_dn,
> LDAP_SCOPE_BASE, attrs, 0,
> "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
>
> /* only 0 or 1 active zone can be returned from query */
> - entry = HEAD(ldap_conn->ldap_entries);
> + entry = HEAD(ldap_qresult->ldap_entries);
> if (entry == NULL) {
> log_debug(3, "Active zone %s not found", zone_dn);
> result = ISC_R_NOTFOUND;
> @@ -2392,14 +2407,14 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> char *owner_zone_dn_ptr = strstr(str_buf(owner_dn_ptr),", ") + 1;
>
> /* Get attribute "idnsAllowDynUpdate" for reverse zone or use default. */
> - ldap_query_free(ldap_conn);
> + ldap_query_free(&ldap_qresult);
> zone_dyn_update = ldap_inst->dyn_update;
> - CHECK(ldap_query(ldap_inst, ldap_conn, owner_zone_dn_ptr,
> + CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_qresult, owner_zone_dn_ptr,
> LDAP_SCOPE_BASE, attrs, 0,
> "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
>
> /* Only 0 or 1 active zone can be returned from query. */
> - entry = HEAD(ldap_conn->ldap_entries);
> + entry = HEAD(ldap_qresult->ldap_entries);
> if (entry == NULL) {
> log_debug(3, "Active zone %s not found", zone_dn);
> result = ISC_R_NOTFOUND;
> @@ -2485,6 +2500,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
> }
>
> cleanup:
> + ldap_query_free(&ldap_qresult);
> ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
> str_destroy(&owner_dn_ptr);
> str_destroy(&owner_dn);
> @@ -2587,7 +2603,6 @@ ldap_pool_getconnection(ldap_pool_t *pool, ldap_connection_t ** conn)
>
> RUNTIME_CHECK(ldap_conn != NULL);
>
> - INIT_LIST(ldap_conn->ldap_entries);
> *conn = ldap_conn;
>
> cleanup:
> @@ -2607,7 +2622,6 @@ ldap_pool_putconnection(ldap_pool_t *pool, ldap_connection_t **conn)
> if (ldap_conn == NULL)
> return;
>
> - ldap_query_free(ldap_conn);
> UNLOCK(&ldap_conn->lock);
> semaphore_signal(&pool->conn_semaphore);
>
> @@ -2739,6 +2753,7 @@ update_action(isc_task_t *task, isc_event_t *event)
> isc_result_t result ;
> ldap_instance_t *inst = NULL;
> ldap_connection_t *conn = NULL;
> + ldap_qresult_t *ldap_qresult = NULL;
> ldap_entry_t *entry;
> isc_boolean_t delete = ISC_TRUE;
> isc_mem_t *mctx;
> @@ -2757,11 +2772,11 @@ update_action(isc_task_t *task, isc_event_t *event)
>
> CHECK(ldap_pool_getconnection(inst->pool, &conn));
>
> - CHECK(ldap_query(inst, conn, pevent->dn,
> + CHECK(ldap_query(inst, conn, &ldap_qresult, pevent->dn,
> LDAP_SCOPE_BASE, attrs, 0,
> "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
>
> - for (entry = HEAD(conn->ldap_entries);
> + for (entry = HEAD(ldap_qresult->ldap_entries);
> entry != NULL;
> entry = NEXT(entry, link)) {
> delete = ISC_FALSE;
> @@ -2779,6 +2794,7 @@ cleanup:
> "Zones can be outdated, run `rndc reload`",
> pevent->dn);
>
> + ldap_query_free(&ldap_qresult);
> ldap_pool_putconnection(inst->pool, &conn);
> isc_mem_free(mctx, pevent->dbname);
> isc_mem_free(mctx, pevent->dn);
> @@ -2793,6 +2809,7 @@ update_config(isc_task_t *task, isc_event_t *event)
> isc_result_t result ;
> ldap_instance_t *inst = NULL;
> ldap_connection_t *conn = NULL;
> + ldap_qresult_t *ldap_qresult = NULL;
> ldap_entry_t *entry;
> isc_mem_t *mctx;
> char *attrs[] = {
> @@ -2810,14 +2827,14 @@ update_config(isc_task_t *task, isc_event_t *event)
>
> CHECK(ldap_pool_getconnection(inst->pool, &conn));
>
> - CHECK(ldap_query(inst, conn, pevent->dn,
> + CHECK(ldap_query(inst, conn, &ldap_qresult, pevent->dn,
> LDAP_SCOPE_BASE, attrs, 0,
> "(objectClass=idnsConfigObject)"));
>
> - if (EMPTY(conn->ldap_entries))
> - log_error("Config object can not be empty");
> + if (EMPTY(ldap_qresult->ldap_entries))
> + log_error("Config object can not be empty"); // WHY?
>
> - for (entry = HEAD(conn->ldap_entries);
> + for (entry = HEAD(ldap_qresult->ldap_entries);
> entry != NULL;
> entry = NEXT(entry, link)) {
> result = ldap_parse_configentry(entry, inst);
> @@ -2832,6 +2849,7 @@ cleanup:
> "Configuration can be outdated, run `rndc reload`",
> pevent->dn);
>
> + ldap_query_free(&ldap_qresult);
> ldap_pool_putconnection(inst->pool, &conn);
> isc_mem_free(mctx, pevent->dbname);
> isc_mem_free(mctx, pevent->dn);
> @@ -3071,6 +3089,7 @@ ldap_psearch_watcher(isc_threadarg_t arg)
> {
> ldap_instance_t *inst = (ldap_instance_t *)arg;
> ldap_connection_t *conn;
> + ldap_qresult_t *ldap_qresult = NULL;
> struct timeval tv;
> int ret, cnt;
> isc_result_t result;
> @@ -3108,6 +3127,11 @@ ldap_psearch_watcher(isc_threadarg_t arg)
> ldap_connect(inst, conn, ISC_TRUE);
> }
>
> + CHECKED_MEM_GET_PTR(conn->mctx, ldap_qresult);
> + ldap_qresult->mctx = conn->mctx;
> + ldap_qresult->result = NULL;
> + INIT_LIST(ldap_qresult->ldap_entries);
This piece of code is on two places. What about
ldap_query_create(ldap_qresult_t **qresultp) function?
It will be better because we can add/remove members of the ldap_qresult_t
structure in the future.
> +
> restart:
> /* Perform initial lookup */
> if (inst->psearch) {
> @@ -3133,8 +3157,9 @@ restart:
> }
>
> while (!inst->exiting) {
> +
> ret = ldap_result(conn->handle, conn->msgid, 0, &tv,
> - &conn->result);
> + &ldap_qresult->result);
>
> if (ret <= 0) {
> int ok;
> @@ -3158,14 +3183,14 @@ restart:
> }
>
> conn->tries = 0;
> - cnt = ldap_count_entries(conn->handle, conn->result);
> + cnt = ldap_count_entries(conn->handle, ldap_qresult->result);
>
> if (cnt > 0) {
> log_debug(3, "Got psearch updates (%d)", cnt);
> - result = ldap_entrylist_append(conn->mctx,
> + result = ldap_entrylist_append(ldap_qresult->mctx,
> conn->handle,
> - conn->result,
> - &conn->ldap_entries);
> + ldap_qresult->result,
> + &ldap_qresult->ldap_entries);
> if (result != ISC_R_SUCCESS) {
> /*
> * Error means inconsistency of our zones
> @@ -3177,7 +3202,7 @@ restart:
> }
>
> ldap_entry_t *entry;
> - for (entry = HEAD(conn->ldap_entries);
> + for (entry = HEAD(ldap_qresult->ldap_entries);
> entry != NULL;
> entry = NEXT(entry, link)) {
> LDAPControl **ctrls = NULL;
> @@ -3195,16 +3220,18 @@ restart:
> psearch_update(inst, entry, ctrls);
> }
> soft_err:
> -
> - ldap_msgfree(conn->result);
> - ldap_entrylist_destroy(conn->mctx,
> - &conn->ldap_entries);
> + ;
Empty label "soft_err:" is useless, please remove it and use "continue;" on
appropriate places;
> }
> + ldap_msgfree(ldap_qresult->result);
> + ldap_qresult->result = NULL;
> + ldap_entrylist_destroy(ldap_qresult->mctx, &ldap_qresult->ldap_entries);
> + INIT_LIST(ldap_qresult->ldap_entries);
I would prefer to add another parameter to the ldap_query_free() which will
indicate if the ldap_qresult() should be free()-ed or just results should be
cleared. Reason is same as above - ldap_qresult_t can be changed and we will
have to change both ldap_query_free() and this piece of code.
> }
>
> log_debug(1, "Ending ldap_psearch_watcher");
>
> cleanup:
> + ldap_query_free(&ldap_qresult);
> ldap_pool_putconnection(inst->pool, &conn);
>
> return (isc_threadresult_t)0;
> --
> 1.7.7.6
>
--
Adam Tkac, Red Hat, Inc.
More information about the Freeipa-devel
mailing list