Re: [SSSD] [PATCH] TESTS: ldap_id_cleanup timeouts
On 08/27/2015 01:01 PM, Lukas Slebodnik wrote: On (19/08/15 20:26), Michal Židek wrote: Hi! This is another patch to avoid failing tests in the CI (make-check-valgrind). This time the ldap_id_cleanup tests. Looks like the one second cache timeout was too short and the tests sometimes failed because they expected the entries to be still valid for a short while after they were added to sysdb. I saw the failures only on Fedora 20 CI machine. See the attached patch. Michal From 35ee376cc0e9674b5fa9ef1c1c3cc3e67152560e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com Date: Wed, 19 Aug 2015 20:10:28 +0200 Subject: [PATCH] TESTS: ldap_id_cleanup timeouts The one second timeout interval was sometimes too short when the tests where running under Valgrind in the CI and the entries expired too soon. --- src/tests/cmocka/test_ldap_id_cleanup.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/tests/cmocka/test_ldap_id_cleanup.c b/src/tests/cmocka/test_ldap_id_cleanup.c index 941427e..bdc2568 100644 --- a/src/tests/cmocka/test_ldap_id_cleanup.c +++ b/src/tests/cmocka/test_ldap_id_cleanup.c @@ -186,23 +186,26 @@ static void test_id_cleanup_exp_group(void **state) const char *empty_special_grp = empty_gr*o/u\\p(2016); const char *empty_grp = empty_grp; const char *grp = grp; +/* This timeout can be bigger because we will call invalidate_group + * to expire entries without waiting. */ +uint64_t cache_timeout = 30; It's better to define such variable with const modifier. So it cannot be misused for other purposes. (ideally with uppercase name of variable) LS I agree. Attached is the same patch that was already ACKed, just with the changed name and added const. Michal From 988de26aa163bc31355b42d573fbe4c0d6d7b77b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com Date: Wed, 19 Aug 2015 20:10:28 +0200 Subject: [PATCH] TESTS: ldap_id_cleanup timeouts The one second timeout interval was sometimes too short when the tests where running under Valgrind in the CI and the entries expired too soon. --- src/tests/cmocka/test_ldap_id_cleanup.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/tests/cmocka/test_ldap_id_cleanup.c b/src/tests/cmocka/test_ldap_id_cleanup.c index 941427e..ce7f909 100644 --- a/src/tests/cmocka/test_ldap_id_cleanup.c +++ b/src/tests/cmocka/test_ldap_id_cleanup.c @@ -186,23 +186,26 @@ static void test_id_cleanup_exp_group(void **state) const char *empty_special_grp = empty_gr*o/u\\p(2016); const char *empty_grp = empty_grp; const char *grp = grp; +/* This timeout can be bigger because we will call invalidate_group + * to expire entries without waiting. */ +const uint64_t CACHE_TIMEOUT = 30; struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state, struct sysdb_test_ctx); ret = sysdb_store_group(test_ctx-domain, special_grp, -10002, NULL, 1, 0); +10002, NULL, CACHE_TIMEOUT, 0); assert_int_equal(ret, EOK); ret = sysdb_store_group(test_ctx-domain, empty_special_grp, -10003, NULL, 1, 0); +10003, NULL, CACHE_TIMEOUT, 0); assert_int_equal(ret, EOK); ret = sysdb_store_group(test_ctx-domain, grp, -10004, NULL, 1, 0); +10004, NULL, CACHE_TIMEOUT, 0); assert_int_equal(ret, EOK); ret = sysdb_store_group(test_ctx-domain, empty_grp, -10005, NULL, 1, 0); +10005, NULL, CACHE_TIMEOUT, 0); assert_int_equal(ret, EOK); ret = sysdb_store_user(test_ctx-domain, test_user, NULL, -- 2.1.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] NSS: Don't ignore backslash in usernames with ldap provider
ehlo,
please review attached patch for regression #2772
LS
From 97f4a26ca92300732b1ba7c1d7f501d28a862051 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik lsleb...@redhat.com
Date: Fri, 28 Aug 2015 07:07:40 +0200
Subject: [PATCH] NSS: Don't ignore backslash in usernames with ldap provider
The regression was caused by changing default domain regex
for ldap provider in ticket #2717
Resolves:
https://fedorahosted.org/sssd/ticket/2772
---
src/responder/nss/nsssrv.c | 4 ++--
src/tests/cmocka/test_nss_srv.c | 4 ++--
src/util/usertools.c| 11 ++-
src/util/util.h | 3 +++
4 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
index
2b3bca892a5b9c483d1f6f099fd4a6493e9afcab..d8eff7968c4929663412aa56d08414689b921a22
100644
--- a/src/responder/nss/nsssrv.c
+++ b/src/responder/nss/nsssrv.c
@@ -552,9 +552,9 @@ int nss_process_init(TALLOC_CTX *mem_ctx,
goto fail;
}
-ret = sss_names_init(nctx, nctx-rctx-cdb, NULL, nctx-global_names);
+ret = sss_ad_default_names_ctx(nctx, nctx-global_names);
if (ret != EOK) {
-DEBUG(SSSDBG_CRIT_FAILURE, sss_names_init failed.\n);
+DEBUG(SSSDBG_CRIT_FAILURE, sss_ad_default_names_ctx failed.\n);
goto fail;
}
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index
2d4fb2204544a2b371f389545a461f3d526f2c0a..eb273b2263fa02294caf826181c7dcb03b6e2149
100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -1045,8 +1045,8 @@ void test_nss_setup(struct sss_test_conf_param params[],
nss_test_ctx-nctx = mock_nctx(nss_test_ctx);
assert_non_null(nss_test_ctx-nctx);
-ret = sss_names_init(nss_test_ctx-nctx, nss_test_ctx-tctx-confdb,
- NULL, nss_test_ctx-nctx-global_names);
+ret = sss_ad_default_names_ctx(nss_test_ctx-nctx,
+ nss_test_ctx-nctx-global_names);
assert_int_equal(ret, EOK);
assert_non_null(nss_test_ctx-nctx-global_names);
diff --git a/src/util/usertools.c b/src/util/usertools.c
index
87a8d7411312c3a80c32374a1fd93bbf0e767a91..ccbf7a0c8c2fb6d1d07afbfe46d978fc33093432
100644
--- a/src/util/usertools.c
+++ b/src/util/usertools.c
@@ -249,7 +249,8 @@ int sss_names_init(TALLOC_CTX *mem_ctx, struct confdb_ctx
*cdb,
}
if (!re_pattern) {
-re_pattern = talloc_strdup(tmpctx, IPA_AD_DEFAULT_RE);
+re_pattern = talloc_strdup(tmpctx,
+ (?Pname[^@]+)@?(?Pdomain[^@]*$));
if (!re_pattern) {
ret = ENOMEM;
goto done;
@@ -294,6 +295,14 @@ done:
return ret;
}
+int sss_ad_default_names_ctx(TALLOC_CTX *mem_ctx,
+ struct sss_names_ctx **_out)
+{
+return sss_names_init_from_args(mem_ctx, IPA_AD_DEFAULT_RE,
+CONFDB_DEFAULT_FULL_NAME_FORMAT,
+_out);
+}
+
int sss_parse_name(TALLOC_CTX *memctx,
struct sss_names_ctx *snctx,
const char *orig, char **_domain, char **_name)
diff --git a/src/util/util.h b/src/util/util.h
index
e20501cbbdf04da41f9199696b848e6705294610..3e29e748768fc2cac547d16d61b449f1b555a56f
100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -299,6 +299,9 @@ int sss_names_init(TALLOC_CTX *mem_ctx,
const char *domain,
struct sss_names_ctx **out);
+int sss_ad_default_names_ctx(TALLOC_CTX *mem_ctx,
+ struct sss_names_ctx **_out);
+
int sss_parse_name(TALLOC_CTX *memctx,
struct sss_names_ctx *snctx,
const char *orig, char **_domain, char **_name);
--
2.5.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] UTIL: Function 2string for enum sss_cli_command
On (18/08/15 15:54), Petr Cech wrote:
On 08/13/2015 11:11 AM, Lukas Slebodnik wrote:
From a93e36f11759cf9a748942e7632d4a07a088b098 Mon Sep 17 00:00:00 2001
From: Petr Cech pc...@redhat.com
Date: Wed, 8 Jul 2015 07:17:28 -0400
Subject: [PATCH] UTIL: Function 2string for enum sss_cli_command
Improvement of debug messages.
Instead of:(0x0400): Running command [17]...
We could see:(0x0400): Running command [17][SSS_NSS_GETPWNAM]...
(It's not used in sss_client. There are only hex numbers of commands.)
Resolves:
https://fedorahosted.org/sssd/ticket/2708
The patch does not apply to master.
I had to use tree way merge.
Please rebase it.
Rebased.
---
Makefile.am | 3 +-
src/providers/dp_pam_data_util.c | 27 +
src/responder/nss/nsssrv_cmd.c | 30 ++---
src/sss_client/pam_sss.c | 6 +-
src/tools/tools_mc_util.c| 4 +-
src/util/sss_cli_cmd.c | 238
+++
src/util/sss_cli_cmd.h | 28 +
7 files changed, 293 insertions(+), 43 deletions(-)
create mode 100644 src/util/sss_cli_cmd.c
create mode 100644 src/util/sss_cli_cmd.h
diff --git a/Makefile.am b/Makefile.am
index
b8cbc6df23ded1edb945a709b6dbe1c44eb54017..430f2292a1be9e0f0b7cb56e8ecbf179e9978dcd
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -678,7 +678,8 @@ endif
pkglib_LTLIBRARIES += libsss_debug.la
libsss_debug_la_SOURCES = \
src/util/debug.c \
-src/util/sss_log.c
+src/util/sss_log.c \
+src/util/sss_cli_cmd.c
We decided to add $NULL at the end of list so in future
you will not need to change two lines if you add new file.
$NULL added.
libsss_debug_la_LIBADD = \
$(SYSLOG_LIBS)
libsss_debug_la_LDFLAGS = \
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index
0129467302f16af3180a5be47ff2e235da65..d37a13820ef857fcf43efba3fb07535c4b6eb509
100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -1656,7 +1656,7 @@ static int pam_sss(enum sss_cli_command task,
pam_handle_t *pamh,
case SSS_PAM_CLOSE_SESSION:
break;
default:
-D((Illegal task [%d], task));
+D((Illegal task [%#x],task));
^
There was a space before change.
Could you return it back.
Returned.
return PAM_SYSTEM_ERR;
}
diff --git a/src/util/sss_cli_cmd.c b/src/util/sss_cli_cmd.c
new file mode 100644
index
..97b967b4014193dc8f7571e5fe821523d469f201
--- /dev/null
+++ b/src/util/sss_cli_cmd.c
@@ -0,0 +1,238 @@
+/*
+ SSSD - cmd2str util
+
+ Copyright (C) Petr Cech pc...@redhat.com 2015
+
+ 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 sss_client/sss_cli.h
+#include util/sss_cli_cmd.h
+#include util/util.h
+
+const char *sss_cmd2str(enum sss_cli_command cmd)
+{
//snip
+
+#if 0
+/* shadow */
+case SSS_NSS_GETSPNAM:
+return SSS_NSS_GETSPNAM;
+case SSS_NSS_GETSPUID:
+return SSS_NSS_GETSPUID;
+case SSS_NSS_SETSPENT:
+return SSS_NSS_SETSPENT;
+case SSS_NSS_GETSPENT:
+return SSS_NSS_GETSPENT;
+case SSS_NSS_ENDSPENT:
+return SSS_NSS_ENDSPENT;
+#endif
I think it's better to be consistent with header file
and we can have unused options here.
But it's better to do not add spaces before '#'
I saw a patter in some header files that spaces was added
after this character.
Something like
#if defined __GNUC__
# if defined __NO_INLINE__
# define HAVE_INLINE 0
# else
# define HAVE_INLINE 1
# ifndef inline
# define inline __inline__
# endif
# endif
#elif defined __cplusplus
Please remove indentation for #if and #endif
in whole file.
Removed.
+
+/* SUDO */
+case SSS_SUDO_GET_SUDORULES:
+return SSS_SUDO_GET_SUDORULES;
+case SSS_SUDO_GET_DEFAULTS:
+return SSS_SUDO_GET_DEFAULTS;
+
//snip
+
+/* ID-SID mapping calls */
+case SSS_NSS_GETSIDBYNAME:
+return SSS_NSS_GETSIDBYNAME;
+case SSS_NSS_GETSIDBYID:
+return SSS_NSS_GETSIDBYID;
+case SSS_NSS_GETNAMEBYSID:
+return SSS_NSS_GETNAMEBYSID;
+case SSS_NSS_GETIDBYSID:
+return SSS_NSS_GETIDBYSID;
+case SSS_NSS_GETORIGBYNAME:
+return SSS_NSS_GETORIGBYNAME;
+default:
+
Re: [SSSD] [PATCH] TESTS: ldap_id_cleanup timeouts
On (19/08/15 20:26), Michal Židek wrote: Hi! This is another patch to avoid failing tests in the CI (make-check-valgrind). This time the ldap_id_cleanup tests. Looks like the one second cache timeout was too short and the tests sometimes failed because they expected the entries to be still valid for a short while after they were added to sysdb. I saw the failures only on Fedora 20 CI machine. See the attached patch. Michal From 35ee376cc0e9674b5fa9ef1c1c3cc3e67152560e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzi...@redhat.com Date: Wed, 19 Aug 2015 20:10:28 +0200 Subject: [PATCH] TESTS: ldap_id_cleanup timeouts The one second timeout interval was sometimes too short when the tests where running under Valgrind in the CI and the entries expired too soon. --- src/tests/cmocka/test_ldap_id_cleanup.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/tests/cmocka/test_ldap_id_cleanup.c b/src/tests/cmocka/test_ldap_id_cleanup.c index 941427e..bdc2568 100644 --- a/src/tests/cmocka/test_ldap_id_cleanup.c +++ b/src/tests/cmocka/test_ldap_id_cleanup.c @@ -186,23 +186,26 @@ static void test_id_cleanup_exp_group(void **state) const char *empty_special_grp = empty_gr*o/u\\p(2016); const char *empty_grp = empty_grp; const char *grp = grp; +/* This timeout can be bigger because we will call invalidate_group + * to expire entries without waiting. */ +uint64_t cache_timeout = 30; It's better to define such variable with const modifier. So it cannot be misused for other purposes. (ideally with uppercase name of variable) LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] sss_override: document --debug options
On (25/08/15 13:00), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2758
From f181b0a94863f082abaf074a0940e83fbf1c89b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrez...@redhat.com
Date: Tue, 25 Aug 2015 12:58:45 +0200
Subject: [PATCH] sss_override: document --debug options
Resolves:
https://fedorahosted.org/sssd/ticket/2758
---
src/man/sss_override.8.xml | 18 +-
src/tools/common/sss_tools.c | 25 +
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/man/sss_override.8.xml b/src/man/sss_override.8.xml
index
d289f5b7dfa7fbd328831b4c71d45b4c555225cf..3db8cbe05322ddf662faaa4810cd3bf6b25f8883
100644
--- a/src/man/sss_override.8.xml
+++ b/src/man/sss_override.8.xml
@@ -38,7 +38,7 @@
all local overrides are lost.
/para
/refsect1
-
+
refsect1 id='commands'
titleAVAILABLE COMMANDS/title
para
@@ -189,6 +189,22 @@
/varlistentry
/variablelist
/refsect1
+
+refsect1 id='options'
+titleCOMMON OPTIONS/title
+para
+Those options are available with all commands.
+/para
+variablelist remap='IP'
+varlistentry
+term
+option-d/option,option--debug/option
+replaceableLEVEL/replaceable
+/term
+xi:include xmlns:xi=http://www.w3.org/2001/XInclude;
href=include/debug_levels.xml /
+/varlistentry
+/variablelist
+/refsect1
xi:include xmlns:xi=http://www.w3.org/2001/XInclude;
href=include/seealso.xml /
diff --git a/src/tools/common/sss_tools.c b/src/tools/common/sss_tools.c
index
6bbce3a25c0b23ebc108a917a38e94981b65..3e732a3411494262ea34a1e5c332e86f5128e771
100644
--- a/src/tools/common/sss_tools.c
+++ b/src/tools/common/sss_tools.c
@@ -36,6 +36,13 @@ struct sss_cmdline {
const char **argv;
};
+static void sss_tool_print_common_opts(void)
+{
+fprintf(stderr, _(Common options:\n));
+fprintf(stderr, -d, --debug=INT%s\n,
+_(Enable debug at level));
NACK
This does not correspond with reality. The sort version does not work.
It would be good to test own patches :-)
[root@host ~]# sss_override -d=9 user-add
Usage:
sss_override COMMAND COMMAND-ARGS
Available commands:
* user-add
* user-del
* group-add
* group-del
[root@host ~]# sss_override --debug=2 user-add
Missing option: Specify name of modified object.
Usage: sss_override user-add NAME [OPTIONS...]
-n, --name=STRING Override name
-u, --uid=INT Override uid (non-zero value)
-g, --gid=INT Override gid (non-zero value)
-h, --home=STRING Override home directory
-s, --shell=STRING Override shell
-c, --gecos=STRING Override gecos
Help options:
-?, --help Show this help message
--usageDisplay brief usage message
(Thu Aug 27 12:32:49:273331 2015) [sssd] [parse_cmdline] (0x0020): Unable to
parse command arguments
(Thu Aug 27 12:32:49:273461 2015) [sssd] [override_user_add] (0x0020): Unable
to parse command line.
As you can see long version works.
I would be also curious why you need to provide hacks
for printing argument description for autohelp.
There is a much more elegant way how to do it with libpopt.
(small hint POPT_ARGFLAG_DOC_HIDDEN in sss_tool_common_opts)
LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] TESTS: Removing part of responder_cache_req-tests
On (24/08/15 16:09), Michal Židek wrote: On 08/21/2015 04:55 PM, Petr Cech wrote: On 08/21/2015 02:35 PM, Michal Židek wrote: Hi, some of the tests you deleted are valid and should not be deleted. Only those tests that rely on time(NULL) being the same as the time of request creation are invalid. All those that test old entries or nonexistent entries are OK. See comments inline. I agree. Those tests have another logic. So I returned them back. Petr ACK. CI results: http://sssd-ci.duckdns.org/logs/job/23/75/summary.html +1 for patch. but please do not close the ticket #2730 after pushing to master. This solution is just a temporary workaround for problematic tests. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] [HBAC]: Better libhbac debuging
On 08/26/2015 09:44 AM, Petr Cech wrote:
+void hbac_debug_messages(const char *file, int line,
+ enum hbac_debug_level level,
+ const char *fmt, ...)
+{
+int loglevel = SSSDBG_UNRESOLVED;
+int ret;
+char *message = NULL;
+
+switch(level) {
+case HBAC_DBG_FATAL:
+loglevel = SSSDBG_FATAL_FAILURE;
+break;
+case HBAC_DBG_ERROR:
+loglevel = SSSDBG_CRIT_FAILURE;
+break;
+case HBAC_DBG_WARNING:
+loglevel = SSSDBG_OP_FAILURE;
+break;
+case HBAC_DBG_INFO:
+loglevel = SSSDBG_MINOR_FAILURE;
+break;
+case HBAC_DBG_TRACE:
+loglevel = SSSDBG_TRACE_ALL;
+break;
+}
Please add default here (http://www.freeipa.org/page/Coding_Style#Switch)
+
+va_list ap;
+va_start(ap, fmt);
+ret = vasprintf(message, fmt, ap);
+va_end(ap);
+if (ret 0) {
+/* ENOMEM */
+return;
+}
+
+if (DEBUG_IS_SET(loglevel)) {
+debug_fn(__FILE__, __LINE__, hbac, loglevel, [%s:%i] %s\n,
+ file, line, message);
+}
+free(message);
+}
+
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] [HBAC]: Better libhbac debuging
* SSSDBG_TRACE_ALL produces:
...hbac_evaluator.c:150] [ hbac_evaluate()
...hbac_evaluator.c:410] REQUEST:
...hbac_evaluator.c:391] service [sshd]
...hbac_evaluator.c:400] service_group (none)
...hbac_evaluator.c:391] user [csikos]
...hbac_evaluator.c:395] user_group:
I think it could be useful to move user and might be user_group to less
verbose level - I think it could be hard to navigate in less verbose
logs otherwise, do you agree?
...hbac_evaluator.c:397] [ipausers]
...hbac_evaluator.c:391] targethost [albireo.cygnus.dev]
...hbac_evaluator.c:400] targethost_group (none)
...hbac_evaluator.c:391] srchost [192.168.122.106]
...hbac_evaluator.c:400] srchost_group (none)
...hbac_evaluator.c:417] request time Fri Jul 24 14:29:36 2015
...hbac_evaluator.c:454] RULE [szabo_allowed] [ENABLED]:
...hbac_evaluator.c:456] services:
...hbac_evaluator.c:427] category [0] [NONE]
...hbac_evaluator.c:435] services_names (none)
...hbac_evaluator.c:440] services_groups:
...hbac_evaluator.c:442] [Sudo]
...hbac_evaluator.c:462] users:
On 08/26/2015 09:44 AM, Petr Cech wrote:
0001-TESTS-Fixing-of-uninitialized-pointer.patch
From e83b609924ed2f6ff35b70355f3c330ec2586a15 Mon Sep 17 00:00:00 2001
From: Petr Cechpc...@redhat.com
Date: Wed, 26 Aug 2015 02:50:26 -0400
Subject: [PATCH 1/2] TESTS: Fixing of uninitialized pointer.
There was a bug with uninitialized pointer during solving ticket 2703.
More details:
rules[0]-services-names[1] is initialized on line 361, but
initializing of rules[0]-srchosts-names[1] was missing.
Resolves:
https://fedorahosted.org/sssd/ticket/2703
---
src/tests/ipa_hbac-tests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tests/ipa_hbac-tests.c b/src/tests/ipa_hbac-tests.c
index
bd56c8f107b05f07b1ba8913fc14a03419d679f7..f2192a6fbc5188a7a7f6b204e03ca5961bb53f75
100644
--- a/src/tests/ipa_hbac-tests.c
+++ b/src/tests/ipa_hbac-tests.c
@@ -367,7 +367,7 @@ START_TEST(ipa_hbac_test_allow_utf8)
fail_if(rules[0]-services-names == NULL);
rules[0]-srchosts-names[0] = (const char *) srchost_utf8_upcase;
-rules[0]-services-names[1] = NULL;
+rules[0]-srchosts-names[1] = NULL;
rules[1] = NULL;
-- 2.4.3
Nice catch! Ci passed. ACK to this patch
0002-HBAC-Better-libhbac-debugging.patch
From 75d97a5336e2b66d4bb187ce024ad9be9b2702b9 Mon Sep 17 00:00:00 2001
From: Petr Cechpc...@redhat.com
Date: Fri, 24 Jul 2015 10:56:49 -0400
Subject: [PATCH 2/2] HBAC: Better libhbac debugging
Added support for logging via external log function.
Log provides information about rules evaluating (HBAC_DBG_INFO level)
and additionally can describe rules (HBAC_DBG_TRACE level).
Resolves:
https://fedorahosted.org/sssd/ticket/2703
---
src/providers/ipa/hbac_evaluator.c | 149 +
src/providers/ipa/ipa_access.c | 45 +++
src/providers/ipa/ipa_hbac.exports | 3 +-
src/providers/ipa/ipa_hbac.h | 22 ++
4 files changed, 218 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/hbac_evaluator.c
b/src/providers/ipa/hbac_evaluator.c
index
f40f9e0a7f16f5e012079c637b89c8e49ec5d15b..976d5887baeecbb45d660c0de5ca54c914fc6367
100644
--- a/src/providers/ipa/hbac_evaluator.c
+++ b/src/providers/ipa/hbac_evaluator.c
@@ -24,6 +24,8 @@
*/
#include stdlib.h
+#include stdio.h
+#include stdarg.h
Are these header files really needed? What do we need from them? I'm
just asking as code seems to compile fine even without them.
#include string.h
#include errno.h
#include providers/ipa/ipa_hbac.h
@@ -38,6 +40,39 @@ typedef int errno_t;
#define EOK 0
#endif
+/* HBAC logging system */
+
+/* debug macro */
+#define HBAC_DEBUG(level, format, ...) do { \
+if (hbac_debug_fn != NULL) { \
+hbac_debug_fn(__FILE__, __LINE__, level, format, ##__VA_ARGS__); \
+} \
+} while (0)
+
+/* static pointer to external logging function */
+static hbac_debug_fn_t hbac_debug_fn = NULL;
+
+/* setup function for external logging function */
+void hbac_enable_debug(hbac_debug_fn_t external_debug_fn)
+{
+hbac_debug_fn = external_debug_fn;
+}
+
+/* auxiliary function for hbac_request_element logging */
+static void hbac_request_element_debug_print(struct hbac_request_element *el,
+ const char *label);
+
+/* auxiliary function for hbac_eval_req logging */
+static void hbac_req_debug_print(struct hbac_eval_req *req);
+
+/* auxiliary function for hbac_rule_element logging */
+static void hbac_rule_element_debug_print(struct hbac_rule_element *el,
+ const char *label);
+
+/* auxiliary function for hbac_rule logging */
+static void hbac_rule_debug_print(struct hbac_rule *rule);
+
+
/* Placeholder structure for future HBAC time-based
* evaluation rules
*/
@@ -114,9 +149,13 @@ enum hbac_eval_result hbac_evaluate(struct
Re: [SSSD] [PATCH v3] Remove trailing whitespace
On (18/08/15 15:31), Nikolai Kondrashov wrote: On 08/17/2015 08:42 PM, Lukas Slebodnik wrote: - Original Message - From: Nikolai Kondrashov nikolai.kondras...@redhat.com To: Development of the System Security Services Daemon sssd-devel@lists.fedorahosted.org Sent: Monday, August 17, 2015 6:40:36 PM Subject: [SSSD] [PATCH v3] Remove trailing whitespace On 08/17/2015 09:37 AM, Lukas Slebodnik wrote: On (13/08/15 15:48), Nikolai Kondrashov wrote: On 08/13/2015 02:57 PM, Lukas Slebodnik wrote: On (13/08/15 14:47), Nikolai Kondrashov wrote: +if GIT_CHECKOUT +export GIT_DIR=$(abs_top_srcdir)/.git +export GIT_WORK_TREE=$(abs_top_srcdir) +endif It's not good idea to export such in general code. you might greak something. I didn't try but you might break something in fedpkg + local build. Better solution would to to export make variable abs_top_srcdir as ABS_TOP_SRCDIR in TESTS_ENVIRONMENT and print warnig/error from test if this variable is not defined. Yeah, it's not nice. I doubt that it will break anything as it is very specific as to where the git repo is, but I wouldn't vouch for it. But on the other hand ABS_TOP_SRCDIR is more abstract and there is higher probability it will be used in other tests. We'll need to pass it both to the TESTS_ENVIRONMENT and to *srpm targets as well, which will make it uglier, but I'll do it. You can still export GIT_DIR and GIT_WORK_TREE inside script if ABS_TOP_SRCDIR is defined. Otherwise test can be skipped https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html Alright here's another version. I didn't change it much. Simply moved export of GIT_DIR and GIT_WORK_TREE lower, to TESTS_ENVIRONMENT and *rpm* targets. I didn't add exporting of ABS_TOP_SRCDIR, because that way the whitespace test would always require it, making manual execution inconvenient, or would need additional logic with assumptions there. This way we avoid having another variable and a way to pass build parameters around. BTW, Lukas, could you please describe what kind of problems you expect in fedpkg + local build? Perhaps it's not a problem after all? fedpkg compile usage: fedpkg compile [-h] [--arch ARCH] [--short-circuit] This command calls rpmbuild to compile the source. By default the prep and configure stages will be done as well, unless the short-circuit option is used And where I can see a problem. fedpkg is a git wrapper. The git contains patches + spec file. If you call fedpkg compile then rpmbuild will extract tarball and apply patches. configure will detect there is a git and the test will fail because patches contains trailing whitespaces. So GIT_WORK_TREE is not an equivalent to ABS_TOP_SRCDIR. This is an example of for fedora. There might be other distribution where it can cause troubles as well. So it's better to be defensive. Thank you, Lukas. I understand your concern. However, the second patch in the series specifically makes configure detect git repo root at the srcdir only. So, if srcdir is in a subdirectory of a git repo and is not a git repo itself (which would be fine, BTW), configure won't detect it. I haven't noticed the 2nd patch. IMHO it's just an unnecessary complication and just decrease readability of Makefile. The solution witn ABS_TOP_SRCDIR is much simpler and more elegant. Adding ABS_TOP_SRCDIR to TESTS_ENVIRONMENT is just one line change and does not require changes in configure.ac. The main purpose of detecting git is to obtain git hash for prerelase version. We should try to avoid (ab)using GIT features in makefile. Enviroment variable ABS_TOP_SRCDIR is more abstract and reusable. It can be used even in other tests where git would not be used/required. BTW if git is not available the test should not PASS. It should be skipped. https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html#Scripts_002dbased-Testsuites When no test protocol is in use, an exit status of 0 from a test script will denote a success, an exit status of 77 a skipped test, an exit status of 99 an hard error, and any other exit status will denote a failure. The only problem I see at the moment is Debian. They store the packaging together with the source repo [1]. However, we can't do anything about that regardless whether we use ABS_TOP_SRCDIR or not, and they should be able to add debian/.* to the exclude patterns of the whitespace test. There should not be complication with ordinary files in directory debian. The problem is with patches and diff files. Just try. sh$ git format-patch -1 0001-NSS-Fix-use-after-free.patch sh$ grep \s\+$ *.patch | wc -l 4 We decided to have blacklist for problematic files and not whitelist which would be longer. We should add **/*.patch and **/*.diff to blacklist as well. It's not just a problem of debian. We had patches in git as part of experimental features. @see commit ac54a88b4b510289a411f334e371282d00e1538d LS ___
Re: [SSSD] [PATCH v3] Remove trailing whitespace
On (18/08/15 14:47), Michal Židek wrote: On 08/18/2015 02:31 PM, Nikolai Kondrashov wrote: On 08/17/2015 08:42 PM, Lukas Slebodnik wrote: - Original Message - From: Nikolai Kondrashov nikolai.kondras...@redhat.com To: Development of the System Security Services Daemon sssd-devel@lists.fedorahosted.org Sent: Monday, August 17, 2015 6:40:36 PM Subject: [SSSD] [PATCH v3] Remove trailing whitespace On 08/17/2015 09:37 AM, Lukas Slebodnik wrote: On (13/08/15 15:48), Nikolai Kondrashov wrote: On 08/13/2015 02:57 PM, Lukas Slebodnik wrote: On (13/08/15 14:47), Nikolai Kondrashov wrote: +if GIT_CHECKOUT +export GIT_DIR=$(abs_top_srcdir)/.git +export GIT_WORK_TREE=$(abs_top_srcdir) +endif It's not good idea to export such in general code. you might greak something. I didn't try but you might break something in fedpkg + local build. Better solution would to to export make variable abs_top_srcdir as ABS_TOP_SRCDIR in TESTS_ENVIRONMENT and print warnig/error from test if this variable is not defined. Yeah, it's not nice. I doubt that it will break anything as it is very specific as to where the git repo is, but I wouldn't vouch for it. But on the other hand ABS_TOP_SRCDIR is more abstract and there is higher probability it will be used in other tests. We'll need to pass it both to the TESTS_ENVIRONMENT and to *srpm targets as well, which will make it uglier, but I'll do it. You can still export GIT_DIR and GIT_WORK_TREE inside script if ABS_TOP_SRCDIR is defined. Otherwise test can be skipped https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html Alright here's another version. I didn't change it much. Simply moved export of GIT_DIR and GIT_WORK_TREE lower, to TESTS_ENVIRONMENT and *rpm* targets. I didn't add exporting of ABS_TOP_SRCDIR, because that way the whitespace test would always require it, making manual execution inconvenient, or would need additional logic with assumptions there. This way we avoid having another variable and a way to pass build parameters around. BTW, Lukas, could you please describe what kind of problems you expect in fedpkg + local build? Perhaps it's not a problem after all? fedpkg compile usage: fedpkg compile [-h] [--arch ARCH] [--short-circuit] This command calls rpmbuild to compile the source. By default the prep and configure stages will be done as well, unless the short-circuit option is used And where I can see a problem. fedpkg is a git wrapper. The git contains patches + spec file. If you call fedpkg compile then rpmbuild will extract tarball and apply patches. configure will detect there is a git and the test will fail because patches contains trailing whitespaces. So GIT_WORK_TREE is not an equivalent to ABS_TOP_SRCDIR. This is an example of for fedora. There might be other distribution where it can cause troubles as well. So it's better to be defensive. Thank you, Lukas. I understand your concern. However, the second patch in the series specifically makes configure detect git repo root at the srcdir only. So, if srcdir is in a subdirectory of a git repo and is not a git repo itself (which would be fine, BTW), configure won't detect it. The only problem I see at the moment is Debian. They store the packaging together with the source repo [1]. However, we can't do anything about that regardless whether we use ABS_TOP_SRCDIR or not, and they should be able to add debian/.* to the exclude patterns of the whitespace test. Nick [1] git://anonscm.debian.org/pkg-sssd/sssd.git Honestly I think it is OK if this test is properly executed on Fedora only and disabled elsewhere. The code is the same everywhere so if there are no trailing whitespaces on Fedora then there are no trailing whitespaces on other systems as well. This is different from testing functionality where we want to check if something is broken on different platforms. Ofcourse if making it multiplatform is easy, then go for it, but I would not invest too much effort in making this test multiplatform because I do not see additional value here. Package maintainers in other distribution usually do not spend time with troubleshooting tests. The result is disabled tests at all. It's not our goal. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
