On Mon, Aug 13, 2012 at 03:15:52PM +0200, Petr Spacek wrote: > 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.
Ack, just check my pedantic comments below, please. > 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; I would recommend to leave this goto here. In future, someone might add code before "cleanup:" label and can forget to add "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 of those REQUIREs are redundant. Functions which use those paramaters check for their validity. > /* 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 > -- Adam Tkac, Red Hat, Inc. _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
