On Tue, 2014-09-30 at 18:30 +0200, thierry bordaz wrote: > On 09/29/2014 08:30 PM, Nathaniel McCallum wrote: > > > On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote: > > > On Sun, 21 Sep 2014 22:33:47 -0400 > > > Nathaniel McCallum <npmccal...@redhat.com> wrote: > > > > > > Comments inline. > > > > > > > + > > > > +#define ch_malloc(type) \ > > > > + (type*) slapi_ch_malloc(sizeof(type)) > > > > +#define ch_calloc(count, type) \ > > > > + (type*) slapi_ch_calloc(count, sizeof(type)) > > > > +#define ch_free(p) \ > > > > + slapi_ch_free((void**) &(p)) > > > please do not redefine slapi functions, it just makes it harder to know > > > what you used. > > > > > > > > > > +typedef struct { > > > > + bool exists; > > > > + long long value; > > > > +} counter; > > > > > > please no typedefs of structures, use "struct counter { ... };" and > > > reference it as "struct counter" in the code. > > > > > > Btw, FWIW you could use a value of -1 to indicate non-existence of the > > > counter value, given counters can only be positive, this would avoid > > > the need for a struct. > > > > > > > +static int > > > > +send_error(Slapi_PBlock *pb, int rc, char *template, ...) > > > > +{ > > > > + va_list ap; > > > > + int res; > > > > + > > > > + va_start(ap, template); > > > > + res = vsnprintf(NULL, 0, template, ap); > > > > + va_end(ap); > > > > + > > > > + if (res > 0) { > > > > + char str[res + 1]; > > > > + > > > > + va_start(ap, template); > > > > + res = vsnprintf(str, sizeof(str), template, ap); > > > > + va_end(ap); > > > > + > > > > + if (res > 0) > > > > + slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL); > > > > + } > > > > + > > > > + if (res <= 0) > > > > + slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL); > > > > + > > > > + slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); > > > > + return rc; > > > > +} > > > This function seem not really useful, you use send_error() only at the > > > end of one single function where you could have the classic scheme of > > > using a done: label and just use directly slapi_ch_smprintf. This would > > > remove the need to use vsnprintf and all the va_ machinery which is > > > more than 50% of this function. > > > > > > > > > > +static long long > > > > +get_value(const LDAPMod *mod, long long def) > > > > +{ > > > > + const struct berval *bv; > > > > + long long val; > > > > + > > > > + if (mod == NULL) > > > > + return def; > > > > + > > > > + if (mod->mod_vals.modv_bvals == NULL) > > > > + return def; > > > > + > > > > + bv = mod->mod_vals.modv_bvals[0]; > > > > + if (bv == NULL) > > > > + return def; > > > In general (here and elsewhere) I prefer to always use {} in if clauses. > > > I have been bitten enough time by people adding an instruction that > > > should be in the braces but forgot to add braces (defensive programming > > > style). But up to you. > > > > > > > + char buf[bv->bv_len + 1]; > > > > + memcpy(buf, bv->bv_val, bv->bv_len); > > > > + buf[sizeof(buf)-1] = '\0'; > > > > + > > > > + val = strtoll(buf, NULL, 10); > > > > + if (val == LLONG_MIN || val == LLONG_MAX) > > > > + return def; > > > > + > > > > + return val; > > > > +} > > > > + > > > > +static struct berval ** > > > > +make_berval_array(long long value) > > > > +{ > > > > + struct berval **bvs; > > > > + > > > > + bvs = ch_calloc(2, struct berval*); > > > > + bvs[0] = ch_malloc(struct berval); > > > > + bvs[0]->bv_val = slapi_ch_smprintf("%lld", value); > > > > + bvs[0]->bv_len = strlen(bvs[0]->bv_val); > > > > + > > > > + return bvs; > > > > +} > > > > + > > > > +static LDAPMod * > > > > +make_ldapmod(int op, const char *attr, long long value) > > > > +{ > > > > + LDAPMod *mod; > > > > + > > > > + mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod)); > > > > + mod->mod_op = op | LDAP_MOD_BVALUES; > > > > + mod->mod_type = slapi_ch_strdup(attr); > > > > + mod->mod_bvalues = make_berval_array(value); > > > > + > > > > + return mod; > > > > +} > > > > + > > > > +static void > > > > +convert_ldapmod_to_bval(LDAPMod *mod) > > > > +{ > > > > + if (mod == NULL || (mod->mod_op & LDAP_MOD_BVALUES)) > > > > + return; > > > > + > > > > + mod->mod_op |= LDAP_MOD_BVALUES; > > > > + > > > > + if (mod->mod_values == NULL) { > > > > + mod->mod_bvalues = NULL; > > > > + return; > > > > + } > > > > + > > > > + for (size_t i = 0; mod->mod_values[i] != NULL; i++) { > > > > + struct berval *bv = ch_malloc(struct berval); > > > > + bv->bv_val = mod->mod_values[i]; > > > > + bv->bv_len = strlen(bv->bv_val); > > > > + mod->mod_bvalues[i] = bv; > > > > + } > > > > +} > > > > + > > > > +static counter > > > > +get_counter(Slapi_Entry *entry, const char *attr) > > > > +{ > > > > + Slapi_Attr *sattr = NULL; > > > > + > > > > + return (counter) { > > > > + slapi_entry_attr_find(entry, attr, &sattr) == 0, > > > > + slapi_entry_attr_get_longlong(entry, attr) > > > > + }; > > > > +} > > > > + > > > > +static void > > > > +berval_free_array(struct berval ***bvals) > > > > +{ > > > > + for (size_t i = 0; (*bvals)[i] != NULL; i++) { > > > > + ch_free((*bvals)[i]->bv_val); > > > > + ch_free((*bvals)[i]); > > > > + } > > > > + > > > > + slapi_ch_free((void**) bvals); > > > > +} > > > > + > > > > +static bool > > > > +is_replication(Slapi_PBlock *pb) > > > > +{ > > > > + int repl = 0; > > > > + slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, &repl); > > > > + return repl != 0; > > > > +} > > > > + > > > > +static const char * > > > > +get_attribute(Slapi_Entry *entry) > > > > +{ > > > > + static struct { > > > > + const char *clss; > > > > + const char *attr; > > > > + } table[] = { > > > > + { "ipatokenHOTP", "ipatokenHOTPcounter" }, > > > > + { "ipatokenTOTP", "ipatokenTOTPwatermark" }, > > > > + { NULL, NULL } > > > > + }; > > > > + > > > > + const char *attr = NULL; > > > > + char **clsses = NULL; > > > > + > > > > + clsses = slapi_entry_attr_get_charray(entry, "objectClass"); > > > > + if (clsses == NULL) > > > > + return NULL; > > > > + > > > > + for (size_t i = 0; attr == NULL && clsses[i] != NULL; i++) { > > > > + for (size_t j = 0; attr == NULL && table[j].clss != NULL; > > > > j++) { > > > > + if (PL_strcasecmp(table[j].clss, clsses[i]) == 0) > > > > + attr = table[j].attr; > > > > + } > > > > + } > > > > + > > > > + slapi_ch_array_free(clsses); > > > > + return attr; > > > > +} > > > Can you put a comment here that explains what you are going to do in > > > each cases in plain English ? This will help people in future figuring > > > out if/how to modify the code or why something happens a specific way. > > > It will also help the reviewer follow what is going on. > > > > > > > > > > +static int > > > > +preop_mod(Slapi_PBlock *pb) > > > > +{ > > > > + size_t count = 0, attrs = 0, extras = 0; > > > > + Slapi_Entry *entry = NULL; > > > > + const char *attr = NULL; > > > > + LDAPMod **inmods = NULL; > > > > + LDAPMod **mods = NULL; > > > > + counter ctr, orig; > > > > + > > > > + /* Get the input values. */ > > > > + slapi_pblock_get(pb, SLAPI_ENTRY_PRE_OP, &entry); > > > > + slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &inmods); > > > > + if (entry == NULL || inmods == NULL) > > > > + return 0; > > > > + > > > > + /* Get the attribute name. */ > > > > + attr = get_attribute(entry); > > > > + if (attr == NULL) > > > > + return 0; /* Not a token. */ > > > > + > > > > + /* Count items. */ > > > > + while (inmods != NULL && inmods[count] != NULL) { > > > ^^ this one would read much better as: > > > for (count = 0; inmods[count] != NULL; count++) { > > > > > > You do not need to check for insmods != NULL as you already check for > > > it and return 0 a few lines above. > > > > > > > + LDAPMod *mod = inmods[count++]; > > > > + > > > > + if (PL_strcasecmp(mod->mod_type, attr) != 0) > > > > + continue; > > > > + > > > > + attrs++; > > > > + switch (mod->mod_op & LDAP_MOD_OP) { > > > > + case LDAP_MOD_REPLACE: > > > > + case LDAP_MOD_INCREMENT: > > > > + extras++; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + /* Not operating on the counter/watermark. */ > > > > + if (attrs == 0) > > > > + return 0; > > > > + > > > > + /* Get the counter. */ > > > > + orig = ctr = get_counter(entry, attr); > > > > + > > > > + /* Filter the modify operations. */ > > > > + mods = ch_calloc(count + extras + 1, LDAPMod*); > > > > + for (size_t i = 0, j = 0; inmods != NULL && inmods[i] != NULL; > > > > mods[j++] = inmods[i++]) { > > > Please remove check for insmods != NULL, it is useless, and removing it > > > will bring back the line under 80columns > > > > > > > + LDAPMod *mod = inmods[i]; > > > > + > > > > + if (PL_strcasecmp(mod->mod_type, attr) != 0) > > > > + continue; > > > > + > > > > + convert_ldapmod_to_bval(mod); > > > > + > > > > + switch (mod->mod_op & LDAP_MOD_OP) { > > > > + case LDAP_MOD_DELETE: > > > > + ctr.exists = false; > > > > + if (mod->mod_bvalues != NULL && mod->mod_bvalues[0] == > > > > NULL) > > > > + berval_free_array(&mod->mod_bvalues); > > > > + > > > > + if (is_replication(pb)) > > > > + berval_free_array(&mod->mod_bvalues); > > > > + > > > > + if (mod->mod_bvalues == NULL) > > > > + mod->mod_bvalues = make_berval_array(ctr.value); > > > > + break; > > > I am not sure I understand this segment, why are you touching the > > > delete operation outside of a replication event ? > > > Doesn't this defeat and admin tool trying to correctly delete/add to > > > avoid races ? > > > > > > > + case LDAP_MOD_INCREMENT: > > > > + mods[j++] = make_ldapmod(LDAP_MOD_DELETE, attr, > > > > ctr.value); + > > > > + ctr.value += get_value(mod, 1); > > > uhmmm if you had an ADD followed by INCREMENT operation, would this > > > cause the value to become value*2+1 instead of just value+1 ? > > > > > > > + berval_free_array(&mod->mod_bvalues); > > > > + mod->mod_op &= ~LDAP_MOD_OP; > > > > + mod->mod_op |= LDAP_MOD_ADD; > > > > + mod->mod_bvalues = make_berval_array(ctr.value); > > > > + break; > > > uhmm why are you converting mod_increment in all cases ? (including > > > replication) > > > > > > > + case LDAP_MOD_REPLACE: > > > > + if (ctr.exists) > > > > + mods[j++] = make_ldapmod(LDAP_MOD_DELETE, attr, > > > > ctr.value); + > > > > + mod->mod_op &= ~LDAP_MOD_OP; > > > > + mod->mod_op |= LDAP_MOD_ADD; > > > same question here, why are you converting a replace coming from > > > replication into a delete/add ? > > > > > > > + case LDAP_MOD_ADD: > > > > + ctr.value = get_value(mod, 0); > > > > + ctr.exists = true; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + /* Set the modified operations. */ > > > > + slapi_pblock_set(pb, SLAPI_MODIFY_MODS, mods); > > > > + ch_free(inmods); > > > > + > > > > + /* Ensure we aren't deleting the counter. */ > > > > + if (orig.exists && !ctr.exists) > > > > + return send_error(pb, LDAP_UNWILLING_TO_PERFORM, > > > > + "Will not delete %s", attr); > > > > + > > > > + /* Ensure we aren't rolling back the counter. */ > > > > + if (orig.value > ctr.value) > > > > + return send_error(pb, LDAP_UNWILLING_TO_PERFORM, > > > > + "Will not decrement %s", attr); > > > > + > > > > + return 0; > > > > +} > > > > > > see above about send_error(). > > > > > > I think what is needed most is the comment that explains the process > > > at the to of the main function. > > > > > > Simo. > > All of the above are fixed. > > > > Also, I have addressed Thierry's concern about putting the plugin in > > betxnpreoperation by splitting the plugin into two phases: modification > > and validation. Now all modifications occur in bepreoperation and all > > validation occurs in betxnpreoperation. > > > > Additionally, I have new code to trigger a new replication in the case > > where a validation error occurs and we are in a replication transaction. > > Thus, when A attempts to push an old counter value to B, B will now > > replicate the latest value back to A. > > > > Nathaniel > > > Hello, > > The plugin looks very good I have very few comments: > trigger_replication, prepare the pblock for internal op but > does not call slapi_modify_internal_pb. > > At the end of txnpreop_mod, you may prefer to use > PR_snprintf(msg, sizeof(msg), "%s %s", err, attr) rather than > strcpy/strcat. > > Reading the fix, I still have problems to understand how > replicated updates will behave. > Let me give an example of my concern: > The initial value of the counter is 10. > On server A, the counter is updated MOD/REPL to the value > 11/csnA > On server B, the counter is updated MOD/REPL to the value > 12/csnB > For some reason csnB<csnA. > On server A, MOD/REPL is translated into (DEL 10+ADD 11)/csnA. > current value is then 11/csnA > On server B, MOD/REPL is translated into (DEL 10+ADD 12)/csnB. > current value is then 12/csnB > > When server A receives the operation value 12/csnB. preop_mod > translates it into (DEL 11/ADD 12)/csnB. txnpreop_mod accepts > the operation because orig.value (11) < ctr.value (12). > Unfortunately when the operation will be applied the final > value kept for the counter will be 11 because csnA>csnB. > > When server B receives the operation value 11/csnA. preop_mod > translates it into (DEL 12/ADD 11)/csn A. txnpreop_mod reject > the operation, updates (DEL 12/ADD 12)/csnB' and returns > unwilling_to_perform. Later the update csnB' will be sent to A > but server A will still be in problem to send csnA update.
I see only two ways to solve this problem: 1. We modify the CSN upon receipt. 2. We compare the received replication request's CSN with the current value and perform the writes ourselves. I don't think #2 is workable because it will cause a replication storm. I'm not sure about #1. Suggestions? > You mentioned that synchronize replication was needed. Do you > mean we need 'synchronous replication' to address the example > above ? I think I was referring to this: http://www.freeipa.org/page/V4/OTPReplayPrevention#Synchronous_Synchronization Nathaniel _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel