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 <fiden...@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 <pc...@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 <pc...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=de...@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=de...@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-...@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-...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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=lsleb...@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 [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): shadowMin is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): shadowMax is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): shadowWarning is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): shadowInactive is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): shadowExpire is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): shadowFlag is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): krbLastPwdChange is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): krbPasswordExpiration is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): pwdAttribute is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): authorizedService is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): adAccountExpires is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): adUserAccountControl is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): nsAccountLock is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): authorizedHost is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): ndsLoginDisabled is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): ndsLoginExpirationTime is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): ndsLoginAllowedTimeMap is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): sshPublicKey is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): authType is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): userCertificate is not available for [lsleb...@example.com].
(Wed Sep 14 15:47:10 2016) [sssd[be[example.com]]] [sdap_attrs_add_ldap_attr]
(0x2000): Adding mail [lsleb...@example.com] to attributes of
[lsleb...@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 [lsleb...@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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
