On Tue, 2014-04-22 at 16:18 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 08:38:46AM -0400, Simo Sorce wrote:
HERE ---^^^^ only 3 elements are NULLed, def_name is missing.
Ah, sorry, I NULL-ed all elements just before sending the patch on the other place, but missed this one.
I see 2 solutions:
- only null .name
- memset(&map[num_entries+nextra], 0, sizeof(struct sdap_attr_map));
They are both fail_safe in case struct sdap_attr_map is changed.
I prefer memset (or some shorthand like Samba's ZERO_STRUCT).
I have another question, though after discussion with Jan (CC-ed).
The way I save the attributes (extra_$name) in this patchset is wrong with respect to how we allow the dbus responder to query the attributes, because the user would have to explicitly allow extra_$name in the attribute ACL which is confusing and would have to be carefully documented.
Also, this approach wouldn't work well with respect to multiple domains with different schemas.
Jan's proposal, which I like, was to change the ldap_user_extra attribute syntax from: ldap_user_extra_attrs = ldap_attr_name1, ldap_attr_name2
to: ldap_user_extra_attrs = ldap_attr_name1:sysdb_attr_name1, ldap_attr_name2:sysdb_attr_name2
The sysdb_attr_name would not be mandatory, if the sysdb name was omitted, then the back end would save the attribute verbatim.
If there was a conflict between the name the user chose (or the original LDAP attribute name), the SSSD would throw an error.
I like this a lot, please do it.
Although I wonder, should the order be the reverse ? I think of it as assignments so mentally I would visualize them as: ldap_user_extra_attrs = internal_name_1:ldap_name_1, internal_name_2:ldap_name_2
Simo.