On 09/22/2016 10:31 AM, Lukas Slebodnik wrote:
On (22/09/16 10:00), Lukas Slebodnik wrote:
On (16/09/16 16:19), Petr Cech wrote:
On 09/14/2016 04:00 PM, Lukas Slebodnik wrote:
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.

The more dynamic way does not work performance decradation
caused by many useless memory allocations.

Your patch calls get_attr_storage every time
even though the result would not be used
due to low debug_level

I prefer one of your previous versions
e.g.
+static const char *get_attr_storage(int state_mask)
+{
+    const char *storage = "";
or maybe default can be "unknown"

+
+    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;
+}
+

Overhead is minimal and the wrong result will not be printed
in case of addition new tye of cache.


Hi Lukas,

new version is attached.

Regards

--
Petr^4 Čech
>From d480cfce5c4e3de6d33ff0d860d8f1edda2385b1 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Tue, 16 Aug 2016 09:32:18 +0200
Subject: [PATCH 1/3] SYSDB: Adding message to inform which cache is used

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb_ops.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 29f4b1d1597bd98541a152dd6462caa864fbf2fd..8b194e3db48870aecd54b21bd3d0b77dc342f9e5 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -27,6 +27,11 @@
 #include "util/cert.h"
 #include <time.h>
 
+
+#define SSS_SYSDB_NO_CACHE 0x0
+#define SSS_SYSDB_CACHE 0x1
+#define SSS_SYSDB_TS_CACHE 0x2
+
 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 +1181,21 @@ done:
     return ret;
 }
 
+static const char *get_attr_storage(int state_mask)
+{
+    const char *storage = "unknown";
+
+    if (state_mask == (SSS_SYSDB_CACHE | SSS_SYSDB_TS_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 +1204,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 +1213,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 +1224,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 eab9f5259af8fc2ce9d88313d3ce95fe2a3ee08f Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Tue, 16 Aug 2016 09:33:46 +0200
Subject: [PATCH 2/3] SYSDB: Adding message about reason why cache changed

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 src/db/sysdb.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 6f0b1b9e9b52bede68f03cb5674f65b91cc28c98..b67769ed11fc0796d1987f09aa568c2db4a0ffab 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_INTERNAL,
+                          "Added attr [%s] to entry [%s]\n",
+                          mod_msg_el->name, ldb_dn_get_linearized(entry_dn));
                     return true;
                 }
                 break;
@@ -1855,12 +1859,15 @@ static bool sysdb_ldb_msg_difference(struct ldb_message *db_msg,
 
             el_differs = ldb_msg_element_compare(db_msg_el, mod_msg_el);
             if (el_differs) {
-                /* We are replacing or extending element, there is a difference. If
-                 * some values already exist and ldb_add is not permissive,
+                /* We are replacing or extending element, there is a difference.
+                 * If some values already exist and ldb_add is not permissive,
                  * ldb will throw an error, but that's not our job to check..
                  */
                 if (is_ts_cache_attr(mod_msg_el->name) == false) {
                     /* We can ignore changes to timestamp attributes */
+                    DEBUG(SSSDBG_TRACE_INTERNAL,
+                          "Replaced/extended attr [%s] of entry [%s]\n",
+                          mod_msg_el->name, ldb_dn_get_linearized(entry_dn) );
                     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_INTERNAL,
+                      "Deleted attr [%s] of entry [%s].\n",
+                      mod_msg_el->name, ldb_dn_get_linearized(entry_dn));
                 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

>From 34f2151c2393ec28075ab8125eb5f2baa8ad282e Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Tue, 16 Aug 2016 14:59:06 +0200
Subject: [PATCH 3/3] SYSDB: Adding wrappers for ldb_* operations

This patch adds 4 wrappers:
* sss_ldb_add()
* sss_ldb_delete()
* sss_ldb_modify()
* sss_ldb_rename()

Those wrappers write ldif message to log if
debug_level = SSSDBG_LDB.

Adding and modifying produce full ldif.
Deleting and renaming produce only short message.

If SSSDBG_LDB is not set, wrappers collapse to normal
ldb_* functions without additonial memory consumption.

Resolves:
https://fedorahosted.org/sssd/ticket/3060
---
 Makefile.am                |   2 +
 src/db/sysdb_ldb_wrapper.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
 src/db/sysdb_ldb_wrapper.h |  35 ++++++++++++
 src/util/debug.h           |   1 +
 4 files changed, 176 insertions(+)
 create mode 100644 src/db/sysdb_ldb_wrapper.c
 create mode 100644 src/db/sysdb_ldb_wrapper.h

diff --git a/Makefile.am b/Makefile.am
index 17c5f26ce9db1e183b30178f1a8714deca1dab03..bbea4383a0fb54599bc9308b0b90ec20f54705b8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -646,6 +646,7 @@ dist_noinst_HEADERS = \
     src/db/sysdb_private.h \
     src/db/sysdb_services.h \
     src/db/sysdb_ssh.h \
+    src/db/sysdb_ldb_wrapper.h \
     src/confdb/confdb.h \
     src/confdb/confdb_private.h \
     src/confdb/confdb_setup.h \
@@ -894,6 +895,7 @@ pkglib_LTLIBRARIES += libsss_util.la
 libsss_util_la_SOURCES = \
     src/confdb/confdb.c \
     src/db/sysdb.c \
+    src/db/sysdb_ldb_wrapper.c \
     src/db/sysdb_ops.c \
     src/db/sysdb_search.c \
     src/db/sysdb_selinux.c \
diff --git a/src/db/sysdb_ldb_wrapper.c b/src/db/sysdb_ldb_wrapper.c
new file mode 100644
index 0000000000000000000000000000000000000000..9aa32648987a241088d2701298e7404a76db2d52
--- /dev/null
+++ b/src/db/sysdb_ldb_wrapper.c
@@ -0,0 +1,138 @@
+/*
+   SSSD
+
+   System Database -- ldb wrappers
+
+   Copyright (C) Petr Cech <pc...@redhat.com>	2016
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <string.h>
+#include <ldb.h>
+#include "util/util.h"
+
+struct sss_ldif_vprint_ctx {
+    char *ldif;
+};
+
+static int ldif_vprintf_fn(void *private_data, const char *fmt, ...)
+{
+    struct sss_ldif_vprint_ctx *print_ctx;
+    va_list ap;
+    int lenght = 0;
+
+    /* Note that the function should return the number of
+     * bytes written, or a negative error code.
+     */
+
+    print_ctx = talloc_get_type(private_data, struct sss_ldif_vprint_ctx);
+
+    if (print_ctx == NULL) {
+        return (-1 * ENOMEM);
+    }
+
+    if (fmt != NULL) {
+        va_start(ap, fmt);
+
+        if (print_ctx->ldif != NULL) {
+            lenght = strlen(print_ctx->ldif);
+        }
+
+        print_ctx->ldif = talloc_vasprintf_append_buffer(print_ctx->ldif,
+                                                         fmt, ap);
+        if (print_ctx->ldif == NULL) {
+            return (-1 * ENOENT);
+        }
+
+        lenght = strlen(print_ctx->ldif) - lenght;
+        va_end(ap);
+    }
+
+    return lenght;
+}
+
+static void sss_ldb_ldif2log(enum ldb_changetype changetype,
+                             struct ldb_context *ldb,
+                             const struct ldb_message *message)
+{
+    int ret;
+    struct ldb_ldif ldif;
+    struct sss_ldif_vprint_ctx *ldb_print_ctx;
+
+    ldb_print_ctx = talloc_zero(ldb, struct sss_ldif_vprint_ctx);
+    if (ldb_print_ctx == NULL) {
+        return;
+    }
+    ldb_print_ctx->ldif = NULL;
+
+    ldif.changetype = changetype;
+    ldif.msg = discard_const_p(struct ldb_message, message);
+
+    ret = ldb_ldif_write(ldb, ldif_vprintf_fn, ldb_print_ctx, &ldif);
+    if (ret < 0) {
+        ret = -1 * ret;
+        DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with [%d][%s].\n",
+                                    ret, sss_strerror(ret));
+        goto done;
+    }
+
+    DEBUG(SSSDBG_LDB, "ldif\n[\n%s\n]\n", ldb_print_ctx->ldif);
+
+done:
+    talloc_free(ldb_print_ctx->ldif);
+    talloc_free(ldb_print_ctx);
+
+    return;
+}
+
+int sss_ldb_add(struct ldb_context *ldb, const struct ldb_message *message)
+{
+    if (DEBUG_IS_SET(SSSDBG_LDB) == true) {
+        sss_ldb_ldif2log(LDB_CHANGETYPE_ADD, ldb, message);
+    }
+
+    return ldb_add(ldb, message);
+}
+
+int sss_ldb_delete(struct ldb_context *ldb, struct ldb_dn *dn)
+{
+    if (DEBUG_IS_SET(SSSDBG_LDB) == true) {
+        DEBUG(SSSDBG_LDB, "Deleting [%s]\n", ldb_dn_get_rdn_name(dn));
+    }
+
+    return ldb_delete(ldb, dn);
+}
+
+int sss_ldb_modify(struct ldb_context *ldb, const struct ldb_message *message)
+{
+    if (DEBUG_IS_SET(SSSDBG_LDB) == true) {
+        sss_ldb_ldif2log(LDB_CHANGETYPE_MODIFY, ldb, message);
+    }
+
+    return ldb_modify(ldb, message);
+}
+
+int sss_ldb_rename(struct ldb_context *ldb,
+                   struct ldb_dn *olddn,
+                   struct ldb_dn *newdn)
+{
+    if (DEBUG_IS_SET(SSSDBG_LDB) == true) {
+        DEBUG(SSSDBG_LDB, "Renaming [%s] to [%s]\n",
+                          ldb_dn_get_rdn_name(olddn),
+                          ldb_dn_get_rdn_name(newdn));
+    }
+
+    return ldb_rename(ldb, olddn, newdn);
+}
diff --git a/src/db/sysdb_ldb_wrapper.h b/src/db/sysdb_ldb_wrapper.h
new file mode 100644
index 0000000000000000000000000000000000000000..07a42f0f110ded75c4ad5048e23dd13429a449c1
--- /dev/null
+++ b/src/db/sysdb_ldb_wrapper.h
@@ -0,0 +1,35 @@
+/*
+   SSSD
+
+   System Database -- ldb wrappers
+
+   Copyright (C) Petr Cech <pc...@redhat.com>	2016
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __SYSDB_LDB_WRAPPER_H__
+#define __SYSDB_LDB_WRAPPER_H__
+
+int sss_ldb_add(struct ldb_context *ldb, const struct ldb_message *message);
+
+int sss_ldb_delete(struct ldb_context *ldb, struct ldb_dn *dn);
+
+int sss_ldb_modify(struct ldb_context *ldb, const struct ldb_message *message);
+
+int sss_ldb_rename(struct ldb_context *ldb,
+                   struct ldb_dn *olddn,
+                   struct ldb_dn *newdn);
+
+#endif /* __SYSDB_LDB_WRAPPER_H__ */
diff --git a/src/util/debug.h b/src/util/debug.h
index 2a1bd4ffd30817d7128805996c21105fe40982a2..610a65b6b6b9a41c9d7062b118abe5f015e10d68 100644
--- a/src/util/debug.h
+++ b/src/util/debug.h
@@ -67,6 +67,7 @@ int get_fd_from_debug_file(void);
 #define SSSDBG_TRACE_INTERNAL 0x2000   /* level 8 */
 #define SSSDBG_TRACE_ALL      0x4000   /* level 9 */
 #define SSSDBG_BE_FO          0x8000   /* level 9 */
+#define SSSDBG_LDB            0x10000  /* level 9 */
 #define SSSDBG_IMPORTANT_INFO SSSDBG_OP_FAILURE
 
 #define SSSDBG_INVALID        -1
-- 
2.7.4

_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

Reply via email to