On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio <fidencio(a)redhat.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