On 08/22/2012 03:35 PM, Adam Tkac wrote:
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.
I partially disagree with one comment below. Amended patch is attached.
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
@@ -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".
Ok.
}
+ /* LDAP call suceeded, errors from ldap_entrylist_create() will
be
+ * handled in cleanup section */
* 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.
Well, zone_dn and rdata checks are really redundant. First use of ldap_inst is
in ldap_inst->mctx, so check is AFAIK necessary.
I left REQUIRE(ldap_inst != NULL); in attached patch, other REQUIREs were
deleted.
/* 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);
--
Petr^2 Spacek
From 83733f183509c44162a955227c702fe8555dc2af 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 | 150 +++++++++++++++++++++++++++++++-----------------------
1 file changed, 85 insertions(+), 65 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 5f1334255311282b9d13f20c242d8422b8255ecc..22cf614788edb63ae5659aa22d9c33f8cbb86ae0 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);
@@ -1753,27 +1751,33 @@ retry:
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;
}
@@ -2109,35 +2113,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) {
@@ -2162,10 +2187,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";
}
@@ -2176,11 +2203,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
@@ -2346,18 +2375,19 @@ 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(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) \
@@ -2369,17 +2399,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;
@@ -2455,7 +2485,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;
}
@@ -2466,7 +2496,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) {
@@ -2646,7 +2676,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));
}
@@ -2945,7 +2975,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;
@@ -2984,8 +3013,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 */
@@ -2997,7 +3025,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;
@@ -3017,7 +3044,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;
@@ -3039,8 +3065,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))"));
@@ -3060,7 +3085,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)"));
@@ -3088,7 +3113,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);
@@ -3105,7 +3129,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;
@@ -3122,9 +3145,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)"));
@@ -3147,7 +3168,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
