[PATCH] Add provider specific default regular expressions

[Sumit Bose]

Hi,

this patch add specific default values for the regular expression to
split user names for the AD and IPA provider. In a perfect patch the
ID provider itself would report back what he would like to use as a
regular expression, but all solutions I could think of would require
some bigger changes to the startup process which are not suitable at
this stage of the 1.9 development. So I picked a more direct solution.

bye,
Sumit

[Pavel Březina]

On 09/19/2012 09:49 PM, Sumit Bose wrote:
> Hi,
>
> this patch add specific default values for the regular expression to
> split user names for the AD and IPA provider. In a perfect patch the
> ID provider itself would report back what he would like to use as a
> regular expression, but all solutions I could think of would require
> some bigger changes to the startup process which are not suitable at
> this stage of the 1.9 development. So I picked a more direct solution.
>
> bye,
> Sumit
>
> @@ -130,6 +191,8 @@ int sss_names_init(TALLOC_CTX *mem_ctx, struct confdb_ctx *cdb,
>   #endif
>       }
> DEBUG(SSSDBG_TRACE_INTERNAL, ("Using re [%s].\n", ctx->re_pattern));
> ret = confdb_get_string(cdb, ctx, conf_path,
>                         CONFDB_FULL_NAME_FORMAT, NULL, &ctx->fq_fmt);
> if (ret != EOK) goto done;
>
> -- 1.7.7.6

Hi,
can you consider using a lower debug level here? Maybe 
SSSDBG_CONF_SETTINGS? I think this information can be useful when 
helping customers, so we should make sure it is present in the logs.

Otherwise the code looks good.

[Sumit Bose]

On Thu, Sep 20, 2012 at 10:19:38AM +0200, Pavel Březina wrote:
> On 09/19/2012 09:49 PM, Sumit Bose wrote:
> > Hi,
> >
> > this patch add specific default values for the regular expression to
> > split user names for the AD and IPA provider. In a perfect patch the
> > ID provider itself would report back what he would like to use as a
> > regular expression, but all solutions I could think of would require
> > some bigger changes to the startup process which are not suitable at
> > this stage of the 1.9 development. So I picked a more direct solution.
> >
> > bye,
> > Sumit
> >
> > @@ -130,6 +191,8 @@ int sss_names_init(TALLOC_CTX *mem_ctx, struct confdb_ctx *cdb,
> >  #endif
> >      }
> > DEBUG(SSSDBG_TRACE_INTERNAL, ("Using re [%s].\n", ctx->re_pattern));
> > ret = confdb_get_string(cdb, ctx, conf_path,
> >                            CONFDB_FULL_NAME_FORMAT, NULL, &ctx->fq_fmt);
> >    if (ret != EOK) goto done;
> >
> > -- 1.7.7.6
>
> Hi,
> can you consider using a lower debug level here? Maybe
> SSSDBG_CONF_SETTINGS? I think this information can be useful when
> helping customers, so we should make sure it is present in the logs.

sure, makes sense. New version attached.
> Otherwise the code looks good.

Thank you for the review.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Thu, Sep 20, 2012 at 11:07:43AM +0200, Sumit Bose wrote:
> On Thu, Sep 20, 2012 at 10:19:38AM +0200, Pavel Březina wrote:
> > On 09/19/2012 09:49 PM, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch add specific default values for the regular expression to
> > > split user names for the AD and IPA provider. In a perfect patch the
> > > ID provider itself would report back what he would like to use as a
> > > regular expression, but all solutions I could think of would require
> > > some bigger changes to the startup process which are not suitable at
> > > this stage of the 1.9 development. So I picked a more direct solution.
> > >
> > > bye,
> > > Sumit
> > >
> > > @@ -130,6 +191,8 @@ int sss_names_init(TALLOC_CTX *mem_ctx, struct confdb_ctx *cdb,
> > >  #endif
> > >      }
> > > DEBUG(SSSDBG_TRACE_INTERNAL, ("Using re [%s].\n", ctx->re_pattern));
> > > ret = confdb_get_string(cdb, ctx, conf_path,
> > >                            CONFDB_FULL_NAME_FORMAT, NULL, &ctx->fq_fmt);
> > >    if (ret != EOK) goto done;
> > >
> > > -- 1.7.7.6
> >
> > Hi,
> > can you consider using a lower debug level here? Maybe
> > SSSDBG_CONF_SETTINGS? I think this information can be useful when
> > helping customers, so we should make sure it is present in the logs.
>
> sure, makes sense. New version attached.
> > Otherwise the code looks good.
>
> Thank you for the review.

index 56340ab..6a5720d 100644
--- a/src/util/usertools.c
+++ b/src/util/usertools.c
@@ -100,7 +100,6 @@ static errno_t get_id_provider_default_re(TALLOC_CTX *mem_ctx,
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE, ("Failed to read ID provider " \
                                   "from conf db.\n"));
-        ret = ret;
         goto done;
     }

And pushed the patch to master.