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.