Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
I see the following warnings in your unit test:
/home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:58:35: error: initializer element is not a constant expression [-Werror] /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:58:35: error: (near initialization for ‘test_def_opts[3].def_val.blob.length’) [-Werror] /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c: In function ‘opt_test_copy_default’: /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:125:14: error: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Werror=cast-qual] /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:131:14: error: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Werror=cast-qual] /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c: In function ‘opt_test_copy_options’: /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:171:14: error: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Werror=cast-qual] /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c: In function ‘opt_test_get’: /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:210:9: error: variable ‘ret’ set but not used [-Werror=unused-but-set-variable] /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c: In function ‘opt_test_getset_blob’: /home/pbrezina/workspace/sssd/src/tests/cmocka/test_dp_opts.c:318:14: error: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Werror=cast-qual]
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit. But then he came up with the argument (which I agree with) that _copy function should only really copy assigned values, not do a half-copy of defaults and half of values..hence the split of copy and copy_defaults.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable with GCC. Especially the first warning was bad and perhaps a compiler bug (I suspect that clang optimizes the strlen for sizeof internally, but I don't think it should do so with -O0)
On 02/26/2014 02:12 PM, Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit. But then he came up with the argument (which I agree with) that _copy function should only really copy assigned values, not do a half-copy of defaults and half of values..hence the split of copy and copy_defaults.
OK, this sounds reasonable.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable with GCC. Especially the first warning was bad and perhaps a compiler bug (I suspect that clang optimizes the strlen for sizeof internally, but I don't think it should do so with -O0)
Ack to both.
On Wed, Feb 26, 2014 at 02:24:44PM +0100, Pavel Březina wrote:
On 02/26/2014 02:12 PM, Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit. But then he came up with the argument (which I agree with) that _copy function should only really copy assigned values, not do a half-copy of defaults and half of values..hence the split of copy and copy_defaults.
OK, this sounds reasonable.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable with GCC. Especially the first warning was bad and perhaps a compiler bug (I suspect that clang optimizes the strlen for sizeof internally, but I don't think it should do so with -O0)
Ack to both.
Pushed to master and sssd-1-11
On (26/02/14 14:12), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit. But then he came up with the argument (which I agree with) that _copy function should only really copy assigned values, not do a half-copy of defaults and half of values..hence the split of copy and copy_defaults.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable with GCC. Especially the first warning was bad and perhaps a compiler bug (I suspect that clang optimizes the strlen for sizeof internally, but I don't think it should do so with -O0)
From f6f735b44870c0c4895fa43a380da571dd2d0f82 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 24 Feb 2014 15:42:51 +0100 Subject: [PATCH 2/2] DP: Provide separate dp_copy_defaults function
Makefile.am | 12 ++ src/providers/ad/ad_common.c | 16 +- src/providers/data_provider.h | 5 + src/providers/data_provider_opts.c | 42 ++-- src/tests/cmocka/test_dp_opts.c | 421 +++++++++++++++++++++++++++++++++++++ src/tests/ipa_ldap_opt-tests.c | 2 +- 6 files changed, 476 insertions(+), 22 deletions(-) create mode 100644 src/tests/cmocka/test_dp_opts.c
diff --git a/Makefile.am b/Makefile.am index e51003c29dd673199949b60575cb8fb5f7eee8a0..11abaf9b81604fb08957a0c185e7bfe2e6c7a5f0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -165,6 +165,7 @@ if HAVE_CMOCKA test_utils \ ad_access_filter_tests \ ad_common_tests \
dp_opt_tests \ test_search_basesendif
@@ -1590,6 +1591,17 @@ ad_common_tests_LDADD = \ libsss_krb5_common.la \ libsss_test_common.la
+dp_opt_tests_SOURCES = \
- src/providers/data_provider_opts.c \
- src/tests/cmocka/test_dp_opts.c
+dp_opt_tests_CFLAGS = \
- $(AM_CFLAGS)
+dp_opt_tests_LDADD = \
- $(CMOCKA_LIBS) \
- $(TALLOC_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
endif
The test cannot be build with disabled link_all_deplibs (debian). (master and sssd-1-11)
Simple patch is attached.
LS
On Mon, Mar 10, 2014 at 07:43:24AM +0100, Lukas Slebodnik wrote:
On (26/02/14 14:12), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit. But then he came up with the argument (which I agree with) that _copy function should only really copy assigned values, not do a half-copy of defaults and half of values..hence the split of copy and copy_defaults.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable with GCC. Especially the first warning was bad and perhaps a compiler bug (I suspect that clang optimizes the strlen for sizeof internally, but I don't think it should do so with -O0)
From f6f735b44870c0c4895fa43a380da571dd2d0f82 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 24 Feb 2014 15:42:51 +0100 Subject: [PATCH 2/2] DP: Provide separate dp_copy_defaults function
Makefile.am | 12 ++ src/providers/ad/ad_common.c | 16 +- src/providers/data_provider.h | 5 + src/providers/data_provider_opts.c | 42 ++-- src/tests/cmocka/test_dp_opts.c | 421 +++++++++++++++++++++++++++++++++++++ src/tests/ipa_ldap_opt-tests.c | 2 +- 6 files changed, 476 insertions(+), 22 deletions(-) create mode 100644 src/tests/cmocka/test_dp_opts.c
diff --git a/Makefile.am b/Makefile.am index e51003c29dd673199949b60575cb8fb5f7eee8a0..11abaf9b81604fb08957a0c185e7bfe2e6c7a5f0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -165,6 +165,7 @@ if HAVE_CMOCKA test_utils \ ad_access_filter_tests \ ad_common_tests \
dp_opt_tests \ test_search_basesendif
@@ -1590,6 +1591,17 @@ ad_common_tests_LDADD = \ libsss_krb5_common.la \ libsss_test_common.la
+dp_opt_tests_SOURCES = \
- src/providers/data_provider_opts.c \
- src/tests/cmocka/test_dp_opts.c
+dp_opt_tests_CFLAGS = \
- $(AM_CFLAGS)
+dp_opt_tests_LDADD = \
- $(CMOCKA_LIBS) \
- $(TALLOC_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
endif
The test cannot be build with disabled link_all_deplibs (debian). (master and sssd-1-11)
Simple patch is attached.
LS
ACK, builds fine.
On Mon, Mar 10, 2014 at 05:22:27PM +0100, Jakub Hrozek wrote:
On Mon, Mar 10, 2014 at 07:43:24AM +0100, Lukas Slebodnik wrote:
On (26/02/14 14:12), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
Hi,
The attached two patches are related to: https://fedorahosted.org/sssd/ticket/2257
The first patch is included pretty much for completeness, as I noted during development of the unit test, the blob type didn't handle any default value.
The second patch directly addresses #2257. The previous code didn't handle copying options well if the option was set to zero, because the code followed logic like: if (oldval) { newval = oldval; else { newval = defval; }
The patch implements Sumit's idea to provide a separate function for copying default values and amend the generic copy function to only create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit. But then he came up with the argument (which I agree with) that _copy function should only really copy assigned values, not do a half-copy of defaults and half of values..hence the split of copy and copy_defaults.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable with GCC. Especially the first warning was bad and perhaps a compiler bug (I suspect that clang optimizes the strlen for sizeof internally, but I don't think it should do so with -O0)
From f6f735b44870c0c4895fa43a380da571dd2d0f82 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 24 Feb 2014 15:42:51 +0100 Subject: [PATCH 2/2] DP: Provide separate dp_copy_defaults function
Makefile.am | 12 ++ src/providers/ad/ad_common.c | 16 +- src/providers/data_provider.h | 5 + src/providers/data_provider_opts.c | 42 ++-- src/tests/cmocka/test_dp_opts.c | 421 +++++++++++++++++++++++++++++++++++++ src/tests/ipa_ldap_opt-tests.c | 2 +- 6 files changed, 476 insertions(+), 22 deletions(-) create mode 100644 src/tests/cmocka/test_dp_opts.c
diff --git a/Makefile.am b/Makefile.am index e51003c29dd673199949b60575cb8fb5f7eee8a0..11abaf9b81604fb08957a0c185e7bfe2e6c7a5f0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -165,6 +165,7 @@ if HAVE_CMOCKA test_utils \ ad_access_filter_tests \ ad_common_tests \
dp_opt_tests \ test_search_basesendif
@@ -1590,6 +1591,17 @@ ad_common_tests_LDADD = \ libsss_krb5_common.la \ libsss_test_common.la
+dp_opt_tests_SOURCES = \
- src/providers/data_provider_opts.c \
- src/tests/cmocka/test_dp_opts.c
+dp_opt_tests_CFLAGS = \
- $(AM_CFLAGS)
+dp_opt_tests_LDADD = \
- $(CMOCKA_LIBS) \
- $(TALLOC_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la
endif
The test cannot be build with disabled link_all_deplibs (debian). (master and sssd-1-11)
Simple patch is attached.
LS
ACK, builds fine.
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina