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.
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