7:15 a.m.
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>
>>>>> 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
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.