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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr] (0x2000):
Adding mail [lslebodn(a)example.com] to attributes of [lslebodn(a)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(a)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