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(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.
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(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
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(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
Right. I changed order of information. Now the entry_dn is last one.
It could help. I mean important information comes first.
Regards
--
Petr^4 Čech