On Mon, May 21, 2012 at 02:32:43PM -0400, Stephen Gallagher wrote: > On Mon, 2012-05-21 at 13:04 +0200, Jakub Hrozek wrote: > > On Mon, May 14, 2012 at 11:04:00AM -0400, Stephen Gallagher wrote: > > > Joshua Roys submitted a patch in > > > https://bugzilla.redhat.com/show_bug.cgi?id=726456 to add support for > > > the Netscape password warning expiration control. I've made some > > > modifications to his original patch to clean it up, so now I'm sending > > > it to the list for further code-review. > > > > > > Fixes https://fedorahosted.org/sssd/ticket/984 > > > > > + if (response_controls[c]->ldctl_value.bv_len > 32) > > > + continue; > > > + if (!state->ppolicy) > > > + state->ppolicy = talloc(state, struct > > > sdap_ppolicy_data); > > > > I would prefer to enclose even single-line statements (well, perhaps the > > continue is OK as well as return <code> would be..). I've been hit > > myself by adding a new statement after if without noticing there's no > > brackets.. > > > > Yeah, I usually do the same. I forgot to change that from the > originally-submitted patch. Fixed. > > > > + /* ensure that bv_val is a null-terminated string */ > > > + nval = talloc_strndup(NULL, > > > + > > > response_controls[c]->ldctl_value.bv_val, > > > + > > > response_controls[c]->ldctl_value.bv_len); > > > + if (nval == NULL) { > > > + ret = ENOMEM; > > > + goto done; > > > + } > > > + state->ppolicy->expire = strtouint32(nval, NULL, 10); > > > + if (errno != 0) { > > > + ret = errno; > > > + DEBUG(SSSDBG_MINOR_FAILURE, > > > + ("Could not convert control response to an > > > integer. ", > > > + "[%s]\n", strerror(ret))); > > > > nval should be freed here (or perhaps allocated on state..) > > > > > Strictly speaking, it would have been freed in the done: label. I > originally did that because I thought I was going to hang onto nval > longer. Since I clearly do not need to, I rearranged that so that it's > freed right after the conversion. > > > I made one additional change. I realized that we were overloading 'ret' > with both errno and ldap return values. I created a new variable 'lret' > to differentiate them and avoid polluting the values.
Ack _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
