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?= <[email protected]> >>> > > > > > 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?= <[email protected]> >>> > > 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?= <[email protected]> >>> > > 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 <[email protected]> >>> > > + >>> > > + 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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
