On Mon, 2014-04-21 at 23:15 +0200, Jakub Hrozek wrote:
Hi,
the attached patches implement ticket #2073 -- the possibility to
extend
the LDAP attribute map with custom attributes.
All attributes are saved to sysdb with the 'extra_' prefix so that the
custom attributes can be distinguished from the sysdb schema.
The first patch fixes a mostly unrelated bug, which I found when
writing
unit tests.
[PATCH 1/3] LDAP: Fix one-by-one bug in sdap_copy_opts
The sdap_copy_opts function copied all the arguments except for the
sentinel.
[PATCH 2/3] LDAP: Make it possible to extend an attribute map
https://fedorahosted.org/sssd/ticket/2073
This commit adds a new option ldap_user_extra_attrs that is unset by
default. When set, the option contains a list of LDAP attributes the
LDAP provider would download and store in addition to the usual set.
The attributes are fetched in sysdb with the 'extra_' prefix to not
conflict with the sssd sdap maps.
[PATCH 3/3] Make LDAP extra attributes available to IPA and AD
https://fedorahosted.org/sssd/ticket/2073
Looks good from 10000 feet, however I saw at least one issue, inline
From cc4b347c955f93d640772fad197a55a1b9034525 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 21 Apr 2014 21:33:36 +0200
Subject: [PATCH 1/3] LDAP: Fix one-by-one bug in sdap_copy_opts
[..]
+ /* Include the sentinel */
+ map[num_entries].def_name = NULL;
+ map[num_entries].sys_name = NULL;
+ map[num_entries].opt_name = NULL;
+ map[num_entries].name = NULL;
Here you explicitly mark all 4 components as NULL.
[..]
Then in the next patch ...
From 40329ebff77b0d05c8051d3d4a2af2f6545c8d86 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Wed, 31 Jul 2013 10:59:43 +0200
Subject: [PATCH 2/3] LDAP: Make it possible to extend an attribute map
[..]
+ for (nextra = 0; extra_attrs[nextra]; nextra++) ;
+ DEBUG(SSSDBG_FUNC_DATA, "%zu extra attributes\n", nextra);
(aside: is this really worth here, you can print it after the next loop
as nextra = i; at less cost)
+ map = talloc_realloc(memctx, src_map, struct sdap_attr_map,
+ num_entries + nextra + 1);
+ if (map == NULL) {
+ return ENOMEM;
+ }
+
+ for (i = 0; extra_attrs[i]; i++) {
+ map[num_entries+i].opt_name = talloc_strdup(map,
extra_attrs[i]);
+ map[num_entries+i].sys_name = talloc_asprintf(map,
+
SYSDB_EXTRA_ATTR_TMPL,
+ map[num_entries
+i].opt_name);
+ map[num_entries+i].name = talloc_strdup(map,
+ map[num_entries
+i].opt_name);
+ map[num_entries+i].def_name = talloc_strdup(map,
+ map[num_entries
+i].opt_name);
+ if (map[num_entries+i].opt_name == NULL ||
+ map[num_entries+i].sys_name == NULL ||
+ map[num_entries+i].name == NULL ||
+ map[num_entries+i].def_name == NULL) {
+ return ENOMEM;
+ }
+ DEBUG(SSSDBG_TRACE_FUNC, "Extending map with %s\n",
extra_attrs[i]);
+ }
+
+ /* Sentinel */
+ map[num_entries+nextra].opt_name = NULL;
+ map[num_entries+nextra].sys_name = NULL;
+ map[num_entries+nextra].name = NULL;
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
I see 2 solutions:
1) only null .name
2) memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
Simo.
--
Simo Sorce * Red Hat, Inc * New York