[Freeipa-devel] [PATCH 0032-0035] Add support for MODDN operation to persistent search implementation

[Petr Spacek]

On 07/19/2012 02:09 PM, Adam Tkac wrote:
> On Thu, Jul 19, 2012 at 01:59:01PM +0200, Petr Spacek wrote:
>> Hello,
>>
>> I have to explain my motivation behind INSIST a bit. Please see comments below.
>>
>> On 07/19/2012 01:43 PM, Adam Tkac wrote:
>>>> On Wed, Jul 18, 2012 at 01:32:10PM +0200, Petr Spacek wrote:
>>>> +	CHECK(ldap_query(inst, conn, &ldap_qresult_zone, pevent->dn,
>>>>> +			 LDAP_SCOPE_BASE, attrs_zone, 0,
>>>>>   			 "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
>> This LDAP query has search scope LDAP_SCOPE_BASE. If I understood
>> LDAP correctly, it can return 0 or 1 result, but no more. (Two
>> objects can't have exactly same DN.)
>>
>> If specified LDAP query returned (or is believed to returned) more
>> than single result, then something went terribly wrong (memory
>> corruption/incorrect pointer arithmetic?).
>>
>> Theoretically this situation should never happen and I can remove
>> this check completely, if you want.
>>
>>>>>
>>>>> -	for (entry = HEAD(ldap_qresult->ldap_entries);
>>>>> -             entry != NULL;
>>>>> -             entry = NEXT(entry, link)) {
>>>>> +	for (entry_zone = HEAD(ldap_qresult_zone->ldap_entries);
>>>>> +			entry_zone != NULL;
>>>>> +			entry_zone = NEXT(entry_zone, link)) {
>>>>>   		delete = ISC_FALSE;
>>>>>   		result = ldap_parse_zoneentry(entry, inst);
>>>>>   		if (result != ISC_R_SUCCESS)
>>>>>   			goto cleanup;
>>>>> +
>>>>> +		if (PSEARCH_MODDN(pevent->chgtype)) {
>>>>> +			if (dn_to_dnsname(inst->mctx, pevent->prevdn, &prevname, NULL)
>>>>> +					== ISC_R_SUCCESS) {
>>>>> +				CHECK(ldap_delete_zone(inst, pevent->prevdn, ISC_TRUE));
>>>>> +			} else {
>>>>> +				log_debug(5, "update_action: old zone wasn't managed "
>>>>> +						"by plugin, dn '%s'", pevent->prevdn);
>>>>> +			}
>>>>> +
>>>>> +			/* fill the cache with records from renamed zone */
>>>>> +			CHECK(ldap_query(inst, conn, &ldap_qresult_record, pevent->dn,
>>>>> +					LDAP_SCOPE_ONELEVEL, attrs_record, 0,
>>>>> +					"(objectClass=idnsRecord)"));
>>>>> +
>>>>> +			/* LDAP schema requires SOA record (at least) */
>>>>> +			INSIST(HEAD(ldap_qresult_record->ldap_entries) != NULL);
>>>>> +			for (entry_record = HEAD(ldap_qresult_record->ldap_entries);
>>>>> +					entry_record != NULL;
>>>>> +					entry_record = NEXT(entry_record, link)) {
>>>>> +
>>>>> +				psearch_update(inst, entry_record, NULL);
>>>>> +			}
>>>>> +		}
>>>>> +
>>>>> +		INSIST(NEXT(entry_zone, link) == NULL); /* no multiple zones with same DN */
>>> This INSIST seems like too strong for me. Imagine situation when administrator
>>> wrongly modifies LDAP database and creates duplicated zone. In this case we will
>> AFAIK it is not possible, because idnsName attribute is part of DN
>> and duplicate DN is not allowed.
>
> Ok, this makes sence. So push the patchset as is, thank you.
>
> Regards, Adam
>
>>> crash. I would prefer to log error here, that multiple zones exist and only the
>>> first occurence is used.
>>>
>>>>>   	}
>>
>> Petr^2 Spacek
>>
>
Pushed to master:

     ​ 
http://git.fedorahosted.org/git?p=bind-dyndb-ldap.git;a=commitdiff;h=2d9e71d47997cd75635412cd81349692a8cac1c2
     ​ 
http://git.fedorahosted.org/git?p=bind-dyndb-ldap.git;a=commitdiff;h=16c402e39e467731422b27a6247e0e222e36586c
     ​ 
http://git.fedorahosted.org/git?p=bind-dyndb-ldap.git;a=commitdiff;h=4083460acbdce1760aa347ec68abd27d25e1059a
     ​ 
http://git.fedorahosted.org/git?p=bind-dyndb-ldap.git;a=commitdiff;h=6f7fd9c9ed9b9c78c1034972f903e8d41de902a8 



Petr^2 Spacek