On Thu, Dec 08, 2011 at 11:02:05AM +0100, Jakub Hrozek wrote:
> [PATCH 1/6] sss_utf8_tolower utility function+unit tests
> This will be used later on to lowercase the usernames. Also includes
> unit tests for the whole sss_utf8 module.
Nack, it seems that this patch breaks the test suite (or perhaps it's the last
one, I'm not 100% sure.
> [PATCH 2/6] Responders: Split getting domain by name into
separate
> function A utility function originally written for the SUDO responder,
> but it turns out it's useful earlier
Ack
> [PATCH 3/6] Use the case sensitivity flag in responders
> Reads the configuration option and includes its handling in the
> responders.
> Unfortunately some parts like negative cache are more complex because one
> domain can be case sensitive and another insensitive during multidomain
> searches.
Nack,
why are you using talloc strdup in functions like nss_cmd_getgrnam_search? I
mean talloc_strdup(dctx, cmdctx->name). I suspect it's for better consistency
with behavior of the sss_tc_utf8_tolower() but there is no real need for it
IMO.
The pd_set_primary_name() doesn't seem like a part of the patch, or is it? I'd
prefer separate patch for it and places where it is used. I'm also thinking
whether or not should it also detect the case_sensitive flag.
> [PATCH 4/6] Refactor saving sdap entities
> There was too much code duplication between
> sdap_save_{user,group,netgroup} and my later patch would add even more.
> This patch removes the most egregious cases.
Nack,
I think that the "multivalued" argument is really not necessary, is it?
Without it, there is also no need for those two wrapper macros.
I noticed that at one place, there was strdup() called before adding the
string to sysdb_attrs. I didn't inspect this much further, so my question is
whether it would make any sense to do that in sdap_attrs_add_ldap_attr() and
sdap_save_all_names()?
> [PATCH 5/6] Use the case sensitivity flag in the LDAP provider
> When saving users,groups and netgroups, saves the lowercase version of
> the name as an alias in addition to the original name that is still
> stored as name as well as part of RDN.
Nack,
I don't think there is a need for aliases to be both in the list both
untransformed and in lowercase. Please check that and if it isn't necessary,
please remove it.
Variable lowername in sdap_save_all_names() is not used. Also instead of
argument "dom", I would prefer "case_sensitive" being directly given
to the
sdap_save_all_names().
> [PATCH 6/6] Use the case sensitivity flag in the simple access
provider
> Performs string comparisons in case sensitive or insensitive manner
> depending on the configuration.
Ack
Jan