On 09/06/2016 01:18 PM, Petr Cech wrote:
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.
OK, my VM is able to push to CI tests (I was afraid I have issue).
But result is not good:
http://sssd-ci.duckdns.org/logs/job/53/00/summary.html
--
Petr^4 Čech