[Freeipa-devel] [PATCH 0046] Separate RR data parsing from LDAP connections
Petr Spacek
pspacek at redhat.com
Wed Aug 15 14:04:26 UTC 2012
On 08/15/2012 03:31 PM, Adam Tkac wrote:
> On Wed, Aug 01, 2012 at 04:19:11PM +0200, Petr Spacek wrote:
>> Hello,
>>
>> this patch finishes LDAP connection vs. LDAP result separation.
>>
>> It is first step necessary for:
>> https://fedorahosted.org/bind-dyndb-ldap/ticket/68
>> Avoid manual connection management outside ldap_query()
>>
>> It should prevent deadlocks in future.
>>
>> Petr^2 Spacek
>
> Thanks for the patch, please check one comment below.
>
> Regards, Adam
>
>> From 4ba44be9e9bb7ef5abc9e077d6620de496ae7c0d Mon Sep 17 00:00:00 2001
>> From: Petr Spacek <pspacek at redhat.com>
>> Date: Tue, 31 Jul 2012 14:33:53 +0200
>> Subject: [PATCH] Separate RR data parsing from LDAP connections.
>>
>> Signed-off-by: Petr Spacek <pspacek at redhat.com>
>> ---
>> src/ldap_helper.c | 76 ++++++++++++++++++++++++++++++-------------------------
>> 1 file changed, 42 insertions(+), 34 deletions(-)
>>
>> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
>> index cc7003a6cdcd2d27404fec936623ed8a3e8fa7f8..af0ec092d29bce170d5c2dfa8836a728440116a3 100644
>> --- a/src/ldap_helper.c
>> +++ b/src/ldap_helper.c
>> @@ -196,11 +196,6 @@ struct ldap_connection {
>> 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;
>> -
>> /* For reconnection logic. */
>> isc_time_t next_reconnect;
>> unsigned int tries;
>> @@ -214,6 +209,11 @@ struct ldap_qresult {
>> ld_string_t *query_string;
>> LDAPMessage *result;
>> ldap_entrylist_t ldap_entries;
>> +
>> + /* Parsing. */
>> + isc_lex_t *lex;
>> + isc_buffer_t rdata_target;
>> + unsigned char *rdata_target_mem;
>> };
>>
>> /*
>> @@ -256,15 +256,15 @@ static void destroy_ldap_connection(ldap_pool_t *pool,
>> static isc_result_t findrdatatype_or_create(isc_mem_t *mctx,
>> ldapdb_rdatalist_t *rdatalist, dns_rdataclass_t rdclass,
>> dns_rdatatype_t rdtype, dns_ttl_t ttl, dns_rdatalist_t **rdlistp);
>> -static isc_result_t add_soa_record(isc_mem_t *mctx, ldap_connection_t *ldap_conn,
>> +static isc_result_t add_soa_record(isc_mem_t *mctx, ldap_qresult_t *qresult,
>> dns_name_t *origin, ldap_entry_t *entry,
>> ldapdb_rdatalist_t *rdatalist, const ld_string_t *fake_mname);
>> -static isc_result_t parse_rdata(isc_mem_t *mctx, ldap_connection_t *ldap_conn,
>> +static isc_result_t parse_rdata(isc_mem_t *mctx, ldap_qresult_t *qresult,
>> dns_rdataclass_t rdclass, dns_rdatatype_t rdtype,
>> dns_name_t *origin, const char *rdata_text,
>> dns_rdata_t **rdatap);
>> static isc_result_t ldap_parse_rrentry(isc_mem_t *mctx, ldap_entry_t *entry,
>> - ldap_connection_t *conn, dns_name_t *origin,
>> + ldap_qresult_t *qresult, dns_name_t *origin,
>> const ld_string_t *fake_mname, ld_string_t *buf,
>> ldapdb_rdatalist_t *rdatalist);
>> static inline isc_result_t ldap_get_zone_serial(ldap_instance_t *inst,
>> @@ -637,8 +637,6 @@ new_ldap_connection(ldap_pool_t *pool, ldap_connection_t **ldap_connp)
>>
>> isc_mem_attach(pool->mctx, &ldap_conn->mctx);
>>
>> - CHECK(isc_lex_create(ldap_conn->mctx, TOKENSIZ, &ldap_conn->lex));
>> - CHECKED_MEM_GET(ldap_conn->mctx, ldap_conn->rdata_target_mem, MINTSIZ);
>> CHECK(ldap_pscontrol_create(ldap_conn->mctx,
>> &ldap_conn->serverctrls[0]));
>>
>> @@ -667,12 +665,6 @@ destroy_ldap_connection(ldap_pool_t *pool, ldap_connection_t **ldap_connp)
>> if (ldap_conn->handle != NULL)
>> ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
>>
>> - if (ldap_conn->lex != NULL)
>> - isc_lex_destroy(&ldap_conn->lex);
>> - if (ldap_conn->rdata_target_mem != NULL) {
>> - isc_mem_put(ldap_conn->mctx,
>> - ldap_conn->rdata_target_mem, MINTSIZ);
>> - }
>> if (ldap_conn->serverctrls[0] != NULL) {
>> ldap_pscontrol_destroy(ldap_conn->mctx,
>> &ldap_conn->serverctrls[0]);
>> @@ -1431,7 +1423,7 @@ free_rdatalist(isc_mem_t *mctx, dns_rdatalist_t *rdlist)
>>
>> static isc_result_t
>> ldap_parse_rrentry(isc_mem_t *mctx, ldap_entry_t *entry,
>> - ldap_connection_t *conn, dns_name_t *origin,
>> + ldap_qresult_t *qresult, dns_name_t *origin,
>> const ld_string_t *fake_mname, ld_string_t *buf,
>> ldapdb_rdatalist_t *rdatalist)
>> {
>> @@ -1443,7 +1435,7 @@ ldap_parse_rrentry(isc_mem_t *mctx, ldap_entry_t *entry,
>> dns_rdatalist_t *rdlist = NULL;
>> ldap_attribute_t *attr;
>>
>> - result = add_soa_record(mctx, conn, origin, entry,
>> + result = add_soa_record(mctx, qresult, origin, entry,
>> rdatalist, fake_mname);
>> if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND)
>> goto cleanup;
>> @@ -1458,7 +1450,7 @@ ldap_parse_rrentry(isc_mem_t *mctx, ldap_entry_t *entry,
>> CHECK(findrdatatype_or_create(mctx, rdatalist, rdclass,
>> rdtype, ttl, &rdlist));
>> while (ldap_attr_nextvalue(attr, buf) != NULL) {
>> - CHECK(parse_rdata(mctx, conn, rdclass,
>> + CHECK(parse_rdata(mctx, qresult, rdclass,
>> rdtype, origin,
>> str_buf(buf), &rdata));
>> APPEND(rdlist->rdata, rdata, link);
>> @@ -1518,7 +1510,7 @@ ldapdb_nodelist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *nam
>> result = ldapdbnode_create(mctx, &node_name, &node);
>> dns_name_free(&node_name, mctx);
>> if (result == ISC_R_SUCCESS) {
>> - result = ldap_parse_rrentry(mctx, entry, ldap_conn,
>> + result = ldap_parse_rrentry(mctx, entry, ldap_qresult,
>> origin, ldap_inst->fake_mname,
>> string, &node->rdatalist);
>> }
>> @@ -1584,7 +1576,7 @@ ldapdb_rdatalist_get(isc_mem_t *mctx, ldap_instance_t *ldap_inst, dns_name_t *na
>> for (entry = HEAD(ldap_qresult->ldap_entries);
>> entry != NULL;
>> entry = NEXT(entry, link)) {
>> - if (ldap_parse_rrentry(mctx, entry, ldap_conn,
>> + if (ldap_parse_rrentry(mctx, entry, ldap_qresult,
>> origin, ldap_inst->fake_mname,
>> string, rdatalist) != ISC_R_SUCCESS ) {
>> log_error("Failed to parse RR entry (%s)", str_buf(string));
>> @@ -1610,7 +1602,7 @@ cleanup:
>> }
>>
>> static isc_result_t
>> -add_soa_record(isc_mem_t *mctx, ldap_connection_t *ldap_conn, dns_name_t *origin,
>> +add_soa_record(isc_mem_t *mctx, ldap_qresult_t *qresult, dns_name_t *origin,
>> ldap_entry_t *entry, ldapdb_rdatalist_t *rdatalist,
>> const ld_string_t *fake_mname)
>> {
>> @@ -1624,7 +1616,7 @@ add_soa_record(isc_mem_t *mctx, ldap_connection_t *ldap_conn, dns_name_t *origin
>>
>> CHECK(ldap_entry_getfakesoa(entry, fake_mname, string));
>> rdclass = ldap_entry_getrdclass(entry);
>> - CHECK(parse_rdata(mctx, ldap_conn, rdclass, dns_rdatatype_soa, origin,
>> + CHECK(parse_rdata(mctx, qresult, rdclass, dns_rdatatype_soa, origin,
>> str_buf(string), &rdata));
>>
>> CHECK(findrdatatype_or_create(mctx, rdatalist, rdclass, dns_rdatatype_soa,
>> @@ -1641,17 +1633,17 @@ cleanup:
>> }
>>
>> static isc_result_t
>> -parse_rdata(isc_mem_t *mctx, ldap_connection_t *ldap_conn,
>> +parse_rdata(isc_mem_t *mctx, ldap_qresult_t *qresult,
>> dns_rdataclass_t rdclass, dns_rdatatype_t rdtype,
>> dns_name_t *origin, const char *rdata_text, dns_rdata_t **rdatap)
>> {
>> isc_result_t result;
>> isc_consttextregion_t text;
>> isc_buffer_t lex_buffer;
>> isc_region_t rdatamem;
>> dns_rdata_t *rdata;
>>
>> - REQUIRE(ldap_conn != NULL);
>> + REQUIRE(qresult != NULL);
>> REQUIRE(rdata_text != NULL);
>> REQUIRE(rdatap != NULL);
>>
>> @@ -1665,30 +1657,30 @@ parse_rdata(isc_mem_t *mctx, ldap_connection_t *ldap_conn,
>> isc_buffer_add(&lex_buffer, text.length);
>> isc_buffer_setactive(&lex_buffer, text.length);
>>
>> - CHECK(isc_lex_openbuffer(ldap_conn->lex, &lex_buffer));
>> + CHECK(isc_lex_openbuffer(qresult->lex, &lex_buffer));
>>
>> - isc_buffer_init(&ldap_conn->rdata_target, ldap_conn->rdata_target_mem,
>> + isc_buffer_init(&qresult->rdata_target, qresult->rdata_target_mem,
>> MINTSIZ);
>> - CHECK(dns_rdata_fromtext(NULL, rdclass, rdtype, ldap_conn->lex, origin,
>> - 0, mctx, &ldap_conn->rdata_target, NULL));
>> + CHECK(dns_rdata_fromtext(NULL, rdclass, rdtype, qresult->lex, origin,
>> + 0, mctx, &qresult->rdata_target, NULL));
>>
>> CHECKED_MEM_GET_PTR(mctx, rdata);
>> dns_rdata_init(rdata);
>>
>> - rdatamem.length = isc_buffer_usedlength(&ldap_conn->rdata_target);
>> + rdatamem.length = isc_buffer_usedlength(&qresult->rdata_target);
>> CHECKED_MEM_GET(mctx, rdatamem.base, rdatamem.length);
>>
>> - memcpy(rdatamem.base, isc_buffer_base(&ldap_conn->rdata_target),
>> + memcpy(rdatamem.base, isc_buffer_base(&qresult->rdata_target),
>> rdatamem.length);
>> dns_rdata_fromregion(rdata, rdclass, rdtype, &rdatamem);
>>
>> - isc_lex_close(ldap_conn->lex);
>> + isc_lex_close(qresult->lex);
>>
>> *rdatap = rdata;
>> return ISC_R_SUCCESS;
>>
>> cleanup:
>> - isc_lex_close(ldap_conn->lex);
>> + isc_lex_close(qresult->lex);
>> if (rdata != NULL)
>> isc_mem_put(mctx, rdata, sizeof(*rdata));
>> if (rdatamem.base != NULL)
>> @@ -1793,14 +1785,25 @@ ldap_query_create(isc_mem_t *mctx, ldap_qresult_t **ldap_qresultp) {
>> ldap_qresult->mctx = mctx;
>> ldap_qresult->result = NULL;
>> ldap_qresult->query_string = NULL;
>> + ldap_qresult->lex = NULL;
>
> I recommend to use ZERO_PTR(ldap_qresult) instead of many "= NULL" assignments.
> It's safer and used in other *_create functions.
Amended patch is attached.
Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0046-2-Separate-RR-data-parsing-from-LDAP-connections.patch
Type: text/x-patch
Size: 9569 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120815/438ae231/attachment.bin>
More information about the Freeipa-devel
mailing list