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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
