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)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
Hello,
there is new patch set attached.
I replaced all ldb_* to new wrapper in whole code.
Regards
--
Petr^4 Čech