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