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>
>>>> 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>
> 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>
> 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>
> +
> + 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