On 10/17/2014 07:22 PM, Nathaniel McCallum wrote: > On Fri, 2014-10-17 at 12:05 +0200, Martin Kosek wrote: >> On 10/16/2014 11:53 PM, Nathaniel McCallum wrote: >>> On Thu, 2014-10-16 at 21:02 +0200, Martin Kosek wrote: >>>> On 10/15/2014 09:22 AM, Martin Kosek wrote: >>>>> On 10/14/2014 09:01 PM, Nathaniel McCallum wrote: >>>>>> On Thu, 2014-10-09 at 18:48 +0200, 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 <npmccal...@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 for https://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. >>>>>> >>>>>> Since the ACK, the upstream 389 fix actually landed in 1.3.3.5. This >>>>>> patch changes nothing except the dependency version. I have tested it >>>>>> against the 1.3.3.5 build. >>>>>> >>>>>> Nathaniel >>>>> >>>>> Great! As soon as the new build land in Fedora 21 (and we add it to our >>>>> Copr), >>>>> the patch can be pushed. >>>>> >>>>> Martin >>>> >>>> Ok, 1.3.3.5 is in koji and our Copr repo. >>> >>> +1 >>> >>>> I almost pushed the patch, but I just >>>> spotted you forgot to solve the upgrades - so NACK. >>> >>> You asked me to do that in another patch, not this one. :) >> >> I know, but that does not change the fact it is still missing :-) >> >>> >>>> "cn=IPA OTP Counter,cn=plugins,cn=config" plugin configuration also needs >>>> to be >>>> added in some update file. >>> >>> So I'm generally confused then. If we have to add the plugin config to >>> an update file, why bother creating >>> daemons/ipa-slapi-plugins/ipa-otp-counter/otp-counter-conf.ldif or >>> modifying ipaserver/install/dsinstance.py at all? Should these changes >>> be removed? >> >> Good question. Most LDAP update related changes are added to update plugins >> only. But plugin registration seemed to be still on 2 places, like 'cn=IPA >> Range-Check,cn=plugins,cn=config' registration. >> >> Up to you/additional developer discussion. > > Fixed. > > Also note that the same problem was present in the OTP Last Token plugin > (committed for 4.0). I have made a patch which fixes this as well: > > https://www.redhat.com/archives/freeipa-devel/2014-October/msg00358.html > > Nathaniel >
Thanks, upgrade part works for me, ACK for that. Pushed to: master: 41bf0ba9403962140a75b28e0b279248ec101a0a ipa-4-1: 2f8dc3b6cc6cdee8230afe50cce694a762010b37 Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
