Crikey, that was fast.

On Fri, Aug 02, 2013 at 04:44:33PM +0300, Alexander Bokovoy wrote:
> >On Thu, 01 Aug 2013, Nalin Dahyabhai wrote:
> >>HEAD~10:
> >>* Add internal whitespace when computing the value to pass to
> >>slapi_ch_malloc().
> >>* Break the declaration and initialization of "str" into two lines.
> >>* Use '\0' to terminate str instead of 0.
> >>* In the new comment in format.h, NULL should be NUL.
> >done

Ok, good.

> >>HEAD~8:
> >>* In, drop the hunk that changes the version number that we
> >>pass to AC_INIT.
> >>* In, one test compares x$use_sss_nss_idmap != xno, while
> >>one shortly after compares $use_sss_nss_idmap = yes.  If
> >>$use_sss_nss_idmap can be empty, then the second test needs to be
> >>ready for that.


> >>* The help text still refers to SSSD specifically, when the code doesn't
> >>enforce or guarantee that SSSD's involved when performing nsswitch
> >>lookups or PAM authentication.
> >
> >The whole setup really makes sense only when SSSD is in use. Aside from
> >that, the whole setup is triggered only if we find libsss_nss_idmap
> >library which is provided by SSSD. Yes, it is used for an optional
> >adding of the SID but linking is explicit.

Yeah, but why is all of that required?  If we want to be able to gateway
whatever's going on at the server, the ipaNTSecurityIdentifier lookup is
already optional at runtime.  Making that part optional at compile-time
would make the rest of the code (at least the nsswitch parts) easier to

> >>HEAD~7:
> >>* Fix formatting of wrapped lines when calling backend_shr_get_vattr_str().
> >>* Check for errors when converting the configured sssd_min_id to a number
> >>by switching from atol() to strtol() or strtoul().
> >done.

Looks good, except that the comment in the checking block implies that
it's imposing a lower limit on ret.sssd_min_id, when it's reassigning
the default in case of a parsing error.  If the value parses as valid,
it appears that a value lower than 1000 would be accepted.

> >>HEAD~6:
> >>* Drop extra whitespace after the type of the "name" member when
> >>defining struct backend_search_filter_config.
> >>* backend_search_filter_has_cn_uid() allocates config->name using
> >>slapi_ch_malloc(), but it is later freed by free().
> >>* backend_retrieve_user_entry_from_sssd(): fix line wrapping in
> >>function signature.
> >>* backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors.
> >>* backend_retrieve_user_entry_from_sssd(): avoid using
> >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit
> >>with the high bit set as a negative value
> >>* backend_retrieve_user_entry_from_sssd(): check for zero-length
> >>pw_shell value, and avoid adding it as a loginShell value, since
> >>that'd be invalid
> >all done.

That last one's just a NULL check now.  Please add a check for a
zero-length value, which would also be skipped.  Otherwise, looks good.

> >>* backend_retrieve_user_entry_from_sssd() adds "ipaNTUserAttrs" as an
> >>objectClass in the temporary entry, but the value isn't copied into
> >>the cache.  What purpose does it serve?
> >I removed this part since we are anyway using extensibleObject for the
> >generated entry.

Makes sense.

> >>* backend_retrieve_user_entry_from_sssd(): extra whitespace when
> >>constructing the new entry's DN.

Both of these still have an extra space after the assignment operator.

> >>* backend_retrieve_user_entry_from_sssd(): memory leak from not freeing
> >>the result of slapi_escape_filter_value().
> >>* backend_retrieve_group_entry_from_sssd(): fix line wrapping in
> >>function signature.
> >>* backend_retrieve_group_entry_from_sssd() needs to handle ERANGE errors.
> >>* backend_retrieve_group_entry_from_sssd(): extra whitespace when
> >>constructing the new entry's DN.
> >>* backend_retrieve_group_entry_from_sssd(): memory leak from not freeing
> >>the result of slapi_escape_filter_value().
> >all done

Otherwise, looks good.

> >>* backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors.
> >>* backend_retrieve_group_entry_from_sssd(): avoid using
> >>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit
> >>with the high bit set as a negative value
> >>* backend_retrieve_group_list_from_sssd(): check for NULL result from
> >>realloc().

Leaks are possible if realloc() fails for grouplist or entries.

> >>* backend_search_sssd(): fix line wrapping for call to
> >>slapi_filter_apply().
> >>* backend_search_sssd(): use strtol() or strtoul() to convert a value to
> >>a number, and check for errors.
> >>* backend_search_sssd(): staged->container_sdn/map_group/map_set are
> >>allocated with slapi_ch_strdup() but freed with free() later.
> >>* backend_retrieve_from_sssd(): check for NULL result from malloc().
> >>* backend_retrieve_from_sssd(): fix line wrapping for calls to
> >>backend_retrieve_user/group_entry_from_sssd.
> >all done

Otherwise, looks good.

> >>HEAD~5:
> >>* free_pam_response() uses slapi_ch_free() to free memory that was
> >>probably allocated with malloc().
> >>* pam_conv_func(): remove extra whitespace in call to slapi_pblock_get()
> >>* pam_conv_func(): use malloc() instead of slapi_ch_calloc() to allocate
> >>replies, because plugins tend to be use free() to free them.
> >>* pam_conv_func(): replace if/else-if/else-if/else-if stacks with a
> >>switch() statement.
> >>* do_pam_auth(): fix line wrapping in function signature.
> >>* do_pam_auth(): comments suggest that it's just entered or left a
> >>critical section, when the function's been called in one.
> >>* do_pam_auth(): replace if/else-if/else-if/else-if stacks with a
> >>switch() statement.
> >>* do_pam_auth(): fix line wrapping when calling PR_smprintf().

Missed one, right after the done: label.

> >>HEAD~4:
> >>* Also needs to add back-sch.h to nisserver_plugin_la_SOURCES.
> >not done. I see no reason to add back-sch.h to
> >nisserver_plugin_la_SOURCES -- it is not used or referenced in any code
> >included into the nisserver plugin.

Whoops, yeah, you're right.  Somehow I missed the part where it was
being added to schemacompat_plugin_la_SOURCES, which is what I meant to
point to.

> >>HEAD~3:
> >>* backend_search_find_set_data_in_group_cb(): fix line wrapping in
> >>function signature.
> >>* backend_search_cb() looks like it'll leak staged->entries[i] if
> >>another thread has added a similarly-named entry to the cache.
> >all done

Looks good.

> >>* backend_search_cb() now appears to be freeing the closest-match
> >>as part of a conditional clause, right before it would unconditionally
> >>do so.
> >removed the duplicate. My original intent in moving that code was to
> >avoid freeing closest-match right before we are printing it with
> >slapi_log_error() and then sending the result with send_ldap_result():
> >
> >
> >I can revert this part back if you wish but I think there is an error in
> >the original code.

The current intent there is to avoid sending a closest-match DN when
we're returning entries as part of the result, to only send such a value
as part of the result if we're sending back an LDAP_NO_SUCH_OBJECT

> >>HEAD~2:
> >>* backend_bind_cb() should avoid having to cast away a "const" by using
> >>non-const variables for the saved copies of the group and set names.
> >all done

I meant to suggest that attempting to use an array to store both a const
and a non-const string still forces a cast somewhere.  Use two

> ... and I did fix couple more comments received from Sumit:
> 1. switched to use usigned long for sssd_min_id
> 2. added initializer for 'dn' in 
> backend_retrieve_user_entry_from_sssd()/backend_retrieve_group_entry_from_sssd
> 3. switched to atoll() with cast of the result to (uid_t)/(gid_t) because
>    we already know at staging phase that the id in as a string will convert 
> without errors to an integer and it is > min id.

Okay.  Though, formatting the uid/gid as a string to pass it into a
local function that converts it right back to an integer seems
excessive.  Anyhow, I guess we're down to minor stuff now.



Freeipa-devel mailing list

Reply via email to