[Freeipa-devel] [PATCH 0192-0196] Write all changes to journal

[Petr Spacek]

On 23.10.2013 17:20, Petr Spacek wrote:
> On 23.10.2013 17:12, Tomas Hozza wrote:
>> On 10/10/2013 07:05 PM, Petr Spacek wrote:
>>> Hello,
>>>
>>> this patch set adds journaling to bind-dyndb-ldap.
>>>
>>> Journaling requires proper SOA serial maintenance, so from now
>>> SOA serial auto-incrementation is mandatory.
>>>
>>> Journal file is deleted on each start, so IXFR is limited to time frame
>>> from last BIND start.
>>>
>>> https://fedorahosted.org/bind-dyndb-ldap/ticket/64
>>>
>>>
>>> You can use my personal branch for testing:
>>> https://github.com/spacekpe/bind-dyndb-ldap/tree/rbtdb.v7
>>>
>>> It contains all unpushed patches. Basically, next master should be identical
>>> to this branch.
>>>
>>> Thank you for your time!
>>>
>>> -- Petr^2 Spacek
>>
>> ACK.
>>
>> IXFR works as should. Also tested that journal is cleared after BIND
>> restart, so server responds with AXFR.
>>
>> Patches look good, I have only one small thing to patch 0193:
>>
>>> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
>>> index
>>> 0786979a1970f4b61ac5b92dd5554bf87032d1ff..89acbe610519bbe0610a07d5fa5d4ffceddc71cd
>>> 100644
>>>
>>> --- a/src/ldap_helper.c
>>>
>>> +++ b/src/ldap_helper.c
>>>
>>> ...
>>>
>>> @@ -1401,7 +1405,155 @@
>>>
>>> cleanup:
>>> return result;
>>> }
>>> +/**
>>> + * Process strictly minimal diff and detect if data were changed
>>> + * and return latest SOA RR.
>>> + *
>>> + * @pre Input diff has to be minimal, i.e. it can't contain DEL & ADD
>>> operation
>>> + * for the same data under the same name and TTL.
>>> + *
>>> + * @pre If the tuple list contains SOA RR, then exactly one SOA RR
>>> deletion
>>> + * has to precede exactly one SOA RR addition.
>>> + * (Each SOA RR deletion has to have matching addition.)
>>> + *
>>> + * @param[in] diff Input diff. List of tuples can be empty.
>>> + * @param[out] soa_latest Pointer to last added SOA RR from tuple list.
>>> + * Result can be NULL if there is no added SOA RR
>>> + * in the tuple list.
>>> + * @param[out] data_changed ISC_TRUE if any data other than SOA serial
>>> were
>>> + * changed. ISC_FALSE if nothing (except SOA
>>> + * serial) was changed.
>>> + *
>>> + */
>>> +static isc_result_t ATTR_NONNULLS
>>> +diff_analyze_serial(dns_diff_t *diff, dns_difftuple_t **soa_latest,
>>> + isc_boolean_t *data_changed) {
>>> + dns_difftuple_t *t = NULL;
>>> + dns_rdata_t *del_soa = NULL; /* last seen SOA with op == DEL */
>>> + dns_difftuple_t *tmp_tuple = NULL; /* tuple used for SOA comparison */
>>> + isc_result_t result = ISC_R_SUCCESS;
>>> + int ret;
>>> +
>>> + REQUIRE(DNS_DIFF_VALID(diff));
>>> + REQUIRE(soa_latest != NULL && *soa_latest == NULL);
>>> + REQUIRE(data_changed != NULL);
>>> +
>>> + *data_changed = ISC_FALSE;
>>> + for (t = HEAD(diff->tuples);
>>> + t != NULL;
>>> + t = NEXT(t, link)) {
>>> + INSIST(tmp_tuple == NULL);
>> after this ^^^ line tmp_tuple is NULL in the current for loop cycle.
>>> + if (t->rdata.type != dns_rdatatype_soa)
>>> + *data_changed = ISC_TRUE;
>>> + else { /* SOA is always special case */
>>> + if (t->op == DNS_DIFFOP_DEL ||
>>> + t->op == DNS_DIFFOP_DELRESIGN) {
>>> + /* delete operation has to precede add */
>>> + INSIST(del_soa == NULL);
>>> + INSIST(tmp_tuple == NULL);
>> so this ^^^ check is duplicit
>>> + del_soa = &t->rdata;
>>> + } else if (t->op == DNS_DIFFOP_ADD ||
>>> + t->op == DNS_DIFFOP_ADDRESIGN) {
>>> + /* add operation has to follow a delete */
>>> + INSIST(del_soa != NULL);
>>> + INSIST(tmp_tuple == NULL);
>> and also this ^^^ check is duplicit
>>> + *soa_latest = t;
>>> +
>>> + /* detect if fields other than serial
>>> + * were changed (compute only if necessary) */
>>> + if (*data_changed == ISC_FALSE) {
>>> + CHECK(dns_difftuple_copy(t, &tmp_tuple));
>>> + dns_soa_setserial(dns_soa_getserial(del_soa),
>>> + &tmp_tuple->rdata);
>>> + ret = dns_rdata_compare(del_soa,
>>> + &tmp_tuple->rdata);
>>> + *data_changed = ISC_TF(ret != 0);
>>> + }
>>> + if (tmp_tuple != NULL)
>>> + dns_difftuple_free(&tmp_tuple);
>>> + /* re-start the SOA delete-add search cycle */
>>> + del_soa = NULL;
>>> + } else {
>>> + INSIST("unexpected diff: op != ADD || DEL"
>>> + == NULL);
>>> + }
>>> + }
>>> + }
>>> + /* SOA deletions & additions has to create self-contained couples */
>>> + INSIST(del_soa == NULL && tmp_tuple == NULL);
>>> +
>>> +cleanup:
>>> + if (tmp_tuple != NULL)
>>> + dns_difftuple_free(&tmp_tuple);
>>> + return result;
>>> +}
>>> +
>>> ...
>>
>> Sorry for the formatting, obviously my email client is not perfect.
>> Hope you can understand it...
>
> I will fix it before push.

Patch 192 was unchanged.

Patch 193 v2
- redundant INSIST calls were removed
- patch was rebased

Patch 194 v2
- copyright header was fixed

Patch 196 v2
- README was updated
- missing header dns/journal.h was added
- fixes in error handling: journal is correctly closed as necessary
- patch was rebased

-- 
Petr^2 Spacek

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0194-2-Create-working-directory-for-each-LDAP-instance.patch
Type: text/x-patch
Size: 7065 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131213/35c45b7e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0193-2-Rework-SOA-serial-auto-incrementation-and-make-it-ma.patch
Type: text/x-patch
Size: 20305 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131213/35c45b7e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0196-2-Record-all-changes-in-DNS-zone-to-journal.patch
Type: text/x-patch
Size: 13688 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131213/35c45b7e/attachment-0002.bin>