On Mon, May 14, 2012 at 04:44:42PM +0200, Petr Spacek wrote:
> On 05/11/2012 12:26 PM, Adam Tkac wrote:
> >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<pspa...@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<pspa...@redhat.com>
> 
> Hello Adam,
> 
> thanks for ideas/improvements!
> 
> Reworked patch is attached. I did all proposed changes except this one:
> 
> @ ldap_psearch_watcher:
> >>  restart:
> (... snip ...)
> >>  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;
> 
> I think "continue" in this place can lead to memory leak, so I
> removed soft_err by other way.

Thanks for the patch, now it looks fine to me, except that it doesn't apply on
the current master:

[atkac@drtic bind-dyndb-ldap]$ git am 
../bind-dyndb-ldap-pspacek-0020-2-Separate-LDAP-result-from-LDAP-connection-fix-deadlo.patch
Applying: Separate LDAP result from LDAP connection, fix deadlock. 
https://fedorahosted.org/bind-dyndb-ldap/ticket/66 Signed-off-by: Petr Spacek 
<pspa...@redhat.com>
error: patch failed: src/ldap_helper.c:271
error: src/ldap_helper.c: patch does not apply
Patch failed at 0001 Separate LDAP result from LDAP connection, fix deadlock. 
https://fedorahosted.org/bind-dyndb-ldap/ticket/66 Signed-off-by: Petr Spacek 
<pspa...@redhat.com>
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

Please rebase the patch and then push it, you don't have to resend it here.

Regards, Adam

> From 4c8c8c1ec7777217d3219a0380f6baec4b41248d Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspa...@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 <pspa...@redhat.com>
> 
> ---
>  src/ldap_helper.c |  269 
> ++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 162 insertions(+), 107 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 
> 304c37296f8f3a428c4c72b45fe4154b2c9e954c..dae3e28804b602ca91d813c4d68d258e7065608f
>  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_qresultp, const char *base, int scope, 
> char **attrs,
> +                int attrsonly, const char *filter, ...);
> +static isc_result_t ldap_query_create(isc_mem_t *mctx, ldap_qresult_t 
> **ldap_qresultp);
> +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,
> @@ -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 for configuration and zones from LDAP and release LDAP 
> connection
> +      * 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(ISC_FALSE, &ldap_config_qresult);
> +     ldap_query_free(ISC_FALSE, &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(ISC_FALSE, &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(ISC_FALSE, &ldap_qresult);
>       ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
>       str_destroy(&string);
>  
> @@ -1601,16 +1611,24 @@ 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_qresultp, const char *base, int scope, char 
> **attrs,
>          int attrsonly, const char *filter, ...)
>  {
>       va_list ap;
>       isc_result_t result;
> +     ldap_qresult_t *ldap_qresult = NULL;
>       int cnt;
>  
>       REQUIRE(ldap_conn != NULL);
> +     REQUIRE(ldap_qresultp != NULL && *ldap_qresultp == NULL);
>  
>       va_start(ap, filter);
>       str_vsprintf(ldap_conn->query_string, filter, ap);
> @@ -1629,62 +1647,96 @@ ldap_query(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>                       return result;
>       }
>  
> +     CHECK(ldap_query_create(ldap_inst->mctx, &ldap_qresult));
> +
>       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;
> +                     break;
>               }
> +             /* Special case is LDAP_NO_SUCH_OBJECT: 
> handle_connection_error() will
> +              * set result = ISC_R_SUCCESS and break the cycle. */
>       } while (handle_connection_error(ldap_inst, ldap_conn, ISC_FALSE, 
> &result));
>  
> +cleanup:
> +     if (result == ISC_R_SUCCESS)
> +             *ldap_qresultp = ldap_qresult;
> +     else
> +             ldap_query_free(ISC_FALSE, &ldap_qresult);
>       return result;
>  }
>  
> +/**
> + * Allocate and initialize new ldap_qresult structure.
> + * @param[out] ldap_qresultp Newly allocated ldap_qresult structure.
> + * @return ISC_R_SUCCESS or ISC_R_NOMEMORY (from CHECKED_MEM_GET_PTR)
> + */
> +static isc_result_t
> +ldap_query_create(isc_mem_t *mctx, ldap_qresult_t **ldap_qresultp) {
> +     ldap_qresult_t *ldap_qresult = NULL;
> +     isc_result_t result;
> +
> +     CHECKED_MEM_GET_PTR(mctx, ldap_qresult);
> +     ldap_qresult->mctx = mctx;
> +     ldap_qresult->result = NULL;
> +     INIT_LIST(ldap_qresult->ldap_entries);
> +
> +     *ldap_qresultp = ldap_qresult;
> +     return ISC_R_SUCCESS;
> +
> +cleanup:
> +     return result;
> +}
> +
> +/**
> + * Free LDAP query result. Can free whole structure or internal parts only.
> + * Freeing internal parts is suitable before reusing the structure.
> + * @param[in] prepare_reuse ISC_TRUE implies freeing internal parts,
> + *                          but not the whole structure.
> + * @param[in,out] ldap_qresultp Pointer to freed query. Will be set to NULL
> + *                              if prepare_reuse == ISC_FALSE.
> + */
>  static void
> -ldap_query_free(ldap_connection_t *ldap_conn)
> +ldap_query_free(isc_boolean_t prepare_reuse, ldap_qresult_t **ldap_qresultp)
>  {
> -     if (ldap_conn == NULL)
> +     ldap_qresult_t *qresult;
> +     REQUIRE(ldap_qresultp != NULL);
> +
> +     qresult = *ldap_qresultp;
> +
> +     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);
> +
> +     if (prepare_reuse) {
> +             INIT_LIST(qresult->ldap_entries);
> +     } else { /* free whole structure */
> +             SAFE_MEM_PUT_PTR(qresult->mctx, qresult);
> +             *ldap_qresultp = NULL;
> +     }
>  }
>  
>  /* FIXME: Tested with SASL/GSSAPI/KRB5 only */
> @@ -2229,6 +2281,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 +2308,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 +2445,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(ISC_FALSE, &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 +2538,7 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t 
> *ldap_inst,
>       }
>       
>  cleanup:
> +     ldap_query_free(ISC_FALSE, &ldap_qresult);
>       ldap_pool_putconnection(ldap_inst->pool, &ldap_conn);
>       str_destroy(&owner_dn_ptr);
>       str_destroy(&owner_dn);
> @@ -2587,7 +2641,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 +2660,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 +2791,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 +2810,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 +2832,7 @@ cleanup:
>                         "Zones can be outdated, run `rndc reload`",
>                         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);
> @@ -2793,6 +2847,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 +2865,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 +2887,7 @@ cleanup:
>                         "Configuration can be outdated, run `rndc reload`",
>                         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);
> @@ -3071,6 +3127,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 +3165,8 @@ ldap_psearch_watcher(isc_threadarg_t arg)
>               ldap_connect(inst, conn, ISC_TRUE);
>       }
>  
> +     CHECK(ldap_query_create(conn->mctx, &ldap_qresult));
> +
>  restart:
>       /* Perform initial lookup */
>       if (inst->psearch) {
> @@ -3134,7 +3193,7 @@ restart:
>  
>       while (!inst->exiting) {
>               ret = ldap_result(conn->handle, conn->msgid, 0, &tv,
> -                               &conn->result);
> +                               &ldap_qresult->result);
>  
>               if (ret <= 0) {
>                       int ok;
> @@ -3153,58 +3212,54 @@ restart:
>               case LDAP_RES_SEARCH_ENTRY:
>                       break;
>               default:
> -                     log_debug(3, "Ignoring psearch msg with retcode %x",
> -                               ret);
> +                     log_debug(3, "Ignoring psearch msg with retcode %x", 
> ret);
> +                     break;
>               }
>  
>               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);
> -                     if (result != ISC_R_SUCCESS) {
> +                                                    ldap_qresult->result,
> +                                                    
> &ldap_qresult->ldap_entries);
> +
> +                     if (result == ISC_R_SUCCESS) {
> +                             ldap_entry_t *entry;
> +                             for (entry = HEAD(ldap_qresult->ldap_entries);
> +                                      entry != NULL;
> +                                      entry = NEXT(entry, link)) {
> +                                     LDAPControl **ctrls = NULL;
> +                                     ret = 
> ldap_get_entry_controls(conn->handle,
> +                                                                       
> entry->ldap_entry,
> +                                                                       
> &ctrls);
> +                                     if (ret != LDAP_SUCCESS) {
> +                                             log_error("failed to extract 
> controls "
> +                                                       "from psearch update. 
> Zones "
> +                                                       "might be outdated, 
> run "
> +                                                       "`rndc reload`");
> +                                             break;
> +                                     }
> +                                     psearch_update(inst, entry, ctrls);
> +                             }
> +                     } else { /* result != ISC_R_SUCCESS */
>                               /*
>                                * Error means inconsistency of our zones
>                                * data.
>                                */
>                               log_error("ldap_psearch_watcher failed, zones "
>                                         "might be outdated. Run `rndc 
> reload`");
> -                             goto soft_err;
>                       }
> -
> -                     ldap_entry_t *entry;
> -                     for (entry = HEAD(conn->ldap_entries);
> -                          entry != NULL;
> -                          entry = NEXT(entry, link)) {
> -                             LDAPControl **ctrls = NULL;
> -                             ret = ldap_get_entry_controls(conn->handle,
> -                                                           entry->ldap_entry,
> -                                                           &ctrls);
> -                             if (ret != LDAP_SUCCESS) {
> -                                     log_error("failed to extract controls "
> -                                               "from psearch update. Zones "
> -                                               "might be outdated, run "
> -                                               "`rndc reload");
> -                                     goto soft_err;
> -                             }
> -
> -                             psearch_update(inst, entry, ctrls);
> -                     }
> -soft_err:
> -
> -                     ldap_msgfree(conn->result);
> -                     ldap_entrylist_destroy(conn->mctx,
> -                                            &conn->ldap_entries);
>               }
> +             ldap_query_free(ISC_TRUE, &ldap_qresult);
>       }
>  
>       log_debug(1, "Ending ldap_psearch_watcher");
>  
>  cleanup:
> +     ldap_query_free(ISC_FALSE, &ldap_qresult);
>       ldap_pool_putconnection(inst->pool, &conn);
>  
>       return (isc_threadresult_t)0;
> -- 
> 1.7.7.6
> 


-- 
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