On (16/09/16 16:19), Petr Cech wrote:
>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 <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.
>
>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 = "";
+
+    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.

LS
_______________________________________________
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