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(a)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(a)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
--
Petr^4 Čech