<html> <head> <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 09/29/2014 08:30 PM, Nathaniel McCallum wrote: </div> <blockquote cite="mid:1412015453.18502.7.camel@redhat.com" type="cite"> <pre wrap="">On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote: </pre> <blockquote type="cite"> <pre wrap="">On Sun, 21 Sep 2014 22:33:47 -0400 Nathaniel McCallum <a class="moz-txt-link-rfc2396E" href="mailto:npmccallum@redhat.com"><npmccallum@redhat.com></a> wrote: Comments inline. </pre> <blockquote type="cite"> <pre wrap="">+ +#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)) </pre> </blockquote> <pre wrap=""> please do not redefine slapi functions, it just makes it harder to know what you used. </pre> <blockquote type="cite"> <pre wrap="">+typedef struct { + bool exists; + long long value; +} counter; </pre> </blockquote> <pre wrap=""> 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. </pre> <blockquote type="cite"> <pre wrap="">+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; +} </pre> </blockquote> <pre wrap=""> 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. </pre> <blockquote type="cite"> <pre wrap="">+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; </pre> </blockquote> <pre wrap=""> 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. </pre> <blockquote type="cite"> <pre wrap="">+ 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; +} </pre> </blockquote> <pre wrap=""> 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. </pre> <blockquote type="cite"> <pre wrap="">+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) { </pre> </blockquote> <pre wrap=""> ^^ 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. </pre> <blockquote type="cite"> <pre wrap="">+ 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++]) { </pre> </blockquote> <pre wrap=""> Please remove check for insmods != NULL, it is useless, and removing it will bring back the line under 80columns </pre> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap=""> 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 ? </pre> <blockquote type="cite"> <pre wrap="">+ case LDAP_MOD_INCREMENT: + mods[j++] = make_ldapmod(LDAP_MOD_DELETE, attr, ctr.value); + + ctr.value += get_value(mod, 1); </pre> </blockquote> <pre wrap=""> 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 ? </pre> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap=""> uhmm why are you converting mod_increment in all cases ? (including replication) </pre> <blockquote type="cite"> <pre wrap="">+ 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; </pre> </blockquote> <pre wrap=""> same question here, why are you converting a replace coming from replication into a delete/add ? </pre> <blockquote type="cite"> <pre wrap="">+ 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; +} </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> <pre wrap=""> 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 </pre> </blockquote> Hello, <blockquote>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. You mentioned that synchronize replication was needed. Do you mean we need 'synchronous replication' to address the example above ? thanks thierry </blockquote> </body> </html>