[PATCH] sssctl: Add config-check command

[Michal Židek]

Hello!

This patch adds new command config-check
for sssctl tool. The output looks like this:


Issues identified by validators: 3
[rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for 
typos.
[rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in 
section 'sssd'. Check for typos.
[rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed 
in section 'sssd'. Check for typos.

Messages generated during configuration merging: 4
File conf.dd did not match provided patterns. Skipping.
Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
Error (5) on line 5: Equal sign is missing.
Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered. 
Skipping.

Used configuration snippet files: 1
/etc/sssd/conf.d/snip-good.conf


Currently it can only be used by root and checks only
configuration in default path.

Michal

[Lukas Slebodnik]

On (01/07/16 17:26), Michal Židek wrote:
> Hello!
>
> This patch adds new command config-check
> for sssctl tool. The output looks like this:
>
>
> Issues identified by validators: 3
> [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for
> typos.
> [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section
> 'sssd'. Check for typos.
> [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in
> section 'sssd'. Check for typos.
>
> Messages generated during configuration merging: 4
> File conf.dd did not match provided patterns. Skipping.
> Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> Error (5) on line 5: Equal sign is missing.
> Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> Skipping.
>
> Used configuration snippet files: 1
> /etc/sssd/conf.d/snip-good.conf
>
>
> Currently it can only be used by root and checks only
> configuration in default path.
>
> Michal 
> From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00
> 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> Date: Fri, 1 Jul 2016 15:10:30 +0200
> Subject: [PATCH 1/4] sss_ini: Small refacoring of  sss_ini_call_validators
>
> Separate logic to fill errobj so that
> the errors can be printed by the caller.
> ---
>  src/util/sss_ini.c | 49 ++++++++++++++++++++++++++++++++++++-------------
>  src/util/sss_ini.h | 12 ++++++++++++
>  2 files changed, 48 insertions(+), 13 deletions(-)
>
> diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> index b4dbb07..78f5490 100644
> --- a/src/util/sss_ini.c
> +++ b/src/util/sss_ini.c
> @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data,
>  {
>  #ifdef HAVE_LIBINI_CONFIG_V1_3
>      int ret;
> -    struct ini_cfgobj *rules_cfgobj = NULL;
>      struct ini_errobj *errobj = NULL;
>  
>      ret = ini_errobj_create(&errobj);
> @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data,
>          goto done;
>      }
>  
> -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> +    ret = sss_ini_call_validators_errobj(data,
> +                                         rules_path,
> +                                         errobj);
>      if (ret != EOK) {
> -        DEBUG(SSSDBG_FATAL_FAILURE,
> -              "Failed to read sssd.conf schema %d [%s]\n", ret,
> strerror(ret));
> -        goto done;
> -    }
> -
> -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj);
> -    if (ret != EOK) {
> -        DEBUG(SSSDBG_FATAL_FAILURE,
> -              "ini_rules_check failed %d [%s]\n", ret, strerror(ret));
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "Failed to get errors from validators.\n");
>          goto done;
>      }
>  
> @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data,
>          ini_errobj_next(errobj);
>      }
>  
> +    ret = EOK;
> +
>  done:
> -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
>      ini_errobj_destroy(&errobj);
> -
>      return ret;
>  #else
>      DEBUG(SSSDBG_TRACE_FUNC,
> @@ -590,3 +584,32 @@ done:
>      return EOK;
>  #endif /* HAVE_LIBINI_CONFIG_V1_3 */
>  }
> +
> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> +                                   const char *rules_path,
> +                                   struct ini_errobj *errobj)
> +{
> +    int ret;
> +    struct ini_cfgobj *rules_cfgobj = NULL;
> +
> +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +              "Failed to read sssd.conf schema %d [%s]\n", ret,
> strerror(ret));
> +        goto done;
> +    }
> +
> +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE,
> +              "ini_rules_check failed %d [%s]\n", ret, strerror(ret));
> +        goto done;
> +    }
> +
> +done:
> +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> +
> +    return ret;
> +}
> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> index 7734bab..77943d6 100644
> --- a/src/util/sss_ini.h
> +++ b/src/util/sss_ini.h
> @@ -27,6 +27,10 @@
>  #ifndef __SSS_INI_H__
>  #define __SSS_INI_H__
>  
> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> +#include <ini_configobj.h>
> +#endif
> +
>  /* Structure declarations */
>  
>  /* INI data structure */
> @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
>  int sss_ini_call_validators(struct sss_ini_initdata *data,
>                              const char *rules_path);
>  
> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> +/* Get errors from validators with ini_errobj */
> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> +                                   const char *rules_path,
> +                                   struct ini_errobj *errobj);
> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> + 
NACK

src/util/sss_ini.h must not contain conditional build.
That's the putpose of implementation file.

Header file shoudl contain just a prototypes
becuase there might be missing prototypes if you forgot
include "config.h" before this header file.

LS

[Michal Židek]

On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
>  On (01/07/16 17:26), Michal Židek wrote:
> > Hello!
> >
> > This patch adds new command config-check
> > for sssctl tool. The output looks like this:
> >
> >
> > Issues identified by validators: 3
> > [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for
> > typos.
> > [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in section
> > 'sssd'. Check for typos.
> > [rule/allowed_sssd_options]: Attribute 'unknown_option' is not allowed in
> > section 'sssd'. Check for typos.
> >
> > Messages generated during configuration merging: 4
> > File conf.dd did not match provided patterns. Skipping.
> > Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> > Error (5) on line 5: Equal sign is missing.
> > Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> > Skipping.
> >
> > Used configuration snippet files: 1
> > /etc/sssd/conf.d/snip-good.conf
> >
> >
> > Currently it can only be used by root and checks only
> > configuration in default path.
> >
> > Michal
>
> >From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> > Date: Fri, 1 Jul 2016 15:10:30 +0200
> > Subject: [PATCH 1/4] sss_ini: Small refacoring of  sss_ini_call_validators
> >
> > Separate logic to fill errobj so that
> > the errors can be printed by the caller.
> > ---
> > src/util/sss_ini.c | 49 ++++++++++++++++++++++++++++++++++++-------------
> > src/util/sss_ini.h | 12 ++++++++++++
> > 2 files changed, 48 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> > index b4dbb07..78f5490 100644
> > --- a/src/util/sss_ini.c
> > +++ b/src/util/sss_ini.c
> > @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct sss_ini_initdata *data,
> > {
> > #ifdef HAVE_LIBINI_CONFIG_V1_3
> >      int ret;
> > -    struct ini_cfgobj *rules_cfgobj = NULL;
> >      struct ini_errobj *errobj = NULL;
> >
> >      ret = ini_errobj_create(&errobj);
> > @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct sss_ini_initdata *data,
> >          goto done;
> >      }
> >
> > -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> > +    ret = sss_ini_call_validators_errobj(data,
> > +                                         rules_path,
> > +                                         errobj);
> >      if (ret != EOK) {
> > -        DEBUG(SSSDBG_FATAL_FAILURE,
> > -              "Failed to read sssd.conf schema %d [%s]\n", ret,
> strerror(ret));
> > -        goto done;
> > -    }
> > -
> > -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj);
> > -    if (ret != EOK) {
> > -        DEBUG(SSSDBG_FATAL_FAILURE,
> > -              "ini_rules_check failed %d [%s]\n", ret, strerror(ret));
> > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > +              "Failed to get errors from validators.\n");
> >          goto done;
> >      }
> >
> > @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct sss_ini_initdata *data,
> >          ini_errobj_next(errobj);
> >      }
> >
> > +    ret = EOK;
> > +
> > done:
> > -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >      ini_errobj_destroy(&errobj);
> > -
> >      return ret;
> > #else
> >      DEBUG(SSSDBG_TRACE_FUNC,
> > @@ -590,3 +584,32 @@ done:
> >      return EOK;
> > #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > }
> > +
> > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> > +                                   const char *rules_path,
> > +                                   struct ini_errobj *errobj)
> > +{
> > +    int ret;
> > +    struct ini_cfgobj *rules_cfgobj = NULL;
> > +
> > +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_FATAL_FAILURE,
> > +              "Failed to read sssd.conf schema %d [%s]\n", ret,
> strerror(ret));
> > +        goto done;
> > +    }
> > +
> > +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL, errobj);
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_FATAL_FAILURE,
> > +              "ini_rules_check failed %d [%s]\n", ret, strerror(ret));
> > +        goto done;
> > +    }
> > +
> > +done:
> > +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> > +
> > +    return ret;
> > +}
> > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> > index 7734bab..77943d6 100644
> > --- a/src/util/sss_ini.h
> > +++ b/src/util/sss_ini.h
> > @@ -27,6 +27,10 @@
> > #ifndef __SSS_INI_H__
> > #define __SSS_INI_H__
> >
> > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > +#include <ini_configobj.h>
> > +#endif
> > +
> > /* Structure declarations */
> >
> > /* INI data structure */
> > @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> > int sss_ini_call_validators(struct sss_ini_initdata *data,
> >                              const char *rules_path);
> >
> > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > +/* Get errors from validators with ini_errobj */
> > +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> > +                                   const char *rules_path,
> > +                                   struct ini_errobj *errobj);
> > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > +
>  NACK
>
>  src/util/sss_ini.h must not contain conditional build.
>  That's the putpose of implementation file.
>
>  Header file shoudl contain just a prototypes
>  becuase there might be missing prototypes if you forgot
>  include "config.h" before this header file.
>
>  LS 
Ok,

I did some refactoring so that the errobj is not
used as parameter to the functions to avoid the
conditions in .h file.

See the attached patches.

I also pushed it to CI:
http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/

Michal

[Michal Židek]

On 07/04/2016 05:27 PM, Michal Židek wrote:
>  On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> > On (01/07/16 17:26), Michal Židek wrote:
> >> Hello!
> >>
> >> This patch adds new command config-check
> >> for sssctl tool. The output looks like this:
> >>
> >>
> >> Issues identified by validators: 3
> >> [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for
> >> typos.
> >> [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed in
> >> section
> >> 'sssd'. Check for typos.
> >> [rule/allowed_sssd_options]: Attribute 'unknown_option' is not
> >> allowed in
> >> section 'sssd'. Check for typos.
> >>
> >> Messages generated during configuration merging: 4
> >> File conf.dd did not match provided patterns. Skipping.
> >> Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> >> Error (5) on line 5: Equal sign is missing.
> >> Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> >> Skipping.
> >>
> >> Used configuration snippet files: 1
> >> /etc/sssd/conf.d/snip-good.conf
> >>
> >>
> >> Currently it can only be used by root and checks only
> >> configuration in default path.
> >>
> >> Michal
> >
> >> From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >> Subject: [PATCH 1/4] sss_ini: Small refacoring of
> >> sss_ini_call_validators
> >>
> >> Separate logic to fill errobj so that
> >> the errors can be printed by the caller.
> >> ---
> >> src/util/sss_ini.c | 49
> >> ++++++++++++++++++++++++++++++++++++-------------
> >> src/util/sss_ini.h | 12 ++++++++++++
> >> 2 files changed, 48 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> >> index b4dbb07..78f5490 100644
> >> --- a/src/util/sss_ini.c
> >> +++ b/src/util/sss_ini.c
> >> @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> >> sss_ini_initdata *data,
> >> {
> >> #ifdef HAVE_LIBINI_CONFIG_V1_3
> >>      int ret;
> >> -    struct ini_cfgobj *rules_cfgobj = NULL;
> >>      struct ini_errobj *errobj = NULL;
> >>
> >>      ret = ini_errobj_create(&errobj);
> >> @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> >> sss_ini_initdata *data,
> >>          goto done;
> >>      }
> >>
> >> -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >> +    ret = sss_ini_call_validators_errobj(data,
> >> +                                         rules_path,
> >> +                                         errobj);
> >>      if (ret != EOK) {
> >> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >> -              "Failed to read sssd.conf schema %d [%s]\n", ret,
> >> strerror(ret));
> >> -        goto done;
> >> -    }
> >> -
> >> -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >> errobj);
> >> -    if (ret != EOK) {
> >> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >> -              "ini_rules_check failed %d [%s]\n", ret,
> strerror(ret));
> >> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >> +              "Failed to get errors from validators.\n");
> >>          goto done;
> >>      }
> >>
> >> @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> >> sss_ini_initdata *data,
> >>          ini_errobj_next(errobj);
> >>      }
> >>
> >> +    ret = EOK;
> >> +
> >> done:
> >> -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>      ini_errobj_destroy(&errobj);
> >> -
> >>      return ret;
> >> #else
> >>      DEBUG(SSSDBG_TRACE_FUNC,
> >> @@ -590,3 +584,32 @@ done:
> >>      return EOK;
> >> #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >> }
> >> +
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >> +                                   const char *rules_path,
> >> +                                   struct ini_errobj *errobj)
> >> +{
> >> +    int ret;
> >> +    struct ini_cfgobj *rules_cfgobj = NULL;
> >> +
> >> +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >> +              "Failed to read sssd.conf schema %d [%s]\n", ret,
> >> strerror(ret));
> >> +        goto done;
> >> +    }
> >> +
> >> +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >> errobj);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >> +              "ini_rules_check failed %d [%s]\n", ret,
> strerror(ret));
> >> +        goto done;
> >> +    }
> >> +
> >> +done:
> >> +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >> +
> >> +    return ret;
> >> +}
> >> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >> diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> >> index 7734bab..77943d6 100644
> >> --- a/src/util/sss_ini.h
> >> +++ b/src/util/sss_ini.h
> >> @@ -27,6 +27,10 @@
> >> #ifndef __SSS_INI_H__
> >> #define __SSS_INI_H__
> >>
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +#include <ini_configobj.h>
> >> +#endif
> >> +
> >> /* Structure declarations */
> >>
> >> /* INI data structure */
> >> @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> >> int sss_ini_call_validators(struct sss_ini_initdata *data,
> >>                              const char *rules_path);
> >>
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +/* Get errors from validators with ini_errobj */
> >> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >> +                                   const char *rules_path,
> >> +                                   struct ini_errobj *errobj);
> >> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >> +
> > NACK
> >
> > src/util/sss_ini.h must not contain conditional build.
> > That's the putpose of implementation file.
> >
> > Header file shoudl contain just a prototypes
> > becuase there might be missing prototypes if you forgot
> > include "config.h" before this header file.
> >
> > LS
>
>  Ok,
>
>  I did some refactoring so that the errobj is not
>  used as parameter to the functions to avoid the
>  conditions in .h file.
>
>  See the attached patches.
>
>  I also pushed it to CI:
>  http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
>
>  Michal
>  
Lukas requested some changes off list.

New patches attached.

I also pushed it to CI:
http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/

Michal

[Lukas Slebodnik]

On (07/07/16 12:37), Michal Židek wrote:
> On 07/04/2016 05:27 PM, Michal Židek wrote:
> > On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> > > On (01/07/16 17:26), Michal Židek wrote:
> > > > Hello!
> > > > 
> > > > This patch adds new command config-check
> > > > for sssctl tool. The output looks like this:
> > > > 
> > > > 
> > > > Issues identified by validators: 3
> > > > [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for
> > > > typos.
> > > > [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed
> in
> > > > section
> > > > 'sssd'. Check for typos.
> > > > [rule/allowed_sssd_options]: Attribute 'unknown_option' is not
> > > > allowed in
> > > > section 'sssd'. Check for typos.
> > > > 
> > > > Messages generated during configuration merging: 4
> > > > File conf.dd did not match provided patterns. Skipping.
> > > > Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> > > > Error (5) on line 5: Equal sign is missing.
> > > > Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> > > > Skipping.
> > > > 
> > > > Used configuration snippet files: 1
> > > > /etc/sssd/conf.d/snip-good.conf
> > > > 
> > > > 
> > > > Currently it can only be used by root and checks only
> > > > configuration in default path.
> > > > 
> > > > Michal
> > > 
> > > > From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> > > > Date: Fri, 1 Jul 2016 15:10:30 +0200
> > > > Subject: [PATCH 1/4] sss_ini: Small refacoring of
> > > > sss_ini_call_validators
> > > > 
> > > > Separate logic to fill errobj so that
> > > > the errors can be printed by the caller.
> > > > ---
> > > > src/util/sss_ini.c | 49
> > > > ++++++++++++++++++++++++++++++++++++-------------
> > > > src/util/sss_ini.h | 12 ++++++++++++
> > > > 2 files changed, 48 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> > > > index b4dbb07..78f5490 100644
> > > > --- a/src/util/sss_ini.c
> > > > +++ b/src/util/sss_ini.c
> > > > @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> > > > sss_ini_initdata *data,
> > > > {
> > > > #ifdef HAVE_LIBINI_CONFIG_V1_3
> > > >      int ret;
> > > > -    struct ini_cfgobj *rules_cfgobj = NULL;
> > > >      struct ini_errobj *errobj = NULL;
> > > > 
> > > >      ret = ini_errobj_create(&errobj);
> > > > @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> > > > sss_ini_initdata *data,
> > > >          goto done;
> > > >      }
> > > > 
> > > > -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> > > > +    ret = sss_ini_call_validators_errobj(data,
> > > > +                                         rules_path,
> > > > +                                         errobj);
> > > >      if (ret != EOK) {
> > > > -        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > -              "Failed to read sssd.conf schema %d [%s]\n", ret,
> > > > strerror(ret));
> > > > -        goto done;
> > > > -    }
> > > > -
> > > > -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> > > > errobj);
> > > > -    if (ret != EOK) {
> > > > -        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > -              "ini_rules_check failed %d [%s]\n", ret,
> strerror(ret));
> > > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > > > +              "Failed to get errors from validators.\n");
> > > >          goto done;
> > > >      }
> > > > 
> > > > @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> > > > sss_ini_initdata *data,
> > > >          ini_errobj_next(errobj);
> > > >      }
> > > > 
> > > > +    ret = EOK;
> > > > +
> > > > done:
> > > > -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> > > >      ini_errobj_destroy(&errobj);
> > > > -
> > > >      return ret;
> > > > #else
> > > >      DEBUG(SSSDBG_TRACE_FUNC,
> > > > @@ -590,3 +584,32 @@ done:
> > > >      return EOK;
> > > > #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > > > }
> > > > +
> > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> > > > +                                   const char *rules_path,
> > > > +                                   struct ini_errobj *errobj)
> > > > +{
> > > > +    int ret;
> > > > +    struct ini_cfgobj *rules_cfgobj = NULL;
> > > > +
> > > > +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> > > > +    if (ret != EOK) {
> > > > +        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > +              "Failed to read sssd.conf schema %d [%s]\n", ret,
> > > > strerror(ret));
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> > > > errobj);
> > > > +    if (ret != EOK) {
> > > > +        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > +              "ini_rules_check failed %d [%s]\n", ret,
> strerror(ret));
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +done:
> > > > +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > > > diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> > > > index 7734bab..77943d6 100644
> > > > --- a/src/util/sss_ini.h
> > > > +++ b/src/util/sss_ini.h
> > > > @@ -27,6 +27,10 @@
> > > > #ifndef __SSS_INI_H__
> > > > #define __SSS_INI_H__
> > > > 
> > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > +#include <ini_configobj.h>
> > > > +#endif
> > > > +
> > > > /* Structure declarations */
> > > > 
> > > > /* INI data structure */
> > > > @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> > > > int sss_ini_call_validators(struct sss_ini_initdata *data,
> > > >                              const char *rules_path);
> > > > 
> > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > +/* Get errors from validators with ini_errobj */
> > > > +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> > > > +                                   const char *rules_path,
> > > > +                                   struct ini_errobj *errobj);
> > > > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > > > +
> > > NACK
> > > 
> > > src/util/sss_ini.h must not contain conditional build.
> > > That's the putpose of implementation file.
> > > 
> > > Header file shoudl contain just a prototypes
> > > becuase there might be missing prototypes if you forgot
> > > include "config.h" before this header file.
> > > 
> > > LS
> > 
> > Ok,
> > 
> > I did some refactoring so that the errobj is not
> > used as parameter to the functions to avoid the
> > conditions in .h file.
> > 
> > See the attached patches.
> > 
> > I also pushed it to CI:
> > http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> > 
> > Michal
> > 
>
> Lukas requested some changes off list.
>
> New patches attached.
>
> I also pushed it to CI:
> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
>
> Michal
>  
> From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00
> 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> Date: Fri, 1 Jul 2016 15:10:30 +0200
> Subject: [PATCH 1/2] sss_ini: Small refacoring of  sss_ini_call_validators
>
> Separate logic to fill errobj so that
> the errors can be printed by the caller.
> --- 
ACK

> From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00
> 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> Date: Mon, 27 Jun 2016 17:33:14 +0200
> Subject: [PATCH 2/2] sssctl: Add config-check command
>
> Fixes:
> https://fedorahosted.org/sssd/ticket/2269
>
> sssctl sconfig-check command allows to
> call SSSD config file validators on
> demand.
> ---
>  Makefile.am                      |   1 +
>  src/tools/sssctl/sssctl.c        |   4 ++
>  src/tools/sssctl/sssctl.h        |   4 ++
>  src/tools/sssctl/sssctl_config.c | 130 +++++++++++++++++++++++++++++++++++++++
>  src/util/sss_ini.c               |   2 -
>  5 files changed, 139 insertions(+), 2 deletions(-)
>  create mode 100644 src/tools/sssctl/sssctl_config.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 4089b69..706b60d 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
>      src/tools/sssctl/sssctl_logs.c \
>      src/tools/sssctl/sssctl_domains.c \
>      src/tools/sssctl/sssctl_sifp.c \
> +    src/tools/sssctl/sssctl_config.c \
>      $(SSSD_TOOLS_OBJ) \
>      $(NULL)
>  sssctl_LDADD = \
> diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> index be5f1b4..86656f1 100644
> --- a/src/tools/sssctl/sssctl.c
> +++ b/src/tools/sssctl/sssctl.c
> @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
>          SSS_TOOL_DELIMITER("Log files tools:"),
>          SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD log
> files", 0, sssctl_remove_logs),
>          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log files in
> tarball", 0, sssctl_fetch_logs),
> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> +        SSS_TOOL_COMMAND("config-check", "Perform static analysis of SSSD
> configuration", 0, sssctl_config_check),
> +#endif
>          {NULL, NULL, 0, NULL}
>      };
>  
> diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> index ae6e62c..be62475 100644
> --- a/src/tools/sssctl/sssctl.h
> +++ b/src/tools/sssctl/sssctl.h
> @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline *cmdline,
>                          struct sss_tool_ctx *tool_ctx,
>                          void *pvt);
>  
> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> +                            struct sss_tool_ctx *tool_ctx,
> +                            void *pvt);
> +
>  #endif /* _SSSCTL_H_ */
> diff --git a/src/tools/sssctl/sssctl_config.c b/src/tools/sssctl/sssctl_config.c
> new file mode 100644
> index 0000000..a2d1350
> --- /dev/null
> +++ b/src/tools/sssctl/sssctl_config.c
> @@ -0,0 +1,130 @@
> +/*
> +    Authors:
> +        Michal Židek <mzidek(a)redhat.com&gt;
> +
> +    Copyright (C) 2016 Red Hat
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>;.
> +*/
> +
> +#include "config.h"
> +
> +#include <popt.h>
> +#include <stdio.h>
> +#include <ini_configobj.h>
> +
> +#include "util/util.h"
> +#include "util/sss_ini.h"
> +#include "tools/common/sss_tools.h"
> +#include "tools/common/sss_process.h"
> +#include "tools/sssctl/sssctl.h"
> +#include "confdb/confdb.h"
> +
> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> +                            struct sss_tool_ctx *tool_ctx,
> +                            void *pvt)
> +{
> +    errno_t ret;
> +    struct ini_errobj *errobj = NULL;
> +    struct sss_ini_initdata *init_data;
> +    struct ref_array *ra;
> +    char *msg;
> +    uint32_t i = 0;
> +    size_t num_errors;
> +    char **strs = NULL;
> +    TALLOC_CTX *tmp_ctx = NULL;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    init_data = sss_ini_initdata_init(tmp_ctx);
> +    if (!init_data) {
> +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    /* Open config file */
> +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              "sss_ini_config_file_open failed: %s [%d]\n", strerror(ret),
> +              ret);
> +        goto done;
> +    }
> +
> +    /* Check the file permissions */
> +    ret = sss_ini_config_access_check(init_data);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "Permission check on config file failed.\n"); 
IMHO, we
shoudl inform about wrong permission here with printf.
So it can be translated.
It's quite important info for users.

> +        ret = EPERM;
> +        goto done;
> +    }
> +
> +    ret = sss_ini_get_config(init_data,
> +                             SSSD_CONFIG_FILE,
> +                             CONFDB_DEFAULT_CONFIG_DIR);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load configuration\n");
> +        goto done;
> +    }
> +
> +    /* Read rules */
> +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> +                                       SSSDDATADIR"/cfg_rules.ini",
> +                                       &strs, &num_errors);
> +    if (ret) {
> +        goto done;
> +    }
> +
> +    /* Output from validators */
> +    printf(_("Issues identified by validators: %lu\n"), num_errors); 
    
                                            ^^^
                                                %zu shoudl be used for size_t
                                                otherwise there is a warning on
                                                32bit architectures.

otherwise LGTM

LS

[Michal Židek]

On 07/07/2016 01:20 PM, Lukas Slebodnik wrote:
>  On (07/07/16 12:37), Michal Židek wrote:
> > On 07/04/2016 05:27 PM, Michal Židek wrote:
> >> On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> >>> On (01/07/16 17:26), Michal Židek wrote:
> >>>> Hello!
> >>>>
> >>>> This patch adds new command config-check
> >>>> for sssctl tool. The output looks like this:
> >>>>
> >>>>
> >>>> Issues identified by validators: 3
> >>>> [rule/allowed_sections]: Section [SectionFOO] is not allowed. Check for
> >>>> typos.
> >>>> [rule/allowed_sssd_options]: Attribute 'unlknowww' is not allowed
> in
> >>>> section
> >>>> 'sssd'. Check for typos.
> >>>> [rule/allowed_sssd_options]: Attribute 'unknown_option' is not
> >>>> allowed in
> >>>> section 'sssd'. Check for typos.
> >>>>
> >>>> Messages generated during configuration merging: 4
> >>>> File conf.dd did not match provided patterns. Skipping.
> >>>> Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> >>>> Error (5) on line 5: Equal sign is missing.
> >>>> Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> >>>> Skipping.
> >>>>
> >>>> Used configuration snippet files: 1
> >>>> /etc/sssd/conf.d/snip-good.conf
> >>>>
> >>>>
> >>>> Currently it can only be used by root and checks only
> >>>> configuration in default path.
> >>>>
> >>>> Michal
> >>>
> >>>>  From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00 2001
> >>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >>>> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >>>> Subject: [PATCH 1/4] sss_ini: Small refacoring of
> >>>> sss_ini_call_validators
> >>>>
> >>>> Separate logic to fill errobj so that
> >>>> the errors can be printed by the caller.
> >>>> ---
> >>>> src/util/sss_ini.c | 49
> >>>> ++++++++++++++++++++++++++++++++++++-------------
> >>>> src/util/sss_ini.h | 12 ++++++++++++
> >>>> 2 files changed, 48 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> >>>> index b4dbb07..78f5490 100644
> >>>> --- a/src/util/sss_ini.c
> >>>> +++ b/src/util/sss_ini.c
> >>>> @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> >>>> sss_ini_initdata *data,
> >>>> {
> >>>> #ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>       int ret;
> >>>> -    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>       struct ini_errobj *errobj = NULL;
> >>>>
> >>>>       ret = ini_errobj_create(&errobj);
> >>>> @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> >>>> sss_ini_initdata *data,
> >>>>           goto done;
> >>>>       }
> >>>>
> >>>> -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >>>> +    ret = sss_ini_call_validators_errobj(data,
> >>>> +                                         rules_path,
> >>>> +                                         errobj);
> >>>>       if (ret != EOK) {
> >>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>> -              "Failed to read sssd.conf schema %d [%s]\n",
> ret,
> >>>> strerror(ret));
> >>>> -        goto done;
> >>>> -    }
> >>>> -
> >>>> -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >>>> errobj);
> >>>> -    if (ret != EOK) {
> >>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>> -              "ini_rules_check failed %d [%s]\n", ret,
> strerror(ret));
> >>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >>>> +              "Failed to get errors from validators.\n");
> >>>>           goto done;
> >>>>       }
> >>>>
> >>>> @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> >>>> sss_ini_initdata *data,
> >>>>           ini_errobj_next(errobj);
> >>>>       }
> >>>>
> >>>> +    ret = EOK;
> >>>> +
> >>>> done:
> >>>> -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>       ini_errobj_destroy(&errobj);
> >>>> -
> >>>>       return ret;
> >>>> #else
> >>>>       DEBUG(SSSDBG_TRACE_FUNC,
> >>>> @@ -590,3 +584,32 @@ done:
> >>>>       return EOK;
> >>>> #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>> }
> >>>> +
> >>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >>>> +                                   const char *rules_path,
> >>>> +                                   struct ini_errobj *errobj)
> >>>> +{
> >>>> +    int ret;
> >>>> +    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>> +
> >>>> +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >>>> +    if (ret != EOK) {
> >>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>> +              "Failed to read sssd.conf schema %d [%s]\n",
> ret,
> >>>> strerror(ret));
> >>>> +        goto done;
> >>>> +    }
> >>>> +
> >>>> +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >>>> errobj);
> >>>> +    if (ret != EOK) {
> >>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>> +              "ini_rules_check failed %d [%s]\n", ret,
> strerror(ret));
> >>>> +        goto done;
> >>>> +    }
> >>>> +
> >>>> +done:
> >>>> +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>> +
> >>>> +    return ret;
> >>>> +}
> >>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>> diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> >>>> index 7734bab..77943d6 100644
> >>>> --- a/src/util/sss_ini.h
> >>>> +++ b/src/util/sss_ini.h
> >>>> @@ -27,6 +27,10 @@
> >>>> #ifndef __SSS_INI_H__
> >>>> #define __SSS_INI_H__
> >>>>
> >>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>> +#include <ini_configobj.h>
> >>>> +#endif
> >>>> +
> >>>> /* Structure declarations */
> >>>>
> >>>> /* INI data structure */
> >>>> @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> >>>> int sss_ini_call_validators(struct sss_ini_initdata *data,
> >>>>                               const char *rules_path);
> >>>>
> >>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>> +/* Get errors from validators with ini_errobj */
> >>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >>>> +                                   const char *rules_path,
> >>>> +                                   struct ini_errobj *errobj);
> >>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>> +
> >>> NACK
> >>>
> >>> src/util/sss_ini.h must not contain conditional build.
> >>> That's the putpose of implementation file.
> >>>
> >>> Header file shoudl contain just a prototypes
> >>> becuase there might be missing prototypes if you forgot
> >>> include "config.h" before this header file.
> >>>
> >>> LS
> >>
> >> Ok,
> >>
> >> I did some refactoring so that the errobj is not
> >> used as parameter to the functions to avoid the
> >> conditions in .h file.
> >>
> >> See the attached patches.
> >>
> >> I also pushed it to CI:
> >> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> >>
> >> Michal
> >>
> >
> > Lukas requested some changes off list.
> >
> > New patches attached.
> >
> > I also pushed it to CI:
> > http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
> >
> > Michal
> >
>
> >From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> > Date: Fri, 1 Jul 2016 15:10:30 +0200
> > Subject: [PATCH 1/2] sss_ini: Small refacoring of  sss_ini_call_validators
> >
> > Separate logic to fill errobj so that
> > the errors can be printed by the caller.
> > ---
>  ACK
>
> >From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> > Date: Mon, 27 Jun 2016 17:33:14 +0200
> > Subject: [PATCH 2/2] sssctl: Add config-check command
> >
> > Fixes:
> > https://fedorahosted.org/sssd/ticket/2269
> >
> > sssctl sconfig-check command allows to
> > call SSSD config file validators on
> > demand.
> > ---
> > Makefile.am                      |   1 +
> > src/tools/sssctl/sssctl.c        |   4 ++
> > src/tools/sssctl/sssctl.h        |   4 ++
> > src/tools/sssctl/sssctl_config.c | 130 +++++++++++++++++++++++++++++++++++++++
> > src/util/sss_ini.c               |   2 -
> > 5 files changed, 139 insertions(+), 2 deletions(-)
> > create mode 100644 src/tools/sssctl/sssctl_config.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 4089b69..706b60d 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
> >      src/tools/sssctl/sssctl_logs.c \
> >      src/tools/sssctl/sssctl_domains.c \
> >      src/tools/sssctl/sssctl_sifp.c \
> > +    src/tools/sssctl/sssctl_config.c \
> >      $(SSSD_TOOLS_OBJ) \
> >      $(NULL)
> > sssctl_LDADD = \
> > diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> > index be5f1b4..86656f1 100644
> > --- a/src/tools/sssctl/sssctl.c
> > +++ b/src/tools/sssctl/sssctl.c
> > @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
> >          SSS_TOOL_DELIMITER("Log files tools:"),
> >          SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD log
> files", 0, sssctl_remove_logs),
> >          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log files in
> tarball", 0, sssctl_fetch_logs),
> > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> > +        SSS_TOOL_COMMAND("config-check", "Perform static analysis of
> SSSD configuration", 0, sssctl_config_check),
> > +#endif
> >          {NULL, NULL, 0, NULL}
> >      };
> >
> > diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> > index ae6e62c..be62475 100644
> > --- a/src/tools/sssctl/sssctl.h
> > +++ b/src/tools/sssctl/sssctl.h
> > @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline *cmdline,
> >                          struct sss_tool_ctx *tool_ctx,
> >                          void *pvt);
> >
> > +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> > +                            struct sss_tool_ctx *tool_ctx,
> > +                            void *pvt);
> > +
> > #endif /* _SSSCTL_H_ */
> > diff --git a/src/tools/sssctl/sssctl_config.c b/src/tools/sssctl/sssctl_config.c
> > new file mode 100644
> > index 0000000..a2d1350
> > --- /dev/null
> > +++ b/src/tools/sssctl/sssctl_config.c
> > @@ -0,0 +1,130 @@
> > +/*
> > +    Authors:
> > +        Michal Židek <mzidek(a)redhat.com&gt;
> > +
> > +    Copyright (C) 2016 Red Hat
> > +
> > +    This program is free software; you can redistribute it and/or modify
> > +    it under the terms of the GNU General Public License as published by
> > +    the Free Software Foundation; either version 3 of the License, or
> > +    (at your option) any later version.
> > +
> > +    This program is distributed in the hope that it will be useful,
> > +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +    GNU General Public License for more details.
> > +
> > +    You should have received a copy of the GNU General Public License
> > +    along with this program.  If not, see <http://www.gnu.org/licenses/>;.
> > +*/
> > +
> > +#include "config.h"
> > +
> > +#include <popt.h>
> > +#include <stdio.h>
> > +#include <ini_configobj.h>
> > +
> > +#include "util/util.h"
> > +#include "util/sss_ini.h"
> > +#include "tools/common/sss_tools.h"
> > +#include "tools/common/sss_process.h"
> > +#include "tools/sssctl/sssctl.h"
> > +#include "confdb/confdb.h"
> > +
> > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> > +                            struct sss_tool_ctx *tool_ctx,
> > +                            void *pvt)
> > +{
> > +    errno_t ret;
> > +    struct ini_errobj *errobj = NULL;
> > +    struct sss_ini_initdata *init_data;
> > +    struct ref_array *ra;
> > +    char *msg;
> > +    uint32_t i = 0;
> > +    size_t num_errors;
> > +    char **strs = NULL;
> > +    TALLOC_CTX *tmp_ctx = NULL;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    init_data = sss_ini_initdata_init(tmp_ctx);
> > +    if (!init_data) {
> > +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> > +        ret = ENOMEM;
> > +        goto done;
> > +    }
> > +
> > +    /* Open config file */
> > +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_TRACE_FUNC,
> > +              "sss_ini_config_file_open failed: %s [%d]\n",
> strerror(ret),
> > +              ret);
> > +        goto done;
> > +    }
> > +
> > +    /* Check the file permissions */
> > +    ret = sss_ini_config_access_check(init_data);
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > +              "Permission check on config file failed.\n");
>  IMHO, we shoudl inform about wrong permission here with printf.
>  So it can be translated.
>  It's quite important info for users.
>
> > +        ret = EPERM;
> > +        goto done;
> > +    }
> > +
> > +    ret = sss_ini_get_config(init_data,
> > +                             SSSD_CONFIG_FILE,
> > +                             CONFDB_DEFAULT_CONFIG_DIR);
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load configuration\n");
> > +        goto done;
> > +    }
> > +
> > +    /* Read rules */
> > +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> > +                                       SSSDDATADIR"/cfg_rules.ini",
> > +                                       &strs, &num_errors);
> > +    if (ret) {
> > +        goto done;
> > +    }
> > +
> > +    /* Output from validators */
> > +    printf(_("Issues identified by validators: %lu\n"), num_errors);
>                                                    ^^^
>                                                   %zu shoudl be used for size_t
>                                                   otherwise there is a warning on
>                                                   32bit architectures.
>
>  otherwise LGTM
>
>  LS 
New patches attached. I also did one more change that
Lukas requested off-list.

Michal

[Michal Židek]

On 07/07/2016 01:54 PM, Michal Židek wrote:
>  On 07/07/2016 01:20 PM, Lukas Slebodnik wrote:
> > On (07/07/16 12:37), Michal Židek wrote:
> >> On 07/04/2016 05:27 PM, Michal Židek wrote:
> >>> On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> >>>> On (01/07/16 17:26), Michal Židek wrote:
> >>>>> Hello!
> >>>>>
> >>>>> This patch adds new command config-check
> >>>>> for sssctl tool. The output looks like this:
> >>>>>
> >>>>>
> >>>>> Issues identified by validators: 3
> >>>>> [rule/allowed_sections]: Section [SectionFOO] is not allowed.
> >>>>> Check for
> >>>>> typos.
> >>>>> [rule/allowed_sssd_options]: Attribute 'unlknowww' is not
> allowed in
> >>>>> section
> >>>>> 'sssd'. Check for typos.
> >>>>> [rule/allowed_sssd_options]: Attribute 'unknown_option' is
> not
> >>>>> allowed in
> >>>>> section 'sssd'. Check for typos.
> >>>>>
> >>>>> Messages generated during configuration merging: 4
> >>>>> File conf.dd did not match provided patterns. Skipping.
> >>>>> Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> >>>>> Error (5) on line 5: Equal sign is missing.
> >>>>> Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> >>>>> Skipping.
> >>>>>
> >>>>> Used configuration snippet files: 1
> >>>>> /etc/sssd/conf.d/snip-good.conf
> >>>>>
> >>>>>
> >>>>> Currently it can only be used by root and checks only
> >>>>> configuration in default path.
> >>>>>
> >>>>> Michal
> >>>>
> >>>>>  From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00
> >>>>> 2001
> >>>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >>>>> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >>>>> Subject: [PATCH 1/4] sss_ini: Small refacoring of
> >>>>> sss_ini_call_validators
> >>>>>
> >>>>> Separate logic to fill errobj so that
> >>>>> the errors can be printed by the caller.
> >>>>> ---
> >>>>> src/util/sss_ini.c | 49
> >>>>> ++++++++++++++++++++++++++++++++++++-------------
> >>>>> src/util/sss_ini.h | 12 ++++++++++++
> >>>>> 2 files changed, 48 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> >>>>> index b4dbb07..78f5490 100644
> >>>>> --- a/src/util/sss_ini.c
> >>>>> +++ b/src/util/sss_ini.c
> >>>>> @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> >>>>> sss_ini_initdata *data,
> >>>>> {
> >>>>> #ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>>       int ret;
> >>>>> -    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>>       struct ini_errobj *errobj = NULL;
> >>>>>
> >>>>>       ret = ini_errobj_create(&errobj);
> >>>>> @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> >>>>> sss_ini_initdata *data,
> >>>>>           goto done;
> >>>>>       }
> >>>>>
> >>>>> -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >>>>> +    ret = sss_ini_call_validators_errobj(data,
> >>>>> +                                         rules_path,
> >>>>> +                                         errobj);
> >>>>>       if (ret != EOK) {
> >>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> -              "Failed to read sssd.conf schema %d [%s]\n",
> ret,
> >>>>> strerror(ret));
> >>>>> -        goto done;
> >>>>> -    }
> >>>>> -
> >>>>> -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >>>>> errobj);
> >>>>> -    if (ret != EOK) {
> >>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> -              "ini_rules_check failed %d [%s]\n", ret,
> >>>>> strerror(ret));
> >>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >>>>> +              "Failed to get errors from validators.\n");
> >>>>>           goto done;
> >>>>>       }
> >>>>>
> >>>>> @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> >>>>> sss_ini_initdata *data,
> >>>>>           ini_errobj_next(errobj);
> >>>>>       }
> >>>>>
> >>>>> +    ret = EOK;
> >>>>> +
> >>>>> done:
> >>>>> -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>>       ini_errobj_destroy(&errobj);
> >>>>> -
> >>>>>       return ret;
> >>>>> #else
> >>>>>       DEBUG(SSSDBG_TRACE_FUNC,
> >>>>> @@ -590,3 +584,32 @@ done:
> >>>>>       return EOK;
> >>>>> #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>> }
> >>>>> +
> >>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >>>>> +                                   const char *rules_path,
> >>>>> +                                   struct ini_errobj *errobj)
> >>>>> +{
> >>>>> +    int ret;
> >>>>> +    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>> +
> >>>>> +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >>>>> +    if (ret != EOK) {
> >>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> +              "Failed to read sssd.conf schema %d [%s]\n",
> ret,
> >>>>> strerror(ret));
> >>>>> +        goto done;
> >>>>> +    }
> >>>>> +
> >>>>> +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >>>>> errobj);
> >>>>> +    if (ret != EOK) {
> >>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> +              "ini_rules_check failed %d [%s]\n", ret,
> >>>>> strerror(ret));
> >>>>> +        goto done;
> >>>>> +    }
> >>>>> +
> >>>>> +done:
> >>>>> +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>> +
> >>>>> +    return ret;
> >>>>> +}
> >>>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>> diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> >>>>> index 7734bab..77943d6 100644
> >>>>> --- a/src/util/sss_ini.h
> >>>>> +++ b/src/util/sss_ini.h
> >>>>> @@ -27,6 +27,10 @@
> >>>>> #ifndef __SSS_INI_H__
> >>>>> #define __SSS_INI_H__
> >>>>>
> >>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>> +#include <ini_configobj.h>
> >>>>> +#endif
> >>>>> +
> >>>>> /* Structure declarations */
> >>>>>
> >>>>> /* INI data structure */
> >>>>> @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> >>>>> int sss_ini_call_validators(struct sss_ini_initdata *data,
> >>>>>                               const char *rules_path);
> >>>>>
> >>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>> +/* Get errors from validators with ini_errobj */
> >>>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >>>>> +                                   const char *rules_path,
> >>>>> +                                   struct ini_errobj *errobj);
> >>>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>> +
> >>>> NACK
> >>>>
> >>>> src/util/sss_ini.h must not contain conditional build.
> >>>> That's the putpose of implementation file.
> >>>>
> >>>> Header file shoudl contain just a prototypes
> >>>> becuase there might be missing prototypes if you forgot
> >>>> include "config.h" before this header file.
> >>>>
> >>>> LS
> >>>
> >>> Ok,
> >>>
> >>> I did some refactoring so that the errobj is not
> >>> used as parameter to the functions to avoid the
> >>> conditions in .h file.
> >>>
> >>> See the attached patches.
> >>>
> >>> I also pushed it to CI:
> >>> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> >>>
> >>> Michal
> >>>
> >>
> >> Lukas requested some changes off list.
> >>
> >> New patches attached.
> >>
> >> I also pushed it to CI:
> >> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
> >>
> >> Michal
> >>
> >
> >> From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >> Subject: [PATCH 1/2] sss_ini: Small refacoring of
> >> sss_ini_call_validators
> >>
> >> Separate logic to fill errobj so that
> >> the errors can be printed by the caller.
> >> ---
> > ACK
> >
> >> From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> Date: Mon, 27 Jun 2016 17:33:14 +0200
> >> Subject: [PATCH 2/2] sssctl: Add config-check command
> >>
> >> Fixes:
> >> https://fedorahosted.org/sssd/ticket/2269
> >>
> >> sssctl sconfig-check command allows to
> >> call SSSD config file validators on
> >> demand.
> >> ---
> >> Makefile.am                      |   1 +
> >> src/tools/sssctl/sssctl.c        |   4 ++
> >> src/tools/sssctl/sssctl.h        |   4 ++
> >> src/tools/sssctl/sssctl_config.c | 130
> >> +++++++++++++++++++++++++++++++++++++++
> >> src/util/sss_ini.c               |   2 -
> >> 5 files changed, 139 insertions(+), 2 deletions(-)
> >> create mode 100644 src/tools/sssctl/sssctl_config.c
> >>
> >> diff --git a/Makefile.am b/Makefile.am
> >> index 4089b69..706b60d 100644
> >> --- a/Makefile.am
> >> +++ b/Makefile.am
> >> @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
> >>      src/tools/sssctl/sssctl_logs.c \
> >>      src/tools/sssctl/sssctl_domains.c \
> >>      src/tools/sssctl/sssctl_sifp.c \
> >> +    src/tools/sssctl/sssctl_config.c \
> >>      $(SSSD_TOOLS_OBJ) \
> >>      $(NULL)
> >> sssctl_LDADD = \
> >> diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> >> index be5f1b4..86656f1 100644
> >> --- a/src/tools/sssctl/sssctl.c
> >> +++ b/src/tools/sssctl/sssctl.c
> >> @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
> >>          SSS_TOOL_DELIMITER("Log files tools:"),
> >>          SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD
> log
> >> files", 0, sssctl_remove_logs),
> >>          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log files
> in
> >> tarball", 0, sssctl_fetch_logs),
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> >> +        SSS_TOOL_COMMAND("config-check", "Perform static analysis
> of
> >> SSSD configuration", 0, sssctl_config_check),
> >> +#endif
> >>          {NULL, NULL, 0, NULL}
> >>      };
> >>
> >> diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> >> index ae6e62c..be62475 100644
> >> --- a/src/tools/sssctl/sssctl.h
> >> +++ b/src/tools/sssctl/sssctl.h
> >> @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline *cmdline,
> >>                          struct sss_tool_ctx *tool_ctx,
> >>                          void *pvt);
> >>
> >> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >> +                            struct sss_tool_ctx *tool_ctx,
> >> +                            void *pvt);
> >> +
> >> #endif /* _SSSCTL_H_ */
> >> diff --git a/src/tools/sssctl/sssctl_config.c
> >> b/src/tools/sssctl/sssctl_config.c
> >> new file mode 100644
> >> index 0000000..a2d1350
> >> --- /dev/null
> >> +++ b/src/tools/sssctl/sssctl_config.c
> >> @@ -0,0 +1,130 @@
> >> +/*
> >> +    Authors:
> >> +        Michal Židek <mzidek(a)redhat.com&gt;
> >> +
> >> +    Copyright (C) 2016 Red Hat
> >> +
> >> +    This program is free software; you can redistribute it and/or
> >> modify
> >> +    it under the terms of the GNU General Public License as
> >> published by
> >> +    the Free Software Foundation; either version 3 of the License, or
> >> +    (at your option) any later version.
> >> +
> >> +    This program is distributed in the hope that it will be useful,
> >> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> +    GNU General Public License for more details.
> >> +
> >> +    You should have received a copy of the GNU General Public License
> >> +    along with this program.  If not, see
> >> <http://www.gnu.org/licenses/>;.
> >> +*/
> >> +
> >> +#include "config.h"
> >> +
> >> +#include <popt.h>
> >> +#include <stdio.h>
> >> +#include <ini_configobj.h>
> >> +
> >> +#include "util/util.h"
> >> +#include "util/sss_ini.h"
> >> +#include "tools/common/sss_tools.h"
> >> +#include "tools/common/sss_process.h"
> >> +#include "tools/sssctl/sssctl.h"
> >> +#include "confdb/confdb.h"
> >> +
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >> +                            struct sss_tool_ctx *tool_ctx,
> >> +                            void *pvt)
> >> +{
> >> +    errno_t ret;
> >> +    struct ini_errobj *errobj = NULL;
> >> +    struct sss_ini_initdata *init_data;
> >> +    struct ref_array *ra;
> >> +    char *msg;
> >> +    uint32_t i = 0;
> >> +    size_t num_errors;
> >> +    char **strs = NULL;
> >> +    TALLOC_CTX *tmp_ctx = NULL;
> >> +
> >> +    tmp_ctx = talloc_new(NULL);
> >> +    init_data = sss_ini_initdata_init(tmp_ctx);
> >> +    if (!init_data) {
> >> +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> >> +        ret = ENOMEM;
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Open config file */
> >> +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_TRACE_FUNC,
> >> +              "sss_ini_config_file_open failed: %s [%d]\n",
> >> strerror(ret),
> >> +              ret);
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Check the file permissions */
> >> +    ret = sss_ini_config_access_check(init_data);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >> +              "Permission check on config file failed.\n");
> > IMHO, we shoudl inform about wrong permission here with printf.
> > So it can be translated.
> > It's quite important info for users.
> >
> >> +        ret = EPERM;
> >> +        goto done;
> >> +    }
> >> +
> >> +    ret = sss_ini_get_config(init_data,
> >> +                             SSSD_CONFIG_FILE,
> >> +                             CONFDB_DEFAULT_CONFIG_DIR);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load
> configuration\n");
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Read rules */
> >> +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> >> +                                       SSSDDATADIR"/cfg_rules.ini",
> >> +                                       &strs, &num_errors);
> >> +    if (ret) {
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Output from validators */
> >> +    printf(_("Issues identified by validators: %lu\n"), num_errors);
> >                                                   ^^^
> >                                                  %zu shoudl be used
> > for size_t
> >                                                  otherwise there is a
> > warning on
> >                                                  32bit architectures.
> >
> > otherwise LGTM
> >
> > LS
>
>  New patches attached. I also did one more change that
>  Lukas requested off-list.
>
>  Michal
>  
Sorry, I sent old patches.

Michal

[Lukas Slebodnik]

On (07/07/16 14:10), Michal Židek wrote:
> On 07/07/2016 01:54 PM, Michal Židek wrote:
> > On 07/07/2016 01:20 PM, Lukas Slebodnik wrote:
> > > On (07/07/16 12:37), Michal Židek wrote:
> > > > On 07/04/2016 05:27 PM, Michal Židek wrote:
> > > > > On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> > > > > > On (01/07/16 17:26), Michal Židek wrote:
> > > > > > > Hello!
> > > > > > > 
> > > > > > > This patch adds new command config-check
> > > > > > > for sssctl tool. The output looks like this:
> > > > > > > 
> > > > > > > 
> > > > > > > Issues identified by validators: 3
> > > > > > > [rule/allowed_sections]: Section [SectionFOO] is not
> allowed.
> > > > > > > Check for
> > > > > > > typos.
> > > > > > > [rule/allowed_sssd_options]: Attribute 'unlknowww'
> is not allowed in
> > > > > > > section
> > > > > > > 'sssd'. Check for typos.
> > > > > > > [rule/allowed_sssd_options]: Attribute
> 'unknown_option' is not
> > > > > > > allowed in
> > > > > > > section 'sssd'. Check for typos.
> > > > > > > 
> > > > > > > Messages generated during configuration merging: 4
> > > > > > > File conf.dd did not match provided patterns. Skipping.
> > > > > > > Errors detected while parsing:
> /etc/sssd/conf.d/snip-bad.conf.
> > > > > > > Error (5) on line 5: Equal sign is missing.
> > > > > > > Due to errors file /etc/sssd/conf.d/snip-bad.conf is not
> considered.
> > > > > > > Skipping.
> > > > > > > 
> > > > > > > Used configuration snippet files: 1
> > > > > > > /etc/sssd/conf.d/snip-good.conf
> > > > > > > 
> > > > > > > 
> > > > > > > Currently it can only be used by root and checks only
> > > > > > > configuration in default path.
> > > > > > > 
> > > > > > > Michal
> > > > > > 
> > > > > > >  From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17
> 00:00:00
> > > > > > > 2001
> > > > > > > From: =?UTF-8?q?Michal=20=C5=BDidek?=
> <mzidek(a)redhat.com&gt;
> > > > > > > Date: Fri, 1 Jul 2016 15:10:30 +0200
> > > > > > > Subject: [PATCH 1/4] sss_ini: Small refacoring of
> > > > > > > sss_ini_call_validators
> > > > > > > 
> > > > > > > Separate logic to fill errobj so that
> > > > > > > the errors can be printed by the caller.
> > > > > > > ---
> > > > > > > src/util/sss_ini.c | 49
> > > > > > > ++++++++++++++++++++++++++++++++++++-------------
> > > > > > > src/util/sss_ini.h | 12 ++++++++++++
> > > > > > > 2 files changed, 48 insertions(+), 13 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> > > > > > > index b4dbb07..78f5490 100644
> > > > > > > --- a/src/util/sss_ini.c
> > > > > > > +++ b/src/util/sss_ini.c
> > > > > > > @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> > > > > > > sss_ini_initdata *data,
> > > > > > > {
> > > > > > > #ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > > > >       int ret;
> > > > > > > -    struct ini_cfgobj *rules_cfgobj = NULL;
> > > > > > >       struct ini_errobj *errobj = NULL;
> > > > > > > 
> > > > > > >       ret = ini_errobj_create(&errobj);
> > > > > > > @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> > > > > > > sss_ini_initdata *data,
> > > > > > >           goto done;
> > > > > > >       }
> > > > > > > 
> > > > > > > -    ret = ini_rules_read_from_file(rules_path,
> &rules_cfgobj);
> > > > > > > +    ret = sss_ini_call_validators_errobj(data,
> > > > > > > +                                         rules_path,
> > > > > > > +                                         errobj);
> > > > > > >       if (ret != EOK) {
> > > > > > > -        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > > > -              "Failed to read sssd.conf schema %d
> [%s]\n", ret,
> > > > > > > strerror(ret));
> > > > > > > -        goto done;
> > > > > > > -    }
> > > > > > > -
> > > > > > > -    ret = ini_rules_check(rules_cfgobj,
> data->sssd_config, NULL,
> > > > > > > errobj);
> > > > > > > -    if (ret != EOK) {
> > > > > > > -        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > > > -              "ini_rules_check failed %d [%s]\n",
> ret,
> > > > > > > strerror(ret));
> > > > > > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > > > > > > +              "Failed to get errors from
> validators.\n");
> > > > > > >           goto done;
> > > > > > >       }
> > > > > > > 
> > > > > > > @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> > > > > > > sss_ini_initdata *data,
> > > > > > >           ini_errobj_next(errobj);
> > > > > > >       }
> > > > > > > 
> > > > > > > +    ret = EOK;
> > > > > > > +
> > > > > > > done:
> > > > > > > -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> > > > > > >       ini_errobj_destroy(&errobj);
> > > > > > > -
> > > > > > >       return ret;
> > > > > > > #else
> > > > > > >       DEBUG(SSSDBG_TRACE_FUNC,
> > > > > > > @@ -590,3 +584,32 @@ done:
> > > > > > >       return EOK;
> > > > > > > #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > > > > > > }
> > > > > > > +
> > > > > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > > > > +int sss_ini_call_validators_errobj(struct sss_ini_initdata
> *data,
> > > > > > > +                                   const char *rules_path,
> > > > > > > +                                   struct ini_errobj
> *errobj)
> > > > > > > +{
> > > > > > > +    int ret;
> > > > > > > +    struct ini_cfgobj *rules_cfgobj = NULL;
> > > > > > > +
> > > > > > > +    ret = ini_rules_read_from_file(rules_path,
> &rules_cfgobj);
> > > > > > > +    if (ret != EOK) {
> > > > > > > +        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > > > +              "Failed to read sssd.conf schema %d
> [%s]\n", ret,
> > > > > > > strerror(ret));
> > > > > > > +        goto done;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    ret = ini_rules_check(rules_cfgobj,
> data->sssd_config, NULL,
> > > > > > > errobj);
> > > > > > > +    if (ret != EOK) {
> > > > > > > +        DEBUG(SSSDBG_FATAL_FAILURE,
> > > > > > > +              "ini_rules_check failed %d [%s]\n",
> ret,
> > > > > > > strerror(ret));
> > > > > > > +        goto done;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +done:
> > > > > > > +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> > > > > > > +
> > > > > > > +    return ret;
> > > > > > > +}
> > > > > > > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > > > > > > diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> > > > > > > index 7734bab..77943d6 100644
> > > > > > > --- a/src/util/sss_ini.h
> > > > > > > +++ b/src/util/sss_ini.h
> > > > > > > @@ -27,6 +27,10 @@
> > > > > > > #ifndef __SSS_INI_H__
> > > > > > > #define __SSS_INI_H__
> > > > > > > 
> > > > > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > > > > +#include <ini_configobj.h>
> > > > > > > +#endif
> > > > > > > +
> > > > > > > /* Structure declarations */
> > > > > > > 
> > > > > > > /* INI data structure */
> > > > > > > @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX
> *mem_ctx,
> > > > > > > int sss_ini_call_validators(struct sss_ini_initdata *data,
> > > > > > >                               const char *rules_path);
> > > > > > > 
> > > > > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > > > > +/* Get errors from validators with ini_errobj */
> > > > > > > +int sss_ini_call_validators_errobj(struct sss_ini_initdata
> *data,
> > > > > > > +                                   const char *rules_path,
> > > > > > > +                                   struct ini_errobj
> *errobj);
> > > > > > > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> > > > > > > +
> > > > > > NACK
> > > > > > 
> > > > > > src/util/sss_ini.h must not contain conditional build.
> > > > > > That's the putpose of implementation file.
> > > > > > 
> > > > > > Header file shoudl contain just a prototypes
> > > > > > becuase there might be missing prototypes if you forgot
> > > > > > include "config.h" before this header file.
> > > > > > 
> > > > > > LS
> > > > > 
> > > > > Ok,
> > > > > 
> > > > > I did some refactoring so that the errobj is not
> > > > > used as parameter to the functions to avoid the
> > > > > conditions in .h file.
> > > > > 
> > > > > See the attached patches.
> > > > > 
> > > > > I also pushed it to CI:
> > > > > http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> > > > > 
> > > > > Michal
> > > > > 
> > > > 
> > > > Lukas requested some changes off list.
> > > > 
> > > > New patches attached.
> > > > 
> > > > I also pushed it to CI:
> > > > http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
> > > > 
> > > > Michal
> > > > 
> > > 
> > > > From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> > > > Date: Fri, 1 Jul 2016 15:10:30 +0200
> > > > Subject: [PATCH 1/2] sss_ini: Small refacoring of
> > > > sss_ini_call_validators
> > > > 
> > > > Separate logic to fill errobj so that
> > > > the errors can be printed by the caller.
> > > > ---
> > > ACK
> > > 
> > > > From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> > > > Date: Mon, 27 Jun 2016 17:33:14 +0200
> > > > Subject: [PATCH 2/2] sssctl: Add config-check command
> > > > 
> > > > Fixes:
> > > > https://fedorahosted.org/sssd/ticket/2269
> > > > 
> > > > sssctl sconfig-check command allows to
> > > > call SSSD config file validators on
> > > > demand.
> > > > ---
> > > > Makefile.am                      |   1 +
> > > > src/tools/sssctl/sssctl.c        |   4 ++
> > > > src/tools/sssctl/sssctl.h        |   4 ++
> > > > src/tools/sssctl/sssctl_config.c | 130
> > > > +++++++++++++++++++++++++++++++++++++++
> > > > src/util/sss_ini.c               |   2 -
> > > > 5 files changed, 139 insertions(+), 2 deletions(-)
> > > > create mode 100644 src/tools/sssctl/sssctl_config.c
> > > > 
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 4089b69..706b60d 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
> > > >      src/tools/sssctl/sssctl_logs.c \
> > > >      src/tools/sssctl/sssctl_domains.c \
> > > >      src/tools/sssctl/sssctl_sifp.c \
> > > > +    src/tools/sssctl/sssctl_config.c \
> > > >      $(SSSD_TOOLS_OBJ) \
> > > >      $(NULL)
> > > > sssctl_LDADD = \
> > > > diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> > > > index be5f1b4..86656f1 100644
> > > > --- a/src/tools/sssctl/sssctl.c
> > > > +++ b/src/tools/sssctl/sssctl.c
> > > > @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
> > > >          SSS_TOOL_DELIMITER("Log files tools:"),
> > > >          SSS_TOOL_COMMAND("remove-logs", "Remove existing
> SSSD log
> > > > files", 0, sssctl_remove_logs),
> > > >          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log
> files in
> > > > tarball", 0, sssctl_fetch_logs),
> > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> > > > +        SSS_TOOL_COMMAND("config-check", "Perform static
> analysis of
> > > > SSSD configuration", 0, sssctl_config_check),
> > > > +#endif
> > > >          {NULL, NULL, 0, NULL}
> > > >      };
> > > > 
> > > > diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> > > > index ae6e62c..be62475 100644
> > > > --- a/src/tools/sssctl/sssctl.h
> > > > +++ b/src/tools/sssctl/sssctl.h
> > > > @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline *cmdline,
> > > >                          struct sss_tool_ctx *tool_ctx,
> > > >                          void *pvt);
> > > > 
> > > > +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> > > > +                            struct sss_tool_ctx *tool_ctx,
> > > > +                            void *pvt);
> > > > +
> > > > #endif /* _SSSCTL_H_ */
> > > > diff --git a/src/tools/sssctl/sssctl_config.c
> > > > b/src/tools/sssctl/sssctl_config.c
> > > > new file mode 100644
> > > > index 0000000..a2d1350
> > > > --- /dev/null
> > > > +++ b/src/tools/sssctl/sssctl_config.c
> > > > @@ -0,0 +1,130 @@
> > > > +/*
> > > > +    Authors:
> > > > +        Michal Židek <mzidek(a)redhat.com&gt;
> > > > +
> > > > +    Copyright (C) 2016 Red Hat
> > > > +
> > > > +    This program is free software; you can redistribute it and/or
> > > > modify
> > > > +    it under the terms of the GNU General Public License as
> > > > published by
> > > > +    the Free Software Foundation; either version 3 of the License, or
> > > > +    (at your option) any later version.
> > > > +
> > > > +    This program is distributed in the hope that it will be useful,
> > > > +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > +    GNU General Public License for more details.
> > > > +
> > > > +    You should have received a copy of the GNU General Public License
> > > > +    along with this program.  If not, see
> > > > <http://www.gnu.org/licenses/>;.
> > > > +*/
> > > > +
> > > > +#include "config.h"
> > > > +
> > > > +#include <popt.h>
> > > > +#include <stdio.h>
> > > > +#include <ini_configobj.h>
> > > > +
> > > > +#include "util/util.h"
> > > > +#include "util/sss_ini.h"
> > > > +#include "tools/common/sss_tools.h"
> > > > +#include "tools/common/sss_process.h"
> > > > +#include "tools/sssctl/sssctl.h"
> > > > +#include "confdb/confdb.h"
> > > > +
> > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> > > > +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> > > > +                            struct sss_tool_ctx *tool_ctx,
> > > > +                            void *pvt)
> > > > +{
> > > > +    errno_t ret;
> > > > +    struct ini_errobj *errobj = NULL;
> > > > +    struct sss_ini_initdata *init_data;
> > > > +    struct ref_array *ra;
> > > > +    char *msg;
> > > > +    uint32_t i = 0;
> > > > +    size_t num_errors;
> > > > +    char **strs = NULL;
> > > > +    TALLOC_CTX *tmp_ctx = NULL;
> > > > +
> > > > +    tmp_ctx = talloc_new(NULL);
> > > > +    init_data = sss_ini_initdata_init(tmp_ctx);
> > > > +    if (!init_data) {
> > > > +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> > > > +        ret = ENOMEM;
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    /* Open config file */
> > > > +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> > > > +    if (ret != EOK) {
> > > > +        DEBUG(SSSDBG_TRACE_FUNC,
> > > > +              "sss_ini_config_file_open failed: %s [%d]\n",
> > > > strerror(ret),
> > > > +              ret);
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    /* Check the file permissions */
> > > > +    ret = sss_ini_config_access_check(init_data);
> > > > +    if (ret != EOK) {
> > > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> > > > +              "Permission check on config file failed.\n");
> > > IMHO, we shoudl inform about wrong permission here with printf.
> > > So it can be translated.
> > > It's quite important info for users.
> > > 
> > > > +        ret = EPERM;
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    ret = sss_ini_get_config(init_data,
> > > > +                             SSSD_CONFIG_FILE,
> > > > +                             CONFDB_DEFAULT_CONFIG_DIR);
> > > > +    if (ret != EOK) {
> > > > +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load
> configuration\n");
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    /* Read rules */
> > > > +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> > > > +                                      
> SSSDDATADIR"/cfg_rules.ini",
> > > > +                                       &strs, &num_errors);
> > > > +    if (ret) {
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    /* Output from validators */
> > > > +    printf(_("Issues identified by validators: %lu\n"),
> num_errors);
> > >                                                   ^^^
> > >                                                  %zu shoudl be used
> > > for size_t
> > >                                                  otherwise there is a
> > > warning on
> > >                                                  32bit architectures.
> > > 
> > > otherwise LGTM
> > > 
> > > LS
> > 
> > New patches attached. I also did one more change that
> > Lukas requested off-list.
> > 
> > Michal
> > 
>
> Sorry, I sent old patches.
>  
ACK

LS

[Lukas Slebodnik]

On (07/07/16 15:21), Lukas Slebodnik wrote:
> On (07/07/16 14:10), Michal Židek wrote:
> >On 07/07/2016 01:54 PM, Michal Židek wrote:
> >> On 07/07/2016 01:20 PM, Lukas Slebodnik wrote:
> >> > On (07/07/16 12:37), Michal Židek wrote:
> >> > > On 07/04/2016 05:27 PM, Michal Židek wrote:
> >> > > > On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> >> > > > > On (01/07/16 17:26), Michal Židek wrote:
> >> > > > > > Hello!
> >> > > > > > 
> >> > > > > > This patch adds new command config-check
> >> > > > > > for sssctl tool. The output looks like this:
> >> > > > > > 
> >> > > > > > 
> >> > > > > > Issues identified by validators: 3
> >> > > > > > [rule/allowed_sections]: Section [SectionFOO] is not
> allowed.
> >> > > > > > Check for
> >> > > > > > typos.
> >> > > > > > [rule/allowed_sssd_options]: Attribute
> 'unlknowww' is not allowed in
> >> > > > > > section
> >> > > > > > 'sssd'. Check for typos.
> >> > > > > > [rule/allowed_sssd_options]: Attribute
> 'unknown_option' is not
> >> > > > > > allowed in
> >> > > > > > section 'sssd'. Check for typos.
> >> > > > > > 
> >> > > > > > Messages generated during configuration merging: 4
> >> > > > > > File conf.dd did not match provided patterns. Skipping.
> >> > > > > > Errors detected while parsing:
> /etc/sssd/conf.d/snip-bad.conf.
> >> > > > > > Error (5) on line 5: Equal sign is missing.
> >> > > > > > Due to errors file /etc/sssd/conf.d/snip-bad.conf is not
> considered.
> >> > > > > > Skipping.
> >> > > > > > 
> >> > > > > > Used configuration snippet files: 1
> >> > > > > > /etc/sssd/conf.d/snip-good.conf
> >> > > > > > 
> >> > > > > > 
> >> > > > > > Currently it can only be used by root and checks only
> >> > > > > > configuration in default path.
> >> > > > > > 
> >> > > > > > Michal
> >> > > > > 
> >> > > > > >  From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep
> 17 00:00:00
> >> > > > > > 2001
> >> > > > > > From: =?UTF-8?q?Michal=20=C5=BDidek?=
> <mzidek(a)redhat.com&gt;
> >> > > > > > Date: Fri, 1 Jul 2016 15:10:30 +0200
> >> > > > > > Subject: [PATCH 1/4] sss_ini: Small refacoring of
> >> > > > > > sss_ini_call_validators
> >> > > > > > 
> >> > > > > > Separate logic to fill errobj so that
> >> > > > > > the errors can be printed by the caller.
> >> > > > > > ---
> >> > > > > > src/util/sss_ini.c | 49
> >> > > > > > ++++++++++++++++++++++++++++++++++++-------------
> >> > > > > > src/util/sss_ini.h | 12 ++++++++++++
> >> > > > > > 2 files changed, 48 insertions(+), 13 deletions(-)
> >> > > > > > 
> >> > > > > > diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> >> > > > > > index b4dbb07..78f5490 100644
> >> > > > > > --- a/src/util/sss_ini.c
> >> > > > > > +++ b/src/util/sss_ini.c
> >> > > > > > @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> >> > > > > > sss_ini_initdata *data,
> >> > > > > > {
> >> > > > > > #ifdef HAVE_LIBINI_CONFIG_V1_3
> >> > > > > >       int ret;
> >> > > > > > -    struct ini_cfgobj *rules_cfgobj = NULL;
> >> > > > > >       struct ini_errobj *errobj = NULL;
> >> > > > > > 
> >> > > > > >       ret = ini_errobj_create(&errobj);
> >> > > > > > @@ -558,17 +557,12 @@ int
> sss_ini_call_validators(struct
> >> > > > > > sss_ini_initdata *data,
> >> > > > > >           goto done;
> >> > > > > >       }
> >> > > > > > 
> >> > > > > > -    ret = ini_rules_read_from_file(rules_path,
> &rules_cfgobj);
> >> > > > > > +    ret = sss_ini_call_validators_errobj(data,
> >> > > > > > +                                         rules_path,
> >> > > > > > +                                         errobj);
> >> > > > > >       if (ret != EOK) {
> >> > > > > > -        DEBUG(SSSDBG_FATAL_FAILURE,
> >> > > > > > -              "Failed to read sssd.conf schema %d
> [%s]\n", ret,
> >> > > > > > strerror(ret));
> >> > > > > > -        goto done;
> >> > > > > > -    }
> >> > > > > > -
> >> > > > > > -    ret = ini_rules_check(rules_cfgobj,
> data->sssd_config, NULL,
> >> > > > > > errobj);
> >> > > > > > -    if (ret != EOK) {
> >> > > > > > -        DEBUG(SSSDBG_FATAL_FAILURE,
> >> > > > > > -              "ini_rules_check failed %d
> [%s]\n", ret,
> >> > > > > > strerror(ret));
> >> > > > > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> >> > > > > > +              "Failed to get errors from
> validators.\n");
> >> > > > > >           goto done;
> >> > > > > >       }
> >> > > > > > 
> >> > > > > > @@ -579,10 +573,10 @@ int
> sss_ini_call_validators(struct
> >> > > > > > sss_ini_initdata *data,
> >> > > > > >           ini_errobj_next(errobj);
> >> > > > > >       }
> >> > > > > > 
> >> > > > > > +    ret = EOK;
> >> > > > > > +
> >> > > > > > done:
> >> > > > > > -    if (rules_cfgobj)
> ini_config_destroy(rules_cfgobj);
> >> > > > > >       ini_errobj_destroy(&errobj);
> >> > > > > > -
> >> > > > > >       return ret;
> >> > > > > > #else
> >> > > > > >       DEBUG(SSSDBG_TRACE_FUNC,
> >> > > > > > @@ -590,3 +584,32 @@ done:
> >> > > > > >       return EOK;
> >> > > > > > #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >> > > > > > }
> >> > > > > > +
> >> > > > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> > > > > > +int sss_ini_call_validators_errobj(struct
> sss_ini_initdata *data,
> >> > > > > > +                                   const char
> *rules_path,
> >> > > > > > +                                   struct ini_errobj
> *errobj)
> >> > > > > > +{
> >> > > > > > +    int ret;
> >> > > > > > +    struct ini_cfgobj *rules_cfgobj = NULL;
> >> > > > > > +
> >> > > > > > +    ret = ini_rules_read_from_file(rules_path,
> &rules_cfgobj);
> >> > > > > > +    if (ret != EOK) {
> >> > > > > > +        DEBUG(SSSDBG_FATAL_FAILURE,
> >> > > > > > +              "Failed to read sssd.conf schema %d
> [%s]\n", ret,
> >> > > > > > strerror(ret));
> >> > > > > > +        goto done;
> >> > > > > > +    }
> >> > > > > > +
> >> > > > > > +    ret = ini_rules_check(rules_cfgobj,
> data->sssd_config, NULL,
> >> > > > > > errobj);
> >> > > > > > +    if (ret != EOK) {
> >> > > > > > +        DEBUG(SSSDBG_FATAL_FAILURE,
> >> > > > > > +              "ini_rules_check failed %d
> [%s]\n", ret,
> >> > > > > > strerror(ret));
> >> > > > > > +        goto done;
> >> > > > > > +    }
> >> > > > > > +
> >> > > > > > +done:
> >> > > > > > +    if (rules_cfgobj)
> ini_config_destroy(rules_cfgobj);
> >> > > > > > +
> >> > > > > > +    return ret;
> >> > > > > > +}
> >> > > > > > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >> > > > > > diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> >> > > > > > index 7734bab..77943d6 100644
> >> > > > > > --- a/src/util/sss_ini.h
> >> > > > > > +++ b/src/util/sss_ini.h
> >> > > > > > @@ -27,6 +27,10 @@
> >> > > > > > #ifndef __SSS_INI_H__
> >> > > > > > #define __SSS_INI_H__
> >> > > > > > 
> >> > > > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> > > > > > +#include <ini_configobj.h>
> >> > > > > > +#endif
> >> > > > > > +
> >> > > > > > /* Structure declarations */
> >> > > > > > 
> >> > > > > > /* INI data structure */
> >> > > > > > @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX
> *mem_ctx,
> >> > > > > > int sss_ini_call_validators(struct sss_ini_initdata
> *data,
> >> > > > > >                               const char *rules_path);
> >> > > > > > 
> >> > > > > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> > > > > > +/* Get errors from validators with ini_errobj */
> >> > > > > > +int sss_ini_call_validators_errobj(struct
> sss_ini_initdata *data,
> >> > > > > > +                                   const char
> *rules_path,
> >> > > > > > +                                   struct ini_errobj
> *errobj);
> >> > > > > > +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >> > > > > > +
> >> > > > > NACK
> >> > > > > 
> >> > > > > src/util/sss_ini.h must not contain conditional build.
> >> > > > > That's the putpose of implementation file.
> >> > > > > 
> >> > > > > Header file shoudl contain just a prototypes
> >> > > > > becuase there might be missing prototypes if you forgot
> >> > > > > include "config.h" before this header file.
> >> > > > > 
> >> > > > > LS
> >> > > > 
> >> > > > Ok,
> >> > > > 
> >> > > > I did some refactoring so that the errobj is not
> >> > > > used as parameter to the functions to avoid the
> >> > > > conditions in .h file.
> >> > > > 
> >> > > > See the attached patches.
> >> > > > 
> >> > > > I also pushed it to CI:
> >> > > > http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> >> > > > 
> >> > > > Michal
> >> > > > 
> >> > > 
> >> > > Lukas requested some changes off list.
> >> > > 
> >> > > New patches attached.
> >> > > 
> >> > > I also pushed it to CI:
> >> > > http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
> >> > > 
> >> > > Michal
> >> > > 
> >> > 
> >> > > From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00 2001
> >> > > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> > > Date: Fri, 1 Jul 2016 15:10:30 +0200
> >> > > Subject: [PATCH 1/2] sss_ini: Small refacoring of
> >> > > sss_ini_call_validators
> >> > > 
> >> > > Separate logic to fill errobj so that
> >> > > the errors can be printed by the caller.
> >> > > ---
> >> > ACK
> >> > 
> >> > > From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00 2001
> >> > > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> > > Date: Mon, 27 Jun 2016 17:33:14 +0200
> >> > > Subject: [PATCH 2/2] sssctl: Add config-check command
> >> > > 
> >> > > Fixes:
> >> > > https://fedorahosted.org/sssd/ticket/2269
> >> > > 
> >> > > sssctl sconfig-check command allows to
> >> > > call SSSD config file validators on
> >> > > demand.
> >> > > ---
> >> > > Makefile.am                      |   1 +
> >> > > src/tools/sssctl/sssctl.c        |   4 ++
> >> > > src/tools/sssctl/sssctl.h        |   4 ++
> >> > > src/tools/sssctl/sssctl_config.c | 130
> >> > > +++++++++++++++++++++++++++++++++++++++
> >> > > src/util/sss_ini.c               |   2 -
> >> > > 5 files changed, 139 insertions(+), 2 deletions(-)
> >> > > create mode 100644 src/tools/sssctl/sssctl_config.c
> >> > > 
> >> > > diff --git a/Makefile.am b/Makefile.am
> >> > > index 4089b69..706b60d 100644
> >> > > --- a/Makefile.am
> >> > > +++ b/Makefile.am
> >> > > @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
> >> > >      src/tools/sssctl/sssctl_logs.c \
> >> > >      src/tools/sssctl/sssctl_domains.c \
> >> > >      src/tools/sssctl/sssctl_sifp.c \
> >> > > +    src/tools/sssctl/sssctl_config.c \
> >> > >      $(SSSD_TOOLS_OBJ) \
> >> > >      $(NULL)
> >> > > sssctl_LDADD = \
> >> > > diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> >> > > index be5f1b4..86656f1 100644
> >> > > --- a/src/tools/sssctl/sssctl.c
> >> > > +++ b/src/tools/sssctl/sssctl.c
> >> > > @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
> >> > >          SSS_TOOL_DELIMITER("Log files tools:"),
> >> > >          SSS_TOOL_COMMAND("remove-logs", "Remove
> existing SSSD log
> >> > > files", 0, sssctl_remove_logs),
> >> > >          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD
> log files in
> >> > > tarball", 0, sssctl_fetch_logs),
> >> > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> > > +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> >> > > +        SSS_TOOL_COMMAND("config-check", "Perform
> static analysis of
> >> > > SSSD configuration", 0, sssctl_config_check),
> >> > > +#endif
> >> > >          {NULL, NULL, 0, NULL}
> >> > >      };
> >> > > 
> >> > > diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> >> > > index ae6e62c..be62475 100644
> >> > > --- a/src/tools/sssctl/sssctl.h
> >> > > +++ b/src/tools/sssctl/sssctl.h
> >> > > @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline
> *cmdline,
> >> > >                          struct sss_tool_ctx *tool_ctx,
> >> > >                          void *pvt);
> >> > > 
> >> > > +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >> > > +                            struct sss_tool_ctx *tool_ctx,
> >> > > +                            void *pvt);
> >> > > +
> >> > > #endif /* _SSSCTL_H_ */
> >> > > diff --git a/src/tools/sssctl/sssctl_config.c
> >> > > b/src/tools/sssctl/sssctl_config.c
> >> > > new file mode 100644
> >> > > index 0000000..a2d1350
> >> > > --- /dev/null
> >> > > +++ b/src/tools/sssctl/sssctl_config.c
> >> > > @@ -0,0 +1,130 @@
> >> > > +/*
> >> > > +    Authors:
> >> > > +        Michal Židek <mzidek(a)redhat.com&gt;
> >> > > +
> >> > > +    Copyright (C) 2016 Red Hat
> >> > > +
> >> > > +    This program is free software; you can redistribute it and/or
> >> > > modify
> >> > > +    it under the terms of the GNU General Public License as
> >> > > published by
> >> > > +    the Free Software Foundation; either version 3 of the License, or
> >> > > +    (at your option) any later version.
> >> > > +
> >> > > +    This program is distributed in the hope that it will be useful,
> >> > > +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > > +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> > > +    GNU General Public License for more details.
> >> > > +
> >> > > +    You should have received a copy of the GNU General Public License
> >> > > +    along with this program.  If not, see
> >> > > <http://www.gnu.org/licenses/>;.
> >> > > +*/
> >> > > +
> >> > > +#include "config.h"
> >> > > +
> >> > > +#include <popt.h>
> >> > > +#include <stdio.h>
> >> > > +#include <ini_configobj.h>
> >> > > +
> >> > > +#include "util/util.h"
> >> > > +#include "util/sss_ini.h"
> >> > > +#include "tools/common/sss_tools.h"
> >> > > +#include "tools/common/sss_process.h"
> >> > > +#include "tools/sssctl/sssctl.h"
> >> > > +#include "confdb/confdb.h"
> >> > > +
> >> > > +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> > > +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >> > > +                            struct sss_tool_ctx *tool_ctx,
> >> > > +                            void *pvt)
> >> > > +{
> >> > > +    errno_t ret;
> >> > > +    struct ini_errobj *errobj = NULL;
> >> > > +    struct sss_ini_initdata *init_data;
> >> > > +    struct ref_array *ra;
> >> > > +    char *msg;
> >> > > +    uint32_t i = 0;
> >> > > +    size_t num_errors;
> >> > > +    char **strs = NULL;
> >> > > +    TALLOC_CTX *tmp_ctx = NULL;
> >> > > +
> >> > > +    tmp_ctx = talloc_new(NULL);
> >> > > +    init_data = sss_ini_initdata_init(tmp_ctx);
> >> > > +    if (!init_data) {
> >> > > +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> >> > > +        ret = ENOMEM;
> >> > > +        goto done;
> >> > > +    }
> >> > > +
> >> > > +    /* Open config file */
> >> > > +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> >> > > +    if (ret != EOK) {
> >> > > +        DEBUG(SSSDBG_TRACE_FUNC,
> >> > > +              "sss_ini_config_file_open failed: %s [%d]\n",
> >> > > strerror(ret),
> >> > > +              ret);
> >> > > +        goto done;
> >> > > +    }
> >> > > +
> >> > > +    /* Check the file permissions */
> >> > > +    ret = sss_ini_config_access_check(init_data);
> >> > > +    if (ret != EOK) {
> >> > > +        DEBUG(SSSDBG_CRIT_FAILURE,
> >> > > +              "Permission check on config file failed.\n");
> >> > IMHO, we shoudl inform about wrong permission here with printf.
> >> > So it can be translated.
> >> > It's quite important info for users.
> >> > 
> >> > > +        ret = EPERM;
> >> > > +        goto done;
> >> > > +    }
> >> > > +
> >> > > +    ret = sss_ini_get_config(init_data,
> >> > > +                             SSSD_CONFIG_FILE,
> >> > > +                             CONFDB_DEFAULT_CONFIG_DIR);
> >> > > +    if (ret != EOK) {
> >> > > +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load
> configuration\n");
> >> > > +        goto done;
> >> > > +    }
> >> > > +
> >> > > +    /* Read rules */
> >> > > +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> >> > > +                                      
> SSSDDATADIR"/cfg_rules.ini",
> >> > > +                                       &strs, &num_errors);
> >> > > +    if (ret) {
> >> > > +        goto done;
> >> > > +    }
> >> > > +
> >> > > +    /* Output from validators */
> >> > > +    printf(_("Issues identified by validators: %lu\n"),
> num_errors);
> >> >                                                   ^^^
> >> >                                                  %zu shoudl be used
> >> > for size_t
> >> >                                                  otherwise there is a
> >> > warning on
> >> >                                                  32bit architectures.
> >> > 
> >> > otherwise LGTM
> >> > 
> >> > LS
> >> 
> >> New patches attached. I also did one more change that
> >> Lukas requested off-list.
> >> 
> >> Michal
> >> 
> >
> >Sorry, I sent old patches.
> >
> ACK
>  
master:
* e088912418fd4db750f2097dfde8ef9b77303f05
* 199984c7972272f8162a356cda139c22f6f08556

LS

[Pavel Březina]

On 07/07/2016 01:54 PM, Michal Židek wrote:
>  On 07/07/2016 01:20 PM, Lukas Slebodnik wrote:
> > On (07/07/16 12:37), Michal Židek wrote:
> >> On 07/04/2016 05:27 PM, Michal Židek wrote:
> >>> On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> >>>> On (01/07/16 17:26), Michal Židek wrote:
> >>>>> Hello!
> >>>>>
> >>>>> This patch adds new command config-check
> >>>>> for sssctl tool. The output looks like this:
> >>>>>
> >>>>>
> >>>>> Issues identified by validators: 3
> >>>>> [rule/allowed_sections]: Section [SectionFOO] is not allowed.
> >>>>> Check for
> >>>>> typos.
> >>>>> [rule/allowed_sssd_options]: Attribute 'unlknowww' is not
> allowed in
> >>>>> section
> >>>>> 'sssd'. Check for typos.
> >>>>> [rule/allowed_sssd_options]: Attribute 'unknown_option' is
> not
> >>>>> allowed in
> >>>>> section 'sssd'. Check for typos.
> >>>>>
> >>>>> Messages generated during configuration merging: 4
> >>>>> File conf.dd did not match provided patterns. Skipping.
> >>>>> Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> >>>>> Error (5) on line 5: Equal sign is missing.
> >>>>> Due to errors file /etc/sssd/conf.d/snip-bad.conf is not considered.
> >>>>> Skipping.
> >>>>>
> >>>>> Used configuration snippet files: 1
> >>>>> /etc/sssd/conf.d/snip-good.conf
> >>>>>
> >>>>>
> >>>>> Currently it can only be used by root and checks only
> >>>>> configuration in default path.
> >>>>>
> >>>>> Michal
> >>>>
> >>>>>  From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17 00:00:00
> >>>>> 2001
> >>>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >>>>> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >>>>> Subject: [PATCH 1/4] sss_ini: Small refacoring of
> >>>>> sss_ini_call_validators
> >>>>>
> >>>>> Separate logic to fill errobj so that
> >>>>> the errors can be printed by the caller.
> >>>>> ---
> >>>>> src/util/sss_ini.c | 49
> >>>>> ++++++++++++++++++++++++++++++++++++-------------
> >>>>> src/util/sss_ini.h | 12 ++++++++++++
> >>>>> 2 files changed, 48 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> >>>>> index b4dbb07..78f5490 100644
> >>>>> --- a/src/util/sss_ini.c
> >>>>> +++ b/src/util/sss_ini.c
> >>>>> @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> >>>>> sss_ini_initdata *data,
> >>>>> {
> >>>>> #ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>>       int ret;
> >>>>> -    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>>       struct ini_errobj *errobj = NULL;
> >>>>>
> >>>>>       ret = ini_errobj_create(&errobj);
> >>>>> @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> >>>>> sss_ini_initdata *data,
> >>>>>           goto done;
> >>>>>       }
> >>>>>
> >>>>> -    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >>>>> +    ret = sss_ini_call_validators_errobj(data,
> >>>>> +                                         rules_path,
> >>>>> +                                         errobj);
> >>>>>       if (ret != EOK) {
> >>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> -              "Failed to read sssd.conf schema %d [%s]\n",
> ret,
> >>>>> strerror(ret));
> >>>>> -        goto done;
> >>>>> -    }
> >>>>> -
> >>>>> -    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >>>>> errobj);
> >>>>> -    if (ret != EOK) {
> >>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> -              "ini_rules_check failed %d [%s]\n", ret,
> >>>>> strerror(ret));
> >>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >>>>> +              "Failed to get errors from validators.\n");
> >>>>>           goto done;
> >>>>>       }
> >>>>>
> >>>>> @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> >>>>> sss_ini_initdata *data,
> >>>>>           ini_errobj_next(errobj);
> >>>>>       }
> >>>>>
> >>>>> +    ret = EOK;
> >>>>> +
> >>>>> done:
> >>>>> -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>>       ini_errobj_destroy(&errobj);
> >>>>> -
> >>>>>       return ret;
> >>>>> #else
> >>>>>       DEBUG(SSSDBG_TRACE_FUNC,
> >>>>> @@ -590,3 +584,32 @@ done:
> >>>>>       return EOK;
> >>>>> #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>> }
> >>>>> +
> >>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >>>>> +                                   const char *rules_path,
> >>>>> +                                   struct ini_errobj *errobj)
> >>>>> +{
> >>>>> +    int ret;
> >>>>> +    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>> +
> >>>>> +    ret = ini_rules_read_from_file(rules_path, &rules_cfgobj);
> >>>>> +    if (ret != EOK) {
> >>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> +              "Failed to read sssd.conf schema %d [%s]\n",
> ret,
> >>>>> strerror(ret));
> >>>>> +        goto done;
> >>>>> +    }
> >>>>> +
> >>>>> +    ret = ini_rules_check(rules_cfgobj, data->sssd_config, NULL,
> >>>>> errobj);
> >>>>> +    if (ret != EOK) {
> >>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>> +              "ini_rules_check failed %d [%s]\n", ret,
> >>>>> strerror(ret));
> >>>>> +        goto done;
> >>>>> +    }
> >>>>> +
> >>>>> +done:
> >>>>> +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>> +
> >>>>> +    return ret;
> >>>>> +}
> >>>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>> diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> >>>>> index 7734bab..77943d6 100644
> >>>>> --- a/src/util/sss_ini.h
> >>>>> +++ b/src/util/sss_ini.h
> >>>>> @@ -27,6 +27,10 @@
> >>>>> #ifndef __SSS_INI_H__
> >>>>> #define __SSS_INI_H__
> >>>>>
> >>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>> +#include <ini_configobj.h>
> >>>>> +#endif
> >>>>> +
> >>>>> /* Structure declarations */
> >>>>>
> >>>>> /* INI data structure */
> >>>>> @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX *mem_ctx,
> >>>>> int sss_ini_call_validators(struct sss_ini_initdata *data,
> >>>>>                               const char *rules_path);
> >>>>>
> >>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>> +/* Get errors from validators with ini_errobj */
> >>>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata *data,
> >>>>> +                                   const char *rules_path,
> >>>>> +                                   struct ini_errobj *errobj);
> >>>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>> +
> >>>> NACK
> >>>>
> >>>> src/util/sss_ini.h must not contain conditional build.
> >>>> That's the putpose of implementation file.
> >>>>
> >>>> Header file shoudl contain just a prototypes
> >>>> becuase there might be missing prototypes if you forgot
> >>>> include "config.h" before this header file.
> >>>>
> >>>> LS
> >>>
> >>> Ok,
> >>>
> >>> I did some refactoring so that the errobj is not
> >>> used as parameter to the functions to avoid the
> >>> conditions in .h file.
> >>>
> >>> See the attached patches.
> >>>
> >>> I also pushed it to CI:
> >>> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> >>>
> >>> Michal
> >>>
> >>
> >> Lukas requested some changes off list.
> >>
> >> New patches attached.
> >>
> >> I also pushed it to CI:
> >> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
> >>
> >> Michal
> >>
> >
> >> From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >> Subject: [PATCH 1/2] sss_ini: Small refacoring of
> >> sss_ini_call_validators
> >>
> >> Separate logic to fill errobj so that
> >> the errors can be printed by the caller.
> >> ---
> > ACK
> >
> >> From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >> Date: Mon, 27 Jun 2016 17:33:14 +0200
> >> Subject: [PATCH 2/2] sssctl: Add config-check command
> >>
> >> Fixes:
> >> https://fedorahosted.org/sssd/ticket/2269
> >>
> >> sssctl sconfig-check command allows to
> >> call SSSD config file validators on
> >> demand.
> >> ---
> >> Makefile.am                      |   1 +
> >> src/tools/sssctl/sssctl.c        |   4 ++
> >> src/tools/sssctl/sssctl.h        |   4 ++
> >> src/tools/sssctl/sssctl_config.c | 130
> >> +++++++++++++++++++++++++++++++++++++++
> >> src/util/sss_ini.c               |   2 -
> >> 5 files changed, 139 insertions(+), 2 deletions(-)
> >> create mode 100644 src/tools/sssctl/sssctl_config.c
> >>
> >> diff --git a/Makefile.am b/Makefile.am
> >> index 4089b69..706b60d 100644
> >> --- a/Makefile.am
> >> +++ b/Makefile.am
> >> @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
> >>      src/tools/sssctl/sssctl_logs.c \
> >>      src/tools/sssctl/sssctl_domains.c \
> >>      src/tools/sssctl/sssctl_sifp.c \
> >> +    src/tools/sssctl/sssctl_config.c \
> >>      $(SSSD_TOOLS_OBJ) \
> >>      $(NULL)
> >> sssctl_LDADD = \
> >> diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> >> index be5f1b4..86656f1 100644
> >> --- a/src/tools/sssctl/sssctl.c
> >> +++ b/src/tools/sssctl/sssctl.c
> >> @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
> >>          SSS_TOOL_DELIMITER("Log files tools:"),
> >>          SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD
> log
> >> files", 0, sssctl_remove_logs),
> >>          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log files
> in
> >> tarball", 0, sssctl_fetch_logs),
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> >> +        SSS_TOOL_COMMAND("config-check", "Perform static analysis
> of
> >> SSSD configuration", 0, sssctl_config_check),
> >> +#endif
> >>          {NULL, NULL, 0, NULL}
> >>      };
> >>
> >> diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> >> index ae6e62c..be62475 100644
> >> --- a/src/tools/sssctl/sssctl.h
> >> +++ b/src/tools/sssctl/sssctl.h
> >> @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline *cmdline,
> >>                          struct sss_tool_ctx *tool_ctx,
> >>                          void *pvt);
> >>
> >> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >> +                            struct sss_tool_ctx *tool_ctx,
> >> +                            void *pvt);
> >> +
> >> #endif /* _SSSCTL_H_ */
> >> diff --git a/src/tools/sssctl/sssctl_config.c
> >> b/src/tools/sssctl/sssctl_config.c
> >> new file mode 100644
> >> index 0000000..a2d1350
> >> --- /dev/null
> >> +++ b/src/tools/sssctl/sssctl_config.c
> >> @@ -0,0 +1,130 @@
> >> +/*
> >> +    Authors:
> >> +        Michal Židek <mzidek(a)redhat.com&gt;
> >> +
> >> +    Copyright (C) 2016 Red Hat
> >> +
> >> +    This program is free software; you can redistribute it and/or
> >> modify
> >> +    it under the terms of the GNU General Public License as
> >> published by
> >> +    the Free Software Foundation; either version 3 of the License, or
> >> +    (at your option) any later version.
> >> +
> >> +    This program is distributed in the hope that it will be useful,
> >> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> +    GNU General Public License for more details.
> >> +
> >> +    You should have received a copy of the GNU General Public License
> >> +    along with this program.  If not, see
> >> <http://www.gnu.org/licenses/>;.
> >> +*/
> >> +
> >> +#include "config.h"
> >> +
> >> +#include <popt.h>
> >> +#include <stdio.h>
> >> +#include <ini_configobj.h>
> >> +
> >> +#include "util/util.h"
> >> +#include "util/sss_ini.h"
> >> +#include "tools/common/sss_tools.h"
> >> +#include "tools/common/sss_process.h"
> >> +#include "tools/sssctl/sssctl.h"
> >> +#include "confdb/confdb.h"
> >> +
> >> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >> +                            struct sss_tool_ctx *tool_ctx,
> >> +                            void *pvt)
> >> +{
> >> +    errno_t ret;
> >> +    struct ini_errobj *errobj = NULL;
> >> +    struct sss_ini_initdata *init_data;
> >> +    struct ref_array *ra;
> >> +    char *msg;
> >> +    uint32_t i = 0;
> >> +    size_t num_errors;
> >> +    char **strs = NULL;
> >> +    TALLOC_CTX *tmp_ctx = NULL;
> >> +
> >> +    tmp_ctx = talloc_new(NULL);
> >> +    init_data = sss_ini_initdata_init(tmp_ctx);
> >> +    if (!init_data) {
> >> +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> >> +        ret = ENOMEM;
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Open config file */
> >> +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_TRACE_FUNC,
> >> +              "sss_ini_config_file_open failed: %s [%d]\n",
> >> strerror(ret),
> >> +              ret);
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Check the file permissions */
> >> +    ret = sss_ini_config_access_check(init_data);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >> +              "Permission check on config file failed.\n");
> > IMHO, we shoudl inform about wrong permission here with printf.
> > So it can be translated.
> > It's quite important info for users.
> >
> >> +        ret = EPERM;
> >> +        goto done;
> >> +    }
> >> +
> >> +    ret = sss_ini_get_config(init_data,
> >> +                             SSSD_CONFIG_FILE,
> >> +                             CONFDB_DEFAULT_CONFIG_DIR);
> >> +    if (ret != EOK) {
> >> +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load
> configuration\n");
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Read rules */
> >> +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> >> +                                       SSSDDATADIR"/cfg_rules.ini",
> >> +                                       &strs, &num_errors);
> >> +    if (ret) {
> >> +        goto done;
> >> +    }
> >> +
> >> +    /* Output from validators */
> >> +    printf(_("Issues identified by validators: %lu\n"), num_errors);
> >                                                   ^^^
> >                                                  %zu shoudl be used
> > for size_t
> >                                                  otherwise there is a
> > warning on
> >                                                  32bit architectures.
> >
> > otherwise LGTM
> >
> > LS
>
>  New patches attached. I also did one more change that
>  Lukas requested off-list.
>
>  Michal 
Hi, please use sss_strerror() instead of strerror() in new code. It may 
be also good to print a nice message to stderr on errors that are 
actually expected to happen such as EPERM. And some generic message for 
other errors would be nice as well so we know there is more information 
when you run it again with --debug.

[Michal Židek]

On 07/07/2016 02:15 PM, Pavel Březina wrote:
>  On 07/07/2016 01:54 PM, Michal Židek wrote:
> > On 07/07/2016 01:20 PM, Lukas Slebodnik wrote:
> >> On (07/07/16 12:37), Michal Židek wrote:
> >>> On 07/04/2016 05:27 PM, Michal Židek wrote:
> >>>> On 07/01/2016 05:35 PM, Lukas Slebodnik wrote:
> >>>>> On (01/07/16 17:26), Michal Židek wrote:
> >>>>>> Hello!
> >>>>>>
> >>>>>> This patch adds new command config-check
> >>>>>> for sssctl tool. The output looks like this:
> >>>>>>
> >>>>>>
> >>>>>> Issues identified by validators: 3
> >>>>>> [rule/allowed_sections]: Section [SectionFOO] is not allowed.
> >>>>>> Check for
> >>>>>> typos.
> >>>>>> [rule/allowed_sssd_options]: Attribute 'unlknowww' is not
> allowed in
> >>>>>> section
> >>>>>> 'sssd'. Check for typos.
> >>>>>> [rule/allowed_sssd_options]: Attribute 'unknown_option'
> is not
> >>>>>> allowed in
> >>>>>> section 'sssd'. Check for typos.
> >>>>>>
> >>>>>> Messages generated during configuration merging: 4
> >>>>>> File conf.dd did not match provided patterns. Skipping.
> >>>>>> Errors detected while parsing: /etc/sssd/conf.d/snip-bad.conf.
> >>>>>> Error (5) on line 5: Equal sign is missing.
> >>>>>> Due to errors file /etc/sssd/conf.d/snip-bad.conf is not
> considered.
> >>>>>> Skipping.
> >>>>>>
> >>>>>> Used configuration snippet files: 1
> >>>>>> /etc/sssd/conf.d/snip-good.conf
> >>>>>>
> >>>>>>
> >>>>>> Currently it can only be used by root and checks only
> >>>>>> configuration in default path.
> >>>>>>
> >>>>>> Michal
> >>>>>
> >>>>>>  From 864091da5010b75d9c38e87ab3be467b8bc6f8f2 Mon Sep 17
> 00:00:00
> >>>>>> 2001
> >>>>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >>>>>> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >>>>>> Subject: [PATCH 1/4] sss_ini: Small refacoring of
> >>>>>> sss_ini_call_validators
> >>>>>>
> >>>>>> Separate logic to fill errobj so that
> >>>>>> the errors can be printed by the caller.
> >>>>>> ---
> >>>>>> src/util/sss_ini.c | 49
> >>>>>> ++++++++++++++++++++++++++++++++++++-------------
> >>>>>> src/util/sss_ini.h | 12 ++++++++++++
> >>>>>> 2 files changed, 48 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>> diff --git a/src/util/sss_ini.c b/src/util/sss_ini.c
> >>>>>> index b4dbb07..78f5490 100644
> >>>>>> --- a/src/util/sss_ini.c
> >>>>>> +++ b/src/util/sss_ini.c
> >>>>>> @@ -549,7 +549,6 @@ int sss_ini_call_validators(struct
> >>>>>> sss_ini_initdata *data,
> >>>>>> {
> >>>>>> #ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>>>       int ret;
> >>>>>> -    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>>>       struct ini_errobj *errobj = NULL;
> >>>>>>
> >>>>>>       ret = ini_errobj_create(&errobj);
> >>>>>> @@ -558,17 +557,12 @@ int sss_ini_call_validators(struct
> >>>>>> sss_ini_initdata *data,
> >>>>>>           goto done;
> >>>>>>       }
> >>>>>>
> >>>>>> -    ret = ini_rules_read_from_file(rules_path,
> &rules_cfgobj);
> >>>>>> +    ret = sss_ini_call_validators_errobj(data,
> >>>>>> +                                         rules_path,
> >>>>>> +                                         errobj);
> >>>>>>       if (ret != EOK) {
> >>>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>> -              "Failed to read sssd.conf schema %d
> [%s]\n", ret,
> >>>>>> strerror(ret));
> >>>>>> -        goto done;
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    ret = ini_rules_check(rules_cfgobj, data->sssd_config,
> NULL,
> >>>>>> errobj);
> >>>>>> -    if (ret != EOK) {
> >>>>>> -        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>> -              "ini_rules_check failed %d [%s]\n",
> ret,
> >>>>>> strerror(ret));
> >>>>>> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >>>>>> +              "Failed to get errors from
> validators.\n");
> >>>>>>           goto done;
> >>>>>>       }
> >>>>>>
> >>>>>> @@ -579,10 +573,10 @@ int sss_ini_call_validators(struct
> >>>>>> sss_ini_initdata *data,
> >>>>>>           ini_errobj_next(errobj);
> >>>>>>       }
> >>>>>>
> >>>>>> +    ret = EOK;
> >>>>>> +
> >>>>>> done:
> >>>>>> -    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>>>       ini_errobj_destroy(&errobj);
> >>>>>> -
> >>>>>>       return ret;
> >>>>>> #else
> >>>>>>       DEBUG(SSSDBG_TRACE_FUNC,
> >>>>>> @@ -590,3 +584,32 @@ done:
> >>>>>>       return EOK;
> >>>>>> #endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>>> }
> >>>>>> +
> >>>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata
> *data,
> >>>>>> +                                   const char *rules_path,
> >>>>>> +                                   struct ini_errobj *errobj)
> >>>>>> +{
> >>>>>> +    int ret;
> >>>>>> +    struct ini_cfgobj *rules_cfgobj = NULL;
> >>>>>> +
> >>>>>> +    ret = ini_rules_read_from_file(rules_path,
> &rules_cfgobj);
> >>>>>> +    if (ret != EOK) {
> >>>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>> +              "Failed to read sssd.conf schema %d
> [%s]\n", ret,
> >>>>>> strerror(ret));
> >>>>>> +        goto done;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    ret = ini_rules_check(rules_cfgobj, data->sssd_config,
> NULL,
> >>>>>> errobj);
> >>>>>> +    if (ret != EOK) {
> >>>>>> +        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>> +              "ini_rules_check failed %d [%s]\n",
> ret,
> >>>>>> strerror(ret));
> >>>>>> +        goto done;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +done:
> >>>>>> +    if (rules_cfgobj) ini_config_destroy(rules_cfgobj);
> >>>>>> +
> >>>>>> +    return ret;
> >>>>>> +}
> >>>>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>>> diff --git a/src/util/sss_ini.h b/src/util/sss_ini.h
> >>>>>> index 7734bab..77943d6 100644
> >>>>>> --- a/src/util/sss_ini.h
> >>>>>> +++ b/src/util/sss_ini.h
> >>>>>> @@ -27,6 +27,10 @@
> >>>>>> #ifndef __SSS_INI_H__
> >>>>>> #define __SSS_INI_H__
> >>>>>>
> >>>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>>> +#include <ini_configobj.h>
> >>>>>> +#endif
> >>>>>> +
> >>>>>> /* Structure declarations */
> >>>>>>
> >>>>>> /* INI data structure */
> >>>>>> @@ -83,4 +87,12 @@ int sss_confdb_create_ldif(TALLOC_CTX
> *mem_ctx,
> >>>>>> int sss_ini_call_validators(struct sss_ini_initdata *data,
> >>>>>>                               const char *rules_path);
> >>>>>>
> >>>>>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>>>>> +/* Get errors from validators with ini_errobj */
> >>>>>> +int sss_ini_call_validators_errobj(struct sss_ini_initdata
> *data,
> >>>>>> +                                   const char *rules_path,
> >>>>>> +                                   struct ini_errobj *errobj);
> >>>>>> +#endif /* HAVE_LIBINI_CONFIG_V1_3 */
> >>>>>> +
> >>>>> NACK
> >>>>>
> >>>>> src/util/sss_ini.h must not contain conditional build.
> >>>>> That's the putpose of implementation file.
> >>>>>
> >>>>> Header file shoudl contain just a prototypes
> >>>>> becuase there might be missing prototypes if you forgot
> >>>>> include "config.h" before this header file.
> >>>>>
> >>>>> LS
> >>>>
> >>>> Ok,
> >>>>
> >>>> I did some refactoring so that the errobj is not
> >>>> used as parameter to the functions to avoid the
> >>>> conditions in .h file.
> >>>>
> >>>> See the attached patches.
> >>>>
> >>>> I also pushed it to CI:
> >>>> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4722/
> >>>>
> >>>> Michal
> >>>>
> >>>
> >>> Lukas requested some changes off list.
> >>>
> >>> New patches attached.
> >>>
> >>> I also pushed it to CI:
> >>> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4742/
> >>>
> >>> Michal
> >>>
> >>
> >>> From c3d40d76723d57e747f73564e7be1034aa842b06 Mon Sep 17 00:00:00 2001
> >>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >>> Date: Fri, 1 Jul 2016 15:10:30 +0200
> >>> Subject: [PATCH 1/2] sss_ini: Small refacoring of
> >>> sss_ini_call_validators
> >>>
> >>> Separate logic to fill errobj so that
> >>> the errors can be printed by the caller.
> >>> ---
> >> ACK
> >>
> >>> From bea75afdecddb012dd8e236be34781ca5426dc68 Mon Sep 17 00:00:00 2001
> >>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com&gt;
> >>> Date: Mon, 27 Jun 2016 17:33:14 +0200
> >>> Subject: [PATCH 2/2] sssctl: Add config-check command
> >>>
> >>> Fixes:
> >>> https://fedorahosted.org/sssd/ticket/2269
> >>>
> >>> sssctl sconfig-check command allows to
> >>> call SSSD config file validators on
> >>> demand.
> >>> ---
> >>> Makefile.am                      |   1 +
> >>> src/tools/sssctl/sssctl.c        |   4 ++
> >>> src/tools/sssctl/sssctl.h        |   4 ++
> >>> src/tools/sssctl/sssctl_config.c | 130
> >>> +++++++++++++++++++++++++++++++++++++++
> >>> src/util/sss_ini.c               |   2 -
> >>> 5 files changed, 139 insertions(+), 2 deletions(-)
> >>> create mode 100644 src/tools/sssctl/sssctl_config.c
> >>>
> >>> diff --git a/Makefile.am b/Makefile.am
> >>> index 4089b69..706b60d 100644
> >>> --- a/Makefile.am
> >>> +++ b/Makefile.am
> >>> @@ -1566,6 +1566,7 @@ sssctl_SOURCES = \
> >>>      src/tools/sssctl/sssctl_logs.c \
> >>>      src/tools/sssctl/sssctl_domains.c \
> >>>      src/tools/sssctl/sssctl_sifp.c \
> >>> +    src/tools/sssctl/sssctl_config.c \
> >>>      $(SSSD_TOOLS_OBJ) \
> >>>      $(NULL)
> >>> sssctl_LDADD = \
> >>> diff --git a/src/tools/sssctl/sssctl.c b/src/tools/sssctl/sssctl.c
> >>> index be5f1b4..86656f1 100644
> >>> --- a/src/tools/sssctl/sssctl.c
> >>> +++ b/src/tools/sssctl/sssctl.c
> >>> @@ -271,6 +271,10 @@ int main(int argc, const char **argv)
> >>>          SSS_TOOL_DELIMITER("Log files tools:"),
> >>>          SSS_TOOL_COMMAND("remove-logs", "Remove existing SSSD
> log
> >>> files", 0, sssctl_remove_logs),
> >>>          SSS_TOOL_COMMAND("fetch-logs", "Archive SSSD log
> files in
> >>> tarball", 0, sssctl_fetch_logs),
> >>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>> +        SSS_TOOL_DELIMITER("Configuration files tools:"),
> >>> +        SSS_TOOL_COMMAND("config-check", "Perform static
> analysis of
> >>> SSSD configuration", 0, sssctl_config_check),
> >>> +#endif
> >>>          {NULL, NULL, 0, NULL}
> >>>      };
> >>>
> >>> diff --git a/src/tools/sssctl/sssctl.h b/src/tools/sssctl/sssctl.h
> >>> index ae6e62c..be62475 100644
> >>> --- a/src/tools/sssctl/sssctl.h
> >>> +++ b/src/tools/sssctl/sssctl.h
> >>> @@ -100,4 +100,8 @@ errno_t sssctl_netgroup(struct sss_cmdline
> >>> *cmdline,
> >>>                          struct sss_tool_ctx *tool_ctx,
> >>>                          void *pvt);
> >>>
> >>> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >>> +                            struct sss_tool_ctx *tool_ctx,
> >>> +                            void *pvt);
> >>> +
> >>> #endif /* _SSSCTL_H_ */
> >>> diff --git a/src/tools/sssctl/sssctl_config.c
> >>> b/src/tools/sssctl/sssctl_config.c
> >>> new file mode 100644
> >>> index 0000000..a2d1350
> >>> --- /dev/null
> >>> +++ b/src/tools/sssctl/sssctl_config.c
> >>> @@ -0,0 +1,130 @@
> >>> +/*
> >>> +    Authors:
> >>> +        Michal Židek <mzidek(a)redhat.com&gt;
> >>> +
> >>> +    Copyright (C) 2016 Red Hat
> >>> +
> >>> +    This program is free software; you can redistribute it and/or
> >>> modify
> >>> +    it under the terms of the GNU General Public License as
> >>> published by
> >>> +    the Free Software Foundation; either version 3 of the License, or
> >>> +    (at your option) any later version.
> >>> +
> >>> +    This program is distributed in the hope that it will be useful,
> >>> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>> +    GNU General Public License for more details.
> >>> +
> >>> +    You should have received a copy of the GNU General Public License
> >>> +    along with this program.  If not, see
> >>> <http://www.gnu.org/licenses/>;.
> >>> +*/
> >>> +
> >>> +#include "config.h"
> >>> +
> >>> +#include <popt.h>
> >>> +#include <stdio.h>
> >>> +#include <ini_configobj.h>
> >>> +
> >>> +#include "util/util.h"
> >>> +#include "util/sss_ini.h"
> >>> +#include "tools/common/sss_tools.h"
> >>> +#include "tools/common/sss_process.h"
> >>> +#include "tools/sssctl/sssctl.h"
> >>> +#include "confdb/confdb.h"
> >>> +
> >>> +#ifdef HAVE_LIBINI_CONFIG_V1_3
> >>> +errno_t sssctl_config_check(struct sss_cmdline *cmdline,
> >>> +                            struct sss_tool_ctx *tool_ctx,
> >>> +                            void *pvt)
> >>> +{
> >>> +    errno_t ret;
> >>> +    struct ini_errobj *errobj = NULL;
> >>> +    struct sss_ini_initdata *init_data;
> >>> +    struct ref_array *ra;
> >>> +    char *msg;
> >>> +    uint32_t i = 0;
> >>> +    size_t num_errors;
> >>> +    char **strs = NULL;
> >>> +    TALLOC_CTX *tmp_ctx = NULL;
> >>> +
> >>> +    tmp_ctx = talloc_new(NULL);
> >>> +    init_data = sss_ini_initdata_init(tmp_ctx);
> >>> +    if (!init_data) {
> >>> +        DEBUG(SSSDBG_FATAL_FAILURE, "Out of memory.\n");
> >>> +        ret = ENOMEM;
> >>> +        goto done;
> >>> +    }
> >>> +
> >>> +    /* Open config file */
> >>> +    ret = sss_ini_config_file_open(init_data, SSSD_CONFIG_FILE);
> >>> +    if (ret != EOK) {
> >>> +        DEBUG(SSSDBG_TRACE_FUNC,
> >>> +              "sss_ini_config_file_open failed: %s [%d]\n",
> >>> strerror(ret),
> >>> +              ret);
> >>> +        goto done;
> >>> +    }
> >>> +
> >>> +    /* Check the file permissions */
> >>> +    ret = sss_ini_config_access_check(init_data);
> >>> +    if (ret != EOK) {
> >>> +        DEBUG(SSSDBG_CRIT_FAILURE,
> >>> +              "Permission check on config file failed.\n");
> >> IMHO, we shoudl inform about wrong permission here with printf.
> >> So it can be translated.
> >> It's quite important info for users.
> >>
> >>> +        ret = EPERM;
> >>> +        goto done;
> >>> +    }
> >>> +
> >>> +    ret = sss_ini_get_config(init_data,
> >>> +                             SSSD_CONFIG_FILE,
> >>> +                             CONFDB_DEFAULT_CONFIG_DIR);
> >>> +    if (ret != EOK) {
> >>> +        DEBUG(SSSDBG_FATAL_FAILURE, "Failed to load
> configuration\n");
> >>> +        goto done;
> >>> +    }
> >>> +
> >>> +    /* Read rules */
> >>> +    ret = sss_ini_call_validators_strs(tmp_ctx, init_data,
> >>> +                                      
> SSSDDATADIR"/cfg_rules.ini",
> >>> +                                       &strs, &num_errors);
> >>> +    if (ret) {
> >>> +        goto done;
> >>> +    }
> >>> +
> >>> +    /* Output from validators */
> >>> +    printf(_("Issues identified by validators: %lu\n"),
> num_errors);
> >>                                                   ^^^
> >>                                                  %zu shoudl be used
> >> for size_t
> >>                                                  otherwise there is a
> >> warning on
> >>                                                  32bit architectures.
> >>
> >> otherwise LGTM
> >>
> >> LS
> >
> > New patches attached. I also did one more change that
> > Lukas requested off-list.
> >
> > Michal
>
>  Hi, please use sss_strerror() instead of strerror() in new code. It may
>  be also good to print a nice message to stderr on errors that are
>  actually expected to happen such as EPERM. And some generic message for
>  other errors would be nice as well so we know there is more information
>  when you run it again with --debug.
>  
For now, I will not add the messages to stderr. I will think about
it, but it basically is just for better debug experience. The
messages for users will be printed to stdout.

New patches attached.

Michal