[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (07/11/16 14:42), Petr Cech wrote:
>Hi all,
>
>after chat with Lukas I attached only first two patches. Author of the third
>one is Lukas and I am not sure if he is finished. (There was question of
>LD_PRELOAD.)
>
>Regards
>
>-- 
>Petr^4 Čech

>From c67ccc872eb5dacc98f626c10740424cef205334 Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 16 Aug 2016 09:32:18 +0200
>Subject: [PATCH 1/3] SYSDB: Adding message to inform which cache is used
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/3060
>---
ACK

>From 1f4e5b03442ea87a117c54a30550fbc357ff10a7 Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 16 Aug 2016 09:33:46 +0200
>Subject: [PATCH 2/3] SYSDB: Adding message about reason why cache changed
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/3060
>---
> src/db/sysdb.c | 24 
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
ACK

http://sssd-ci.duckdns.org/logs/job/56/52/summary.html

LS
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

Hi all,

after chat with Lukas I attached only first two patches. Author of the 
third one is Lukas and I am not sure if he is finished. (There was 
question of LD_PRELOAD.)


Regards

--
Petr^4 Čech
>From c67ccc872eb5dacc98f626c10740424cef205334 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/3] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 29f4b1d1597bd98541a152dd6462caa864fbf2fd..8b194e3db48870aecd54b21bd3d0b77dc342f9e5 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,11 @@
 #include "util/cert.h"
 #include 
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+
 static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
 {
 const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1181,21 @@ done:
 return ret;
 }
 
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "unknown";
+
+if (state_mask == (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
 int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
  struct ldb_dn *entry_dn,
  struct sysdb_attrs *attrs,
@@ -1184,6 +1204,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 bool sysdb_write = true;
 errno_t ret = EOK;
 errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;
 
 sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
 if (sysdb_write == true) {
@@ -1192,6 +1213,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
   "Cannot set attrs for %s, %d [%s]\n",
   ldb_dn_get_linearized(entry_dn), ret, sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
 }
 }
 
@@ -1201,9 +1224,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
 "Cannot set ts attrs for %s\n", ldb_dn_get_linearized(entry_dn));
 /* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
 }
 }
 
+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
 return ret;
 }
 
-- 
2.7.4

>From 1f4e5b03442ea87a117c54a30550fbc357ff10a7 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/3] SYSDB: Adding message about reason why cache changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..b67769ed11fc0796d1987f09aa568c2db4a0ffab 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message *old_entry,
 return true;
 }
 
-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
  struct ldb_message *mod_msg)
 {
 struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
  */
 if (mod_msg_el->num_values > 0) {
 /* We can ignore additions of timestamp attributes */
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Added attr [%s] to entry [%s]\n",
+  mod_msg_el->name, ldb_dn_get_linearized(entry_dn));
 return true;
 }
 break;
@@ -1855,12 +1859,15 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
 
 el_differs = ldb_msg_element_compare(db_msg_el, mod_msg_el);
 if (el_differs) {
-/* We are replacing or extending element, there is a difference. If
- * some values already exist and ldb_add is not permissive,
+/* We are replacing or extending element, there is a difference.
+ * If some values already exist and ldb_add is not permissive,
  * ldb will throw an error, but that's not our job to check..
  */
  

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

Bump.

--
Petr^4 Čech
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 10/20/2016 01:14 PM, Petr Cech wrote:

On 09/22/2016 01:04 PM, Lukas Slebodnik wrote:

Attached is an alternative solution for debugging ldb functions
How to test:
LD_PRELOAD=.libs/sss_ldb_debug.so ./sysdb-tests -d 10

The only think would be to find out why LD_PRELOAD in
/etc/sysconfig/sssd is not passwd to child processes.
MY_LD_PRELOAD is passed without issue.

LS


Hello all,

I just replaced wrappers with Lukas patch. Thanks.

I tested manually LD_PRELOAD, it worked fine if you use
export LD_PRELOAD... how it has been described above in Lukas answer.
I wasn't successful with /etc/sysconfig/sssd too. And uncle google is
silent :-(

I propose to change the commit message of the third patch to `export
LD_PRELAOD=...` instead of `/etc/sysconfig/sssd`. So it should work.

Any other idea?


So,

I changed commit message in last commit to
`export LD_PRELAOD=...`
New patch set is attached.

Regards

--
Petr^4 Čech
>From c67ccc872eb5dacc98f626c10740424cef205334 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/3] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 29f4b1d1597bd98541a152dd6462caa864fbf2fd..8b194e3db48870aecd54b21bd3d0b77dc342f9e5 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,11 @@
 #include "util/cert.h"
 #include 
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+
 static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
 {
 const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1181,21 @@ done:
 return ret;
 }
 
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "unknown";
+
+if (state_mask == (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
 int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
  struct ldb_dn *entry_dn,
  struct sysdb_attrs *attrs,
@@ -1184,6 +1204,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 bool sysdb_write = true;
 errno_t ret = EOK;
 errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;
 
 sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
 if (sysdb_write == true) {
@@ -1192,6 +1213,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
   "Cannot set attrs for %s, %d [%s]\n",
   ldb_dn_get_linearized(entry_dn), ret, sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
 }
 }
 
@@ -1201,9 +1224,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
 "Cannot set ts attrs for %s\n", ldb_dn_get_linearized(entry_dn));
 /* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
 }
 }
 
+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
 return ret;
 }
 
-- 
2.7.4

>From 1f4e5b03442ea87a117c54a30550fbc357ff10a7 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/3] SYSDB: Adding message about reason why cache changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..b67769ed11fc0796d1987f09aa568c2db4a0ffab 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message *old_entry,
 return true;
 }
 
-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
  struct ldb_message *mod_msg)
 {
 struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
  */
 if (mod_msg_el->num_values > 0) {
 /* We can ignore additions of timestamp attributes */
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Added attr [%s] to entry [%s]\n",
+  mod_msg_el->name, 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/22/2016 01:04 PM, Lukas Slebodnik wrote:

Attached is an alternative solution for debugging ldb functions
How to test:
LD_PRELOAD=.libs/sss_ldb_debug.so ./sysdb-tests -d 10

The only think would be to find out why LD_PRELOAD in
/etc/sysconfig/sssd is not passwd to child processes.
MY_LD_PRELOAD is passed without issue.

LS


Hello all,

I just replaced wrappers with Lukas patch. Thanks.

I tested manually LD_PRELOAD, it worked fine if you use
export LD_PRELOAD... how it has been described above in Lukas answer.
I wasn't successful with /etc/sysconfig/sssd too. And uncle google is 
silent :-(


I propose to change the commit message of the third patch to `export 
LD_PRELAOD=...` instead of `/etc/sysconfig/sssd`. So it should work.


Any other idea?

Regards

--
Petr^4 Čech
>From 15b113dcea02e445dc297f336c543d71cb4ea338 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/3] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 29f4b1d1597bd98541a152dd6462caa864fbf2fd..8b194e3db48870aecd54b21bd3d0b77dc342f9e5 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,11 @@
 #include "util/cert.h"
 #include 
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+
 static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
 {
 const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1181,21 @@ done:
 return ret;
 }
 
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "unknown";
+
+if (state_mask == (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
 int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
  struct ldb_dn *entry_dn,
  struct sysdb_attrs *attrs,
@@ -1184,6 +1204,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 bool sysdb_write = true;
 errno_t ret = EOK;
 errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;
 
 sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
 if (sysdb_write == true) {
@@ -1192,6 +1213,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
   "Cannot set attrs for %s, %d [%s]\n",
   ldb_dn_get_linearized(entry_dn), ret, sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
 }
 }
 
@@ -1201,9 +1224,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
 "Cannot set ts attrs for %s\n", ldb_dn_get_linearized(entry_dn));
 /* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
 }
 }
 
+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
 return ret;
 }
 
-- 
2.7.4

>From 6b3eea9fbdc0775bce530a1567e51bafcfee3163 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/3] SYSDB: Adding message about reason why cache changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..b67769ed11fc0796d1987f09aa568c2db4a0ffab 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message *old_entry,
 return true;
 }
 
-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
  struct ldb_message *mod_msg)
 {
 struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
  */
 if (mod_msg_el->num_values > 0) {
 /* We can ignore additions of timestamp attributes */
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Added attr [%s] to entry [%s]\n",
+  mod_msg_el->name, ldb_dn_get_linearized(entry_dn));
 return true;
 }
 break;
@@ -1855,12 +1859,15 @@ static bool 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

Bump.

--
Petr^4 Čech
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (22/09/16 10:44), Petr Cech wrote:
>On 09/22/2016 10:31 AM, Lukas Slebodnik wrote:
>> On (22/09/16 10:00), Lukas Slebodnik wrote:
>> > On (16/09/16 16:19), Petr Cech wrote:
>> > > On 09/14/2016 04:00 PM, Lukas Slebodnik wrote:
>> > > > Let's assume that we will add new type of cache in future
>> > > > (e.g. SSS_SYSDB_SECRET_CACHE)
>> > > > 
>> > > > If the value of "state_mask" was CACHE | TS_CACHE SECRET_CACHE
>> > > > then this condition would be true but return incorrent string.
>> > > 
>> > > So, I did it more dynamic way now. See attached patch please.
>> > > 
>> > The more dynamic way does not work performance decradation
>> > caused by many useless memory allocations.
>> > 
>> > Your patch calls get_attr_storage every time
>> > even though the result would not be used
>> > due to low debug_level
>> > 
>> > I prefer one of your previous versions
>> > e.g.
>> > +static const char *get_attr_storage(int state_mask)
>> > +{
>> > +const char *storage = "";
>> or maybe default can be "unknown"
>> 
>> > +
>> > +if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
>> > +storage = "cache, ts_cache";
>> > +} else if (state_mask == SSS_SYSDB_TS_CACHE) {
>> > +storage = "ts_cache";
>> > +} else if (state_mask == SSS_SYSDB_CACHE) {
>> > +storage = "cache";
>> > +}
>> > +
>> > +return storage;
>> > +}
>> > +
>> > 
>> > Overhead is minimal and the wrong result will not be printed
>> > in case of addition new tye of cache.
>> > 
>
>Hi Lukas,
>
>new version is attached.
>
>Regards
>
>-- 
>Petr^4 Čech

Attached is an alternative solution for debugging ldb functions
How to test:
LD_PRELOAD=.libs/sss_ldb_debug.so ./sysdb-tests -d 10

The only think would be to find out why LD_PRELOAD in
/etc/sysconfig/sssd is not passwd to child processes.
MY_LD_PRELOAD is passed without issue.

LS
From b210980b7aa90882145e83355c0e25dfa463eac2 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 14:59:06 +0200
Subject: [PATCH] UTIL: Add LD_PRELOAD module for debugging ldb_* operations

This patch adds 4 wrappers:
* ldb_add()
* ldb_delete()
* ldb_modify()
* ldb_rename()

Adding and modifying produce full ldif.
Deleting and renaming produce only short message.

Usage:
echo "LD_PRELOAD=/path/to/module/sss_ldb_debug.so" >> /etc/sysconfig/sssd
and restart sssd.

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 Makefile.am  |  16 
 src/tests/dlopen-tests.c |   1 +
 src/util/sss_ldb_debug.c | 186 +++
 3 files changed, 203 insertions(+)
 create mode 100644 src/util/sss_ldb_debug.c

diff --git a/Makefile.am b/Makefile.am
index 
17c5f26ce9db1e183b30178f1a8714deca1dab03..2b032048d7d739eed752f290eacc3ff59a47f06a
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -979,6 +979,22 @@ libsss_util_la_LIBADD += stap_generated_probes.lo
 endif
 libsss_util_la_LDFLAGS = -avoid-version
 
+noinst_LTLIBRARIES += sss_ldb_debug.la
+sss_ldb_debug_la_SOURCES= \
+src/util/sss_ldb_debug.c \
+$(NULL)
+sss_ldb_debug_la_LIBADD = \
+$(LIBADD_DL) \
+$(LDB_LIBS) \
+libsss_debug.la \
+$(NULL)
+sss_ldb_debug_la_LDFLAGS = \
+-shared \
+-module \
+-avoid-version \
+-rpath $(abs_top_builddir) \
+$(NULL)
+
 pkglib_LTLIBRARIES += libsss_semanage.la
 libsss_semanage_la_CFLAGS = \
 $(AM_CFLAGS) \
diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c
index 
332b268e20d73393293d9c31357406b3756df2fe..dfdd0ac1db62d8737809c0abca2b5dfcd34ecfae
 100644
--- a/src/tests/dlopen-tests.c
+++ b/src/tests/dlopen-tests.c
@@ -113,6 +113,7 @@ struct so {
 #ifdef HAVE_CONFIG_LIB
 { "libsss_config.so", { LIBPFX"libsss_config.so", NULL } },
 #endif
+{ "sss_ldb_debug.so", { LIBPFX"sss_ldb_debug.so", NULL } },
 { NULL }
 };
 
diff --git a/src/util/sss_ldb_debug.c b/src/util/sss_ldb_debug.c
new file mode 100644
index 
..5f8cf0322674c4f4c0949a0e7b2c3f9fa175032c
--- /dev/null
+++ b/src/util/sss_ldb_debug.c
@@ -0,0 +1,186 @@
+/*
+SSSD
+
+Authors:
+Lukas Slebodnik 
+
+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 .
+*/
+
+typedef int errno_t;
+
+#include "config.h"
+
+#include 
+#include 
+#include 
+#include 
+#include "util/debug.h"
+
+struct sss_ldif_vprint_ctx {
+char *ldif;
+};
+

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/22/2016 10:31 AM, Lukas Slebodnik wrote:

On (22/09/16 10:00), Lukas Slebodnik wrote:

On (16/09/16 16:19), Petr Cech wrote:

On 09/14/2016 04:00 PM, Lukas Slebodnik wrote:

Let's assume that we will add new type of cache in future
(e.g. SSS_SYSDB_SECRET_CACHE)

If the value of "state_mask" was CACHE | TS_CACHE SECRET_CACHE
then this condition would be true but return incorrent string.


So, I did it more dynamic way now. See attached patch please.


The more dynamic way does not work performance decradation
caused by many useless memory allocations.

Your patch calls get_attr_storage every time
even though the result would not be used
due to low debug_level

I prefer one of your previous versions
e.g.
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";

or maybe default can be "unknown"


+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+

Overhead is minimal and the wrong result will not be printed
in case of addition new tye of cache.



Hi Lukas,

new version is attached.

Regards

--
Petr^4 Čech
>From d480cfce5c4e3de6d33ff0d860d8f1edda2385b1 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/3] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 29f4b1d1597bd98541a152dd6462caa864fbf2fd..8b194e3db48870aecd54b21bd3d0b77dc342f9e5 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,11 @@
 #include "util/cert.h"
 #include 
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+
 static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
 {
 const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1181,21 @@ done:
 return ret;
 }
 
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "unknown";
+
+if (state_mask == (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
 int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
  struct ldb_dn *entry_dn,
  struct sysdb_attrs *attrs,
@@ -1184,6 +1204,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 bool sysdb_write = true;
 errno_t ret = EOK;
 errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;
 
 sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
 if (sysdb_write == true) {
@@ -1192,6 +1213,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
   "Cannot set attrs for %s, %d [%s]\n",
   ldb_dn_get_linearized(entry_dn), ret, sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
 }
 }
 
@@ -1201,9 +1224,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
 "Cannot set ts attrs for %s\n", ldb_dn_get_linearized(entry_dn));
 /* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
 }
 }
 
+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
 return ret;
 }
 
-- 
2.7.4

>From eab9f5259af8fc2ce9d88313d3ce95fe2a3ee08f Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/3] SYSDB: Adding message about reason why cache changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..b67769ed11fc0796d1987f09aa568c2db4a0ffab 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message *old_entry,
 return true;
 }
 
-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
  struct ldb_message *mod_msg)
 {
 struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,9 @@ static bool 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (22/09/16 10:00), Lukas Slebodnik wrote:
>On (16/09/16 16:19), Petr Cech wrote:
>>On 09/14/2016 04:00 PM, Lukas Slebodnik wrote:
>>> Let's assume that we will add new type of cache in future
>>> (e.g. SSS_SYSDB_SECRET_CACHE)
>>>
>>> If the value of "state_mask" was CACHE | TS_CACHE SECRET_CACHE
>>> then this condition would be true but return incorrent string.
>>
>>So, I did it more dynamic way now. See attached patch please.
>>
>The more dynamic way does not work performance decradation
>caused by many useless memory allocations.
>
>Your patch calls get_attr_storage every time
>even though the result would not be used
>due to low debug_level
>
>I prefer one of your previous versions
>e.g.
>+static const char *get_attr_storage(int state_mask)
>+{
>+const char *storage = "";
or maybe default can be "unknown"

>+
>+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
>+storage = "cache, ts_cache";
>+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
>+storage = "ts_cache";
>+} else if (state_mask == SSS_SYSDB_CACHE) {
>+storage = "cache";
>+}
>+
>+return storage;
>+}
>+
>
>Overhead is minimal and the wrong result will not be printed
>in case of addition new tye of cache.
>
LS
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

bump

--
Petr^4 Čech
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (16/09/16 16:19), Petr Cech wrote:
>On 09/14/2016 04:00 PM, Lukas Slebodnik wrote:
>> On (06/09/16 13:15), Petr Cech wrote:
>> > On 09/05/2016 02:31 PM, Fabiano Fidêncio wrote:
>> > > On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
>> > > wrote:
>> > > > Petr,
>> > > >
>> > > > I went through your patches and in general they look good to me.
>> > > > However, I haven't done any tests yet with your patches (and I'll do
>> > > > it after lunch).
>> > >
>> > > I've done some tests and I've been able to see the ldif changes in the
>> > > domain log. So, I assume it's working.
>> > > For sure it's a good improvement! Would be worth to link some
>> > > documentation about ldiff as it may be confusing for someone who is
>> > > not used to it.
>> > >
>> > > I'll wait for a new version of the patches and go through them again.
>> > >
>> > > I really would like to have someone's else opinion on this series.
>> > >
>> > > >
>> > > > Please, below you can see a few comments. Feel completely free to
>> > > > ignore the first one if you feel like doing it, it's just a minor :-)
>> > > > For the other comments, I'd like to understand a few changes you have 
>> > > > done.
>> > > >
>> > > >
>> > > > Patch 0001: SYSDB: Adding message to inform which cache is used
>> > > >
>> > > > About the following part of the patch:
>> > > > +static const char *get_attr_storage(int state_mask)
>> > > > +{
>> > > > +const char *storage = "";
>> > > > +
>> > > > +if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
>> > > > +storage = "cache, ts_cache";
>> > > > +} else if (state_mask == SSS_SYSDB_TS_CACHE) {
>> > > > +storage = "ts_cache";
>> > > > +} else if (state_mask == SSS_SYSDB_CACHE) {
>> > > > +storage = "cache";
>> > > > +}
>> > > > +
>> > > > +return storage;
>> > > > +}
>> > > >
>> > > > I personally don't like this kind of comparison done with flags. I'd
>> > > > go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
>> > > > ...
>> > > > But this is a really minor and feel free to ignore it.
>> > > >
>> > > >
>> > > > Patch 0002: SYSDB: Adding message about reason why cache changed
>> > > >
>> > > > LGTM
>> > > >
>> > > >
>> > > > Patch 0003: SYSDB: Adding wrappers for ldb_* operations
>> > > >
>> > > > About the following parts of the patch:
>> > > >
>> > > > On src/db/sysdb_ldb_wrapper.c
>> > > >
>> > > > +#define ERR_FN_ENOMEM (-1 * ENOMEM)
>> > > > +#define ERR_FN_ENOENT (-1 * ENOENT)
>> > > >
>> > > > Why? I failed to understand why you're doing this here.
>> > > >
>> > > > +if (print_ctx == NULL) {
>> > > > +return -1;
>> > > > +return ERR_FN_ENOMEM;
>> > > > +}
>> > > >
>> > > > I guess the return -1 is a leftover :-)
>> > > >
>> > > > +if (print_ctx->ldif == NULL) {
>> > > > +return -2;
>> > > > +return ERR_FN_ENOENT;
>> > > > +}
>> > > >
>> > > > I guess the return -2 is also a leftover :-)
>> > > >
>> > > > +if (ret < 0) {
>> > > > +DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with 
>> > > > [%d][%s].\n",
>> > > > +-1 * ret, sss_strerror(-1 * ret));
>> > > > +goto done;
>> > > > +}
>> > > >
>> > > > And here again this dance multiplying by -1 that I don't understand
>> > > > the reason :-\
>> > > >
>> > > > +done:
>> > > > +if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
>> > > > +talloc_free(ldb_print_ctx->ldif);
>> > > > +}
>> > > > +talloc_free(ldb_print_ctx);
>> > > >
>> > > > AFAIU talloc_free can gracefully handle NULL. Considering that's the
>> > > > case I'd just check for (if ldb_print_ctx != NULL)
>> > > > talloc_free(ldb_print_ctx->ldif);
>> > > > Considering it doesn't, we may have some issues on trying to free
>> > > > (ldb_print_ctx)
>> > > >
>> > > > On src/db/sysdb_ldb_wrapper.h:
>> > > >
>> > > > +int sss_ldb_rename(struct ldb_context *ldb,
>> > > > +   struct ldb_dn * olddn,
>> > > > +   struct ldb_dn *newdn);
>> > > >
>> > > > Just a really minor codying style change here, remove the extra space
>> > > > between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,
>> > > >
>> > > >
>> > > > Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
>> > > > Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
>> > > > Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
>> > > > Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb
>> > > >
>> > > > LGTM
>> > > >
>> > > >
>> > > > Best Regards,
>> > > > --
>> > > > Fabiano Fidêncio
>> >
>> > Hello,
>> >
>> >
>> > there is new patch set attached.
>> > I replaced all ldb_* to new wrapper in whole code.
>> >
>> > Regards
>> >
>> > --
>> > Petr^4 Čech
>>
>> > From 529b0d3009f8310b8257d5a69639a0fafa30140c Mon Sep 17 00:00:00 2001
>> > From: Petr Cech 
>> > Date: Tue, 16 Aug 2016 09:32:18 +0200
>> > Subject: [PATCH 1/7] SYSDB: Adding message to 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/14/2016 04:00 PM, Lukas Slebodnik wrote:

On (06/09/16 13:15), Petr Cech wrote:

On 09/05/2016 02:31 PM, Fabiano Fidêncio wrote:

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).


I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.



Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have done.


Patch 0001: SYSDB: Adding message to inform which cache is used

About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}

I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.


Patch 0002: SYSDB: Adding message about reason why cache changed

LGTM


Patch 0003: SYSDB: Adding wrappers for ldb_* operations

About the following parts of the patch:

On src/db/sysdb_ldb_wrapper.c

+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)

Why? I failed to understand why you're doing this here.

+if (print_ctx == NULL) {
+return -1;
+return ERR_FN_ENOMEM;
+}

I guess the return -1 is a leftover :-)

+if (print_ctx->ldif == NULL) {
+return -2;
+return ERR_FN_ENOENT;
+}

I guess the return -2 is also a leftover :-)

+if (ret < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with [%d][%s].\n",
+-1 * ret, sss_strerror(-1 * ret));
+goto done;
+}

And here again this dance multiplying by -1 that I don't understand
the reason :-\

+done:
+if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+talloc_free(ldb_print_ctx->ldif);
+}
+talloc_free(ldb_print_ctx);

AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)

On src/db/sysdb_ldb_wrapper.h:

+int sss_ldb_rename(struct ldb_context *ldb,
+   struct ldb_dn * olddn,
+   struct ldb_dn *newdn);

Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,


Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb

LGTM


Best Regards,
--
Fabiano Fidêncio


Hello,


there is new patch set attached.
I replaced all ldb_* to new wrapper in whole code.

Regards

--
Petr^4 Čech



From 529b0d3009f8310b8257d5a69639a0fafa30140c Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/7] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
src/db/sysdb_ops.c | 32 
1 file changed, 32 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 
5d9c9fb24a149f8215b3027dcb4b0e1a183e4b43..847b663bdb2ec31de3eb3b4c33e2b942145a4c42
 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,12 @@
#include "util/cert.h"
#include 

+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)
+
static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
{
const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1182,21 @@ done:
return ret;
}

+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0) {

Let's assume that we will add new type of cache in future
(e.g. SSS_SYSDB_SECRET_CACHE)

If the value of "state_mask" was CACHE | TS_CACHE SECRET_CACHE
then this condition would be true but return incorrent string.


So, I 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (06/09/16 13:15), Petr Cech wrote:
>On 09/05/2016 02:31 PM, Fabiano Fidêncio wrote:
>> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
>> wrote:
>> > Petr,
>> > 
>> > I went through your patches and in general they look good to me.
>> > However, I haven't done any tests yet with your patches (and I'll do
>> > it after lunch).
>> 
>> I've done some tests and I've been able to see the ldif changes in the
>> domain log. So, I assume it's working.
>> For sure it's a good improvement! Would be worth to link some
>> documentation about ldiff as it may be confusing for someone who is
>> not used to it.
>> 
>> I'll wait for a new version of the patches and go through them again.
>> 
>> I really would like to have someone's else opinion on this series.
>> 
>> > 
>> > Please, below you can see a few comments. Feel completely free to
>> > ignore the first one if you feel like doing it, it's just a minor :-)
>> > For the other comments, I'd like to understand a few changes you have done.
>> > 
>> > 
>> > Patch 0001: SYSDB: Adding message to inform which cache is used
>> > 
>> > About the following part of the patch:
>> > +static const char *get_attr_storage(int state_mask)
>> > +{
>> > +const char *storage = "";
>> > +
>> > +if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
>> > +storage = "cache, ts_cache";
>> > +} else if (state_mask == SSS_SYSDB_TS_CACHE) {
>> > +storage = "ts_cache";
>> > +} else if (state_mask == SSS_SYSDB_CACHE) {
>> > +storage = "cache";
>> > +}
>> > +
>> > +return storage;
>> > +}
>> > 
>> > I personally don't like this kind of comparison done with flags. I'd
>> > go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
>> > ...
>> > But this is a really minor and feel free to ignore it.
>> > 
>> > 
>> > Patch 0002: SYSDB: Adding message about reason why cache changed
>> > 
>> > LGTM
>> > 
>> > 
>> > Patch 0003: SYSDB: Adding wrappers for ldb_* operations
>> > 
>> > About the following parts of the patch:
>> > 
>> > On src/db/sysdb_ldb_wrapper.c
>> > 
>> > +#define ERR_FN_ENOMEM (-1 * ENOMEM)
>> > +#define ERR_FN_ENOENT (-1 * ENOENT)
>> > 
>> > Why? I failed to understand why you're doing this here.
>> > 
>> > +if (print_ctx == NULL) {
>> > +return -1;
>> > +return ERR_FN_ENOMEM;
>> > +}
>> > 
>> > I guess the return -1 is a leftover :-)
>> > 
>> > +if (print_ctx->ldif == NULL) {
>> > +return -2;
>> > +return ERR_FN_ENOENT;
>> > +}
>> > 
>> > I guess the return -2 is also a leftover :-)
>> > 
>> > +if (ret < 0) {
>> > +DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with 
>> > [%d][%s].\n",
>> > +-1 * ret, sss_strerror(-1 * ret));
>> > +goto done;
>> > +}
>> > 
>> > And here again this dance multiplying by -1 that I don't understand
>> > the reason :-\
>> > 
>> > +done:
>> > +if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
>> > +talloc_free(ldb_print_ctx->ldif);
>> > +}
>> > +talloc_free(ldb_print_ctx);
>> > 
>> > AFAIU talloc_free can gracefully handle NULL. Considering that's the
>> > case I'd just check for (if ldb_print_ctx != NULL)
>> > talloc_free(ldb_print_ctx->ldif);
>> > Considering it doesn't, we may have some issues on trying to free
>> > (ldb_print_ctx)
>> > 
>> > On src/db/sysdb_ldb_wrapper.h:
>> > 
>> > +int sss_ldb_rename(struct ldb_context *ldb,
>> > +   struct ldb_dn * olddn,
>> > +   struct ldb_dn *newdn);
>> > 
>> > Just a really minor codying style change here, remove the extra space
>> > between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,
>> > 
>> > 
>> > Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
>> > Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
>> > Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
>> > Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb
>> > 
>> > LGTM
>> > 
>> > 
>> > Best Regards,
>> > --
>> > Fabiano Fidêncio
>
>Hello,
>
>
>there is new patch set attached.
>I replaced all ldb_* to new wrapper in whole code.
>
>Regards
>
>-- 
>Petr^4 Čech

>From 529b0d3009f8310b8257d5a69639a0fafa30140c Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 16 Aug 2016 09:32:18 +0200
>Subject: [PATCH 1/7] SYSDB: Adding message to inform which cache is used
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/3060
>---
> src/db/sysdb_ops.c | 32 
> 1 file changed, 32 insertions(+)
>
>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
>index 
>5d9c9fb24a149f8215b3027dcb4b0e1a183e4b43..847b663bdb2ec31de3eb3b4c33e2b942145a4c42
> 100644
>--- a/src/db/sysdb_ops.c
>+++ b/src/db/sysdb_ops.c
>@@ -27,6 +27,12 @@
> #include "util/cert.h"
> #include 
> 
>+
>+#define SSS_SYSDB_NO_CACHE 0x0
>+#define SSS_SYSDB_CACHE 0x1
>+#define SSS_SYSDB_TS_CACHE 0x2
>+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/12/2016 10:01 AM, Lukas Slebodnik wrote:

On (11/09/16 23:49), Jakub Hrozek wrote:

On Thu, Sep 08, 2016 at 12:56:08PM +0200, Lukas Slebodnik wrote:

Let me explain why wrappers are not good idea in production.
There was introduced new wrapper(#1991) for ldb_search
SSS_LDB_SEARCH. It should guarantee that ENONET will be
returned and not EOK + res->count == 0.

I found just a single usage of this wrapper since introducing
but many usage of ldb_search (I stopped counting after 10).
And there will be the same problem with newly introduced wrappers.
It's crystal clear that review does not help. Otherwise we would use
SSS_LDB_SEARCH everywhere.

That is a reason why I am fine with using wrappers just for a for development
but not for productions. Or try to propose some automatic way
how to simply log ldifs for *ALL* required ldb functions.
IMHO, it would be the best to implement it in libldb itself.


You have a point here (and I regret adding the ENOENT retval in general,
but the difference is that ldb_search wrapper changes /functionality/, this
just adds logging. So the only thing we would miss if we forget to use
the wrapper is the extra debugging. And in that case we would have to
build a new package and commit the messages to master, but that's no
different from missing debug messages in general.


Inconsistencies are bad. it does not matter wheter it's about ENOENT
or logging.


There's been quite a few cases where I wanted to see what exactly is
being added for example with duplicate member: attributes or a member
attribute that points nowhere or 'binary' attributes. These patches
would solve it nicely.

And for such few cases you can prepare custom build of sssd.

I am fine with first 3 patches but rest of patches (replacement
ldb_* with wrappers) have NACK or
a) you can find other way how to consistently log messages from ldb_ functions

BTW you will still be able to prepare test builds for debugging because
wrappers will be part of upstream (but will be ununsed by default.)


Hello Lukas,

I will be glad if we push first 3 patches now.

I am not happy we have no consensus for the other 4.
We could discuss if there is other way how to consistently
log messages from ldb_ functions on SSSD meeting.

Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (12/09/16 10:23), Petr Cech wrote:
>On 09/12/2016 10:01 AM, Lukas Slebodnik wrote:
>> > You have a point here (and I regret adding the ENOENT retval in general,
>> > >but the difference is that ldb_search wrapper changes /functionality/, 
>> > >this
>> > >just adds logging. So the only thing we would miss if we forget to use
>> > >the wrapper is the extra debugging. And in that case we would have to
>> > >build a new package and commit the messages to master, but that's no
>> > >different from missing debug messages in general.
>> > >
>> Inconsistencies are bad. it does not matter wheter it's about ENOENT
>> or logging.
>
>Hi Lukas,
>
>if you are afraid of inconsistency in using wrappers at all places in code I
>think I could prepare test which say you that you should write sss_ldb_*
>instead od ldb_*.
>
You would need to write a plugin to compiler + linker to be 100% sure.
Or you can write logging feature to the libldb.

I already wrote in previous mails. Wrappers are bad.
I am fine with using wrappers for developer porposes or for testing builds.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/12/2016 10:01 AM, Lukas Slebodnik wrote:

You have a point here (and I regret adding the ENOENT retval in general,
>but the difference is that ldb_search wrapper changes /functionality/, this
>just adds logging. So the only thing we would miss if we forget to use
>the wrapper is the extra debugging. And in that case we would have to
>build a new package and commit the messages to master, but that's no
>different from missing debug messages in general.
>

Inconsistencies are bad. it does not matter wheter it's about ENOENT
or logging.


Hi Lukas,

if you are afraid of inconsistency in using wrappers at all places in 
code I think I could prepare test which say you that you should write 
sss_ldb_* instead od ldb_*.


Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (11/09/16 23:49), Jakub Hrozek wrote:
>On Thu, Sep 08, 2016 at 12:56:08PM +0200, Lukas Slebodnik wrote:
>> Let me explain why wrappers are not good idea in production.
>> There was introduced new wrapper(#1991) for ldb_search
>> SSS_LDB_SEARCH. It should guarantee that ENONET will be
>> returned and not EOK + res->count == 0.
>> 
>> I found just a single usage of this wrapper since introducing
>> but many usage of ldb_search (I stopped counting after 10).
>> And there will be the same problem with newly introduced wrappers.
>> It's crystal clear that review does not help. Otherwise we would use
>> SSS_LDB_SEARCH everywhere.
>> 
>> That is a reason why I am fine with using wrappers just for a for development
>> but not for productions. Or try to propose some automatic way
>> how to simply log ldifs for *ALL* required ldb functions.
>> IMHO, it would be the best to implement it in libldb itself.
>
>You have a point here (and I regret adding the ENOENT retval in general,
>but the difference is that ldb_search wrapper changes /functionality/, this
>just adds logging. So the only thing we would miss if we forget to use
>the wrapper is the extra debugging. And in that case we would have to
>build a new package and commit the messages to master, but that's no
>different from missing debug messages in general.
>
Inconsistencies are bad. it does not matter wheter it's about ENOENT
or logging.

>There's been quite a few cases where I wanted to see what exactly is
>being added for example with duplicate member: attributes or a member
>attribute that points nowhere or 'binary' attributes. These patches
>would solve it nicely.
And for such few cases you can prepare custom build of sssd.

I am fine with first 3 patches but rest of patches (replacement
ldb_* with wrappers) have NACK or
a) you can find other way how to consistently log messages from ldb_ functions

BTW you will still be able to prepare test builds for debugging because
wrappers will be part of upstream (but will be ununsed by default.)

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Jakub Hrozek]

On Thu, Sep 08, 2016 at 12:56:08PM +0200, Lukas Slebodnik wrote:
> On (07/09/16 09:53), Jakub Hrozek wrote:
> >On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote:
> >> On (05/09/16 16:07), Jakub Hrozek wrote:
> >> >On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:
> >> >> On (05/09/16 15:24), Jakub Hrozek wrote:
> >> >> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
> >> >> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio 
> >> >> >>  wrote:
> >> >> >> > Petr,
> >> >> >> >
> >> >> >> > I went through your patches and in general they look good to me.
> >> >> >> > However, I haven't done any tests yet with your patches (and I'll 
> >> >> >> > do
> >> >> >> > it after lunch).
> >> >> >> 
> >> >> >> I've done some tests and I've been able to see the ldif changes in 
> >> >> >> the
> >> >> >> domain log. So, I assume it's working.
> >> >> >> For sure it's a good improvement! Would be worth to link some
> >> >> >> documentation about ldiff as it may be confusing for someone who is
> >> >> >> not used to it.
> >> >> >> 
> >> >> >> I'll wait for a new version of the patches and go through them again.
> >> >> >> 
> >> >> >> I really would like to have someone's else opinion on this series.
> >> >> >
> >> >> >I quickly scrolled through the patches and the primary thing I don't
> >> >> >understand is why are the wrappers used only in sysdb? I think we 
> >> >> >should
> >> >> >just use them everywhere..
> >> >> I do not like wrappers.
> >> >> We should not log ldif by default.
> >> >
> >> >That's why there is a separate log level, you need to turn these on
> >> >separately (yes, logging LDIFs by default would be too much..)
> >> >
> >> Even though it is a separate debug level users still might
> >> enable them by a chance.
> >
> >How, except reading the code? This new debug level is not documented
> >anywhere and even starts off at a different base so neither debug_level=10
> >nor debug_level=0xFFF0 will trigger this.
> >
> >> IMH0 it will be confusing for them.
> >> There are many users which are confused when try to analyze
> >> sudo logs. They can see some "LDAP like" filters which
> >> are used for internal searching. Users think we use wrong attribute
> >> due to sudoRule -> sudoRole.
> >
> >really? Someone who will discover a magic constant on SSSD will then be
> >confused by more logging?
> >
> >btw what if this extra debugging was controlled by an environment
> >variable, do you think then it would be safer?
> >
> Eviroment variable and new debug_level are just a way how to enable logging.
> But my main concern is that we should not log them.
> 
> >Or if we prepend the LDIF with something like "SSSD cache modification
> >message" ?
> >
> Our long standing goal is to make log files much better for users.
> We have a big mess in debug_level and we log irelevant/confusing
> messages with hight debug level.
> 
> Maybe I still miss the point but how this debug_level improves it.
> And remeber that nothing is *undocumented* in open-source.

I'm not sure I understand..? We already have some undocumented
options. I agree it's easy to discover the option if you look into the
source, but if anyone is able to look at the source, I'm pretty sure
they won't be suprised by LDIF messages in cache..

> We(developers) are used to recomend full debul level for debugging
> issues. So it would very soon to become a standart to require
> debug_level 0x0. And if you coombine it with debug logs from AD
> with enabled logging from nsupdate it will be a real mess.

No, the recommended debug level would stay the same. Also, what about
the environment variable? Do you think we should start recommending to
also put LDB_LDIF_DEBUG=1 and debug_level=10/0xfff0?

> 
> >> 
> >> These wrappers should not be used in sssd upstream.
> >> They can be prepared for debugging purposes in development process.
> >
> >...which means we will have to rebase the patches, build the correct
> >version, pass it on to the person and care about correct versioning...
> >
> Why should we rebase patches?
> It rarely happen that we provides custom builds with extra debugging.
> And everythime it is a different custom build for different user.
> And it would be the same case with using custom builds with using
> wrappers.
> 
> >Sorry, I just don't agree with any of the arguments, but I'm curious
> >to hear more.
> Let me explain why wrappers are not good idea in production.
> There was introduced new wrapper(#1991) for ldb_search
> SSS_LDB_SEARCH. It should guarantee that ENONET will be
> returned and not EOK + res->count == 0.
> 
> I found just a single usage of this wrapper since introducing
> but many usage of ldb_search (I stopped counting after 10).
> And there will be the same problem with newly introduced wrappers.
> It's crystal clear that review does not help. Otherwise we would use
> SSS_LDB_SEARCH everywhere.
> 
> That is a reason why I am fine with using wrappers just for a 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (07/09/16 09:53), Jakub Hrozek wrote:
>On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote:
>> On (05/09/16 16:07), Jakub Hrozek wrote:
>> >On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:
>> >> On (05/09/16 15:24), Jakub Hrozek wrote:
>> >> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
>> >> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio 
>> >> >>  wrote:
>> >> >> > Petr,
>> >> >> >
>> >> >> > I went through your patches and in general they look good to me.
>> >> >> > However, I haven't done any tests yet with your patches (and I'll do
>> >> >> > it after lunch).
>> >> >> 
>> >> >> I've done some tests and I've been able to see the ldif changes in the
>> >> >> domain log. So, I assume it's working.
>> >> >> For sure it's a good improvement! Would be worth to link some
>> >> >> documentation about ldiff as it may be confusing for someone who is
>> >> >> not used to it.
>> >> >> 
>> >> >> I'll wait for a new version of the patches and go through them again.
>> >> >> 
>> >> >> I really would like to have someone's else opinion on this series.
>> >> >
>> >> >I quickly scrolled through the patches and the primary thing I don't
>> >> >understand is why are the wrappers used only in sysdb? I think we should
>> >> >just use them everywhere..
>> >> I do not like wrappers.
>> >> We should not log ldif by default.
>> >
>> >That's why there is a separate log level, you need to turn these on
>> >separately (yes, logging LDIFs by default would be too much..)
>> >
>> Even though it is a separate debug level users still might
>> enable them by a chance.
>
>How, except reading the code? This new debug level is not documented
>anywhere and even starts off at a different base so neither debug_level=10
>nor debug_level=0xFFF0 will trigger this.
>
>> IMH0 it will be confusing for them.
>> There are many users which are confused when try to analyze
>> sudo logs. They can see some "LDAP like" filters which
>> are used for internal searching. Users think we use wrong attribute
>> due to sudoRule -> sudoRole.
>
>really? Someone who will discover a magic constant on SSSD will then be
>confused by more logging?
>
>btw what if this extra debugging was controlled by an environment
>variable, do you think then it would be safer?
>
Eviroment variable and new debug_level are just a way how to enable logging.
But my main concern is that we should not log them.

>Or if we prepend the LDIF with something like "SSSD cache modification
>message" ?
>
Our long standing goal is to make log files much better for users.
We have a big mess in debug_level and we log irelevant/confusing
messages with hight debug level.

Maybe I still miss the point but how this debug_level improves it.
And remeber that nothing is *undocumented* in open-source.
We(developers) are used to recomend full debul level for debugging
issues. So it would very soon to become a standart to require
debug_level 0x0. And if you coombine it with debug logs from AD
with enabled logging from nsupdate it will be a real mess.

>> 
>> These wrappers should not be used in sssd upstream.
>> They can be prepared for debugging purposes in development process.
>
>...which means we will have to rebase the patches, build the correct
>version, pass it on to the person and care about correct versioning...
>
Why should we rebase patches?
It rarely happen that we provides custom builds with extra debugging.
And everythime it is a different custom build for different user.
And it would be the same case with using custom builds with using
wrappers.

>Sorry, I just don't agree with any of the arguments, but I'm curious
>to hear more.
Let me explain why wrappers are not good idea in production.
There was introduced new wrapper(#1991) for ldb_search
SSS_LDB_SEARCH. It should guarantee that ENONET will be
returned and not EOK + res->count == 0.

I found just a single usage of this wrapper since introducing
but many usage of ldb_search (I stopped counting after 10).
And there will be the same problem with newly introduced wrappers.
It's crystal clear that review does not help. Otherwise we would use
SSS_LDB_SEARCH everywhere.

That is a reason why I am fine with using wrappers just for a for development
but not for productions. Or try to propose some automatic way
how to simply log ldifs for *ALL* required ldb functions.
IMHO, it would be the best to implement it in libldb itself.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/07/2016 09:53 AM, Jakub Hrozek wrote:

On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote:

On (05/09/16 16:07), Jakub Hrozek wrote:

On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:

On (05/09/16 15:24), Jakub Hrozek wrote:

On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).


I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.


I quickly scrolled through the patches and the primary thing I don't
understand is why are the wrappers used only in sysdb? I think we should
just use them everywhere..

I do not like wrappers.
We should not log ldif by default.


That's why there is a separate log level, you need to turn these on
separately (yes, logging LDIFs by default would be too much..)


Even though it is a separate debug level users still might
enable them by a chance.


How, except reading the code? This new debug level is not documented
anywhere and even starts off at a different base so neither debug_level=10
nor debug_level=0xFFF0 will trigger this.


IMH0 it will be confusing for them.
There are many users which are confused when try to analyze
sudo logs. They can see some "LDAP like" filters which
are used for internal searching. Users think we use wrong attribute
due to sudoRule -> sudoRole.


really? Someone who will discover a magic constant on SSSD will then be
confused by more logging?

btw what if this extra debugging was controlled by an environment
variable, do you think then it would be safer?

Or if we prepend the LDIF with something like "SSSD cache modification
message" ?



These wrappers should not be used in sssd upstream.
They can be prepared for debugging purposes in development process.


...which means we will have to rebase the patches, build the correct
version, pass it on to the person and care about correct versioning...

Sorry, I just don't agree with any of the arguments, but I'm curious
to hear more.


Thanks, Jakub.

Lukas, I am afraid I am not able to imagine that we have some
wrappers which is not used in code but software engineers use them
for their daily job. It sounds like obstacle for me.

I understand that we have to be care of logs... but this high debug 
level isn't mentioned in man page. So if user will use it it will be 
accident anyway.


Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Jakub Hrozek]

On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote:
> On (05/09/16 16:07), Jakub Hrozek wrote:
> >On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:
> >> On (05/09/16 15:24), Jakub Hrozek wrote:
> >> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
> >> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
> >> >> wrote:
> >> >> > Petr,
> >> >> >
> >> >> > I went through your patches and in general they look good to me.
> >> >> > However, I haven't done any tests yet with your patches (and I'll do
> >> >> > it after lunch).
> >> >> 
> >> >> I've done some tests and I've been able to see the ldif changes in the
> >> >> domain log. So, I assume it's working.
> >> >> For sure it's a good improvement! Would be worth to link some
> >> >> documentation about ldiff as it may be confusing for someone who is
> >> >> not used to it.
> >> >> 
> >> >> I'll wait for a new version of the patches and go through them again.
> >> >> 
> >> >> I really would like to have someone's else opinion on this series.
> >> >
> >> >I quickly scrolled through the patches and the primary thing I don't
> >> >understand is why are the wrappers used only in sysdb? I think we should
> >> >just use them everywhere..
> >> I do not like wrappers.
> >> We should not log ldif by default.
> >
> >That's why there is a separate log level, you need to turn these on
> >separately (yes, logging LDIFs by default would be too much..)
> >
> Even though it is a separate debug level users still might
> enable them by a chance.

How, except reading the code? This new debug level is not documented
anywhere and even starts off at a different base so neither debug_level=10
nor debug_level=0xFFF0 will trigger this.

> IMH0 it will be confusing for them.
> There are many users which are confused when try to analyze
> sudo logs. They can see some "LDAP like" filters which
> are used for internal searching. Users think we use wrong attribute
> due to sudoRule -> sudoRole.

really? Someone who will discover a magic constant on SSSD will then be
confused by more logging?

btw what if this extra debugging was controlled by an environment
variable, do you think then it would be safer?

Or if we prepend the LDIF with something like "SSSD cache modification
message" ?

> 
> These wrappers should not be used in sssd upstream.
> They can be prepared for debugging purposes in development process.

...which means we will have to rebase the patches, build the correct
version, pass it on to the person and care about correct versioning...

Sorry, I just don't agree with any of the arguments, but I'm curious
to hear more.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (05/09/16 15:35), Petr Cech wrote:
>On 09/05/2016 03:32 PM, Lukas Slebodnik wrote:
>> On (05/09/16 15:24), Jakub Hrozek wrote:
>> > On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
>> > > On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
>> > > wrote:
>> > > > Petr,
>> > > > 
>> > > > I went through your patches and in general they look good to me.
>> > > > However, I haven't done any tests yet with your patches (and I'll do
>> > > > it after lunch).
>> > > 
>> > > I've done some tests and I've been able to see the ldif changes in the
>> > > domain log. So, I assume it's working.
>> > > For sure it's a good improvement! Would be worth to link some
>> > > documentation about ldiff as it may be confusing for someone who is
>> > > not used to it.
>> > > 
>> > > I'll wait for a new version of the patches and go through them again.
>> > > 
>> > > I really would like to have someone's else opinion on this series.
>> > 
>> > I quickly scrolled through the patches and the primary thing I don't
>> > understand is why are the wrappers used only in sysdb? I think we should
>> > just use them everywhere..
>> I do not like wrappers.
>> We should not log ldif by default.
>> I thought they would be used just for development purposes.
>> therefore they should not be used anywhere and not everywhere.
>> 
>> LS
>
>Hello Lukas,
>
>please, are you satisfied with those wrappers at really high debug level?
>
See my other answer.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (05/09/16 16:07), Jakub Hrozek wrote:
>On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:
>> On (05/09/16 15:24), Jakub Hrozek wrote:
>> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
>> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
>> >> wrote:
>> >> > Petr,
>> >> >
>> >> > I went through your patches and in general they look good to me.
>> >> > However, I haven't done any tests yet with your patches (and I'll do
>> >> > it after lunch).
>> >> 
>> >> I've done some tests and I've been able to see the ldif changes in the
>> >> domain log. So, I assume it's working.
>> >> For sure it's a good improvement! Would be worth to link some
>> >> documentation about ldiff as it may be confusing for someone who is
>> >> not used to it.
>> >> 
>> >> I'll wait for a new version of the patches and go through them again.
>> >> 
>> >> I really would like to have someone's else opinion on this series.
>> >
>> >I quickly scrolled through the patches and the primary thing I don't
>> >understand is why are the wrappers used only in sysdb? I think we should
>> >just use them everywhere..
>> I do not like wrappers.
>> We should not log ldif by default.
>
>That's why there is a separate log level, you need to turn these on
>separately (yes, logging LDIFs by default would be too much..)
>
Even though it is a separate debug level users still might
enable them by a chance. IMH0 it will be confusing for them.
There are many users which are confused when try to analyze
sudo logs. They can see some "LDAP like" filters which
are used for internal searching. Users think we use wrong attribute
due to sudoRule -> sudoRole.

These wrappers should not be used in sssd upstream.
They can be prepared for debugging purposes in development process.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/06/2016 01:18 PM, Petr Cech wrote:



On 09/06/2016 01:15 PM, Petr Cech wrote:

On 09/05/2016 02:31 PM, Fabiano Fidêncio wrote:

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio
 wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).


I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.



Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have
done.


Patch 0001: SYSDB: Adding message to inform which cache is used

About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}

I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.


Patch 0002: SYSDB: Adding message about reason why cache changed

LGTM


Patch 0003: SYSDB: Adding wrappers for ldb_* operations

About the following parts of the patch:

On src/db/sysdb_ldb_wrapper.c

+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)

Why? I failed to understand why you're doing this here.

+if (print_ctx == NULL) {
+return -1;
+return ERR_FN_ENOMEM;
+}

I guess the return -1 is a leftover :-)

+if (print_ctx->ldif == NULL) {
+return -2;
+return ERR_FN_ENOENT;
+}

I guess the return -2 is also a leftover :-)

+if (ret < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with
[%d][%s].\n",
+-1 * ret, sss_strerror(-1 * ret));
+goto done;
+}

And here again this dance multiplying by -1 that I don't understand
the reason :-\

+done:
+if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+talloc_free(ldb_print_ctx->ldif);
+}
+talloc_free(ldb_print_ctx);

AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)

On src/db/sysdb_ldb_wrapper.h:

+int sss_ldb_rename(struct ldb_context *ldb,
+   struct ldb_dn * olddn,
+   struct ldb_dn *newdn);

Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,


Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb

LGTM


Best Regards,
--
Fabiano Fidêncio


Hello,


there is new patch set attached.
I replaced all ldb_* to new wrapper in whole code.


I wondered if my VM could push patches to our CI.
I will link the CI results after they finish.


OK, my VM is able to push to CI tests (I was afraid I have issue).
But result is not good:
http://sssd-ci.duckdns.org/logs/job/53/00/summary.html

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/06/2016 01:15 PM, Petr Cech wrote:

On 09/05/2016 02:31 PM, Fabiano Fidêncio wrote:

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio
 wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).


I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.



Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have
done.


Patch 0001: SYSDB: Adding message to inform which cache is used

About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}

I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.


Patch 0002: SYSDB: Adding message about reason why cache changed

LGTM


Patch 0003: SYSDB: Adding wrappers for ldb_* operations

About the following parts of the patch:

On src/db/sysdb_ldb_wrapper.c

+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)

Why? I failed to understand why you're doing this here.

+if (print_ctx == NULL) {
+return -1;
+return ERR_FN_ENOMEM;
+}

I guess the return -1 is a leftover :-)

+if (print_ctx->ldif == NULL) {
+return -2;
+return ERR_FN_ENOENT;
+}

I guess the return -2 is also a leftover :-)

+if (ret < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with
[%d][%s].\n",
+-1 * ret, sss_strerror(-1 * ret));
+goto done;
+}

And here again this dance multiplying by -1 that I don't understand
the reason :-\

+done:
+if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+talloc_free(ldb_print_ctx->ldif);
+}
+talloc_free(ldb_print_ctx);

AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)

On src/db/sysdb_ldb_wrapper.h:

+int sss_ldb_rename(struct ldb_context *ldb,
+   struct ldb_dn * olddn,
+   struct ldb_dn *newdn);

Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,


Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb

LGTM


Best Regards,
--
Fabiano Fidêncio


Hello,


there is new patch set attached.
I replaced all ldb_* to new wrapper in whole code.


I wondered if my VM could push patches to our CI.
I will link the CI results after they finish.

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Jakub Hrozek]

On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:
> On (05/09/16 15:24), Jakub Hrozek wrote:
> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
> >> wrote:
> >> > Petr,
> >> >
> >> > I went through your patches and in general they look good to me.
> >> > However, I haven't done any tests yet with your patches (and I'll do
> >> > it after lunch).
> >> 
> >> I've done some tests and I've been able to see the ldif changes in the
> >> domain log. So, I assume it's working.
> >> For sure it's a good improvement! Would be worth to link some
> >> documentation about ldiff as it may be confusing for someone who is
> >> not used to it.
> >> 
> >> I'll wait for a new version of the patches and go through them again.
> >> 
> >> I really would like to have someone's else opinion on this series.
> >
> >I quickly scrolled through the patches and the primary thing I don't
> >understand is why are the wrappers used only in sysdb? I think we should
> >just use them everywhere..
> I do not like wrappers.
> We should not log ldif by default.

That's why there is a separate log level, you need to turn these on
separately (yes, logging LDIFs by default would be too much..)

> I thought they would be used just for development purposes.
> therefore they should not be used anywhere and not everywhere.
> 
> LS
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/05/2016 03:32 PM, Lukas Slebodnik wrote:

On (05/09/16 15:24), Jakub Hrozek wrote:

On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).


I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.


I quickly scrolled through the patches and the primary thing I don't
understand is why are the wrappers used only in sysdb? I think we should
just use them everywhere..

I do not like wrappers.
We should not log ldif by default.
I thought they would be used just for development purposes.
therefore they should not be used anywhere and not everywhere.

LS


Hello Lukas,

please, are you satisfied with those wrappers at really high debug level?

Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (05/09/16 15:24), Jakub Hrozek wrote:
>On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
>> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  
>> wrote:
>> > Petr,
>> >
>> > I went through your patches and in general they look good to me.
>> > However, I haven't done any tests yet with your patches (and I'll do
>> > it after lunch).
>> 
>> I've done some tests and I've been able to see the ldif changes in the
>> domain log. So, I assume it's working.
>> For sure it's a good improvement! Would be worth to link some
>> documentation about ldiff as it may be confusing for someone who is
>> not used to it.
>> 
>> I'll wait for a new version of the patches and go through them again.
>> 
>> I really would like to have someone's else opinion on this series.
>
>I quickly scrolled through the patches and the primary thing I don't
>understand is why are the wrappers used only in sysdb? I think we should
>just use them everywhere..
I do not like wrappers.
We should not log ldif by default.
I thought they would be used just for development purposes.
therefore they should not be used anywhere and not everywhere.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/05/2016 03:24 PM, Jakub Hrozek wrote:

On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).


I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.


I quickly scrolled through the patches and the primary thing I don't
understand is why are the wrappers used only in sysdb? I think we should
just use them everywhere..


Hi Jakub,

it was one of my question earlier in this thread. I did it only for the 
[ts]_cache, but I can quickly expand this solution into whole code.


Thanks for opinion.

Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Jakub Hrozek]

On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote:
> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  wrote:
> > Petr,
> >
> > I went through your patches and in general they look good to me.
> > However, I haven't done any tests yet with your patches (and I'll do
> > it after lunch).
> 
> I've done some tests and I've been able to see the ldif changes in the
> domain log. So, I assume it's working.
> For sure it's a good improvement! Would be worth to link some
> documentation about ldiff as it may be confusing for someone who is
> not used to it.
> 
> I'll wait for a new version of the patches and go through them again.
> 
> I really would like to have someone's else opinion on this series.

I quickly scrolled through the patches and the primary thing I don't
understand is why are the wrappers used only in sysdb? I think we should
just use them everywhere..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Fabiano Fidêncio]

On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio  wrote:
> Petr,
>
> I went through your patches and in general they look good to me.
> However, I haven't done any tests yet with your patches (and I'll do
> it after lunch).

I've done some tests and I've been able to see the ldif changes in the
domain log. So, I assume it's working.
For sure it's a good improvement! Would be worth to link some
documentation about ldiff as it may be confusing for someone who is
not used to it.

I'll wait for a new version of the patches and go through them again.

I really would like to have someone's else opinion on this series.

>
> Please, below you can see a few comments. Feel completely free to
> ignore the first one if you feel like doing it, it's just a minor :-)
> For the other comments, I'd like to understand a few changes you have done.
>
>
> Patch 0001: SYSDB: Adding message to inform which cache is used
>
> About the following part of the patch:
> +static const char *get_attr_storage(int state_mask)
> +{
> +const char *storage = "";
> +
> +if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
> +storage = "cache, ts_cache";
> +} else if (state_mask == SSS_SYSDB_TS_CACHE) {
> +storage = "ts_cache";
> +} else if (state_mask == SSS_SYSDB_CACHE) {
> +storage = "cache";
> +}
> +
> +return storage;
> +}
>
> I personally don't like this kind of comparison done with flags. I'd
> go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
> ...
> But this is a really minor and feel free to ignore it.
>
>
> Patch 0002: SYSDB: Adding message about reason why cache changed
>
> LGTM
>
>
> Patch 0003: SYSDB: Adding wrappers for ldb_* operations
>
> About the following parts of the patch:
>
> On src/db/sysdb_ldb_wrapper.c
>
> +#define ERR_FN_ENOMEM (-1 * ENOMEM)
> +#define ERR_FN_ENOENT (-1 * ENOENT)
>
> Why? I failed to understand why you're doing this here.
>
> +if (print_ctx == NULL) {
> +return -1;
> +return ERR_FN_ENOMEM;
> +}
>
> I guess the return -1 is a leftover :-)
>
> +if (print_ctx->ldif == NULL) {
> +return -2;
> +return ERR_FN_ENOENT;
> +}
>
> I guess the return -2 is also a leftover :-)
>
> +if (ret < 0) {
> +DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with 
> [%d][%s].\n",
> +-1 * ret, sss_strerror(-1 * ret));
> +goto done;
> +}
>
> And here again this dance multiplying by -1 that I don't understand
> the reason :-\
>
> +done:
> +if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
> +talloc_free(ldb_print_ctx->ldif);
> +}
> +talloc_free(ldb_print_ctx);
>
> AFAIU talloc_free can gracefully handle NULL. Considering that's the
> case I'd just check for (if ldb_print_ctx != NULL)
> talloc_free(ldb_print_ctx->ldif);
> Considering it doesn't, we may have some issues on trying to free
> (ldb_print_ctx)
>
> On src/db/sysdb_ldb_wrapper.h:
>
> +int sss_ldb_rename(struct ldb_context *ldb,
> +   struct ldb_dn * olddn,
> +   struct ldb_dn *newdn);
>
> Just a really minor codying style change here, remove the extra space
> between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,
>
>
> Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
> Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
> Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
> Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb
>
> LGTM
>
>
> Best Regards,
> --
> Fabiano Fidêncio
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 09/05/2016 11:59 AM, Fabiano Fidêncio wrote:

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).

Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have done.


Hi Fabiano,

thanks for the code review. Please, see my inline comments below:


Patch 0001: SYSDB: Adding message to inform which cache is used

About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}

I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.


I agree, it is better to us it this way. Addressed.


Patch 0002: SYSDB: Adding message about reason why cache changed

LGTM


Patch 0003: SYSDB: Adding wrappers for ldb_* operations

About the following parts of the patch:

On src/db/sysdb_ldb_wrapper.c

+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)

Why? I failed to understand why you're doing this here.


I removed this definitions, it was useless.

But the reason is: The second argument of function ldb_ldif_write() is 
pointer to function ldif_vprintf_fn. The condition on this is that

errors < 0, because ret >= is length of written debug message.

I wrote comment on it to the code. I am sorry, it wasn't obvious.



+if (print_ctx == NULL) {
+return -1;
+return ERR_FN_ENOMEM;
+}

I guess the return -1 is a leftover :-)


Right, it was leftover.


+if (print_ctx->ldif == NULL) {
+return -2;
+return ERR_FN_ENOENT;
+}

I guess the return -2 is also a leftover :-)


The same.


+if (ret < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with [%d][%s].\n",
+-1 * ret, sss_strerror(-1 * ret));
+goto done;
+}

And here again this dance multiplying by -1 that I don't understand
the reason :-\





+done:
+if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+talloc_free(ldb_print_ctx->ldif);
+}
+talloc_free(ldb_print_ctx);

AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)


Addressed.


On src/db/sysdb_ldb_wrapper.h:

+int sss_ldb_rename(struct ldb_context *ldb,
+   struct ldb_dn * olddn,
+   struct ldb_dn *newdn);

Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,


Thanks :-), addressed.


Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb

LGTM


Best Regards,
--
Fabiano Fidêncio


There was the question about testing... every time SSSD writes to the 
cache or ts_cache and debug_level in domain section is appropriate high, 
ldif debug message appears.


debug_level = 0x0 in the domain section

sudo su -c "truncate -s0 /var/log/sssd/*.log"
systemctl restart sssd
sss_cache -E
getent passwd remote_user

You can try modify the user or delete him after. It will change the ldif 
message.


PS: New patch set is attached.


Regards

--
Petr^4 Čech
>From 0b6ec52d3d43b8f0706272b5642d86da8b2381c9 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/7] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 5d9c9fb24a149f8215b3027dcb4b0e1a183e4b43..847b663bdb2ec31de3eb3b4c33e2b942145a4c42 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,12 @@
 #include "util/cert.h"
 #include 
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)
+
 static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
 {
 const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1182,21 @@ done:
 return ret;
 }
 
+static const char 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Fabiano Fidêncio]

Petr,

I went through your patches and in general they look good to me.
However, I haven't done any tests yet with your patches (and I'll do
it after lunch).

Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have done.


Patch 0001: SYSDB: Adding message to inform which cache is used

About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}

I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.


Patch 0002: SYSDB: Adding message about reason why cache changed

LGTM


Patch 0003: SYSDB: Adding wrappers for ldb_* operations

About the following parts of the patch:

On src/db/sysdb_ldb_wrapper.c

+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)

Why? I failed to understand why you're doing this here.

+if (print_ctx == NULL) {
+return -1;
+return ERR_FN_ENOMEM;
+}

I guess the return -1 is a leftover :-)

+if (print_ctx->ldif == NULL) {
+return -2;
+return ERR_FN_ENOENT;
+}

I guess the return -2 is also a leftover :-)

+if (ret < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with [%d][%s].\n",
+-1 * ret, sss_strerror(-1 * ret));
+goto done;
+}

And here again this dance multiplying by -1 that I don't understand
the reason :-\

+done:
+if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+talloc_free(ldb_print_ctx->ldif);
+}
+talloc_free(ldb_print_ctx);

AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)

On src/db/sysdb_ldb_wrapper.h:

+int sss_ldb_rename(struct ldb_context *ldb,
+   struct ldb_dn * olddn,
+   struct ldb_dn *newdn);

Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,


Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb

LGTM


Best Regards,
--
Fabiano Fidêncio
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

Bump.

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

Bump.

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 08/23/2016 10:58 AM, Petr Cech wrote:

Hello,

there is new patch number 3 which is a WIP.
I would like to ask you which version
of ldif printing do you prefer.

You can see [1] which shows the outputs.

Difference is that version A is step by step
reading of ldb messsage.

But, version B uses function
ldb_ldif_write(). This function needs
my_vprintf_fn() but the information isn't structured.

What do you prefer? :-)

[1] ldif.txt



There is new patch set attached.

Well, I choose version B because it is uses native ldb function
ldb_ldif_write().

I added new debug level. It is not only extra debug level which SSSD
has. But I didn't see any other extra in man pages. So I didn't add
this one too.


Bump.

Notes:

1) If you prefer merging of last 4 patches -- renaming of ldb_*(), 
please, tell me.


2) Those patches rename only ldb_*() in sysdb. But those functions are 
in other parts of code too. So if you prefer renaming in whole SSSD, 
please tell me.


Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 08/16/2016 02:32 PM, Petr Cech wrote:

On 08/16/2016 01:22 PM, Petr Cech wrote:



On 08/16/2016 01:02 PM, Lukas Slebodnik wrote:

On (16/08/16 12:52), Petr Cech wrote:

On 08/16/2016 10:15 AM, Jakub Hrozek wrote:

On Tue, Aug 16, 2016 at 09:50:19AM +0200, Petr Cech wrote:

Hello list,

I am solving ticket [1] now. There are three
points mentioned. A have prepared patches for
the first two. I would like to ask anybody it
is right or if I miss something.

The third point is about full LDIFF in special
debug level. What does it mean 'special debug
level'? Is it new option, for example?


[1] https://fedorahosted.org/sssd/ticket/3060

Regards




Please no magic constants in SSSD code :)


Hello Jakub,

there is fixed version without magic :-)

--
Petr^4 'magician' Čech



From 2ca78a82c579c5244aebd9a58b56a9886f6bc4b5 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/2] SYSDB: Adding message to inform which cache is
used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
src/db/sysdb_ops.c | 32 
1 file changed, 32 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index
44fb5b70e6d33fffbca5824f831a3229254ecb57..a81840b2515d09f91d1dfa783bcf08f0fad112b4

100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,12 @@
#include "util/cert.h"
#include 

+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)
+
static uint32_t get_attr_as_uint32(struct ldb_message *msg, const
char *attr)
{
const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1182,21 @@ done:
return ret;
}

+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 struct ldb_dn *entry_dn,
 struct sysdb_attrs *attrs,
@@ -1184,6 +1205,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
bool sysdb_write = true;
errno_t ret = EOK;
errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;

sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs,
mod_op);
if (sysdb_write == true) {
@@ -1192,6 +1214,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
DEBUG(SSSDBG_MINOR_FAILURE,
  "Cannot set attrs for %s, %d [%s]\n",
  ldb_dn_get_linearized(entry_dn), ret,
sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
}
}

@@ -1201,9 +1225,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx
*sysdb,
DEBUG(SSSDBG_MINOR_FAILURE,
"Cannot set ts attrs for %s\n",
ldb_dn_get_linearized(entry_dn));
/* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
}
}

+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
return ret;
}

--
2.7.4




From 6e7143b26fb5696a9b684c0da96353a7d5d07700 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/2] SYSDB: Adding message about reason why cache
changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
src/db/sysdb.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index
6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..9d1abc2b3dd0ce5db626544673795eebfbc28bcd

100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct
ldb_message *old_entry,
return true;
}

-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
 struct ldb_message *mod_msg)
{
struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,10 @@ static bool sysdb_ldb_msg_difference(struct
ldb_message *db_msg,
 */
if (mod_msg_el->num_values > 0) {
/* We can ignore additions of timestamp
attributes */
+DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,
reason: " \
+ "attr [%s] is new.\n",
+
ldb_dn_get_linearized(entry_dn),
+ mod_msg_el->name);
return true;
}

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 08/16/2016 01:22 PM, Petr Cech wrote:



On 08/16/2016 01:02 PM, Lukas Slebodnik wrote:

On (16/08/16 12:52), Petr Cech wrote:

On 08/16/2016 10:15 AM, Jakub Hrozek wrote:

On Tue, Aug 16, 2016 at 09:50:19AM +0200, Petr Cech wrote:

Hello list,

I am solving ticket [1] now. There are three
points mentioned. A have prepared patches for
the first two. I would like to ask anybody it
is right or if I miss something.

The third point is about full LDIFF in special
debug level. What does it mean 'special debug
level'? Is it new option, for example?


[1] https://fedorahosted.org/sssd/ticket/3060

Regards




Please no magic constants in SSSD code :)


Hello Jakub,

there is fixed version without magic :-)

--
Petr^4 'magician' Čech



From 2ca78a82c579c5244aebd9a58b56a9886f6bc4b5 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/2] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
src/db/sysdb_ops.c | 32 
1 file changed, 32 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index
44fb5b70e6d33fffbca5824f831a3229254ecb57..a81840b2515d09f91d1dfa783bcf08f0fad112b4
100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,12 @@
#include "util/cert.h"
#include 

+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)
+
static uint32_t get_attr_as_uint32(struct ldb_message *msg, const
char *attr)
{
const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1182,21 @@ done:
return ret;
}

+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 struct ldb_dn *entry_dn,
 struct sysdb_attrs *attrs,
@@ -1184,6 +1205,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
bool sysdb_write = true;
errno_t ret = EOK;
errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;

sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs,
mod_op);
if (sysdb_write == true) {
@@ -1192,6 +1214,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
DEBUG(SSSDBG_MINOR_FAILURE,
  "Cannot set attrs for %s, %d [%s]\n",
  ldb_dn_get_linearized(entry_dn), ret,
sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
}
}

@@ -1201,9 +1225,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
DEBUG(SSSDBG_MINOR_FAILURE,
"Cannot set ts attrs for %s\n",
ldb_dn_get_linearized(entry_dn));
/* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
}
}

+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
return ret;
}

--
2.7.4




From 6e7143b26fb5696a9b684c0da96353a7d5d07700 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/2] SYSDB: Adding message about reason why cache
changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
src/db/sysdb.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index
6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..9d1abc2b3dd0ce5db626544673795eebfbc28bcd
100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct
ldb_message *old_entry,
return true;
}

-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
 struct ldb_message *mod_msg)
{
struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,10 @@ static bool sysdb_ldb_msg_difference(struct
ldb_message *db_msg,
 */
if (mod_msg_el->num_values > 0) {
/* We can ignore additions of timestamp
attributes */
+DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,
reason: " \
+ "attr [%s] is new.\n",
+
ldb_dn_get_linearized(entry_dn),
+ mod_msg_el->name);
return true;
}
break;
@@ -1861,6 +1866,11 @@ 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (16/08/16 12:52), Petr Cech wrote:
>On 08/16/2016 10:15 AM, Jakub Hrozek wrote:
>> On Tue, Aug 16, 2016 at 09:50:19AM +0200, Petr Cech wrote:
>> > Hello list,
>> > 
>> > I am solving ticket [1] now. There are three
>> > points mentioned. A have prepared patches for
>> > the first two. I would like to ask anybody it
>> > is right or if I miss something.
>> > 
>> > The third point is about full LDIFF in special
>> > debug level. What does it mean 'special debug
>> > level'? Is it new option, for example?
>> > 
>> > 
>> > [1] https://fedorahosted.org/sssd/ticket/3060
>> > 
>> > Regards
>
>> 
>> Please no magic constants in SSSD code :)
>
>Hello Jakub,
>
>there is fixed version without magic :-)
>
>-- 
>Petr^4 'magician' Čech

>From 2ca78a82c579c5244aebd9a58b56a9886f6bc4b5 Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 16 Aug 2016 09:32:18 +0200
>Subject: [PATCH 1/2] SYSDB: Adding message to inform which cache is used
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/3060
>---
> src/db/sysdb_ops.c | 32 
> 1 file changed, 32 insertions(+)
>
>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
>index 
>44fb5b70e6d33fffbca5824f831a3229254ecb57..a81840b2515d09f91d1dfa783bcf08f0fad112b4
> 100644
>--- a/src/db/sysdb_ops.c
>+++ b/src/db/sysdb_ops.c
>@@ -27,6 +27,12 @@
> #include "util/cert.h"
> #include 
> 
>+
>+#define SSS_SYSDB_NO_CACHE 0x0
>+#define SSS_SYSDB_CACHE 0x1
>+#define SSS_SYSDB_TS_CACHE 0x2
>+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)
>+
> static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
> {
> const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
>@@ -1176,6 +1182,21 @@ done:
> return ret;
> }
> 
>+static const char *get_attr_storage(int state_mask)
>+{
>+const char *storage = "";
>+
>+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
>+storage = "cache, ts_cache";
>+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
>+storage = "ts_cache";
>+} else if (state_mask == SSS_SYSDB_CACHE) {
>+storage = "cache";
>+}
>+
>+return storage;
>+}
>+
> int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
>  struct ldb_dn *entry_dn,
>  struct sysdb_attrs *attrs,
>@@ -1184,6 +1205,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
> bool sysdb_write = true;
> errno_t ret = EOK;
> errno_t tret = EOK;
>+int state_mask = SSS_SYSDB_NO_CACHE;
> 
> sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
> if (sysdb_write == true) {
>@@ -1192,6 +1214,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
> DEBUG(SSSDBG_MINOR_FAILURE,
>   "Cannot set attrs for %s, %d [%s]\n",
>   ldb_dn_get_linearized(entry_dn), ret, sss_strerror(ret));
>+} else {
>+state_mask |= SSS_SYSDB_CACHE;
> }
> }
> 
>@@ -1201,9 +1225,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
> DEBUG(SSSDBG_MINOR_FAILURE,
> "Cannot set ts attrs for %s\n", 
> ldb_dn_get_linearized(entry_dn));
> /* Not fatal */
>+} else {
>+state_mask |= SSS_SYSDB_TS_CACHE;
> }
> }
> 
>+if (state_mask != SSS_SYSDB_NO_CACHE) {
>+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
>+ldb_dn_get_linearized(entry_dn),
>+get_attr_storage(state_mask));
>+}
>+
> return ret;
> }
> 
>-- 
>2.7.4
>

>From 6e7143b26fb5696a9b684c0da96353a7d5d07700 Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 16 Aug 2016 09:33:46 +0200
>Subject: [PATCH 2/2] SYSDB: Adding message about reason why cache changed
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/3060
>---
> src/db/sysdb.c | 24 ++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
>diff --git a/src/db/sysdb.c b/src/db/sysdb.c
>index 
>6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..9d1abc2b3dd0ce5db626544673795eebfbc28bcd
> 100644
>--- a/src/db/sysdb.c
>+++ b/src/db/sysdb.c
>@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message 
>*old_entry,
> return true;
> }
> 
>-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
>+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
>+ struct ldb_message *db_msg,
>  struct ldb_message *mod_msg)
> {
> struct ldb_message_element *mod_msg_el;
>@@ -1848,6 +1849,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message 
>*db_msg,
>  */
> if (mod_msg_el->num_values > 0) {
> /* We can ignore additions of timestamp attributes */
>+DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: " \
>+ "attr [%s] is new.\n",
>+ 

[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache

[Petr Cech]

On 08/16/2016 10:15 AM, Jakub Hrozek wrote:

On Tue, Aug 16, 2016 at 09:50:19AM +0200, Petr Cech wrote:

Hello list,

I am solving ticket [1] now. There are three
points mentioned. A have prepared patches for
the first two. I would like to ask anybody it
is right or if I miss something.

The third point is about full LDIFF in special
debug level. What does it mean 'special debug
level'? Is it new option, for example?


[1] https://fedorahosted.org/sssd/ticket/3060

Regards




Please no magic constants in SSSD code :)


Hello Jakub,

there is fixed version without magic :-)

--
Petr^4 'magician' Čech
>From 2ca78a82c579c5244aebd9a58b56a9886f6bc4b5 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/2] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 44fb5b70e6d33fffbca5824f831a3229254ecb57..a81840b2515d09f91d1dfa783bcf08f0fad112b4 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,12 @@
 #include "util/cert.h"
 #include 
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+#define SSS_SYSDB_BOTH_CACHE (SSS_SYSDB_CACHE | SSS_SYSDB_TS_CACHE)
+
 static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
 {
 const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr);
@@ -1176,6 +1182,21 @@ done:
 return ret;
 }
 
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}
+
 int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
  struct ldb_dn *entry_dn,
  struct sysdb_attrs *attrs,
@@ -1184,6 +1205,7 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 bool sysdb_write = true;
 errno_t ret = EOK;
 errno_t tret = EOK;
+int state_mask = SSS_SYSDB_NO_CACHE;
 
 sysdb_write = sysdb_entry_attrs_diff(sysdb, entry_dn, attrs, mod_op);
 if (sysdb_write == true) {
@@ -1192,6 +1214,8 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
   "Cannot set attrs for %s, %d [%s]\n",
   ldb_dn_get_linearized(entry_dn), ret, sss_strerror(ret));
+} else {
+state_mask |= SSS_SYSDB_CACHE;
 }
 }
 
@@ -1201,9 +1225,17 @@ int sysdb_set_entry_attr(struct sysdb_ctx *sysdb,
 DEBUG(SSSDBG_MINOR_FAILURE,
 "Cannot set ts attrs for %s\n", ldb_dn_get_linearized(entry_dn));
 /* Not fatal */
+} else {
+state_mask |= SSS_SYSDB_TS_CACHE;
 }
 }
 
+if (state_mask != SSS_SYSDB_NO_CACHE) {
+DEBUG(SSSDBG_FUNC_DATA, "Entry [%s] has set [%s] attrs.\n",
+ldb_dn_get_linearized(entry_dn),
+get_attr_storage(state_mask));
+}
+
 return ret;
 }
 
-- 
2.7.4

>From 6e7143b26fb5696a9b684c0da96353a7d5d07700 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/2] SYSDB: Adding message about reason why cache changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb.c | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..9d1abc2b3dd0ce5db626544673795eebfbc28bcd 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -1821,7 +1821,8 @@ bool sysdb_msg_attrs_modts_differs(struct ldb_message *old_entry,
 return true;
 }
 
-static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
+static bool sysdb_ldb_msg_difference(struct ldb_dn *entry_dn,
+ struct ldb_message *db_msg,
  struct ldb_message *mod_msg)
 {
 struct ldb_message_element *mod_msg_el;
@@ -1848,6 +1849,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
  */
 if (mod_msg_el->num_values > 0) {
 /* We can ignore additions of timestamp attributes */
+DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: " \
+ "attr [%s] is new.\n",
+ ldb_dn_get_linearized(entry_dn),
+ mod_msg_el->name);
 return true;
 }
 break;
@@ -1861,6 +1866,11 @@ static bool sysdb_ldb_msg_difference(struct