On Mon, May 27, 2013 at 8:28 AM, Jakub Hrozek <jhrozek@redhat.com> wrote:>> On Fri, May 24, 2013 at 04:41:13PM -0700, C. S. wrote:
> > Add ldap_autofs_map_master_name option so that the auto master map name> > change be changed from the default auto.master.>> Hi Cove,>> this quite a good patch, thank you very much for the contribution.
>> I haven't done much testing yet, but I only have a couple of small> suggestions before we can apply the patch to master.>> Here is a diff that summarizes them with comments inline:
>> diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c> index 3e019d9..201dcbe 100644> --- a/src/providers/ldap/ldap_common.c> +++ b/src/providers/ldap/ldap_common.c
> @@ -415,7 +415,6 @@ int ldap_get_autofs_options(TALLOC_CTX *memctx,> /* automount master map name */> master_map = dp_opt_get_string(opts->basic, SDAP_AUTOFS_MAP_MASTER_NAME);> if (master_map == NULL) {
> - master_map = strdup(AUTOFS_MAP_MASTER_NAME);>> You don't have to strdup the option value, the call to dp_opt_set_string()> duplicates the value itself and maintains the memory hierarchy as it
> uses the talloc allocator.>> ret = dp_opt_set_string(opts->basic, SDAP_AUTOFS_MAP_MASTER_NAME,> master_map);> if (ret != EOK) {> diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
> index e56690a..7d2d3f5 100644> --- a/src/providers/ldap/sdap.h> +++ b/src/providers/ldap/sdap.h>> In the LDAP maps it's OK to add the new option to the "enum> sdap_basic_opt". The reason is that SSSD maintains a couple of "option
> maps" for both the LDAP provider as a whole and for the individual> object maps. The LDAP provider maps describe how to get the object from> LDAP (for instance it contains the search bases, timeouts, schema etc.)
> and then the per-object maps describe how to get the individual objects> (what attribute name describes which attribute, for instance "cn"> usually stands for "name).>> I was wondering for a short while whether the new attribute belongs to
> the generic LDAP map or the autofs map object map, but I think this> solution is OK. It describes a mean to get the master map, not any> master map attribute.>> @@ -328,7 +328,6 @@ enum sdap_service_attrs {
> };>> enum sdap_autofs_map_attrs {> - SDAP_AT_AUTOFS_MAP_MASTER_NAME,> SDAP_OC_AUTOFS_MAP,> SDAP_AT_AUTOFS_MAP_NAME,>> @@ -336,7 +335,6 @@ enum sdap_autofs_map_attrs {
> };>> enum sdap_autofs_entry_attrs {> - _SDAP_AT_AUTOFS_MAP_MASTER_NAME,> SDAP_OC_AUTOFS_ENTRY,> SDAP_AT_AUTOFS_ENTRY_KEY,> SDAP_AT_AUTOFS_ENTRY_VALUE,
>>> Thank you again for the patch!> _______________________________________________> sssd-devel mailing list> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel