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

[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 <pspacek at 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 <pspacek at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0047-2-Avoid-manual-connection-management-outside-ldap_quer.patch
Type: text/x-patch
Size: 14878 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120828/711f67a4/attachment.bin>