[Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin

[Martin Kosek]

On 10/09/2014 06:48 PM, thierry bordaz wrote:
> On 10/09/2014 05:51 PM, Nathaniel McCallum wrote:
>> On Thu, 2014-10-09 at 11:44 +0200, thierry bordaz wrote:
>>> On 10/09/2014 12:15 AM, Nathaniel McCallum wrote:
>>>
>>>> On Wed, 2014-10-08 at 17:19 -0400, Simo Sorce wrote:
>>>>> On Wed, 08 Oct 2014 15:53:39 -0400
>>>>> Nathaniel McCallum<npmccallum at redhat.com>  wrote:
>>>>>
>>>>>> As I understand my code, all servers will have csnD. Some servers will
>>>>>> have valueB and others will have valueD, but valueB == valueD.
>>>>>>
>>>>>> We *never* discard a CSN. We only discard the counter/watermark mods
>>>>>> in the replication operation.
>>>>> What Thierry is saying is that the individual attributes in the entry
>>>>> have associate the last CSN that modified them. Because you remove the
>>>>> mods when ValueD == ValueB the counter attribute will not have the
>>>>> associated CSN changed. But it doesn't really matter because the plugin
>>>>> will always keep things consistent.
>>>> Attached is a new version. It removes this optimization. If server X has
>>>> valueB/csnB and receives valueD/csnD and valueB == valueD, the
>>>> replication will be applied without any modification. However, if valueB
>>>>> valueD and csnD > csnB, the counter mods will still be stripped.
>>>> It also collapses the error check from betxnpre to bepre now that we
>>>> have a fix forhttps://fedorahosted.org/389/ticket/47919  committed. The
>>>> betxnpre functions are completely removed. Also, a dependency on 389
>>>> 1.3.3.4 (not yet released) is added.
>>>>
>>>> Nathaniel
>>> Hello Nathaniel,
>>>
>>>          For me the code is fine. Ack.
>> New attached patch.
>>
>>>          I have two minor comments:
>>>                * in preop_mod, when a direct update moves the counter
>>>                  backward you send UNWILLING to perform with a message.
>>>                  The message is allocated with slapi_ch_smprintf, you
>>>                  may free it with slapi_ch_free_string (rather than
>>>                  'free').
>> Fixed.
>>
>>>                * About this message, for example when you have these
>>>                  MODS (I admit they make no sens):
>>>
>>>                  changetype: modify
>>>                  ipatokenHOTPcounter: MOD_DELETE
>>>                  -
>>>                  ipatokenHOTPcounter: MOD_INCREMENT
>>>
>>>                  The returned message will be "Will not decrement
>>>                  ipatokenHOTPcounter", because 'simulate' will return
>>>                  'COUNTER_UNSET+1'.
>>>                  Is it the message you expected ?
>> I changed the logic in simulate(). Please review it.
>>
>> Nathaniel
>>
> Hello Nathaniel,
>
>
>     The patch is ok for me. Ack.
>
>     Thank you
>     thierry

Great! Thanks for tough review. However, we will still need to wait for 389 to 
release so that we can add the new required DS version at least to FreeIPA 
Copr. Otherwise all development/CI would break.

Martin