On Fri, Jan 15, 2016 at 10:21:25AM +0100, Pavel Březina wrote:
On 01/14/2016 06:37 PM, Sumit Bose wrote:
>On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
>>On 01/13/2016 05:12 PM, Sumit Bose wrote:
>>>>
>>>>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
>>>>
>>>> if (ctx == NULL) {
>>>> DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
>>>>- ctx->ret = ERR_INTERNAL;
>>>> return false;
>>>> }
>>>>
>>>>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
>>>>
>>>> if (ctx == NULL) {
>>>> DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
>>>>- ctx->ret = ERR_INTERNAL;
>>>> return false;
>>>> }
>>>>
>>>
>>>it looks like the last 2 hunks are not available in the latest tar ball.
>>
>>Sorry about that. Added.
>>
>>>and there are some trailing whitespaces in patch 9 at line 41.
>>
>>Fixed by git.
>>
>>I also attached two more patches then converts usn into number and then use
>>in filter entryUSN >= (current + 1) instead of (entryUSN >= cur)
&&
>>(entryUSN != cur) as per Thierry request.
>>
>>It could probably be squashed and replace some previous patches but I think
>>sending it as new patches is easier for you to review, given the previous
>>patches are basically acked.
>
>>+ errno = 0;
>> usn_number = strtoul(usn, &endptr, 10);
>>- if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
>>- && (usn_number > srv_opts->last_usn)) {
>>- srv_opts->last_usn = usn_number;
>>+ if (endptr != NULL && *endptr != '\0') {
>
>Currently an empty string in usn would result in usn_number==0, I wonder
>if this is valid or if it would be better to error out in this case?
Zero value will not change anything since we always compare if the current
usn value is lower than the new one, we switch them only in such case. So I
think it is not necessary to error out in this case since strtoul succeeds.
ok, that was my thinking, too.
>
>Otherwise all looks good, CI and Coverity test are currently running.
Coverity didn't found any new issues and CI passed
(
http://sssd-ci.duckdns.org/logs/job/35/58/summary.html) expect on Debian
where the test_add_remove_user integration tested failed. But since the
same test passed on other platform I expect that the failure it not
related to your tests.
ACK.
bye,
Sumit