On 05/14/2015 05:10 PM, Jakub Hrozek wrote:
On Thu, May 14, 2015 at 01:55:01PM +0200, Pavel Reichl wrote:
> Please see updated patch.
Thanks, this looks much better. I only have some nitpicks now:
> From 4f0bd09258e52ae633bc91603e30edf9ce02b279 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl(a)redhat.com>
> Date: Thu, 30 Apr 2015 06:43:05 -0400
> Subject: [PATCH] krb5: new option krb5_map_user
>
> New option `krb5_map_user` providing mapping of ID provider names to
> Kerberos principals.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2509
[...]
> + </para>
> + <para>
> + `joe` and `vince` are UNIX user names and `juser`
> + and `rraines` are primaries of kerberos principals.
> + For user `joe` resp. `dick` SSSD will try to kinit
> + as `juser@REALM` resp. `richard@REALM`
Could we use something else than backticks in the XML. Remember we used
XML because it's possible to render other formats than troff, we already
render HTML. Would <quote> be a better choice?
done
> + </para>
> +
> + <para>
> + Default: not set
> + </para>
> + </listitem>
> + </varlistentry>
> +
> </variablelist>
> </para>
> </refsect1>
[...]
> +static errno_t split_tuple(TALLOC_CTX *mem_ctx, const char *tuple,
> + const char **_first, const char **_second)
> +{
> + errno_t ret;
> + char **list;
> + int n;
> + talloc(mem_ctx, int);
huh?
Sorry, but I need more changed lines to stats :-).
fixed
> +
> + ret = split_on_separator(mem_ctx, tuple, ':', true, true, &list,
&n);
> +
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "split_on_separator failed - %s:[%d]\n",
> + sss_strerror(ret), ret);
> + goto done;
> + } else if (n != 2) {
> + DEBUG(SSSDBG_MINOR_FAILURE, "split_on_separator failed.\n");
> + ret = EINVAL;
> + goto done;
> + }
> +
> + *_first = list[0];
> + *_second = list[1];
> +
> +done:
> + return ret;
> +}
> +
> +static errno_t
> +fill_name_to_primary_map(TALLOC_CTX *mem_ctx, char **map,
> + struct map_id_name_to_krb_primary *name_to_primary,
> + int size)
Can we use size_t for size?
sure, but it takes input from split_on_separator()
which is int, should
I fix split_on_separator() to use size_t instead if int?
> +{
> + int i;
> + errno_t ret;
> +
> + for (i = 0; i < size; i++) {
> + ret = split_tuple(mem_ctx, map[i],
> + &name_to_primary[i].id_name,
> + &name_to_primary[i].krb_primary);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "split_tuple failed - %s:[%d]\n", sss_strerror(ret),
ret);
> + goto done;
> + }
> + }
> +
> + ret = EOK;
> +
> +done:
> + return ret;
> +}
> +
> +errno_t
> +parse_krb5_map_user(TALLOC_CTX *mem_ctx, const char *krb5_map_user,
> + struct map_id_name_to_krb_primary **_name_to_primary)
> +{
> + int size;
> + char **map;
> + errno_t ret;
> + TALLOC_CTX *tmp_ctx;
> + struct map_id_name_to_krb_primary *name_to_primary;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + if (krb5_map_user == NULL || strlen(krb5_map_user) == 0) {
> + DEBUG(SSSDBG_FUNC_DATA, "Warning: krb5_map_user is empty!\n");
> + size = 0;
> + ret = EOK;
> + goto done;
> + }
I won't block the patch over this but I still don't understand why you
prefer to use the NULL pointer for "an empty mapping" instead of always
allocating the sentinel. I'd argue that using the sentinel would
ultimately make the code easier because you wouldn't have to check for
NULL mapping pointer, but just loop over the mapping array..one less
special case..
done
> +
> + ret = split_on_separator(tmp_ctx, krb5_map_user, ',', true, true,
> + &map, &size);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_OP_FAILURE, "Failed to parse krb5_map_user!\n");
> + goto done;
> + }
> +
> + name_to_primary = talloc_zero_array(tmp_ctx,
> + struct map_id_name_to_krb_primary,
> + size + 1);
> + if (name_to_primary == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> + /* sentinel */
> + name_to_primary[size].id_name = NULL;
> + name_to_primary[size].krb_primary = NULL;
> +
> + ret = fill_name_to_primary_map(name_to_primary, map, name_to_primary,
> + size);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "fill_name_to_primary_map failed: %s:[%d]\n",
> + sss_strerror(ret), ret);
> + goto done;
> + }
> +
> + ret = EOK;
> +
> +done:
> + if (ret == EOK) {
> + if (size == 0) {
> + *_name_to_primary = NULL;
> + } else {
> + *_name_to_primary = talloc_steal(mem_ctx, name_to_primary);
> + }
> + }
> + talloc_free(tmp_ctx);
> + return ret;
> +}
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel