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