Re: [Freeipa-devel] [PATCH 0047] Avoid manual connection management outside ldap_query()

2012-08-28 Thread Petr Spacek 

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




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

Re: [Freeipa-devel] [PATCH 0047] Avoid manual connection management outside ldap_query()

2012-08-28 Thread Petr Spacek 

On 08/28/2012 09:57 AM, Adam Tkac wrote:

On Tue, Aug 28, 2012 at 08:51:31AM +0200, Petr Spacek wrote:

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.



...


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.


You are right, sorry for false alarm.

Ack.

Regards, Adam


Pushed to master:
https://fedorahosted.org/bind-dyndb-ldap/changeset/8d77eac3ac3d601c043566c83ebaca581e7591fe

--
Petr^2 Spacek

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0047] Avoid manual connection management outside ldap_query()

2012-08-22 Thread Adam Tkac 
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: