[SSSD] [PATCH] IFP: touch config when changing debug level temporarily

Pavel Březina pbrezina at redhat.com
Wed Jul 9 11:34:16 UTC 2014


On 07/09/2014 01:31 PM, Jakub Hrozek wrote:
> On Wed, Jul 09, 2014 at 09:41:08AM +0200, Pavel Reichl wrote:
>> On Tue, 2014-07-08 at 10:23 +0200, Pavel Březina wrote:
>>> On 07/07/2014 02:14 PM, Pavel Reichl wrote:
>>>> On Wed, 2014-07-02 at 13:10 +0200, Pavel Březina wrote:
>>>>> This fixes a bug found by Jan Šafránek. The debug level was not restored
>>>>> after sssd restart because the time of last modification of sssd.conf
>>>>> was older than config.ldb.
>>>>> _______________________________________________
>>>>> sssd-devel mailing list
>>>>> sssd-devel at lists.fedorahosted.org
>>>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>
>>>> Hi Pavel,
>>>>
>>>>
>>>>>        ret = change_debug_level_tmp(ctx->rctx->cdb, name, type, arg_new_level);
>>>>> +    if (ret != EOK) {
>>>>> +        goto done;
>>>>> +    }
>>>>> +
>>>>> +    errno = 0;
>>>>
>>>> I just wonder if there is any need to initialize errno as it is set by utime() in case of failure and we directly check return value of utime().
>>>> I'm not sure but I don't think it's a common practice in SSSD codebase to always initialize errno.
>>>
>>> You are correct sir. I removed it also from sss_debuglevel tool.
>>>
>>>>
>>>> I think it would be better to replace this line with comment like you put in commit message, something like
>>>>
>>>> /* Touch configuration file to make sure debug level is reloaded. */
>>>>
>>>
>>> Done.
>>>
>>>>> +    if (utime(CONFDB_DEFAULT_CONFIG_FILE, NULL) == -1) {
>>>>> +        ret = errno;
>>>>> +        goto done;
>>>>> +    }
>>>>> +
>>>>> +    ret = EOK;
>>>>
>>>> Thanks,
>>>>
>>>> PR
>>>>
>>>>
>>>> _______________________________________________
>>>> sssd-devel mailing list
>>>> sssd-devel at lists.fedorahosted.org
>>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>
>>>
>>> Patches are attached.
>>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>> Hi, thanks for the update.
>>
>> Patches LGTM and fixes the issue.
>>
>> ACK to both.
>
> I pushed the first one so far, do you think the extra errno setting is
> worth a patch? If so, I will push it, too..

No, but why not when it is already written...

>
> master: 09641f85aae8fff72d0573394c40c8c3c002179a
>
>>
>> Regards,
>>
>> PR
>>
>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>




More information about the sssd-devel mailing list