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.