On 09/06/2016 01:15 PM, 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)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.
I wondered if my VM could push patches to our CI.
I will link the CI results after they finish.
--
Petr^4 Čech