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
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
-- Petr^4 Čech
From 6ac105b6a3ad9e424c5053f05aa1eefd55cafb71 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com 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 | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 44fb5b70e6d33fffbca5824f831a3229254ecb57..e199bd946d5d58be2acd3dfda7050c112688d20d 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -1176,6 +1176,31 @@ done: return ret; }
+static const char *get_attr_storage(int state) +{
- const char *storage;
- switch (state) {
- case 0:
storage = "";break;- case 1:
storage = "cache";break;- case 2:
storage = "ts_cache";break;- case 3:
storage = "cache, ts_cache";break;- default:
storage = "";break;- }
- return storage;
+}
Please no magic constants in SSSD code :)
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 :-)
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 pcech@redhat.com 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 <time.h>
+#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 pcech@redhat.com 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 ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: " \
^^ this is not required It will be concatenated even without it.
"attr [%s] is replaced " \"or extended.\n",ldb_dn_get_linearized(entry_dn),mod_msg_el->name);
return true; } }@@ -1869,6 +1879,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: " \"attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn),mod_msg_el->name);
a) it's not alligned properly (1 extra space :-) b) IMH0; it's it's easier to read the debug messages in nested code if it start on next line. e.g. DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: attr [%s] is deleted.\n", ldb_dn_get_linearized(entry_dn), mod_msg_el->name);
LS
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 pcech@redhat.com 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 <time.h>
+#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 pcech@redhat.com 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 ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: " \^^ this is not required It will be concatenated even without it.
"attr [%s] is replaced " \"or extended.\n",ldb_dn_get_linearized(entry_dn),mod_msg_el->name);return true; } }@@ -1869,6 +1879,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: " \"attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn),mod_msg_el->name);a) it's not alligned properly (1 extra space :-) b) IMH0; it's it's easier to read the debug messages in nested code if it start on next line. e.g. DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: attr [%s] is deleted.\n", ldb_dn_get_linearized(entry_dn), mod_msg_el->name);
LS
Thanks, Lukas.
Fixed patch set is attached. Some debug messages crossed 80 characters :-(
Regards
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 pcech@redhat.com 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 <time.h>
+#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 pcech@redhat.com 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 ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,reason: " \
^^ this is not required It will be
concatenated even without it.
"attr [%s] is replaced " \"or extended.\n",ldb_dn_get_linearized(entry_dn),
mod_msg_el->name);return true; } }@@ -1869,6 +1879,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,reason: " \
"attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn),
mod_msg_el->name);a) it's not alligned properly (1 extra space :-) b) IMH0; it's it's easier to read the debug messages in nested code if it start on next line. e.g. DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: attr [%s] is deleted.\n", ldb_dn_get_linearized(entry_dn), mod_msg_el->name);
LS
Thanks, Lukas.
Fixed patch set is attached. Some debug messages crossed 80 characters :-(
Regards
There were little misspelling, sorry. Fixed patches are attached.
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 pcech@redhat.com 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 <time.h>
+#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 pcech@redhat.com 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 ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,reason: " \
^^ this is not required It will be
concatenated even without it.
"attr [%s] is replaced" \
"or extended.\n",ldb_dn_get_linearized(entry_dn),
mod_msg_el->name);return true; } }@@ -1869,6 +1879,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,reason: " \
"attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn),
mod_msg_el->name);a) it's not alligned properly (1 extra space :-) b) IMH0; it's it's easier to read the debug messages in nested code if it start on next line. e.g. DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: attr [%s] is deleted.\n", ldb_dn_get_linearized(entry_dn), mod_msg_el->name);
LS
Thanks, Lukas.
Fixed patch set is attached. Some debug messages crossed 80 characters :-(
Regards
There were little misspelling, sorry. Fixed patches are attached.
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
Regards
On 08/18/2016 01:01 PM, Petr Cech wrote:
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 pcech@redhat.com 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 <time.h>
+#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 pcech@redhat.com 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 ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,reason: " \
^^ this is not required It will be
concatenated even without it.
"attr [%s] is replaced" \
"or extended.\n",ldb_dn_get_linearized(entry_dn),
mod_msg_el->name);return true; } }@@ -1869,6 +1879,10 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs,reason: " \
"attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn),
mod_msg_el->name);a) it's not alligned properly (1 extra space :-) b) IMH0; it's it's easier to read the debug messages in nested code if it start on next line. e.g. DEBUG(SSSDBG_TRACE_FUNC, "Entry [%s] differs, reason: attr [%s] is deleted.\n", ldb_dn_get_linearized(entry_dn), mod_msg_el->name);
LS
Thanks, Lukas.
Fixed patch set is attached. Some debug messages crossed 80 characters :-(
Regards
There were little misspelling, sorry. Fixed patches are attached.
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.
Regards
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
Bump.
Bump.
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
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 = 0xFFFF0 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
On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio fidencio@redhat.com 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
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 fidencio@redhat.com 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..
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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.
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 fidencio@redhat.com 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
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 fidencio@redhat.com 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 0xffff0. 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
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 fidencio@redhat.com 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 0xffff0. 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 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.
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.
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
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
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
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
On 09/05/2016 02:31 PM, Fabiano Fidêncio wrote:
On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio fidencio@redhat.com 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
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 fidencio@redhat.com 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.
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 fidencio@redhat.com 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
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 fidencio@example.com 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 pcech@example.com 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 <time.h>
+#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.
storage = "cache, ts_cache";- } else if ((state_mask != SSS_SYSDB_TS_CACHE) != 0) {
storage = "ts_cache";- } else if ((state_mask &= SSS_SYSDB_CACHE) != 0) {
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 abad5d0a7c730f8eb0699509ed21e559e6896f12 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@example.com Date: Tue, 16 Aug 2016 09:33:46 +0200 Subject: [PATCH 2/7] SYSDB: Adding message about reason why cache changed
Resolves: https://fedorahosted.org/sssd/ticket/3060
src/db/sysdb.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..a76e8b47afc902d6c0c0ed5302b7f9231a11ade3 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_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 +1865,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: attr [%s] is replaced or extended.\n",
^^ more then 80 columns
ldb_dn_get_linearized(entry_dn), mod_msg_el->name); return true; } }@@ -1869,6 +1876,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn), mod_msg_el->name); return true; } break;@@ -1892,10 +1902,16 @@ bool sysdb_entry_attrs_diff(struct sysdb_ctx *sysdb, const char *attrnames[attrs->num+1];
if (sysdb->ldb_ts == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: there is no ts_cache yet.\n",ldb_dn_get_linearized(entry_dn)); return true;}
if (is_ts_ldb_dn(entry_dn) == false) {
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: ts_cache doesn't trace this type of entry.\n",ldb_dn_get_linearized(entry_dn)); return true;}
@@ -1930,7 +1946,7 @@ bool sysdb_entry_attrs_diff(struct sysdb_ctx *sysdb, goto done; }
- differs = sysdb_ldb_msg_difference(res->msgs[0], new_entry_msg);
- differs = sysdb_ldb_msg_difference(entry_dn, res->msgs[0], new_entry_msg);
done: talloc_free(tmp_ctx); return differs; -- 2.7.4
We should also increase the debug_level for messages.
@see [root@host ~]# grep "Entry [" /var/log/sssd/sssd_example.com.log (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [originalDN] is new. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [lastUpdate] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=devel@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [lastUpdate] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=devel@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=idm-dev-lab@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [lastUpdate] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=idm-dev-lab@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [initgrExpireTimestamp] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [ccacheFile] is new. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [cachedPassword] is new. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [originalModifyTimestamp] is new. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [cachedPassword] is replaced or extended. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs.
Other simillar debug messages use much higher debug level e.g.
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowLastChange is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowMin is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowMax is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowWarning is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowInactive is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowExpire is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowFlag is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): krbLastPwdChange is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): krbPasswordExpiration is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): pwdAttribute is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): authorizedService is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): adAccountExpires is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): adUserAccountControl is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): nsAccountLock is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): authorizedHost is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): ndsLoginDisabled is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): ndsLoginExpirationTime is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): ndsLoginAllowedTimeMap is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): sshPublicKey is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): authType is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): userCertificate is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): Adding mail [lslebodn@example.com] to attributes of [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sysdb_attrs_get_aliases] (0x2000): Domain is case-insensitive; will add lowercased aliases (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [is_email_from_domain] (0x4000): Email [lslebodn@example.com] is from domain [example.com].
BTW new debug messages are 200 collumns long (in average). and most of users use longer domains then example.com. Is it intentional? Could it be shorter a little bit or it would lower usability of debug messages.
LS
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 fidencio@example.com 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 pcech@example.com 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 <time.h>
+#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 did it more dynamic way now. See attached patch please.
storage = "cache, ts_cache";- } else if ((state_mask != SSS_SYSDB_TS_CACHE) != 0) {
storage = "ts_cache";- } else if ((state_mask &= SSS_SYSDB_CACHE) != 0) {
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 abad5d0a7c730f8eb0699509ed21e559e6896f12 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@example.com Date: Tue, 16 Aug 2016 09:33:46 +0200 Subject: [PATCH 2/7] SYSDB: Adding message about reason why cache changed
Resolves: https://fedorahosted.org/sssd/ticket/3060
src/db/sysdb.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..a76e8b47afc902d6c0c0ed5302b7f9231a11ade3 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_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 +1865,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, */ if (is_ts_cache_attr(mod_msg_el->name) == false) { /* We can ignore changes to timestamp attributes */
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: attr [%s] is replaced or extended.\n",^^ more then 80 columns
Addressed.
ldb_dn_get_linearized(entry_dn), mod_msg_el->name); return true; } }@@ -1869,6 +1876,9 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg, db_msg_el = ldb_msg_find_element(db_msg, mod_msg_el->name); if (db_msg_el != NULL) { /* We are deleting a valid element, there is a difference */
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: attr [%s] is deleted.\n",ldb_dn_get_linearized(entry_dn), mod_msg_el->name); return true; } break;@@ -1892,10 +1902,16 @@ bool sysdb_entry_attrs_diff(struct sysdb_ctx *sysdb, const char *attrnames[attrs->num+1];
if (sysdb->ldb_ts == NULL) {
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: there is no ts_cache yet.\n",ldb_dn_get_linearized(entry_dn)); return true;}
if (is_ts_ldb_dn(entry_dn) == false) {
DEBUG(SSSDBG_TRACE_FUNC,"Entry [%s] differs, reason: ts_cache doesn't trace this type of entry.\n",ldb_dn_get_linearized(entry_dn)); return true;}
@@ -1930,7 +1946,7 @@ bool sysdb_entry_attrs_diff(struct sysdb_ctx *sysdb, goto done; }
- differs = sysdb_ldb_msg_difference(res->msgs[0], new_entry_msg);
- differs = sysdb_ldb_msg_difference(entry_dn, res->msgs[0], new_entry_msg);
done: talloc_free(tmp_ctx); return differs; -- 2.7.4
We should also increase the debug_level for messages.
Addressed.
@see [root@host ~]# grep "Entry [" /var/log/sssd/sssd_example.com.log (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [originalDN] is new. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [lastUpdate] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=devel@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [lastUpdate] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=devel@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=idm-dev-lab@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [lastUpdate] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=idm-dev-lab@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [initgrExpireTimestamp] is new. (Wed Sep 14 15:47:11 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [ccacheFile] is new. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [cachedPassword] is new. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] differs, reason: attr [originalModifyTimestamp] is new. (Wed Sep 14 15:47:15 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=groups,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_ldb_msg_difference] (0x0400): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] differs, reason: attr [cachedPassword] is replaced or extended. (Wed Sep 14 15:47:23 2016) [sssd[be[example.com]]] [sysdb_set_entry_attr] (0x0200): Entry [name=lslebodn@example.com,cn=users,cn=example.com,cn=sysdb] has set [cache, ts_cache] attrs.
Other simillar debug messages use much higher debug level e.g.
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowLastChange is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowMin is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowMax is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowWarning is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowInactive is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowExpire is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): shadowFlag is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): krbLastPwdChange is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): krbPasswordExpiration is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): pwdAttribute is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): authorizedService is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): adAccountExpires is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): adUserAccountControl is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): nsAccountLock is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): authorizedHost is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): ndsLoginDisabled is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): ndsLoginExpirationTime is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): ndsLoginAllowedTimeMap is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): sshPublicKey is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): authType is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): userCertificate is not available for [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000): Adding mail [lslebodn@example.com] to attributes of [lslebodn@example.com]. (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sysdb_attrs_get_aliases] (0x2000): Domain is case-insensitive; will add lowercased aliases (Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [is_email_from_domain] (0x4000): Email [lslebodn@example.com] is from domain [example.com].
BTW new debug messages are 200 collumns long (in average). and most of users use longer domains then example.com. Is it intentional? Could it be shorter a little bit or it would lower usability of debug messages.
LS
Right. I changed order of information. Now the entry_dn is last one. It could help. I mean important information comes first.
Regards
bump
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 fidencio@example.com 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 pcech@example.com 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 <time.h>
+#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 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 = ""; + + 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
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
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
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
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
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
Bump.
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
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 pcech@redhat.com 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 pcech@redhat.com 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
On (08/11/16 16:49), Lukas Slebodnik wrote:
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 pcech@redhat.com 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 pcech@redhat.com 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
master: * 0be56bb4a2e3cb833396878dee05094c6b83031e * cb056fe82410a31ad3426b1cc58c81710ff9ac83
LS
Bump.
sssd-devel@lists.fedorahosted.org
-
Fabiano Fidêncio -
Jakub Hrozek -
Lukas Slebodnik -
Petr Cech