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.
