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.


-- 
Simo Sorce * Red Hat, Inc * New York

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to