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
>
--
Adam Tkac, Red Hat, Inc.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
