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 <pspa...@redhat.com>
> 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 <pspa...@redhat.com>
> ---
>  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
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to