[Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
thierry bordaz
tbordaz at redhat.com
Tue Sep 30 16:30:05 UTC 2014
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 <npmccallum at 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.
You mentioned that synchronize replication was needed. Do you mean
we need 'synchronous replication' to address the example above ?
thanks
thierry
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/d5fb1388/attachment.htm>
More information about the Freeipa-devel
mailing list