Hello, this patch improves connection management in bind-dyndb-ldap and closes https://fedorahosted.org/bind-dyndb-ldap/ticket/68 .
It should prevent all deadlocks on connection pool in future. Petr^2 Spacek
From 0cd91def54ea9ac92e25ee50e54c5e55034e2c47 Mon Sep 17 00:00:00 2001 From: Petr Spacek <[email protected]> Date: Mon, 13 Aug 2012 15:06:50 +0200 Subject: [PATCH] Avoid manual connection management outside ldap_query() https://fedorahosted.org/bind-dyndb-ldap/ticket/68 Signed-off-by: Petr Spacek <[email protected]> --- src/ldap_helper.c | 153 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 87 insertions(+), 66 deletions(-) diff --git a/src/ldap_helper.c b/src/ldap_helper.c index c34b881a8430980f41eb02d2bb0f0229421d7fa1..fdc629e1c0cd1fabde27d887e391ef81823453c1 100644 --- a/src/ldap_helper.c +++ b/src/ldap_helper.c @@ -289,8 +289,9 @@ static isc_result_t ldap_query_create(isc_mem_t *mctx, ldap_qresult_t **ldap_qre static void ldap_query_free(isc_boolean_t prepare_reuse, ldap_qresult_t **ldap_qresultp); /* Functions for writing to LDAP. */ -static isc_result_t ldap_modify_do(ldap_connection_t *ldap_conn, const char *dn, - LDAPMod **mods, isc_boolean_t delete_node); +static isc_result_t ldap_modify_do(ldap_instance_t *ldap_inst, + ldap_connection_t *ldap_conn, const char *dn, LDAPMod **mods, + isc_boolean_t delete_node); static isc_result_t ldap_rdttl_to_ldapmod(isc_mem_t *mctx, dns_rdatalist_t *rdlist, LDAPMod **changep); static isc_result_t ldap_rdatalist_to_ldapmod(isc_mem_t *mctx, @@ -1476,7 +1477,6 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam dns_name_t *origin, ldapdb_nodelist_t *nodelist) { 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; @@ -1488,13 +1488,11 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam REQUIRE(nodelist != NULL); /* RRs aren't in the cache, perform ordinary LDAP query */ - CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn)); - INIT_LIST(*nodelist); CHECK(str_new(mctx, &string)); CHECK(dnsname_to_dn(ldap_inst->zone_register, name, string)); - CHECK(ldap_query(ldap_inst, ldap_conn, &ldap_qresult, str_buf(string), + CHECK(ldap_query(ldap_inst, NULL, &ldap_qresult, str_buf(string), LDAP_SCOPE_SUBTREE, NULL, 0, "(objectClass=idnsRecord)")); if (EMPTY(ldap_qresult->ldap_entries)) { @@ -1536,7 +1534,6 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam cleanup: ldap_query_free(ISC_FALSE, &ldap_qresult); - ldap_pool_putconnection(ldap_inst->pool, &ldap_conn); str_destroy(&string); return result; @@ -1547,7 +1544,6 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na dns_name_t *origin, ldapdb_rdatalist_t *rdatalist) { 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; @@ -1570,8 +1566,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na CHECK(str_new(mctx, &string)); 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, &ldap_qresult, str_buf(string), + CHECK(ldap_query(ldap_inst, NULL, &ldap_qresult, str_buf(string), LDAP_SCOPE_BASE, NULL, 0, "(objectClass=idnsRecord)")); if (EMPTY(ldap_qresult->ldap_entries)) { @@ -1598,7 +1593,6 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na cleanup: ldap_query_free(ISC_FALSE, &ldap_qresult); - ldap_pool_putconnection(ldap_inst->pool, &ldap_conn); str_destroy(&string); if (result != ISC_R_SUCCESS) @@ -1713,11 +1707,15 @@ ldap_query(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn, int ret; int ldap_err_code; int once = 0; + isc_boolean_t autoconn = (ldap_conn == NULL); - REQUIRE(ldap_conn != NULL); + REQUIRE(ldap_inst != NULL); + REQUIRE(base != NULL); REQUIRE(ldap_qresultp != NULL && *ldap_qresultp == NULL); CHECK(ldap_query_create(ldap_inst->mctx, &ldap_qresult)); + if (autoconn) + CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn)); va_start(ap, filter); str_vsprintf(ldap_qresult->query_string, filter, ap); @@ -1751,29 +1749,34 @@ retry: &ldap_qresult->ldap_entries); if (result != ISC_R_SUCCESS) { log_error("failed to save LDAP query results"); - goto cleanup; } + /* LDAP call suceeded, errors from ldap_entrylist_create() will be + * handled in cleanup section */ + } else { /* LDAP error - continue with error handler */ + result = ISC_R_FAILURE; + ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE, + (void *)&ldap_err_code); + if (ret == LDAP_OPT_SUCCESS && ldap_err_code == LDAP_NO_SUCH_OBJECT) { + result = ISC_R_NOTFOUND; + } else if (!once) { + /* some error happened during ldap_search, try to recover */ + once++; + result = handle_connection_error(ldap_inst, ldap_conn, + ISC_FALSE); + if (result == ISC_R_SUCCESS) + goto retry; + } + } + +cleanup: + if (autoconn) + ldap_pool_putconnection(ldap_inst->pool, &ldap_conn); + if (result != ISC_R_SUCCESS) { + ldap_query_free(ISC_FALSE, &ldap_qresult); + } else { *ldap_qresultp = ldap_qresult; - return ISC_R_SUCCESS; - } else { - result = ISC_R_FAILURE; } - - ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE, - (void *)&ldap_err_code); - if (ret == LDAP_OPT_SUCCESS && ldap_err_code == LDAP_NO_SUCH_OBJECT) { - result = ISC_R_NOTFOUND; - } else if (!once) { - /* some error happened during ldap_search, try to recover */ - once++; - result = handle_connection_error(ldap_inst, ldap_conn, - ISC_FALSE); - if (result == ISC_R_SUCCESS) - goto retry; - } -cleanup: - ldap_query_free(ISC_FALSE, &ldap_qresult); return result; } @@ -2111,35 +2114,56 @@ reconnect: return ISC_R_FAILURE; } -/* FIXME: Handle the case where the LDAP handle is NULL -> try to reconnect. */ static isc_result_t -ldap_modify_do(ldap_connection_t *ldap_conn, const char *dn, LDAPMod **mods, - isc_boolean_t delete_node) +ldap_modify_do(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn, + const char *dn, LDAPMod **mods, isc_boolean_t delete_node) { int ret; int err_code; const char *operation_str; + isc_result_t result; + isc_boolean_t autoconn = (ldap_conn == NULL); - REQUIRE(ldap_conn != NULL); REQUIRE(dn != NULL); REQUIRE(mods != NULL); + REQUIRE(ldap_inst != NULL); + + if (autoconn) + CHECK(ldap_pool_getconnection(ldap_inst->pool, &ldap_conn)); + + if (ldap_conn->handle == NULL) { + /* + * handle can be NULL when the first connection to LDAP wasn't + * successful + * TODO: handle this case inside ldap_pool_getconnection()? + */ + CHECK(ldap_connect(ldap_inst, ldap_conn, ISC_FALSE)); + } if (delete_node) { log_debug(2, "deleting whole node: '%s'", dn); ret = ldap_delete_ext_s(ldap_conn->handle, dn, NULL, NULL); } else { log_debug(2, "writing to '%s'", dn); ret = ldap_modify_ext_s(ldap_conn->handle, dn, mods, NULL, NULL); } + result = (ret == LDAP_SUCCESS) ? ISC_R_SUCCESS : ISC_R_FAILURE; if (ret == LDAP_SUCCESS) - return ISC_R_SUCCESS; + goto cleanup; - ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE, &err_code); if (mods[0]->mod_op == LDAP_MOD_ADD) operation_str = "modifying(add)"; - else + else if (mods[0]->mod_op == LDAP_MOD_DELETE) operation_str = "modifying(del)"; + else { + operation_str = "modifying(unknown operation)"; + CHECK(ISC_R_NOTIMPLEMENTED); + } + + LDAP_OPT_CHECK(ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE, + &err_code), "ldap_modify_do(%s) failed to obtain ldap error code", + operation_str); /* If there is no object yet, create it with an ldap add operation. */ if (mods[0]->mod_op == LDAP_MOD_ADD && err_code == LDAP_NO_SUCH_OBJECT) { @@ -2164,10 +2188,12 @@ ldap_modify_do(ldap_connection_t *ldap_conn, const char *dn, LDAPMod **mods, new_mods[i + 1] = NULL; ret = ldap_add_ext_s(ldap_conn->handle, dn, new_mods, NULL, NULL); + result = (ret == LDAP_SUCCESS) ? ISC_R_SUCCESS : ISC_R_FAILURE; if (ret == LDAP_SUCCESS) - return ISC_R_SUCCESS; - ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE, - &err_code); + goto cleanup; + LDAP_OPT_CHECK(ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE, + &err_code), + "ldap_modify_do(add) failed to obtain ldap error code"); operation_str = "adding"; } @@ -2178,11 +2204,13 @@ ldap_modify_do(ldap_connection_t *ldap_conn, const char *dn, LDAPMod **mods, * unexisting attribute */ if (mods[0]->mod_op != LDAP_MOD_DELETE || err_code != LDAP_NO_SUCH_ATTRIBUTE) { - - return ISC_R_FAILURE; + result = ISC_R_FAILURE; } +cleanup: + if (autoconn) + ldap_pool_putconnection(ldap_inst->pool, &ldap_conn); - return ISC_R_SUCCESS; + return result; } static isc_result_t @@ -2348,18 +2376,21 @@ cleanup: * refresh, retry, expire and minimum attributes for each SOA record. */ static isc_result_t -modify_soa_record(ldap_connection_t *ldap_conn, const char *zone_dn, - dns_rdata_t *rdata) +modify_soa_record(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn, + const char *zone_dn, dns_rdata_t *rdata) { isc_result_t result; - isc_mem_t *mctx = ldap_conn->mctx; dns_rdata_soa_t soa; LDAPMod change[5]; LDAPMod *changep[6] = { &change[0], &change[1], &change[2], &change[3], &change[4], NULL }; + REQUIRE(zone_dn != NULL); + REQUIRE(rdata != NULL); + REQUIRE(ldap_inst != NULL); + /* all values in SOA record are isc_uint32_t, i.e. max. 2^32-1 */ #define MAX_SOANUM_LENGTH (10 + 1) #define SET_LDAP_MOD(index, name) \ @@ -2371,17 +2402,17 @@ modify_soa_record(ldap_connection_t *ldap_conn, const char *zone_dn, CHECK(isc_string_printf(change[index].mod_values[0], \ MAX_SOANUM_LENGTH, "%u", soa.name)); - dns_rdata_tostruct(rdata, (void *)&soa, mctx); + dns_rdata_tostruct(rdata, (void *)&soa, ldap_inst->mctx); SET_LDAP_MOD(0, serial); SET_LDAP_MOD(1, refresh); SET_LDAP_MOD(2, retry); SET_LDAP_MOD(3, expire); SET_LDAP_MOD(4, minimum); dns_rdata_freestruct((void *)&soa); - result = ldap_modify_do(ldap_conn, zone_dn, changep, ISC_FALSE); + result = ldap_modify_do(ldap_inst, ldap_conn, zone_dn, changep, ISC_FALSE); cleanup: return result; @@ -2457,7 +2488,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst, CHECK(discard_from_cache(cache, owner)); if (rdlist->type == dns_rdatatype_soa) { - result = modify_soa_record(ldap_conn, str_buf(owner_dn), + result = modify_soa_record(ldap_inst, ldap_conn, str_buf(owner_dn), HEAD(rdlist->rdata)); goto cleanup; } @@ -2468,7 +2499,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst, CHECK(ldap_rdttl_to_ldapmod(mctx, rdlist, &change[1])); } - CHECK(ldap_modify_do(ldap_conn, str_buf(owner_dn), change, delete_node)); + CHECK(ldap_modify_do(ldap_inst, ldap_conn, str_buf(owner_dn), change, delete_node)); /* Keep the PTR of corresponding A/AAAA record synchronized. */ if (rdlist->type == dns_rdatatype_a || rdlist->type == dns_rdatatype_aaaa) { @@ -2648,7 +2679,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst, change_ptr = NULL; /* Modify PTR record. */ - CHECK(ldap_modify_do(ldap_conn, str_buf(owner_dn_ptr), change, delete_node)); + CHECK(ldap_modify_do(ldap_inst, ldap_conn, str_buf(owner_dn_ptr), change, delete_node)); (void) discard_from_cache(ldap_instance_getcache(ldap_inst), dns_fixedname_name(&name)); } @@ -2947,7 +2978,6 @@ static isc_result_t soa_serial_increment(isc_mem_t *mctx, ldap_instance_t *inst, dns_name_t *zone_name) { isc_result_t result = ISC_R_FAILURE; - ldap_connection_t * conn = NULL; ld_string_t *zone_dn = NULL; ldapdb_rdatalist_t rdatalist; dns_rdatalist_t *rdlist = NULL; @@ -2986,8 +3016,7 @@ soa_serial_increment(isc_mem_t *mctx, ldap_instance_t *inst, dns_soa_setserial(new_serial, soa_rdata); /* write the new serial back to DB */ - CHECK(ldap_pool_getconnection(inst->pool, &conn)); - CHECK(modify_soa_record(conn, str_buf(zone_dn), soa_rdata)); + CHECK(modify_soa_record(inst, NULL, str_buf(zone_dn), soa_rdata)); CHECK(discard_from_cache(ldap_instance_getcache(inst), zone_name)); /* put the new SOA to inst->cache and compare old and new serials */ @@ -2999,7 +3028,6 @@ cleanup: log_error("SOA serial number incrementation failed in zone '%s'", str_buf(zone_dn)); - ldap_pool_putconnection(inst->pool, &conn); str_destroy(&zone_dn); ldapdb_rdatalist_destroy(mctx, &rdatalist); return result; @@ -3019,7 +3047,6 @@ update_zone(isc_task_t *task, isc_event_t *event) ldap_psearchevent_t *pevent = (ldap_psearchevent_t *)event; isc_result_t result ; ldap_instance_t *inst = NULL; - ldap_connection_t *conn = NULL; ldap_qresult_t *ldap_qresult_zone = NULL; ldap_qresult_t *ldap_qresult_record = NULL; ldap_entry_t *entry_zone = NULL; @@ -3041,8 +3068,7 @@ update_zone(isc_task_t *task, isc_event_t *event) dns_name_init(&prevname, NULL); CHECK(manager_get_ldap_instance(pevent->dbname, &inst)); - CHECK(ldap_pool_getconnection(inst->pool, &conn)); - CHECK(ldap_query(inst, conn, &ldap_qresult_zone, pevent->dn, + CHECK(ldap_query(inst, NULL, &ldap_qresult_zone, pevent->dn, LDAP_SCOPE_BASE, attrs_zone, 0, "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))")); @@ -3062,7 +3088,7 @@ update_zone(isc_task_t *task, isc_event_t *event) } /* fill the cache with records from renamed zone */ - CHECK(ldap_query(inst, conn, &ldap_qresult_record, pevent->dn, + CHECK(ldap_query(inst, NULL, &ldap_qresult_record, pevent->dn, LDAP_SCOPE_ONELEVEL, attrs_record, 0, "(objectClass=idnsRecord)")); @@ -3090,7 +3116,6 @@ cleanup: ldap_query_free(ISC_FALSE, &ldap_qresult_zone); ldap_query_free(ISC_FALSE, &ldap_qresult_record); - ldap_pool_putconnection(inst->pool, &conn); if (dns_name_dynamic(&prevname)) dns_name_free(&prevname, inst->mctx); isc_mem_free(mctx, pevent->dbname); @@ -3107,7 +3132,6 @@ update_config(isc_task_t *task, isc_event_t *event) ldap_psearchevent_t *pevent = (ldap_psearchevent_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; @@ -3124,9 +3148,7 @@ update_config(isc_task_t *task, isc_event_t *event) if (result != ISC_R_SUCCESS) goto cleanup; - CHECK(ldap_pool_getconnection(inst->pool, &conn)); - - CHECK(ldap_query(inst, conn, &ldap_qresult, pevent->dn, + CHECK(ldap_query(inst, NULL, &ldap_qresult, pevent->dn, LDAP_SCOPE_BASE, attrs, 0, "(objectClass=idnsConfigObject)")); @@ -3149,7 +3171,6 @@ cleanup: pevent->dn); ldap_query_free(ISC_FALSE, &ldap_qresult); - ldap_pool_putconnection(inst->pool, &conn); isc_mem_free(mctx, pevent->dbname); isc_mem_free(mctx, pevent->dn); isc_mem_detach(&mctx); -- 1.7.11.2
_______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
