On Mon, Aug 25, 2014 at 08:18:15PM +0200, Pavel Reichl wrote:On 08/25/2014 06:21 PM, Jakub Hrozek wrote:On Mon, Aug 25, 2014 at 05:23:44PM +0200, Pavel Reichl wrote:Hello, I tested the patch and it fixes the problem described in the ticket. I personally dislike the break and mainly the continue check - I believe that people reading the code would benefit from encapsulating the inner loop into function (return value would indicate what is happening and function name is the best comment anyway) and same would go for the outer loop IMO.That's a fair comment. Would you prefer to split the for loop into one function or two?two, but I can live with one :-)OK, see the attached patches. Please note I wasn't able to test them properly because my new AD test servers don't carry POSIX attributes yet. I'll test the patches in a short while, in the meantime, feel free to test in your environment and/or look at the code.
+static bool has_member(struct ldb_message_element *member_el,
+ char *member)
+{
+ struct ldb_val val;
+
+ val.data = (uint8_t *) member;
+ val.length = strlen(member);
+
+ /* This is bad complexity, but the this loop should only be invoked in
+ * the very rare scenario of AD POSIX group that is primary group of
+ * some users but has user member attributes at the same time
+ */
+ if (ldb_msg_find_val(member_el, &val)) {
+ return true;
+ }
+
+ return false;
+}
+
static void link_pgroup_members(struct sysdb_attrs *group_attrs,
struct ldb_message_element *member_el,
char **userdns,
size_t nuserdns)
{
- int i;
+ int i, j;
+ j = 0;
for (i=0; i < nuserdns; i++) {
- member_el->values[member_el->num_values + i].data = (uint8_t *) \
- talloc_steal(group_attrs, userdns[i]);
- member_el->values[member_el->num_values + i].length = strlen(userdns[i]);
+ if (has_member(member_el, userdns[i]) == true) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ "Member %s already included, skipping\n", userdns[i]);
+ continue;
+ }
+
+ member_el->values[member_el->num_values + j].data = (uint8_t *) \
+ talloc_steal(group_attrs, userdns[i]);
+ member_el->values[member_el->num_values + j].length = \
+ strlen(userdns[i]);
+ j++;
}
- member_el->num_values += nuserdns;
+ member_el->num_values += j;
}
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel