On 08/26/2014 11:37 AM, Jakub Hrozek wrote:
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.
After applying the patches I can't replicate the problem any longer.
Thanks for the code style change I think it's better now.
Just 2 little details, please see inline, but I'm willing to ACK patches
as are now.
+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)) {
ldb_msg_find_val returns pointer so the if condition should look like:
+ if (ldb_msg_find_val(member_el, &val) != NULL) {
right?
+ 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) {
has_member returns boolean so the '== true' is not needed, right?
+ 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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel