[PATCHES] sudo provider improvements

[Pavel Březina]

Hi,
I'm sending some sudo provider patches. I wanted to fix/improve things 
in the ldap sudo provider prior my work on ipa provider so I get 
familiar with it again and avoid making the same mistakes.

It fixes tevent style, shuffles the code around a little bit, convert 
periodic task to use be_ptask module, renew hostinfo when needed, fix 
sdap_id_op logic, recude code duplication, remove dead code, simplify 
error handling, etc.

Ticket fixed:
https://fedorahosted.org/sssd/ticket/1943
https://fedorahosted.org/sssd/ticket/2672

I let Dan run downstream tests on those patches. We had to fix one test 
that was prone to a race condition which my patches revealed, but 
everything is green now.

[Lukas Slebodnik]

On (24/11/15 13:23), Pavel Březina wrote:
> Hi,
> I'm sending some sudo provider patches. I wanted to fix/improve things in the
> ldap sudo provider prior my work on ipa provider so I get familiar with it
> again and avoid making the same mistakes.
>
> It fixes tevent style, shuffles the code around a little bit, convert
> periodic task to use be_ptask module, renew hostinfo when needed, fix
> sdap_id_op logic, recude code duplication, remove dead code, simplify error
> handling, etc.
>
> Ticket fixed:
> https://fedorahosted.org/sssd/ticket/1943
> https://fedorahosted.org/sssd/ticket/2672
>
> I let Dan run downstream tests on those patches. We had to fix one test that
> was prone to a race condition which my patches revealed, but everything is
> green now.

I got following valgrind errors with patches

==17279== 1 errors in context 3 of 7:
==17279== Conditional jump or move depends on uninitialised value(s)
==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
==17279== 
==17279== 
==17279== 1 errors in context 4 of 7:
==17279== Conditional jump or move depends on uninitialised value(s)
==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)

LS

[Lukas Slebodnik]

On (01/12/15 14:02), Lukas Slebodnik wrote:
> On (24/11/15 13:23), Pavel Březina wrote:
> > Hi,
> > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > again and avoid making the same mistakes.
> >
> > It fixes tevent style, shuffles the code around a little bit, convert
> > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > handling, etc.
> >
> > Ticket fixed:
> > https://fedorahosted.org/sssd/ticket/1943
> > https://fedorahosted.org/sssd/ticket/2672
> >
> > I let Dan run downstream tests on those patches. We had to fix one test that
> > was prone to a race condition which my patches revealed, but everything is
> > green now.
>
> I got following valgrind errors with patches
>
> ==17279== 1 errors in context 3 of 7:
> ==17279== Conditional jump or move depends on uninitialised value(s)
> ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> ==17279== 
> ==17279== 
> ==17279== 1 errors in context 4 of 7:
> ==17279== Conditional jump or move depends on uninitialised value(s)
> ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)

I can see error in sssd_sudo as well
but it should not be caused by your patches.

==21931== Invalid read of size 8
==21931==    at 0x114F24: sss_dp_callback_destructor (responder_dp.c:60)
==21931==    by 0x87A65C8: _talloc_free_internal (talloc.c:993)
==21931==    by 0x87A0642: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A0642: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A0642: _talloc_free (talloc.c:1594)
==21931==    by 0x859436F: tevent_req_received (tevent_req.c:247)
==21931==    by 0x85943A8: tevent_req_destructor (tevent_req.c:99)
==21931==    by 0x87A65C8: _talloc_free_internal (talloc.c:993)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A0642: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A0642: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A0642: _talloc_free (talloc.c:1594)
==21931==  Address 0xb906e70 is 688 bytes inside a block of size 797 free'd
==21931==    at 0x4C28D17: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==21931==    by 0x87A6283: _talloc_free_internal (talloc.c:1063)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A0642: _talloc_free_children_internal (talloc.c:1472)
==21931==    by 0x87A0642: _talloc_free_internal (talloc.c:1019)
==21931==    by 0x87A0642: _talloc_free (talloc.c:1594)
==21931==    by 0x8DED028: __run_exit_handlers (exit.c:82)
==21931==    by 0x8DED074: exit (exit.c:104)
==21931==    by 0x4E74A1C: orderly_shutdown (server.c:257)
==21931==    by 0x8596716: tevent_common_check_signal (tevent_signal.c:450)
==21931==    by 0x85983EB: epoll_event_loop (tevent_epoll.c:647)
==21931==    by 0x85983EB: epoll_event_loop_once (tevent_epoll.c:926)
==21931==    by 0x8596936: std_event_loop_once (tevent_standard.c:114)

LS

[Pavel Březina]

On 12/01/2015 02:19 PM, Lukas Slebodnik wrote:
> On (01/12/15 14:02), Lukas Slebodnik wrote:
> > On (24/11/15 13:23), Pavel Březina wrote:
> > > Hi,
> > > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > > again and avoid making the same mistakes.
> > >
> > > It fixes tevent style, shuffles the code around a little bit, convert
> > > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > > handling, etc.
> > >
> > > Ticket fixed:
> > > https://fedorahosted.org/sssd/ticket/1943
> > > https://fedorahosted.org/sssd/ticket/2672
> > >
> > > I let Dan run downstream tests on those patches. We had to fix one test that
> > > was prone to a race condition which my patches revealed, but everything is
> > > green now.
> >
> > I got following valgrind errors with patches
> >
> > ==17279== 1 errors in context 3 of 7:
> > ==17279== Conditional jump or move depends on uninitialised value(s)
> > ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> > ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> > ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > ==17279==
> > ==17279==
> > ==17279== 1 errors in context 4 of 7:
> > ==17279== Conditional jump or move depends on uninitialised value(s)
> > ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> > ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)
>
> I can see error in sssd_sudo as well
> but it should not be caused by your patches.
>
> ==21931== Invalid read of size 8
> ==21931==    at 0x114F24: sss_dp_callback_destructor (responder_dp.c:60)
> ==21931==    by 0x87A65C8: _talloc_free_internal (talloc.c:993)
> ==21931==    by 0x87A0642: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A0642: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A0642: _talloc_free (talloc.c:1594)
> ==21931==    by 0x859436F: tevent_req_received (tevent_req.c:247)
> ==21931==    by 0x85943A8: tevent_req_destructor (tevent_req.c:99)
> ==21931==    by 0x87A65C8: _talloc_free_internal (talloc.c:993)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A0642: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A0642: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A0642: _talloc_free (talloc.c:1594)
> ==21931==  Address 0xb906e70 is 688 bytes inside a block of size 797 free'd
> ==21931==    at 0x4C28D17: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==21931==    by 0x87A6283: _talloc_free_internal (talloc.c:1063)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A61E2: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A61E2: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A0642: _talloc_free_children_internal (talloc.c:1472)
> ==21931==    by 0x87A0642: _talloc_free_internal (talloc.c:1019)
> ==21931==    by 0x87A0642: _talloc_free (talloc.c:1594)
> ==21931==    by 0x8DED028: __run_exit_handlers (exit.c:82)
> ==21931==    by 0x8DED074: exit (exit.c:104)
> ==21931==    by 0x4E74A1C: orderly_shutdown (server.c:257)
> ==21931==    by 0x8596716: tevent_common_check_signal (tevent_signal.c:450)
> ==21931==    by 0x85983EB: epoll_event_loop (tevent_epoll.c:647)
> ==21931==    by 0x85983EB: epoll_event_loop_once (tevent_epoll.c:926)
> ==21931==    by 0x8596936: std_event_loop_once (tevent_standard.c:114)
>
> LS

Can you check other responders for this as well? It looks like it 
happens on responder shutdown. Also feel free to send a patch.

[Pavel Březina]

On 12/01/2015 02:02 PM, Lukas Slebodnik wrote:
> On (24/11/15 13:23), Pavel Březina wrote:
> > Hi,
> > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > again and avoid making the same mistakes.
> >
> > It fixes tevent style, shuffles the code around a little bit, convert
> > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > handling, etc.
> >
> > Ticket fixed:
> > https://fedorahosted.org/sssd/ticket/1943
> > https://fedorahosted.org/sssd/ticket/2672
> >
> > I let Dan run downstream tests on those patches. We had to fix one test that
> > was prone to a race condition which my patches revealed, but everything is
> > green now.
>
> I got following valgrind errors with patches
>
> ==17279== 1 errors in context 3 of 7:
> ==17279== Conditional jump or move depends on uninitialised value(s)
> ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> ==17279==
> ==17279==
> ==17279== 1 errors in context 4 of 7:
> ==17279== Conditional jump or move depends on uninitialised value(s)
> ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)

I can't see a codepath where usn could be uninitialized, do you?

[Lukas Slebodnik]

On (02/12/15 11:05), Pavel Březina wrote:
> On 12/01/2015 02:02 PM, Lukas Slebodnik wrote:
> > On (24/11/15 13:23), Pavel Březina wrote:
> > > Hi,
> > > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > > again and avoid making the same mistakes.
> > >
> > > It fixes tevent style, shuffles the code around a little bit, convert
> > > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > > handling, etc.
> > >
> > > Ticket fixed:
> > > https://fedorahosted.org/sssd/ticket/1943
> > > https://fedorahosted.org/sssd/ticket/2672
> > >
> > > I let Dan run downstream tests on those patches. We had to fix one test that
> > > was prone to a race condition which my patches revealed, but everything is
> > > green now.
> >
> > I got following valgrind errors with patches
> >
> > ==17279== 1 errors in context 3 of 7:
> > ==17279== Conditional jump or move depends on uninitialised value(s)
> > ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> > ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> > ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > ==17279==
> > ==17279==
> > ==17279== 1 errors in context 4 of 7:
> > ==17279== Conditional jump or move depends on uninitialised value(s)
> > ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> > ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)
>
> I can't see a codepath where usn could be uninitialized, do you?

I didn't try but static analysers helped me,
at least I hope it will help you :-)
Maybe, once you will learn how to use them :-) :-) :-)

Error: UNINIT (CWE-457): [#def1]
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690: var_decl: Declaring variable "usn" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750: uninit_use_in_call: Using uninitialized value "usn" when calling "sdap_sudo_set_usn".
#  748|   
#  749|       /* remember new usn */
#  750|->     sdap_sudo_set_usn(state->srv_opts, usn);
#  751|   
#  752|       ret = EOK;

Error: CLANG_WARNING: [#def2]
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: warning: Function call argument is an uninitialized value
#    sdap_sudo_set_usn(state->srv_opts, usn);
#    ^                                  ~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690:5: note: 'usn' declared without an initial value
#    char *usn;
#    ^~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:9: note: Assuming 'dp_error' is not equal to 0
#    if (dp_error == DP_ERR_OK && ret != EOK) {
#        ^~~~~~~~~~~~~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:31: note: Left side of '&&' is false
#    if (dp_error == DP_ERR_OK && ret != EOK) {
#                              ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:9: note: Assuming 'ret' is equal to 0
#    if (ret != EOK) {
#        ^~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:5: note: Taking false branch
#    if (ret != EOK) {
#    ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:9: note: Assuming 'ret' is equal to 0
#    if (ret != EOK) {
#        ^~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:5: note: Taking false branch
#    if (ret != EOK) {
#    ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Calling 'sdap_sudo_store_sudoers'
#    ret = sdap_sudo_store_sudoers(state, state->domain,
#          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:285:26: note: Left side of '||' is true
#    if (rules_count == 0 || rules == NULL) {
#                         ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Returning from 'sdap_sudo_store_sudoers'
#    ret = sdap_sudo_store_sudoers(state, state->domain,
#          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:735:5: note: Taking false branch
#    if (ret != EOK) {
#    ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:9: note: Assuming 'ret' is equal to 0
#    if (ret != EOK) {
#        ^~~~~~~~~~
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:5: note: Taking false branch
#    if (ret != EOK) {
#    ^
sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: note: Function call argument is an uninitialized value
#    sdap_sudo_set_usn(state->srv_opts, usn);
#    ^                                  ~~~
#  748|   
#  749|       /* remember new usn */
#  750|->     sdap_sudo_set_usn(state->srv_opts, usn);
#  751|   
#  752|       ret = EOK;

Error: UNINIT (CWE-457): [#def3]
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:130: var_decl: Declaring variable "deleted" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:142: uninit_use: Using uninitialized value "deleted".
#  140|       case BE_REQ_SUDO_RULES:
#  141|           ret = sdap_sudo_rules_refresh_recv(req, &dp_error, &deleted);
#  142|->         if (ret == EOK && deleted == true) {
#  143|               ret = ENOENT;
#  144|           }

Error: UNINIT (CWE-457): [#def4]
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:129: var_decl: Declaring variable "dp_error" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_sudo.c:155: uninit_use_in_call: Using uninitialized value "dp_error" when calling "sdap_handler_done".
#  153|   
#  154|       talloc_zfree(req);
#  155|->     sdap_handler_done(be_req, dp_error, ret, strerror(ret));
#  156|   }
#  157|

Error: UNINIT (CWE-457): [#def5]
sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:388: var_decl: Declaring variable "downloaded_rules_num" without initializer.
sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:401: uninit_use: Using uninitialized value "downloaded_rules_num".
#  399|       }
#  400|   
#  401|->     state->deleted = downloaded_rules_num != state->num_rules ? true : false;
#  402|   
#  403|   done:

LS

[Lukas Slebodnik]

On (02/12/15 14:06), Lukas Slebodnik wrote:
> On (02/12/15 11:05), Pavel Březina wrote:
> > On 12/01/2015 02:02 PM, Lukas Slebodnik wrote:
> > > On (24/11/15 13:23), Pavel Březina wrote:
> > > > Hi,
> > > > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > > > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > > > again and avoid making the same mistakes.
> > > >
> > > > It fixes tevent style, shuffles the code around a little bit, convert
> > > > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > > > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > > > handling, etc.
> > > >
> > > > Ticket fixed:
> > > > https://fedorahosted.org/sssd/ticket/1943
> > > > https://fedorahosted.org/sssd/ticket/2672
> > > >
> > > > I let Dan run downstream tests on those patches. We had to fix one test that
> > > > was prone to a race condition which my patches revealed, but everything is
> > > > green now.
> > >
> > > I got following valgrind errors with patches
> > >
> > > ==17279== 1 errors in context 3 of 7:
> > > ==17279== Conditional jump or move depends on uninitialised value(s)
> > > ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> > > ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> > > ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > > ==17279==
> > > ==17279==
> > > ==17279== 1 errors in context 4 of 7:
> > > ==17279== Conditional jump or move depends on uninitialised value(s)
> > > ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> > > ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > > ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)
> >
> > I can't see a codepath where usn could be uninitialized, do you?
>
> I didn't try but static analysers helped me,

I cannot see such errors with current master.

LS

[Pavel Březina]

On 12/02/2015 02:06 PM, Lukas Slebodnik wrote:
> On (02/12/15 11:05), Pavel Březina wrote:
> > On 12/01/2015 02:02 PM, Lukas Slebodnik wrote:
> > > On (24/11/15 13:23), Pavel Březina wrote:
> > > > Hi,
> > > > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > > > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > > > again and avoid making the same mistakes.
> > > >
> > > > It fixes tevent style, shuffles the code around a little bit, convert
> > > > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > > > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > > > handling, etc.
> > > >
> > > > Ticket fixed:
> > > > https://fedorahosted.org/sssd/ticket/1943
> > > > https://fedorahosted.org/sssd/ticket/2672
> > > >
> > > > I let Dan run downstream tests on those patches. We had to fix one test that
> > > > was prone to a race condition which my patches revealed, but everything is
> > > > green now.
> > >
> > > I got following valgrind errors with patches
> > >
> > > ==17279== 1 errors in context 3 of 7:
> > > ==17279== Conditional jump or move depends on uninitialised value(s)
> > > ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> > > ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> > > ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > > ==17279==
> > > ==17279==
> > > ==17279== 1 errors in context 4 of 7:
> > > ==17279== Conditional jump or move depends on uninitialised value(s)
> > > ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> > > ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > > ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)
> >
> > I can't see a codepath where usn could be uninitialized, do you?
>
> I didn't try but static analysers helped me,
> at least I hope it will help you :-)
> Maybe, once you will learn how to use them :-) :-) :-)
>
> Error: UNINIT (CWE-457): [#def1]
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690: var_decl: Declaring variable "usn" without initializer.
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750: uninit_use_in_call: Using uninitialized value "usn" when calling "sdap_sudo_set_usn".
> #  748|
> #  749|       /* remember new usn */
> #  750|->     sdap_sudo_set_usn(state->srv_opts, usn);
> #  751|
> #  752|       ret = EOK;
>
> Error: CLANG_WARNING: [#def2]
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: warning: Function call argument is an uninitialized value
> #    sdap_sudo_set_usn(state->srv_opts, usn);
> #    ^                                  ~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690:5: note: 'usn' declared without an initial value
> #    char *usn;
> #    ^~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:9: note: Assuming 'dp_error' is not equal to 0
> #    if (dp_error == DP_ERR_OK && ret != EOK) {
> #        ^~~~~~~~~~~~~~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:31: note: Left side of '&&' is false
> #    if (dp_error == DP_ERR_OK && ret != EOK) {
> #                              ^
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:9: note: Assuming 'ret' is equal to 0
> #    if (ret != EOK) {
> #        ^~~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:5: note: Taking false branch
> #    if (ret != EOK) {
> #    ^
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:9: note: Assuming 'ret' is equal to 0
> #    if (ret != EOK) {
> #        ^~~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:5: note: Taking false branch
> #    if (ret != EOK) {
> #    ^
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Calling 'sdap_sudo_store_sudoers'
> #    ret = sdap_sudo_store_sudoers(state, state->domain,
> #          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:285:26: note: Left side of '||' is true
> #    if (rules_count == 0 || rules == NULL) {
> #                         ^
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Returning from 'sdap_sudo_store_sudoers'
> #    ret = sdap_sudo_store_sudoers(state, state->domain,
> #          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:735:5: note: Taking false branch
> #    if (ret != EOK) {
> #    ^
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:9: note: Assuming 'ret' is equal to 0
> #    if (ret != EOK) {
> #        ^~~~~~~~~~
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:5: note: Taking false branch
> #    if (ret != EOK) {
> #    ^
> sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: note: Function call argument is an uninitialized value
> #    sdap_sudo_set_usn(state->srv_opts, usn);
> #    ^                                  ~~~
> #  748|
> #  749|       /* remember new usn */
> #  750|->     sdap_sudo_set_usn(state->srv_opts, usn);
> #  751|
> #  752|       ret = EOK;
>
> Error: UNINIT (CWE-457): [#def3]
> sssd-1.13.90/src/providers/ldap/sdap_sudo.c:130: var_decl: Declaring variable "deleted" without initializer.
> sssd-1.13.90/src/providers/ldap/sdap_sudo.c:142: uninit_use: Using uninitialized value "deleted".
> #  140|       case BE_REQ_SUDO_RULES:
> #  141|           ret = sdap_sudo_rules_refresh_recv(req, &dp_error, &deleted);
> #  142|->         if (ret == EOK && deleted == true) {
> #  143|               ret = ENOENT;
> #  144|           }
>
> Error: UNINIT (CWE-457): [#def4]
> sssd-1.13.90/src/providers/ldap/sdap_sudo.c:129: var_decl: Declaring variable "dp_error" without initializer.
> sssd-1.13.90/src/providers/ldap/sdap_sudo.c:155: uninit_use_in_call: Using uninitialized value "dp_error" when calling "sdap_handler_done".
> #  153|
> #  154|       talloc_zfree(req);
> #  155|->     sdap_handler_done(be_req, dp_error, ret, strerror(ret));
> #  156|   }
> #  157|
>
> Error: UNINIT (CWE-457): [#def5]
> sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:388: var_decl: Declaring variable "downloaded_rules_num" without initializer.
> sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:401: uninit_use: Using uninitialized value "downloaded_rules_num".
> #  399|       }
> #  400|
> #  401|->     state->deleted = downloaded_rules_num != state->num_rules ? true : false;
> #  402|
> #  403|   done:

Thanks. This helped. New patches attached, also including two more small 
patches.

[Lukas Slebodnik]

On (02/12/15 14:40), Pavel Březina wrote:
> On 12/02/2015 02:06 PM, Lukas Slebodnik wrote:
> > On (02/12/15 11:05), Pavel Březina wrote:
> > > On 12/01/2015 02:02 PM, Lukas Slebodnik wrote:
> > > > On (24/11/15 13:23), Pavel Březina wrote:
> > > > > Hi,
> > > > > I'm sending some sudo provider patches. I wanted to fix/improve things in the
> > > > > ldap sudo provider prior my work on ipa provider so I get familiar with it
> > > > > again and avoid making the same mistakes.
> > > > >
> > > > > It fixes tevent style, shuffles the code around a little bit, convert
> > > > > periodic task to use be_ptask module, renew hostinfo when needed, fix
> > > > > sdap_id_op logic, recude code duplication, remove dead code, simplify error
> > > > > handling, etc.
> > > > >
> > > > > Ticket fixed:
> > > > > https://fedorahosted.org/sssd/ticket/1943
> > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > >
> > > > > I let Dan run downstream tests on those patches. We had to fix one test that
> > > > > was prone to a race condition which my patches revealed, but everything is
> > > > > green now.
> > > >
> > > > I got following valgrind errors with patches
> > > >
> > > > ==17279== 1 errors in context 3 of 7:
> > > > ==17279== Conditional jump or move depends on uninitialised value(s)
> > > > ==17279==    at 0x8BC76FE: _talloc_steal_loc (talloc.c:1162)
> > > > ==17279==    by 0x13DA3775: sdap_sudo_set_usn (sdap_async_sudo.c:318)
> > > > ==17279==    by 0x13DA3775: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > > > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > > > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > > > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > > > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > > > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > > > ==17279==
> > > > ==17279==
> > > > ==17279== 1 errors in context 4 of 7:
> > > > ==17279== Conditional jump or move depends on uninitialised value(s)
> > > > ==17279==    at 0x13DA3738: sdap_sudo_set_usn (sdap_async_sudo.c:307)
> > > > ==17279==    by 0x13DA3738: sdap_sudo_refresh_done (sdap_async_sudo.c:750)
> > > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > > ==17279==    by 0x13DA3321: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:170)
> > > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > > ==17279==    by 0x89B4201: _tevent_req_error (tevent_req.c:167)
> > > > ==17279==    by 0x13D71ADB: generic_ext_search_handler.isra.3 (sdap_async.c:1651)
> > > > ==17279==    by 0x89B3923: tevent_common_loop_immediate (tevent_immediate.c:135)
> > > > ==17279==    by 0x89B822D: epoll_event_loop_once (tevent_epoll.c:907)
> > > > ==17279==    by 0x89B6936: std_event_loop_once (tevent_standard.c:114)
> > > > ==17279==    by 0x89B30FC: _tevent_loop_once (tevent.c:533)
> > > > ==17279==    by 0x89B329A: tevent_common_loop_wait (tevent.c:637)
> > > > ==17279==    by 0x89B68D6: std_event_loop_wait (tevent_standard.c:140)
> > >
> > > I can't see a codepath where usn could be uninitialized, do you?
> >
> > I didn't try but static analysers helped me,
> > at least I hope it will help you :-)
> > Maybe, once you will learn how to use them :-) :-) :-)
> >
> > Error: UNINIT (CWE-457): [#def1]
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690: var_decl: Declaring variable "usn" without initializer.
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750: uninit_use_in_call: Using uninitialized value "usn" when calling "sdap_sudo_set_usn".
> > #  748|
> > #  749|       /* remember new usn */
> > #  750|->     sdap_sudo_set_usn(state->srv_opts, usn);
> > #  751|
> > #  752|       ret = EOK;
> >
> > Error: CLANG_WARNING: [#def2]
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: warning: Function call argument is an uninitialized value
> > #    sdap_sudo_set_usn(state->srv_opts, usn);
> > #    ^                                  ~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:690:5: note: 'usn' declared without an initial value
> > #    char *usn;
> > #    ^~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:9: note: Assuming 'dp_error' is not equal to 0
> > #    if (dp_error == DP_ERR_OK && ret != EOK) {
> > #        ^~~~~~~~~~~~~~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:704:31: note: Left side of '&&' is false
> > #    if (dp_error == DP_ERR_OK && ret != EOK) {
> > #                              ^
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:9: note: Assuming 'ret' is equal to 0
> > #    if (ret != EOK) {
> > #        ^~~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:717:5: note: Taking false branch
> > #    if (ret != EOK) {
> > #    ^
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:9: note: Assuming 'ret' is equal to 0
> > #    if (ret != EOK) {
> > #        ^~~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:726:5: note: Taking false branch
> > #    if (ret != EOK) {
> > #    ^
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Calling 'sdap_sudo_store_sudoers'
> > #    ret = sdap_sudo_store_sudoers(state, state->domain,
> > #          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:285:26: note: Left side of '||' is true
> > #    if (rules_count == 0 || rules == NULL) {
> > #                         ^
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:732:11: note: Returning from 'sdap_sudo_store_sudoers'
> > #    ret = sdap_sudo_store_sudoers(state, state->domain,
> > #          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:735:5: note: Taking false branch
> > #    if (ret != EOK) {
> > #    ^
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:9: note: Assuming 'ret' is equal to 0
> > #    if (ret != EOK) {
> > #        ^~~~~~~~~~
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:741:5: note: Taking false branch
> > #    if (ret != EOK) {
> > #    ^
> > sssd-1.13.90/src/providers/ldap/sdap_async_sudo.c:750:5: note: Function call argument is an uninitialized value
> > #    sdap_sudo_set_usn(state->srv_opts, usn);
> > #    ^                                  ~~~
> > #  748|
> > #  749|       /* remember new usn */
> > #  750|->     sdap_sudo_set_usn(state->srv_opts, usn);
> > #  751|
> > #  752|       ret = EOK;
> >
> > Error: UNINIT (CWE-457): [#def3]
> > sssd-1.13.90/src/providers/ldap/sdap_sudo.c:130: var_decl: Declaring variable "deleted" without initializer.
> > sssd-1.13.90/src/providers/ldap/sdap_sudo.c:142: uninit_use: Using uninitialized value "deleted".
> > #  140|       case BE_REQ_SUDO_RULES:
> > #  141|           ret = sdap_sudo_rules_refresh_recv(req, &dp_error, &deleted);
> > #  142|->         if (ret == EOK && deleted == true) {
> > #  143|               ret = ENOENT;
> > #  144|           }
> >
> > Error: UNINIT (CWE-457): [#def4]
> > sssd-1.13.90/src/providers/ldap/sdap_sudo.c:129: var_decl: Declaring variable "dp_error" without initializer.
> > sssd-1.13.90/src/providers/ldap/sdap_sudo.c:155: uninit_use_in_call: Using uninitialized value "dp_error" when calling "sdap_handler_done".
> > #  153|
> > #  154|       talloc_zfree(req);
> > #  155|->     sdap_handler_done(be_req, dp_error, ret, strerror(ret));
> > #  156|   }
> > #  157|
> >
> > Error: UNINIT (CWE-457): [#def5]
> > sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:388: var_decl: Declaring variable "downloaded_rules_num" without initializer.
> > sssd-1.13.90/src/providers/ldap/sdap_sudo_refresh.c:401: uninit_use: Using uninitialized value "downloaded_rules_num".
> > #  399|       }
> > #  400|
> > #  401|->     state->deleted = downloaded_rules_num != state->num_rules ? true : false;
> > #  402|
> > #  403|   done:
>
> Thanks. This helped. New patches attached, also including two more small
> patches.

yes, it is fixed.
> From 38c5524b82d94db68450636020472869dc362070 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 6 Nov 2015 13:00:47 +0100
> Subject: [PATCH 01/28] SUDO: convert periodical refreshes to be_ptask
>
> This removes old sudo timer and simplyfies code a lot. It also
> allows to manage offline/online state.
> Full and smart refresh are disabled when offline.
> Full refresh is run immediately when sssd is back online.
> Smart refresh is scheduled normally when sssd is back online.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1943

The sudo tests seems to passed for refresh.

I still have some issues with some test which passes on rhel7.2.
But I will need to try with master and maybe bisect.
> Makefile.am                                |   2 +-
> src/providers/ldap/sdap_async_sudo_timer.c | 178 -----------
> src/providers/ldap/sdap_sudo.c             | 470 +----------------------------
> src/providers/ldap/sdap_sudo.h             |  37 +--
> src/providers/ldap/sdap_sudo_refresh.c     | 157 ++++++++++
> 5 files changed, 186 insertions(+), 658 deletions(-)
> delete mode 100644 src/providers/ldap/sdap_async_sudo_timer.c
> create mode 100644 src/providers/ldap/sdap_sudo_refresh.c
>
> diff --git a/Makefile.am b/Makefile.am
> index d7a6f29520980d933cf0afddf8d041b2d6f3b2ef..23d4ae38e78baf795f8bbecb3e50e5473ba0d709 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2841,8 +2841,8 @@ if BUILD_SUDO
> libsss_ldap_common_la_SOURCES += \
>     src/providers/ldap/sdap_sudo_cache.c \
>     src/providers/ldap/sdap_async_sudo.c \
> src/providers/ldap/sdap_async_sudo_timer.c \
> src/providers/ldap/sdap_async_sudo_hostinfo.c \
> src/providers/ldap/sdap_sudo_refresh.c \
> src/providers/ldap/sdap_sudo.c
>
> endif
>
> struct tevent_req * sdap_sudo_get_hostinfo_send(TALLOC_CTX *mem_ctx,
> diff --git a/src/providers/ldap/sdap_sudo_refresh.c b/src/providers/ldap/sdap_sudo_refresh.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..39821840a8a550dcc1a6c954dfb53fc73245e9f6
> --- /dev/null
> +++ b/src/providers/ldap/sdap_sudo_refresh.c
> @@ -0,0 +1,157 @@
> +/*
> Authors:
>    Pavel Březina <[e-mail]>
> Copyright (C) 2015 Red Hat
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program.  If not, see http://www.gnu.org/licenses/.
>
> +*/
>
> +#include <errno.h>
> +#include <talloc.h>
> +#include <tevent.h>
>
> +#include "util/util.h"
> +#include "providers/dp_ptask.h"
> +#include "providers/ldap/sdap_sudo.h"
> +#include "db/sysdb_sudo.h"
>
> +static struct tevent_req *
> +sdap_sudo_ptask_full_refresh_send(TALLOC_CTX *mem_ctx,
>                              struct tevent_context *ev,
>                              struct be_ctx *be_ctx,
>                              struct be_ptask *be_ptask,
>                              void *pvt)
>
> +{
> struct sdap_sudo_ctx *sudo_ctx;
> sudo_ctx = talloc_get_type(pvt, struct sdap_sudo_ctx);
> return sdap_sudo_full_refresh_send(mem_ctx, sudo_ctx);
>
> +}
>
> +static errno_t
> +sdap_sudo_ptask_full_refresh_recv(struct tevent_req *req)
> +{
> int dp_error;
> int error;
> return sdap_sudo_full_refresh_recv(req, &dp_error, &error);
>
> +}
>
> +static struct tevent_req *
> +sdap_sudo_ptask_smart_refresh_send(TALLOC_CTX *mem_ctx,
>                               struct tevent_context *ev,
>                               struct be_ctx *be_ctx,
>                               struct be_ptask *be_ptask,
>                               void *pvt)
>
> +{
> struct sdap_sudo_ctx *sudo_ctx;
> sudo_ctx = talloc_get_type(pvt, struct sdap_sudo_ctx);
> return sdap_sudo_smart_refresh_send(mem_ctx, sudo_ctx);
>
> +}
>
> +static errno_t
> +sdap_sudo_ptask_smart_refresh_recv(struct tevent_req *req)
> +{
> int dp_error;
> int error;
> return sdap_sudo_smart_refresh_recv(req, &dp_error, &error);
>
> +}
>
> +errno_t
> +sdap_sudo_ptask_setup(struct be_ctx *be_ctx, struct sdap_sudo_ctx *sudo_ctx)
> +{
> struct dp_option *opts = sudo_ctx->id_ctx->opts->basic;
> time_t smart;
> time_t full;
> time_t delay;
> time_t last_refresh;
> errno_t ret;
> smart = dp_opt_get_int(opts, SDAP_SUDO_SMART_REFRESH_INTERVAL);
> full = dp_opt_get_int(opts, SDAP_SUDO_FULL_REFRESH_INTERVAL);
> if (smart == 0 && full == 0) {
>    /* We don't allow both types to be disabled. At least smart refresh
>     * needs to be enabled. In this case smart refresh will catch up new
>     * and modified rules and deleted rules are caught when expired. */
>    smart = opts[SDAP_SUDO_SMART_REFRESH_INTERVAL].def_val.number;
>    DEBUG(SSSDBG_CONF_SETTINGS, "At least smart refresh needs to be "
>          "enabled. Setting smart refresh interval to default value "
>          "(%ld) seconds.\n", smart);
> } else if (full <= smart) {
>    /* In this case it does not make any sense to run smart refresh. */
>    smart = 0;
>    DEBUG(SSSDBG_CONF_SETTINGS, "Smart refresh interval has to be lower "
>          "than full refresh interval. Periodical smart refresh will be "
>          "disabled.\n");
> }
> ret = sysdb_sudo_get_last_full_refresh(be_ctx->domain, &last_refresh);
> if (ret != EOK) {
>    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to obtain time of last full "
>          "refresh. Assuming none was performed so far.\n");
>    last_refresh = 0;
> }
> if (last_refresh == 0) {
>    /* If this is the first startup, we need to kick off an refresh
>     * immediately, to close a window where clients requesting sudo
>     * information won't get an immediate reply with no entries */
>    delay = 0;
> } else {
>    /* At least one update has previously run, so clients will get cached
>     * data. We will delay the refresh so we don't slow down the startup
>     * process if this is happening during system boot. */
>    delay = 10;
> }
> /* Full refresh.
> *
> * Disable when offline and run immediately when SSSD goes back online.
> * Since we have periodical online check we don't have to run this task
> * when offline. */
> ret = be_ptask_create(be_ctx, be_ctx, full, delay, 0, 0, full,
>                      BE_PTASK_OFFLINE_DISABLE, 0,
>                      sdap_sudo_ptask_full_refresh_send,
>                      sdap_sudo_ptask_full_refresh_recv,
>                      sudo_ctx, "SUDO Full Refresh", NULL);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to setup full refresh ptask "
>          "[%d]: %s\n", ret, sss_strerror(ret));

It's easeir to read debug message if the format string si on separate line
or is alligned in block

e.g.
DEBUG(SSSDBG_CRIT_FAILURE,
      "Lorem Ipsum is simply dummy text of the printing and typesetting "
      "industry. Lorem Ipsum has been the industry's standard dummy text "
      "ever since the 1500s, when an unknown printer took a galley of type "
      "and scrambled it to make a type specimen book"

or
DEBUG(SSSDBG_CRIT_FAILURE, "Lorem Ipsum is simply dummy text of the printing "
                           "and typesetting industry. Lorem Ipsum has been "
                           "the industry's standard dummy text ever since the "
                           "1500s, when an unknown printer took a galley of "
                           "type and scrambled it to make a type specimen book."

I prefer 1st version becuase it is usualy shorter.

Such issues fwith debug messages are also on other places.
But I will not mention them on other places.
It would be good if it was consistent. And you might change it
also in patches which has ACK.
>    return ret;
> }
> /* Smart refresh.
> *
> * Disable when offline and reschedule normally when SSSD goes back online.
> * Since we have periodical online check we don't have to run this task
> * when offline. */
> ret = be_ptask_create(be_ctx, be_ctx, smart, delay + smart, smart, 0, smart,
>                      BE_PTASK_OFFLINE_DISABLE, 0,
>                      sdap_sudo_ptask_smart_refresh_send,
>                      sdap_sudo_ptask_smart_refresh_recv,
>                      sudo_ctx, "SUDO Smart Refresh", NULL);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to setup smart refresh ptask "
>          "[%d]: %s\n", ret, sss_strerror(ret));
>    return ret;
> }
> return EOK;
> +}
>
> 2.1.0
> From f4cbc452dc98eb4a7ea198b1bac2c8dd6c0bc586 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 6 Nov 2015 13:32:34 +0100
> Subject: [PATCH 02/28] SUDO: move refreshes from sdap_sudo.c to
> sdap_sudo_refresh.c
> sdap_sudo.c will contain only initialization and handlers.
>
> src/providers/ldap/sdap_sudo.c         | 642 ---------------------------------
> src/providers/ldap/sdap_sudo.h         |  11 +
> src/providers/ldap/sdap_sudo_refresh.c | 628 ++++++++++++++++++++++++++++++++
> 3 files changed, 639 insertions(+), 642 deletions(-)

There were many moves, which I didn't check properly.
So LGTM
> From 563f3c2d1fd6eacaa17872f42f2d6733370ce9d5 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 12:26:07 +0100
> Subject: [PATCH 03/28] SUDO: move offline check to handler
>
> We let sdap_id_op decide if we are offline or not here but we
> should not get to this code since ptask is disabled and we will
> not get through sudo handler if offline.
> This simplyfies the code and make it more similar to other providers.

ACK
> From 4203cf889f8049687d910b7bb1e6b003763099c4 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 12:44:13 +0100
> Subject: [PATCH 04/28] SUDO: simplify error handling
>
> This patch removes state->error and uses only ret instead since
> state->error was only duplication anyway.

ACK
> From 6d57c053fe17f2ecd325cb32d687ad7dd9bc9b0f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 12:52:35 +0100
> Subject: [PATCH 05/28] SUDO: fix sdap_id_op logic
> Adds missing sdap_id_op_done call and retry logic.
>
> src/providers/ldap/sdap_async_sudo.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)

ACK
> From 1e8079faa704d586a53a724a0dec5fa37355a267 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 13:19:39 +0100
> Subject: [PATCH 06/28] SUDO: fix tevent style
>
> Rearrage and rename functions in sdap_async_sudo.c to obey
> tevent style and improve readability.
>
> src/providers/ldap/sdap_async_sudo.c   | 533 ++++++++++++++++-----------------
> src/providers/ldap/sdap_sudo.c         |   6 +-
> src/providers/ldap/sdap_sudo.h         |   8 +-
> src/providers/ldap/sdap_sudo_refresh.c |  18 +-
> 4 files changed, 265 insertions(+), 300 deletions(-)

I hope you just moved and renamed function.
becuase I do not plant to compare moved functions line be line.

other changes were OK

LGTM
> From 7646bed4c9d381e2c048e48dac45484a8e7cc955 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 13:34:45 +0100
> Subject: [PATCH 07/28] SUDO: fix sdap_sudo_smart_refresh_recv()
>
> This was really creepy.

^^^^^^^^^^^^^
  I hope it is just a WIP sentence.
  Such sentence is not useful for anyone.

otherwise ACK
> From c3446b2b5979e426834afa0099ef5380190f4a97 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 13:59:48 +0100
> Subject: [PATCH 08/28] SUDO: sdap_sudo_load_sudoers improve iterator
>
> The old search base iterator was difficult to read since its logic
> spread through all functions. This patch also shorten names.
>
> src/providers/ldap/sdap_async_sudo.c | 122 ++++++++++++++++-------------------
> 1 file changed, 54 insertions(+), 68 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> index 259a79516cafd6f0351fa0a09d4feb1593fefb3f..71e1b3322a557beaf82b51316967b3147fb61f76 100644
> --- a/src/providers/ldap/sdap_async_sudo.c
> +++ b/src/providers/ldap/sdap_async_sudo.c
> @@ -38,14 +38,15 @@ struct sdap_sudo_load_sudoers_state {
>     struct tevent_context *ev;
>     struct sdap_options *opts;
>     struct sdap_handle *sh;
> struct sysdb_attrs **ldap_rules; /* search result will be stored here */
> size_t ldap_rules_count;         /* search result will be stored here */
> int timeout;

Could you explain why did you move "timeout" from end of structure
to the middle?

I do not see a reason why it cannot be the last.

BTW it's good to groups variables with small size together or
at the end of struct. It's mostly to avoid unnecessary padding
added by compiler. (and thus reduced size of struct)

I know that it will not have an effect here.
But I do not understand why it had to be moved.
> From f60af3b7bfece6e371ab7582cc23cf01d483d434 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 10:34:41 +0100
> Subject: [PATCH 09/28] SUDO: set USN inside sdap_sudo_refresh request
>
> Reduce code duplication.

Nice,

ACK
> From 9cac17688fd1cbd8d47d0cd7c545bea8f60e2051 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 11:31:50 +0100
> Subject: [PATCH 10/28] SUDO: built host filter inside sdap_sudo_refresh
> request
>
> Preparation for:
> https://fedorahosted.org/sssd/ticket/2672
>
> src/providers/ldap/sdap_async_sudo.c   | 197 +++++++++++++++++++++++---
> src/providers/ldap/sdap_sudo.h         |   6 +-
> src/providers/ldap/sdap_sudo_refresh.c | 251 +++++----------------------------
> 3 files changed, 212 insertions(+), 242 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> index 7a90ccde9cbd2efc0d0509f21e072cd745c57844..84eff3b2a99e9f7c38e49c870cd301f4f88b4d02 100644
> --- a/src/providers/ldap/sdap_async_sudo.c
> +++ b/src/providers/ldap/sdap_async_sudo.c
> @@ -328,7 +328,148 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
>                              srv_opts->max_sudo_value);
> }
>
> +static char *sdap_sudo_build_host_filter(TALLOC_CTX *mem_ctx,
>                                     struct sdap_attr_map *map,
>                                     char **hostnames,
>                                     char **ip_addr,
>                                     bool netgroups,
>                                     bool regexp)

//snip
> +static char *sdap_sudo_get_filter(TALLOC_CTX *mem_ctx,
>                              struct sdap_attr_map *map,
>                              struct sdap_sudo_ctx *sudo_ctx,
>                              const char *rule_filter)
>
> +{

These functions were moved in oppsite direction few patches before.

It would be easier for review If you could make functions
public and later private again.
Because it's difficult to spot changes in moved function.
> @@ -238,11 +88,6 @@ struct tevent_req *sdap_sudo_full_refresh_send(TALLOC_CTX *mem_ctx,
> tevent_req_set_callback(subreq, sdap_sudo_full_refresh_done, req);
>
> /* free filters */
> talloc_free(ldap_filter);
> talloc_free(ldap_full_filter);
> talloc_free(sysdb_filter);

I could not see added these lines elsewhere.
But I might miss something.
Could you explain it why?
> @@ -389,10 +225,6 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx,
> tevent_req_set_callback(subreq, sdap_sudo_smart_refresh_done, req);
>
> /* free filters */
> talloc_free(ldap_filter);
> talloc_free(ldap_full_filter);
> return req;
> From f44affe1bcb0ce92ab304657af75973f7a36a968 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 11:34:14 +0100
> Subject: [PATCH 11/28] SUDO: remove dead code from smart refresh
>
> We fail at the beginning if USN is NULL so it can't be NULL further
> in the code.
>
> src/providers/ldap/sdap_sudo_refresh.c | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_sudo_refresh.c b/src/providers/ldap/sdap_sudo_refresh.c
> index 80627f1c84cf6ef0d5b41a1368c9e0a0b7e5ed32..4f6339c5469fb8c3a01ea5179a8aa5b66355d531 100644
> --- a/src/providers/ldap/sdap_sudo_refresh.c
> +++ b/src/providers/ldap/sdap_sudo_refresh.c
> @@ -182,7 +182,7 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx,
>     }
> if (!sudo_ctx->full_refresh_done
>        && (srv_opts == NULL || srv_opts->max_sudo_value == 0)) {
>        && (srv_opts == NULL || srv_opts->max_sudo_value == NULL)) {

srv_opts->max_sudo_value (and thus "usn) might be checked
for NULL if sudo_ctx->full_refresh_done if true.
> From 324f22ad7f2f8486bc720167a0eeb855514ecd11 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 12:34:11 +0100
> Subject: [PATCH 12/28] SUDO: fix potential memory leak in sdap_sudo_init
>
> src/providers/ldap/sdap_sudo.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)

obvious goodness

ACK
> From 26d6e47c64652effe261becdf3e5a44e9e535e73 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 12:32:16 +0100
> Subject: [PATCH 13/28] SUDO: obtain host information when going online
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2672
>
> src/providers/ldap/sdap_async_sudo.c | 75 ++++++++++++++++++++++++++++++----
> src/providers/ldap/sdap_sudo.c       | 78 ++++++++++++++----------------------
> src/providers/ldap/sdap_sudo.h       |  2 +
> 3 files changed, 100 insertions(+), 55 deletions(-)

Code LGTM, but I didn't test issue in ticket #2672
We might ask reporter in BZ
> From e9d47fd509b6cf57f3dcd8c70015f3ca350fb5ec Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 25 Nov 2015 13:16:17 +0100
> Subject: [PATCH 14/28] SUDO: remove finalizer
> It is not used anywhere anyway.
>
> src/providers/ldap/sdap_sudo.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_sudo.c b/src/providers/ldap/sdap_sudo.c
> index a212bdd5e87042cca1484717c6fe6353fa040bfa..4efa3a678ffd81618a88de0ed2c95f0307f41b0c 100644
> --- a/src/providers/ldap/sdap_sudo.c
> +++ b/src/providers/ldap/sdap_sudo.c
> @@ -30,15 +30,9 @@
> #include "providers/ldap/sdap_sudo_cache.h"
> #include "db/sysdb_sudo.h"
>
> -static void
> -sdap_sudo_shutdown(struct be_req *req)
> -{
> sdap_handler_done(req, DP_ERR_OK, EOK, NULL);
>
> -}
>
> struct bet_ops sdap_sudo_ops = {
>     .handler = sdap_sudo_handler,
> .finalize = sdap_sudo_shutdown
> .finalize = NULL

ipa_autofs_ops does not have initialized .finalize either.

ACK
> From d756cd4513d6a4231fba98bc0ee732c2adb9432c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 25 Nov 2015 13:21:49 +0100
> Subject: [PATCH 15/28] SUDO: make sdap_sudo_handler static

ACK

LS

[Pavel Březina]

On 12/04/2015 03:51 PM, Lukas Slebodnik wrote:
> > From 38c5524b82d94db68450636020472869dc362070 Mon Sep 17 00:00:00 2001
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to setup full refresh ptask "
> >          "[%d]: %s\n", ret, sss_strerror(ret));
>
> It's easeir to read debug message if the format string si on separate line
> or is alligned in block
>
> e.g.
> DEBUG(SSSDBG_CRIT_FAILURE,
>        "Lorem Ipsum is simply dummy text of the printing and typesetting "
>        "industry. Lorem Ipsum has been the industry's standard dummy text "
>        "ever since the 1500s, when an unknown printer took a galley of type "
>        "and scrambled it to make a type specimen book"
>
> or
> DEBUG(SSSDBG_CRIT_FAILURE, "Lorem Ipsum is simply dummy text of the printing "
>                             "and typesetting industry. Lorem Ipsum has been "
>                             "the industry's standard dummy text ever since the "
>                             "1500s, when an unknown printer took a galley of "
>                             "type and scrambled it to make a type specimen book."

I rarely align it this way since we removed brackets around message. I 
didn't touch the old messages that were converted to new format when a 
code was moved. I always use what is shorter these days.
> I prefer 1st version becuase it is usualy shorter.
>
> Such issues fwith debug messages are also on other places.
> But I will not mention them on other places.
> It would be good if it was consistent. And you might change it
> also in patches which has ACK.
> > From 7646bed4c9d381e2c048e48dac45484a8e7cc955 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > Subject: [PATCH 07/28] SUDO: fix sdap_sudo_smart_refresh_recv()
> >
> > This was really creepy.
>        ^^^^^^^^^^^^^
>
> I hope it is just a WIP sentence.
>    Such sentence is not useful for anyone.

Sure, this line can be removed. And no, it is not left over from wip, I 
really mean it. I'm the author of the original code and it is really 
creepy :-)
> otherwise ACK
> > From c3446b2b5979e426834afa0099ef5380190f4a97 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > Subject: [PATCH 08/28] SUDO: sdap_sudo_load_sudoers improve iterator
> >
> > The old search base iterator was difficult to read since its logic
> > spread through all functions. This patch also shorten names.
> >
> > src/providers/ldap/sdap_async_sudo.c | 122 ++++++++++++++++-------------------
> > 1 file changed, 54 insertions(+), 68 deletions(-)
> >
> > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > index 259a79516cafd6f0351fa0a09d4feb1593fefb3f..71e1b3322a557beaf82b51316967b3147fb61f76 100644
> > --- a/src/providers/ldap/sdap_async_sudo.c
> > +++ b/src/providers/ldap/sdap_async_sudo.c
> > @@ -38,14 +38,15 @@ struct sdap_sudo_load_sudoers_state {
> >      struct tevent_context *ev;
> >      struct sdap_options *opts;
> >      struct sdap_handle *sh;
> > struct sysdb_attrs **ldap_rules; /* search result will be stored here */
> > size_t ldap_rules_count;         /* search result will be stored here */
> > int timeout;
>
> Could you explain why did you move "timeout" from end of structure
> to the middle?

No specific reason, it just happened. I probably temporarily removed it 
and then add it again.
> I do not see a reason why it cannot be the last.
>
> BTW it's good to groups variables with small size together or
> at the end of struct. It's mostly to avoid unnecessary padding
> added by compiler. (and thus reduced size of struct)
>
> I know that it will not have an effect here.
> But I do not understand why it had to be moved.
> > From f60af3b7bfece6e371ab7582cc23cf01d483d434 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > Subject: [PATCH 09/28] SUDO: set USN inside sdap_sudo_refresh request
> >
> > Reduce code duplication.
>
> Nice,
>
> ACK
> > From 9cac17688fd1cbd8d47d0cd7c545bea8f60e2051 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > Subject: [PATCH 10/28] SUDO: built host filter inside sdap_sudo_refresh
> > request
> >
> > Preparation for:
> > https://fedorahosted.org/sssd/ticket/2672
> >
> > src/providers/ldap/sdap_async_sudo.c   | 197 +++++++++++++++++++++++---
> > src/providers/ldap/sdap_sudo.h         |   6 +-
> > src/providers/ldap/sdap_sudo_refresh.c | 251 +++++----------------------------
> > 3 files changed, 212 insertions(+), 242 deletions(-)
> >
> > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > index 7a90ccde9cbd2efc0d0509f21e072cd745c57844..84eff3b2a99e9f7c38e49c870cd301f4f88b4d02 100644
> > --- a/src/providers/ldap/sdap_async_sudo.c
> > +++ b/src/providers/ldap/sdap_async_sudo.c
> > @@ -328,7 +328,148 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> >                               srv_opts->max_sudo_value);
> > }
> >
> > +static char *sdap_sudo_build_host_filter(TALLOC_CTX *mem_ctx,
> >                                     struct sdap_attr_map *map,
> >                                     char **hostnames,
> >                                     char **ip_addr,
> >                                     bool netgroups,
> >                                     bool regexp)
>
> //snip
> > +static char *sdap_sudo_get_filter(TALLOC_CTX *mem_ctx,
> >                              struct sdap_attr_map *map,
> >                              struct sdap_sudo_ctx *sudo_ctx,
> >                              const char *rule_filter)
> >
> > +{
>
> These functions were moved in oppsite direction few patches before.

No, it was moved from sdap_sudo.c to sdap_sudo_refresh.c. Now when it 
makes sense it is moved to sdap_sudo_async.c.
> It would be easier for review If you could make functions
> public and later private again.
> Because it's difficult to spot changes in moved function.
> > @@ -238,11 +88,6 @@ struct tevent_req *sdap_sudo_full_refresh_send(TALLOC_CTX *mem_ctx,
> >  tevent_req_set_callback(subreq, sdap_sudo_full_refresh_done, req);
> >
> > /* free filters */
> > talloc_free(ldap_filter);
> > talloc_free(ldap_full_filter);
> > talloc_free(sysdb_filter);
>
> I could not see added these lines elsewhere.
> But I might miss something.
> Could you explain it why?

It is attached to state and it takes only a small amount of memory.
> > @@ -389,10 +225,6 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx,
> >  tevent_req_set_callback(subreq, sdap_sudo_smart_refresh_done, req);
> >
> > /* free filters */
> > talloc_free(ldap_filter);
> > talloc_free(ldap_full_filter);
> > return req;
> > From f44affe1bcb0ce92ab304657af75973f7a36a968 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > Subject: [PATCH 11/28] SUDO: remove dead code from smart refresh
> >
> > We fail at the beginning if USN is NULL so it can't be NULL further
> > in the code.
> >
> > src/providers/ldap/sdap_sudo_refresh.c | 20 +++++++-------------
> > 1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/providers/ldap/sdap_sudo_refresh.c b/src/providers/ldap/sdap_sudo_refresh.c
> > index 80627f1c84cf6ef0d5b41a1368c9e0a0b7e5ed32..4f6339c5469fb8c3a01ea5179a8aa5b66355d531 100644
> > --- a/src/providers/ldap/sdap_sudo_refresh.c
> > +++ b/src/providers/ldap/sdap_sudo_refresh.c
> > @@ -182,7 +182,7 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx,
> >      }
> >  if (!sudo_ctx->full_refresh_done
> >        && (srv_opts == NULL || srv_opts->max_sudo_value == 0)) {
> >        && (srv_opts == NULL || srv_opts->max_sudo_value == NULL)) {
>
> srv_opts->max_sudo_value (and thus "usn) might be checked
> for NULL if sudo_ctx->full_refresh_done if true.

You are right, it should say or.

[Jakub Hrozek]

On Fri, Dec 11, 2015 at 11:55:04AM +0100, Pavel Březina wrote:
> On 12/04/2015 03:51 PM, Lukas Slebodnik wrote:
> > > From 38c5524b82d94db68450636020472869dc362070 Mon Sep 17 00:00:00 2001
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "Unable to setup full refresh ptask "
> > >          "[%d]: %s\n", ret, sss_strerror(ret));
> >
> > It's easeir to read debug message if the format string si on separate line
> > or is alligned in block
> >
> > e.g.
> > DEBUG(SSSDBG_CRIT_FAILURE,
> >       "Lorem Ipsum is simply dummy text of the printing and typesetting "
> >       "industry. Lorem Ipsum has been the industry's standard dummy text "
> >       "ever since the 1500s, when an unknown printer took a galley of type "
> >       "and scrambled it to make a type specimen book"
> >
> > or
> > DEBUG(SSSDBG_CRIT_FAILURE, "Lorem Ipsum is simply dummy text of the printing "
> >                            "and typesetting industry. Lorem Ipsum has been "
> >                            "the industry's standard dummy text ever since the "
> >                            "1500s, when an unknown printer took a galley of "
> >                            "type and scrambled it to make a type specimen book."
>
> I rarely align it this way since we removed brackets around message. I
> didn't touch the old messages that were converted to new format when a code
> was moved. I always use what is shorter these days.
> > I prefer 1st version becuase it is usualy shorter.
> >
> > Such issues fwith debug messages are also on other places.
> > But I will not mention them on other places.
> > It would be good if it was consistent. And you might change it
> > also in patches which has ACK.
> > > From 7646bed4c9d381e2c048e48dac45484a8e7cc955 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > Subject: [PATCH 07/28] SUDO: fix sdap_sudo_smart_refresh_recv()
> > >
> > > This was really creepy.
> >       ^^^^^^^^^^^^^
> >
> > I hope it is just a WIP sentence.
> >   Such sentence is not useful for anyone.
>
> Sure, this line can be removed. And no, it is not left over from wip, I
> really mean it. I'm the author of the original code and it is really creepy
> :-)
> > otherwise ACK
> > > From c3446b2b5979e426834afa0099ef5380190f4a97 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > Subject: [PATCH 08/28] SUDO: sdap_sudo_load_sudoers improve iterator
> > >
> > > The old search base iterator was difficult to read since its logic
> > > spread through all functions. This patch also shorten names.
> > >
> > > src/providers/ldap/sdap_async_sudo.c | 122 ++++++++++++++++-------------------
> > > 1 file changed, 54 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > index 259a79516cafd6f0351fa0a09d4feb1593fefb3f..71e1b3322a557beaf82b51316967b3147fb61f76 100644
> > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > @@ -38,14 +38,15 @@ struct sdap_sudo_load_sudoers_state {
> > >     struct tevent_context *ev;
> > >     struct sdap_options *opts;
> > >     struct sdap_handle *sh;
> > > struct sysdb_attrs **ldap_rules; /* search result will be stored here */
> > > size_t ldap_rules_count;         /* search result will be stored here */
> > > int timeout;
> >
> > Could you explain why did you move "timeout" from end of structure
> > to the middle?
>
> No specific reason, it just happened. I probably temporarily removed it and
> then add it again.
> > I do not see a reason why it cannot be the last.
> >
> > BTW it's good to groups variables with small size together or
> > at the end of struct. It's mostly to avoid unnecessary padding
> > added by compiler. (and thus reduced size of struct)
> >
> > I know that it will not have an effect here.
> > But I do not understand why it had to be moved.
> > > From f60af3b7bfece6e371ab7582cc23cf01d483d434 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > Subject: [PATCH 09/28] SUDO: set USN inside sdap_sudo_refresh request
> > >
> > > Reduce code duplication.
> >
> > Nice,
> >
> > ACK
> > > From 9cac17688fd1cbd8d47d0cd7c545bea8f60e2051 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > Subject: [PATCH 10/28] SUDO: built host filter inside sdap_sudo_refresh
> > > request
> > >
> > > Preparation for:
> > > https://fedorahosted.org/sssd/ticket/2672
> > >
> > > src/providers/ldap/sdap_async_sudo.c   | 197 +++++++++++++++++++++++---
> > > src/providers/ldap/sdap_sudo.h         |   6 +-
> > > src/providers/ldap/sdap_sudo_refresh.c | 251 +++++----------------------------
> > > 3 files changed, 212 insertions(+), 242 deletions(-)
> > >
> > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > index 7a90ccde9cbd2efc0d0509f21e072cd745c57844..84eff3b2a99e9f7c38e49c870cd301f4f88b4d02 100644
> > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > @@ -328,7 +328,148 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > >                              srv_opts->max_sudo_value);
> > > }
> > >
> > > +static char *sdap_sudo_build_host_filter(TALLOC_CTX *mem_ctx,
> > >                                     struct sdap_attr_map *map,
> > >                                     char **hostnames,
> > >                                     char **ip_addr,
> > >                                     bool netgroups,
> > >                                     bool regexp)
> >
> > //snip
> > > +static char *sdap_sudo_get_filter(TALLOC_CTX *mem_ctx,
> > >                              struct sdap_attr_map *map,
> > >                              struct sdap_sudo_ctx *sudo_ctx,
> > >                              const char *rule_filter)
> > >
> > > +{
> >
> > These functions were moved in oppsite direction few patches before.
>
> No, it was moved from sdap_sudo.c to sdap_sudo_refresh.c. Now when it makes
> sense it is moved to sdap_sudo_async.c.
> > It would be easier for review If you could make functions
> > public and later private again.
> > Because it's difficult to spot changes in moved function.
> > > @@ -238,11 +88,6 @@ struct tevent_req *sdap_sudo_full_refresh_send(TALLOC_CTX *mem_ctx,
> > > tevent_req_set_callback(subreq, sdap_sudo_full_refresh_done, req);
> > >
> > > /* free filters */
> > > talloc_free(ldap_filter);
> > > talloc_free(ldap_full_filter);
> > > talloc_free(sysdb_filter);
> >
> > I could not see added these lines elsewhere.
> > But I might miss something.
> > Could you explain it why?
>
> It is attached to state and it takes only a small amount of memory.
> > > @@ -389,10 +225,6 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx,
> > > tevent_req_set_callback(subreq, sdap_sudo_smart_refresh_done, req);
> > >
> > > /* free filters */
> > > talloc_free(ldap_filter);
> > > talloc_free(ldap_full_filter);
> > > return req;
> > > From f44affe1bcb0ce92ab304657af75973f7a36a968 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > Subject: [PATCH 11/28] SUDO: remove dead code from smart refresh
> > >
> > > We fail at the beginning if USN is NULL so it can't be NULL further
> > > in the code.
> > >
> > > src/providers/ldap/sdap_sudo_refresh.c | 20 +++++++-------------
> > > 1 file changed, 7 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/providers/ldap/sdap_sudo_refresh.c b/src/providers/ldap/sdap_sudo_refresh.c
> > > index 80627f1c84cf6ef0d5b41a1368c9e0a0b7e5ed32..4f6339c5469fb8c3a01ea5179a8aa5b66355d531 100644
> > > --- a/src/providers/ldap/sdap_sudo_refresh.c
> > > +++ b/src/providers/ldap/sdap_sudo_refresh.c
> > > @@ -182,7 +182,7 @@ struct tevent_req *sdap_sudo_smart_refresh_send(TALLOC_CTX *mem_ctx,
> > >     }
> > > if (!sudo_ctx->full_refresh_done
> > >        && (srv_opts == NULL || srv_opts->max_sudo_value == 0)) {
> > >        && (srv_opts == NULL || srv_opts->max_sudo_value == NULL)) {
> >
> > srv_opts->max_sudo_value (and thus "usn) might be checked
> > for NULL if sudo_ctx->full_refresh_done if true.
>
> You are right, it should say or.
> From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 6 Nov 2015 13:00:47 +0100
> Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask

LGTM, not tested yet. One question inline...
> This removes old sudo timer and simplyfies code a lot. It also
> allows to manage offline/online state.
> Full and smart refresh are disabled when offline.
> Full refresh is run immediately when sssd is back online.
> Smart refresh is scheduled normally when sssd is back online.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1943

I agree with the logic, but it's different from the original code, so is
the change intended?
> if (smart == 0 && full == 0) {
>    /* We don't allow both types to be disabled. At least smart refresh
>     * needs to be enabled. In this case smart refresh will catch up new
>     * and modified rules and deleted rules are caught when expired. */
>    smart = opts[SDAP_SUDO_SMART_REFRESH_INTERVAL].def_val.number;
>    DEBUG(SSSDBG_CONF_SETTINGS, "At least smart refresh needs to be "
>          "enabled. Setting smart refresh interval to default value "
>          "(%ld) seconds.\n", smart);
> } else if (full <= smart) {
>    /* In this case it does not make any sense to run smart refresh. */
>    smart = 0;
>    DEBUG(SSSDBG_CONF_SETTINGS, "Smart refresh interval has to be lower "
>          "than full refresh interval. Periodical smart refresh will be "
>          "disabled.\n");
> }
> From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Fri, 6 Nov 2015 13:32:34 +0100
> Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
>  sdap_sudo_refresh.c

LGTM, I wonder why git doesn't show the changes as move since such a
substantial part of sdap_sudo.c changed?
> From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 12:26:07 +0100
> Subject: [PATCH 03/35] SUDO: move offline check to handler
>
> We let sdap_id_op decide if we are offline or not here but we
> should not get to this code since ptask is disabled and we will
> not get through sudo handler if offline.

LGTM, the connection error would still take us offline, which is what we
want.
> From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 12:44:13 +0100
> Subject: [PATCH 04/35] SUDO: simplify error handling

LGTM
> From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 12:52:35 +0100
> Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic

LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
fixed in another patch. If you end up changing this patchset, it might
also be better to fix the "i" iterators to use size_t instead of int,
but if you don't change them, it's fine.
> From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 13:34:45 +0100
> Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
>
> This fix huge violation of tevent coding style.

LGTM
> From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Mon, 9 Nov 2015 13:59:48 +0100
> Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
>
> The old search base iterator was difficult to read since its logic
> spread through all functions. This patch also shorten names.

LGTM
> From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 10:34:41 +0100
> Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> Reduce code duplication.
>
> src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
>  src/providers/ldap/sdap_sudo.h         |  2 +-
>  src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
>  3 files changed, 49 insertions(+), 60 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> --- a/src/providers/ldap/sdap_async_sudo.c
> +++ b/src/providers/ldap/sdap_async_sudo.c
> @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
>  /* Empty sudoers? Done. */
>  if (rules_count == 0 || rules == NULL) {
>    *_usn = NULL;
>    return EOK;
> }
>
> @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
>      return EOK;
>  }
>
> +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> +{
> unsigned int usn_number;
> char *endptr = NULL;
> if (usn == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
>    return;
> }
> if (srv_opts == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
>    return;
> }
> talloc_zfree(srv_opts->max_sudo_value);
> srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> 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;
> }
> DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
>                         srv_opts->max_sudo_value);
>
> +}

What is the logic behind this function? Why do we store the USN both as
a string unconditionally and as a number conditionally? Wouldn't it make
sense to just store the data in one format?
> From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 11:31:50 +0100
> Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
>  request
>
> Preparation for:
> https://fedorahosted.org/sssd/ticket/2672

LGTM
> From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 11:34:14 +0100
> Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
>
> USN should never be unknown when we run smart refresh so this is
> more a safety condition for a code bug. Nevertheless, we should
> not immitate full refresh here under those conditions.

LGTM
> From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 12:34:11 +0100
> Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init

LGTM
> From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 10 Nov 2015 12:32:16 +0100
> Subject: [PATCH 13/35] SUDO: obtain host information when going online
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2672

[...]
> +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> +{
> struct sdap_sudo_ctx *sudo_ctx;
> struct sdap_sudo_refresh_state *state;
> struct tevent_req *req;
> errno_t ret;
> req = tevent_req_callback_data(subreq, struct tevent_req);
> state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> sudo_ctx = state->sudo_ctx;
> ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
>                                  &sudo_ctx->ip_addr);
> talloc_zfree(subreq);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
>                             "host filter will be disabled [%d]: %s\n",
>                             ret, sss_strerror(ret));
>    sudo_ctx->use_host_filter = false;
> }

I think you forgot an else here..
> sudo_ctx->use_host_filter = true;
> ret = sdap_sudo_refresh_sudoers(req);
> if (ret != EAGAIN) {
>    state->dp_error = DP_ERR_FATAL;
>    tevent_req_error(req, ret);
> }
>
> +}
> From c8106caf2c5a40560c95f7d763bc2815dd161c42 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 25 Nov 2015 13:16:17 +0100
> Subject: [PATCH 14/35] SUDO: remove finalizer
>
> It is not used anywhere anyway.

LGTM
> From 52c2be3a24a0a7f48e8fe95c4758276eb6083a9b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 25 Nov 2015 13:21:49 +0100
> Subject: [PATCH 15/35] SUDO: make sdap_sudo_handler static

LGTM

[Pavel Březina]

On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
>
> LGTM, not tested yet. One question inline...
> > This removes old sudo timer and simplyfies code a lot. It also
> > allows to manage offline/online state.
> > Full and smart refresh are disabled when offline.
> > Full refresh is run immediately when sssd is back online.
> > Smart refresh is scheduled normally when sssd is back online.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1943
>
> I agree with the logic, but it's different from the original code, so is
> the change intended?

Yes, the functionality remains the same but we handle online/offline 
transition much better here.
> > From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> >   sdap_sudo_refresh.c
>
> LGTM, I wonder why git doesn't show the changes as move since such a
> substantial part of sdap_sudo.c changed?

Dunno.
> > From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > Subject: [PATCH 03/35] SUDO: move offline check to handler
> >
> > We let sdap_id_op decide if we are offline or not here but we
> > should not get to this code since ptask is disabled and we will
> > not get through sudo handler if offline.
>
> LGTM, the connection error would still take us offline, which is what we
> want.
> > From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > Subject: [PATCH 04/35] SUDO: simplify error handling
>
> LGTM
> > From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
>
> LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> fixed in another patch. If you end up changing this patchset, it might
> also be better to fix the "i" iterators to use size_t instead of int,
> but if you don't change them, it's fine.

Ok, new patch is attached to the patch set. Thanks.
> > From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> >
> > This fix huge violation of tevent coding style.
>
> LGTM
> > From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> >
> > The old search base iterator was difficult to read since its logic
> > spread through all functions. This patch also shorten names.
>
> LGTM
> > From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > Reduce code duplication.
> >
> > src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> >   src/providers/ldap/sdap_sudo.h         |  2 +-
> >   src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> >   3 files changed, 49 insertions(+), 60 deletions(-)
> >
> > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > --- a/src/providers/ldap/sdap_async_sudo.c
> > +++ b/src/providers/ldap/sdap_async_sudo.c
> > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> >   /* Empty sudoers? Done. */
> >   if (rules_count == 0 || rules == NULL) {
> >    *_usn = NULL;
> >     return EOK;
> > }
> >
> > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> >       return EOK;
> >   }
> >
> > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > +{
> > unsigned int usn_number;
> > char *endptr = NULL;
> > if (usn == NULL) {
> >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> >    return;
> > }
> > if (srv_opts == NULL) {
> >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> >    return;
> > }
> > talloc_zfree(srv_opts->max_sudo_value);
> > srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > 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;
> > }
> > DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> >                         srv_opts->max_sudo_value);
> >
> > +}
>
> What is the logic behind this function? Why do we store the USN both as
> a string unconditionally and as a number conditionally? Wouldn't it make
> sense to just store the data in one format?

max_sudo_value is internal to sudo, it is max value found in sudo 
subtree. It's string since it is not needed to be a number. last_usn is 
the largers value on the whole server, it needs to be compared.

max_sudo_value can be a number also, but it would be inconsistent with 
other providers.
> > From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> >   request
> >
> > Preparation for:
> > https://fedorahosted.org/sssd/ticket/2672
>
> LGTM
> > From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> >
> > USN should never be unknown when we run smart refresh so this is
> > more a safety condition for a code bug. Nevertheless, we should
> > not immitate full refresh here under those conditions.
>
> LGTM
> > From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
>
> LGTM
> > From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2672
>
> [...]
> > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > +{
> > struct sdap_sudo_ctx *sudo_ctx;
> > struct sdap_sudo_refresh_state *state;
> > struct tevent_req *req;
> > errno_t ret;
> > req = tevent_req_callback_data(subreq, struct tevent_req);
> > state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > sudo_ctx = state->sudo_ctx;
> > ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> >                                  &sudo_ctx->ip_addr);
> > talloc_zfree(subreq);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> >                             "host filter will be disabled [%d]: %s\n",
> >                             ret, sss_strerror(ret));
> >    sudo_ctx->use_host_filter = false;
> > }
>
> I think you forgot an else here..

Yes. thanks.
> > sudo_ctx->use_host_filter = true;
> > ret = sdap_sudo_refresh_sudoers(req);
> > if (ret != EAGAIN) {
> >    state->dp_error = DP_ERR_FATAL;
> >    tevent_req_error(req, ret);
> > }
> >
> > +}
> > From c8106caf2c5a40560c95f7d763bc2815dd161c42 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 25 Nov 2015 13:16:17 +0100
> > Subject: [PATCH 14/35] SUDO: remove finalizer
> >
> > It is not used anywhere anyway.
>
> LGTM
> > From 52c2be3a24a0a7f48e8fe95c4758276eb6083a9b Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 25 Nov 2015 13:21:49 +0100
> > Subject: [PATCH 15/35] SUDO: make sdap_sudo_handler static
>
> LGTM
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Lukas Slebodnik]

On (11/12/15 15:56), Pavel Březina wrote:
> On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> >
> > LGTM, not tested yet. One question inline...
> > > This removes old sudo timer and simplyfies code a lot. It also
> > > allows to manage offline/online state.
> > > Full and smart refresh are disabled when offline.
> > > Full refresh is run immediately when sssd is back online.
> > > Smart refresh is scheduled normally when sssd is back online.
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/1943
> >
> > I agree with the logic, but it's different from the original code, so is
> > the change intended?
>
> Yes, the functionality remains the same but we handle online/offline
> transition much better here.
> > > From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > >  sdap_sudo_refresh.c
> >
> > LGTM, I wonder why git doesn't show the changes as move since such a
> > substantial part of sdap_sudo.c changed?
>
> Dunno.
> > > From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > Subject: [PATCH 03/35] SUDO: move offline check to handler
> > >
> > > We let sdap_id_op decide if we are offline or not here but we
> > > should not get to this code since ptask is disabled and we will
> > > not get through sudo handler if offline.
> >
> > LGTM, the connection error would still take us offline, which is what we
> > want.
> > > From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > Subject: [PATCH 04/35] SUDO: simplify error handling
> >
> > LGTM
> > > From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> >
> > LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > fixed in another patch. If you end up changing this patchset, it might
> > also be better to fix the "i" iterators to use size_t instead of int,
> > but if you don't change them, it's fine.
>
> Ok, new patch is attached to the patch set. Thanks.
> > > From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > >
> > > This fix huge violation of tevent coding style.
> >
> > LGTM
> > > From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > >
> > > The old search base iterator was difficult to read since its logic
> > > spread through all functions. This patch also shorten names.
> >
> > LGTM
> > > From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > Reduce code duplication.
> > >
> > > src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > >  src/providers/ldap/sdap_sudo.h         |  2 +-
> > >  src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > >  3 files changed, 49 insertions(+), 60 deletions(-)
> > >
> > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > >  /* Empty sudoers? Done. */
> > >  if (rules_count == 0 || rules == NULL) {
> > >    *_usn = NULL;
> > >    return EOK;
> > > }
> > >
> > > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > >      return EOK;
> > >  }
> > >
> > > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > +{
> > > unsigned int usn_number;
> > > char *endptr = NULL;
> > > if (usn == NULL) {
> > >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > >    return;
> > > }
> > > if (srv_opts == NULL) {
> > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > >    return;
> > > }
> > > talloc_zfree(srv_opts->max_sudo_value);
> > > srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > 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;
> > > }
> > > DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > >                         srv_opts->max_sudo_value);
> > >
> > > +}
> >
> > What is the logic behind this function? Why do we store the USN both as
> > a string unconditionally and as a number conditionally? Wouldn't it make
> > sense to just store the data in one format?
>
> max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> It's string since it is not needed to be a number. last_usn is the largers
> value on the whole server, it needs to be compared.
>
> max_sudo_value can be a number also, but it would be inconsistent with other
> providers.
> > > From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > >  request
> > >
> > > Preparation for:
> > > https://fedorahosted.org/sssd/ticket/2672
> >
> > LGTM
> > > From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > >
> > > USN should never be unknown when we run smart refresh so this is
> > > more a safety condition for a code bug. Nevertheless, we should
> > > not immitate full refresh here under those conditions.
> >
> > LGTM
> > > From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> >
> > LGTM
> > > From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2672
> >
> > [...]
> > > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > +{
> > > struct sdap_sudo_ctx *sudo_ctx;
> > > struct sdap_sudo_refresh_state *state;
> > > struct tevent_req *req;
> > > errno_t ret;
> > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > sudo_ctx = state->sudo_ctx;
> > > ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > >                                  &sudo_ctx->ip_addr);
> > > talloc_zfree(subreq);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > >                             "host filter will be disabled [%d]: %s\n",
> > >                             ret, sss_strerror(ret));
> > >    sudo_ctx->use_host_filter = false;
> > > }
> >
> > I think you forgot an else here..
>
> Yes. thanks.

We had a discussion on IRC with Pavel and Jakub
and patch set caused a bug. The smart refresh was not executed
if USN was NULL. (full refresh did not found any sudo rules)

e.g.
* install freeIPA (sssd is started)
* add sudo rules
* sudo rules will be downloaded after full refresh
  (which is 1 day by default or after restarting sssd)

LS

[Pavel Březina]

On 12/14/2015 12:05 PM, Lukas Slebodnik wrote:
> On (11/12/15 15:56), Pavel Březina wrote:
> > On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > > From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> > >
> > > LGTM, not tested yet. One question inline...
> > > > This removes old sudo timer and simplyfies code a lot. It also
> > > > allows to manage offline/online state.
> > > > Full and smart refresh are disabled when offline.
> > > > Full refresh is run immediately when sssd is back online.
> > > > Smart refresh is scheduled normally when sssd is back online.
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/1943
> > >
> > > I agree with the logic, but it's different from the original code, so is
> > > the change intended?
> >
> > Yes, the functionality remains the same but we handle online/offline
> > transition much better here.
> > > > From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > > >   sdap_sudo_refresh.c
> > >
> > > LGTM, I wonder why git doesn't show the changes as move since such a
> > > substantial part of sdap_sudo.c changed?
> >
> > Dunno.
> > > > From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > > Subject: [PATCH 03/35] SUDO: move offline check to handler
> > > >
> > > > We let sdap_id_op decide if we are offline or not here but we
> > > > should not get to this code since ptask is disabled and we will
> > > > not get through sudo handler if offline.
> > >
> > > LGTM, the connection error would still take us offline, which is what we
> > > want.
> > > > From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > > Subject: [PATCH 04/35] SUDO: simplify error handling
> > >
> > > LGTM
> > > > From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> > >
> > > LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > > fixed in another patch. If you end up changing this patchset, it might
> > > also be better to fix the "i" iterators to use size_t instead of int,
> > > but if you don't change them, it's fine.
> >
> > Ok, new patch is attached to the patch set. Thanks.
> > > > From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > > >
> > > > This fix huge violation of tevent coding style.
> > >
> > > LGTM
> > > > From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > > >
> > > > The old search base iterator was difficult to read since its logic
> > > > spread through all functions. This patch also shorten names.
> > >
> > > LGTM
> > > > From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > > Reduce code duplication.
> > > >
> > > > src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > > >   src/providers/ldap/sdap_sudo.h         |  2 +-
> > > >   src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > > >   3 files changed, 49 insertions(+), 60 deletions(-)
> > > >
> > > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > >   /* Empty sudoers? Done. */
> > > >   if (rules_count == 0 || rules == NULL) {
> > > >    *_usn = NULL;
> > > >     return EOK;
> > > > }
> > > >
> > > > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > >       return EOK;
> > > >   }
> > > >
> > > > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > > +{
> > > > unsigned int usn_number;
> > > > char *endptr = NULL;
> > > > if (usn == NULL) {
> > > >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > >    return;
> > > > }
> > > > if (srv_opts == NULL) {
> > > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > >    return;
> > > > }
> > > > talloc_zfree(srv_opts->max_sudo_value);
> > > > srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > > 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;
> > > > }
> > > > DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > > >                         srv_opts->max_sudo_value);
> > > >
> > > > +}
> > >
> > > What is the logic behind this function? Why do we store the USN both as
> > > a string unconditionally and as a number conditionally? Wouldn't it make
> > > sense to just store the data in one format?
> >
> > max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> > It's string since it is not needed to be a number. last_usn is the largers
> > value on the whole server, it needs to be compared.
> >
> > max_sudo_value can be a number also, but it would be inconsistent with other
> > providers.
> > > > From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > > >   request
> > > >
> > > > Preparation for:
> > > > https://fedorahosted.org/sssd/ticket/2672
> > >
> > > LGTM
> > > > From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > > >
> > > > USN should never be unknown when we run smart refresh so this is
> > > > more a safety condition for a code bug. Nevertheless, we should
> > > > not immitate full refresh here under those conditions.
> > >
> > > LGTM
> > > > From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> > >
> > > LGTM
> > > > From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2672
> > >
> > > [...]
> > > > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > > +{
> > > > struct sdap_sudo_ctx *sudo_ctx;
> > > > struct sdap_sudo_refresh_state *state;
> > > > struct tevent_req *req;
> > > > errno_t ret;
> > > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > > sudo_ctx = state->sudo_ctx;
> > > > ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > > >                                  &sudo_ctx->ip_addr);
> > > > talloc_zfree(subreq);
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > > >                             "host filter will be disabled [%d]: %s\n",
> > > >                             ret, sss_strerror(ret));
> > > >    sudo_ctx->use_host_filter = false;
> > > > }
> > >
> > > I think you forgot an else here..
> >
> > Yes. thanks.
>
> We had a discussion on IRC with Pavel and Jakub
> and patch set caused a bug. The smart refresh was not executed
> if USN was NULL. (full refresh did not found any sudo rules)
>
> e.g.
> install freeIPA (sssd is started)
> add sudo rules
> sudo rules will be downloaded after full refresh
>  (which is 1 day by default or after restarting sssd)
>
> LS

I have changed the patch 0009 in a way so we do not imitate full refresh 
in smart refresh but still be able to run smart refresh if no rules were 
found.

[Lukas Slebodnik]

On (14/12/15 12:38), Pavel Březina wrote:
> On 12/14/2015 12:05 PM, Lukas Slebodnik wrote:
> > On (11/12/15 15:56), Pavel Březina wrote:
> > > On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > > > From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > > > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> > > >
> > > > LGTM, not tested yet. One question inline...
> > > > > This removes old sudo timer and simplyfies code a lot. It also
> > > > > allows to manage offline/online state.
> > > > > Full and smart refresh are disabled when offline.
> > > > > Full refresh is run immediately when sssd is back online.
> > > > > Smart refresh is scheduled normally when sssd is back online.
> > > > >
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/1943
> > > >
> > > > I agree with the logic, but it's different from the original code, so is
> > > > the change intended?
> > >
> > > Yes, the functionality remains the same but we handle online/offline
> > > transition much better here.
> > > > > From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > > > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > > > >  sdap_sudo_refresh.c
> > > >
> > > > LGTM, I wonder why git doesn't show the changes as move since such a
> > > > substantial part of sdap_sudo.c changed?
> > >
> > > Dunno.
> > > > > From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > > > Subject: [PATCH 03/35] SUDO: move offline check to handler
> > > > >
> > > > > We let sdap_id_op decide if we are offline or not here but we
> > > > > should not get to this code since ptask is disabled and we will
> > > > > not get through sudo handler if offline.
> > > >
> > > > LGTM, the connection error would still take us offline, which is what we
> > > > want.
> > > > > From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > > > Subject: [PATCH 04/35] SUDO: simplify error handling
> > > >
> > > > LGTM
> > > > > From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > > > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> > > >
> > > > LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > > > fixed in another patch. If you end up changing this patchset, it might
> > > > also be better to fix the "i" iterators to use size_t instead of int,
> > > > but if you don't change them, it's fine.
> > >
> > > Ok, new patch is attached to the patch set. Thanks.
> > > > > From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > > > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > > > >
> > > > > This fix huge violation of tevent coding style.
> > > >
> > > > LGTM
> > > > > From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > > > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > > > >
> > > > > The old search base iterator was difficult to read since its logic
> > > > > spread through all functions. This patch also shorten names.
> > > >
> > > > LGTM
> > > > > From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > > > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > > > Reduce code duplication.
> > > > >
> > > > > src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > > > >  src/providers/ldap/sdap_sudo.h         |  2 +-
> > > > >  src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > > > >  3 files changed, 49 insertions(+), 60 deletions(-)
> > > > >
> > > > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > > > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > > > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > >  /* Empty sudoers? Done. */
> > > > >  if (rules_count == 0 || rules == NULL) {
> > > > >    *_usn = NULL;
> > > > >    return EOK;
> > > > > }
> > > > >
> > > > > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > >      return EOK;
> > > > >  }
> > > > >
> > > > > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > > > +{
> > > > > unsigned int usn_number;
> > > > > char *endptr = NULL;
> > > > > if (usn == NULL) {
> > > > >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > > >    return;
> > > > > }
> > > > > if (srv_opts == NULL) {
> > > > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > > >    return;
> > > > > }
> > > > > talloc_zfree(srv_opts->max_sudo_value);
> > > > > srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > > > 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;
> > > > > }
> > > > > DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > > > >                         srv_opts->max_sudo_value);
> > > > >
> > > > > +}
> > > >
> > > > What is the logic behind this function? Why do we store the USN both as
> > > > a string unconditionally and as a number conditionally? Wouldn't it make
> > > > sense to just store the data in one format?
> > >
> > > max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> > > It's string since it is not needed to be a number. last_usn is the largers
> > > value on the whole server, it needs to be compared.
> > >
> > > max_sudo_value can be a number also, but it would be inconsistent with other
> > > providers.
> > > > > From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > > > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > > > >  request
> > > > >
> > > > > Preparation for:
> > > > > https://fedorahosted.org/sssd/ticket/2672
> > > >
> > > > LGTM
> > > > > From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > > > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > > > >
> > > > > USN should never be unknown when we run smart refresh so this is
> > > > > more a safety condition for a code bug. Nevertheless, we should
> > > > > not immitate full refresh here under those conditions.
> > > >
> > > > LGTM
> > > > > From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > > > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> > > >
> > > > LGTM
> > > > > From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > > > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > > > >
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/2672
> > > >
> > > > [...]
> > > > > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > > > +{
> > > > > struct sdap_sudo_ctx *sudo_ctx;
> > > > > struct sdap_sudo_refresh_state *state;
> > > > > struct tevent_req *req;
> > > > > errno_t ret;
> > > > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > > state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > > > sudo_ctx = state->sudo_ctx;
> > > > > ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > > > >                                  &sudo_ctx->ip_addr);
> > > > > talloc_zfree(subreq);
> > > > > if (ret != EOK) {
> > > > >    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > > > >                             "host filter will be disabled [%d]: %s\n",
> > > > >                             ret, sss_strerror(ret));
> > > > >    sudo_ctx->use_host_filter = false;
> > > > > }
> > > >
> > > > I think you forgot an else here..
> > >
> > > Yes. thanks.
> >
> > We had a discussion on IRC with Pavel and Jakub
> > and patch set caused a bug. The smart refresh was not executed
> > if USN was NULL. (full refresh did not found any sudo rules)
> >
> > e.g.
> > install freeIPA (sssd is started)
> > add sudo rules
> > sudo rules will be downloaded after full refresh
> > (which is 1 day by default or after restarting sssd)
> >
> > LS
>
> I have changed the patch 0009 in a way so we do not imitate full refresh in
> smart refresh but still be able to run smart refresh if no rules were found.

Here is a diff:
diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
index 9f66d6c..13824c5 100644
--- a/src/providers/ldap/sdap_async_sudo.c
+++ b/src/providers/ldap/sdap_async_sudo.c
@@ -305,13 +305,21 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
     unsigned int usn_number;
     char *endptr = NULL;

-    if (usn == NULL) {
-        DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
+    if (srv_opts == NULL) {
+        DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
         return;
     }

-    if (srv_opts == NULL) {
-        DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
+    if (usn == NULL) {
+        /* If the USN value is unknown and we don't have max_sudo_value set
+         * (possibly first full refresh which did not find any rule) we will
+         * set zero so smart refresh can pick up. */
+        if (srv_opts->max_sudo_value == NULL) {
+            srv_opts->max_sudo_value = "0";
+            return;
+        }
+
+        DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
         return;
     }

@@ -748,9 +756,7 @@ static void sdap_sudo_refresh_done(struct tevent_req *subreq)
     DEBUG(SSSDBG_TRACE_FUNC, "Sudoers is successfuly stored in cache\n");

/* remember new usn */
-    if (usn != NULL) {
-        sdap_sudo_set_usn(state->srv_opts, usn);
-    }
+    sdap_sudo_set_usn(state->srv_opts, usn);

ret = EOK;
     state->num_rules = rules_count;

But unfortunatelly it does not work.
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_load_sudoers_done] (0x0400): Receiving sudo rules with base [dc=example,dc=com]
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_id_op_done] (0x4000): releasing operation connection
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_refresh_done] (0x0400): Received 0 rules
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sysdb_sudo_purge_byfilter] (0x0400): No rules matched
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [ldb] (0x4000): commit ldb transaction (nesting: 0)
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_refresh_done] (0x0400): Sudoers is successfuly stored in cache
(Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_set_usn] (0x0400): Bug: srv_opts is NULL

Result is that max_sudo_value was not stored
and smart refresh was not executed.

(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_execute] (0x0400): Task [SUDO Smart Refresh]: executing task, timeout 1 seconds
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [sdap_sudo_smart_refresh_send] (0x0400): USN value is unknown, waiting for full refresh!
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_done] (0x0040): Task [SUDO Smart Refresh]: failed with [22]: Invalid argument
(Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_schedule] (0x0400): Task [SUDO Smart Refresh]: scheduling task 1 seconds from now [1450095792]

LS

[Pavel Březina]

On 12/14/2015 01:34 PM, Lukas Slebodnik wrote:
> On (14/12/15 12:38), Pavel Březina wrote:
> > On 12/14/2015 12:05 PM, Lukas Slebodnik wrote:
> > > On (11/12/15 15:56), Pavel Březina wrote:
> > > > On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > > > > From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > > > > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> > > > >
> > > > > LGTM, not tested yet. One question inline...
> > > > > > This removes old sudo timer and simplyfies code a lot. It also
> > > > > > allows to manage offline/online state.
> > > > > > Full and smart refresh are disabled when offline.
> > > > > > Full refresh is run immediately when sssd is back online.
> > > > > > Smart refresh is scheduled normally when sssd is back online.
> > > > > >
> > > > > > Resolves:
> > > > > > https://fedorahosted.org/sssd/ticket/1943
> > > > >
> > > > > I agree with the logic, but it's different from the original code, so is
> > > > > the change intended?
> > > >
> > > > Yes, the functionality remains the same but we handle online/offline
> > > > transition much better here.
> > > > > > From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > > > > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > > > > >   sdap_sudo_refresh.c
> > > > >
> > > > > LGTM, I wonder why git doesn't show the changes as move since such a
> > > > > substantial part of sdap_sudo.c changed?
> > > >
> > > > Dunno.
> > > > > > From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > > > > Subject: [PATCH 03/35] SUDO: move offline check to handler
> > > > > >
> > > > > > We let sdap_id_op decide if we are offline or not here but we
> > > > > > should not get to this code since ptask is disabled and we will
> > > > > > not get through sudo handler if offline.
> > > > >
> > > > > LGTM, the connection error would still take us offline, which is what we
> > > > > want.
> > > > > > From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > > > > Subject: [PATCH 04/35] SUDO: simplify error handling
> > > > >
> > > > > LGTM
> > > > > > From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > > > > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> > > > >
> > > > > LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > > > > fixed in another patch. If you end up changing this patchset, it might
> > > > > also be better to fix the "i" iterators to use size_t instead of int,
> > > > > but if you don't change them, it's fine.
> > > >
> > > > Ok, new patch is attached to the patch set. Thanks.
> > > > > > From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > > > > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > > > > >
> > > > > > This fix huge violation of tevent coding style.
> > > > >
> > > > > LGTM
> > > > > > From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > > > > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > > > > >
> > > > > > The old search base iterator was difficult to read since its logic
> > > > > > spread through all functions. This patch also shorten names.
> > > > >
> > > > > LGTM
> > > > > > From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > > > > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > > > > Reduce code duplication.
> > > > > >
> > > > > > src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > > > > >   src/providers/ldap/sdap_sudo.h         |  2 +-
> > > > > >   src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > > > > >   3 files changed, 49 insertions(+), 60 deletions(-)
> > > > > >
> > > > > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > > > > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > > > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > > > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > > > > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > >   /* Empty sudoers? Done. */
> > > > > >   if (rules_count == 0 || rules == NULL) {
> > > > > >    *_usn = NULL;
> > > > > >     return EOK;
> > > > > > }
> > > > > >
> > > > > > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > >       return EOK;
> > > > > >   }
> > > > > >
> > > > > > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > > > > +{
> > > > > > unsigned int usn_number;
> > > > > > char *endptr = NULL;
> > > > > > if (usn == NULL) {
> > > > > >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > > > >    return;
> > > > > > }
> > > > > > if (srv_opts == NULL) {
> > > > > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > > > >    return;
> > > > > > }
> > > > > > talloc_zfree(srv_opts->max_sudo_value);
> > > > > > srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > > > > 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;
> > > > > > }
> > > > > > DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > > > > >                         srv_opts->max_sudo_value);
> > > > > >
> > > > > > +}
> > > > >
> > > > > What is the logic behind this function? Why do we store the USN both as
> > > > > a string unconditionally and as a number conditionally? Wouldn't it make
> > > > > sense to just store the data in one format?
> > > >
> > > > max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> > > > It's string since it is not needed to be a number. last_usn is the largers
> > > > value on the whole server, it needs to be compared.
> > > >
> > > > max_sudo_value can be a number also, but it would be inconsistent with other
> > > > providers.
> > > > > > From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > > > > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > > > > >   request
> > > > > >
> > > > > > Preparation for:
> > > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > >
> > > > > LGTM
> > > > > > From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > > > > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > > > > >
> > > > > > USN should never be unknown when we run smart refresh so this is
> > > > > > more a safety condition for a code bug. Nevertheless, we should
> > > > > > not immitate full refresh here under those conditions.
> > > > >
> > > > > LGTM
> > > > > > From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > > > > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> > > > >
> > > > > LGTM
> > > > > > From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > > > > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > > > > >
> > > > > > Resolves:
> > > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > >
> > > > > [...]
> > > > > > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > > > > +{
> > > > > > struct sdap_sudo_ctx *sudo_ctx;
> > > > > > struct sdap_sudo_refresh_state *state;
> > > > > > struct tevent_req *req;
> > > > > > errno_t ret;
> > > > > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > > > state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > > > > sudo_ctx = state->sudo_ctx;
> > > > > > ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > > > > >                                  &sudo_ctx->ip_addr);
> > > > > > talloc_zfree(subreq);
> > > > > > if (ret != EOK) {
> > > > > >    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > > > > >                             "host filter will be disabled [%d]: %s\n",
> > > > > >                             ret, sss_strerror(ret));
> > > > > >    sudo_ctx->use_host_filter = false;
> > > > > > }
> > > > >
> > > > > I think you forgot an else here..
> > > >
> > > > Yes. thanks.
> > >
> > > We had a discussion on IRC with Pavel and Jakub
> > > and patch set caused a bug. The smart refresh was not executed
> > > if USN was NULL. (full refresh did not found any sudo rules)
> > >
> > > e.g.
> > > install freeIPA (sssd is started)
> > > add sudo rules
> > > sudo rules will be downloaded after full refresh
> > >  (which is 1 day by default or after restarting sssd)
> > >
> > > LS
> >
> > I have changed the patch 0009 in a way so we do not imitate full refresh in
> > smart refresh but still be able to run smart refresh if no rules were found.
>
> Here is a diff:
> diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> index 9f66d6c..13824c5 100644
> --- a/src/providers/ldap/sdap_async_sudo.c
> +++ b/src/providers/ldap/sdap_async_sudo.c
> @@ -305,13 +305,21 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
>       unsigned int usn_number;
>       char *endptr = NULL;
> if (usn == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> if (srv_opts == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
>     return;
> }
>
> if (srv_opts == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> if (usn == NULL) {
>    /* If the USN value is unknown and we don't have max_sudo_value set
>     * (possibly first full refresh which did not find any rule) we will
>     * set zero so smart refresh can pick up. */
>    if (srv_opts->max_sudo_value == NULL) {
>        srv_opts->max_sudo_value = "0";
>        return;
>    }
>    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
>     return;
> }
>
> @@ -748,9 +756,7 @@ static void sdap_sudo_refresh_done(struct tevent_req *subreq)
>       DEBUG(SSSDBG_TRACE_FUNC, "Sudoers is successfuly stored in cache\n");
>   /* remember new usn */
> if (usn != NULL) {
>    sdap_sudo_set_usn(state->srv_opts, usn);
> }
>
> sdap_sudo_set_usn(state->srv_opts, usn);
>
> ret = EOK;
>  state->num_rules = rules_count;
>
> But unfortunatelly it does not work.
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_load_sudoers_done] (0x0400): Receiving sudo rules with base [dc=example,dc=com]
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_id_op_done] (0x4000): releasing operation connection
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_refresh_done] (0x0400): Received 0 rules
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sysdb_sudo_purge_byfilter] (0x0400): No rules matched
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [ldb] (0x4000): commit ldb transaction (nesting: 0)
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_refresh_done] (0x0400): Sudoers is successfuly stored in cache
> (Mon Dec 14 07:23:04 2015) [sssd[be[LDAP]]] [sdap_sudo_set_usn] (0x0400): Bug: srv_opts is NULL
>
> Result is that max_sudo_value was not stored
> and smart refresh was not executed.
>
> (Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_execute] (0x0400): Task [SUDO Smart Refresh]: executing task, timeout 1 seconds
> (Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [sdap_sudo_smart_refresh_send] (0x0400): USN value is unknown, waiting for full refresh!
> (Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_done] (0x0040): Task [SUDO Smart Refresh]: failed with [22]: Invalid argument
> (Mon Dec 14 07:23:11 2015) [sssd[be[LDAP]]] [be_ptask_schedule] (0x0400): Task [SUDO Smart Refresh]: scheduling task 1 seconds from now [1450095792]

Thanks. It is a race condition... if sudo is the first to connect 
srv_opts may be NULL. I added it as a new patch.

[Lukas Slebodnik]

On (14/12/15 13:34), Lukas Slebodnik wrote:
> On (14/12/15 12:38), Pavel Březina wrote:
> > On 12/14/2015 12:05 PM, Lukas Slebodnik wrote:
> > > On (11/12/15 15:56), Pavel Březina wrote:
> > > > On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > > > > From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > > > > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> > > > >
> > > > > LGTM, not tested yet. One question inline...
> > > > > > This removes old sudo timer and simplyfies code a lot. It also
> > > > > > allows to manage offline/online state.
> > > > > > Full and smart refresh are disabled when offline.
> > > > > > Full refresh is run immediately when sssd is back online.
> > > > > > Smart refresh is scheduled normally when sssd is back online.
> > > > > >
> > > > > > Resolves:
> > > > > > https://fedorahosted.org/sssd/ticket/1943
> > > > >
> > > > > I agree with the logic, but it's different from the original code, so is
> > > > > the change intended?
> > > >
> > > > Yes, the functionality remains the same but we handle online/offline
> > > > transition much better here.
> > > > > > From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > > > > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > > > > >  sdap_sudo_refresh.c
> > > > >
> > > > > LGTM, I wonder why git doesn't show the changes as move since such a
> > > > > substantial part of sdap_sudo.c changed?
> > > >
> > > > Dunno.
> > > > > > From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > > > > Subject: [PATCH 03/35] SUDO: move offline check to handler
> > > > > >
> > > > > > We let sdap_id_op decide if we are offline or not here but we
> > > > > > should not get to this code since ptask is disabled and we will
> > > > > > not get through sudo handler if offline.
> > > > >
> > > > > LGTM, the connection error would still take us offline, which is what we
> > > > > want.
> > > > > > From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > > > > Subject: [PATCH 04/35] SUDO: simplify error handling
> > > > >
> > > > > LGTM
> > > > > > From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > > > > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> > > > >
> > > > > LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > > > > fixed in another patch. If you end up changing this patchset, it might
> > > > > also be better to fix the "i" iterators to use size_t instead of int,
> > > > > but if you don't change them, it's fine.
> > > >
> > > > Ok, new patch is attached to the patch set. Thanks.
> > > > > > From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > > > > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > > > > >
> > > > > > This fix huge violation of tevent coding style.
> > > > >
> > > > > LGTM
> > > > > > From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > > > > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > > > > >
> > > > > > The old search base iterator was difficult to read since its logic
> > > > > > spread through all functions. This patch also shorten names.
> > > > >
> > > > > LGTM
> > > > > > From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > > > > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > > > > Reduce code duplication.
> > > > > >
> > > > > > src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > > > > >  src/providers/ldap/sdap_sudo.h         |  2 +-
> > > > > >  src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > > > > >  3 files changed, 49 insertions(+), 60 deletions(-)
> > > > > >
> > > > > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > > > > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > > > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > > > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > > > > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > >  /* Empty sudoers? Done. */
> > > > > >  if (rules_count == 0 || rules == NULL) {
> > > > > >    *_usn = NULL;
> > > > > >    return EOK;
> > > > > > }
> > > > > >
> > > > > > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > >      return EOK;
> > > > > >  }
> > > > > >
> > > > > > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > > > > +{
> > > > > > unsigned int usn_number;
> > > > > > char *endptr = NULL;
> > > > > > if (usn == NULL) {
> > > > > >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > > > >    return;
> > > > > > }
> > > > > > if (srv_opts == NULL) {
> > > > > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > > > >    return;
> > > > > > }
> > > > > > talloc_zfree(srv_opts->max_sudo_value);
> > > > > > srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > > > > 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;
> > > > > > }
> > > > > > DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > > > > >                         srv_opts->max_sudo_value);
> > > > > >
> > > > > > +}
> > > > >
> > > > > What is the logic behind this function? Why do we store the USN both as
> > > > > a string unconditionally and as a number conditionally? Wouldn't it make
> > > > > sense to just store the data in one format?
> > > >
> > > > max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> > > > It's string since it is not needed to be a number. last_usn is the largers
> > > > value on the whole server, it needs to be compared.
> > > >
> > > > max_sudo_value can be a number also, but it would be inconsistent with other
> > > > providers.
> > > > > > From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > > > > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > > > > >  request
> > > > > >
> > > > > > Preparation for:
> > > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > >
> > > > > LGTM
> > > > > > From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > > > > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > > > > >
> > > > > > USN should never be unknown when we run smart refresh so this is
> > > > > > more a safety condition for a code bug. Nevertheless, we should
> > > > > > not immitate full refresh here under those conditions.
> > > > >
> > > > > LGTM
> > > > > > From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > > > > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> > > > >
> > > > > LGTM
> > > > > > From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > > > > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > > > > >
> > > > > > Resolves:
> > > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > >
> > > > > [...]
> > > > > > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > > > > +{
> > > > > > struct sdap_sudo_ctx *sudo_ctx;
> > > > > > struct sdap_sudo_refresh_state *state;
> > > > > > struct tevent_req *req;
> > > > > > errno_t ret;
> > > > > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > > > state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > > > > sudo_ctx = state->sudo_ctx;
> > > > > > ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > > > > >                                  &sudo_ctx->ip_addr);
> > > > > > talloc_zfree(subreq);
> > > > > > if (ret != EOK) {
> > > > > >    DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > > > > >                             "host filter will be disabled [%d]: %s\n",
> > > > > >                             ret, sss_strerror(ret));
> > > > > >    sudo_ctx->use_host_filter = false;
> > > > > > }
> > > > >
> > > > > I think you forgot an else here..
> > > >
> > > > Yes. thanks.
> > >
> > > We had a discussion on IRC with Pavel and Jakub
> > > and patch set caused a bug. The smart refresh was not executed
> > > if USN was NULL. (full refresh did not found any sudo rules)
> > >
> > > e.g.
> > > install freeIPA (sssd is started)
> > > add sudo rules
> > > sudo rules will be downloaded after full refresh
> > > (which is 1 day by default or after restarting sssd)
> > >
> > > LS
> >
> > I have changed the patch 0009 in a way so we do not imitate full refresh in
> > smart refresh but still be able to run smart refresh if no rules were found.
>
> Here is a diff:
> diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> index 9f66d6c..13824c5 100644
> --- a/src/providers/ldap/sdap_async_sudo.c
> +++ b/src/providers/ldap/sdap_async_sudo.c
> @@ -305,13 +305,21 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
>     unsigned int usn_number;
>     char *endptr = NULL;
> if (usn == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> if (srv_opts == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
>   return;
> }
> if (srv_opts == NULL) {
>    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> if (usn == NULL) {
>    /* If the USN value is unknown and we don't have max_sudo_value set
>     * (possibly first full refresh which did not find any rule) we will
>     * set zero so smart refresh can pick up. */
>    if (srv_opts->max_sudo_value == NULL) {
>        srv_opts->max_sudo_value = "0";

I'm sorry that I didn't noticed it earlier.
You assign static allocated constant here.
But there is zfree :-) It was not visible from diff

313     if (usn == NULL) {
314         /* If the USN value is unknown and we don't have max_sudo_value set
315          * (possibly first full refresh which did not find any rule) we will
316          * set zero so smart refresh can pick up. */
317         if (srv_opts->max_sudo_value == NULL) {
318             srv_opts->max_sudo_value = "0";
319             return;
320         }
321 
322         DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
323         return;
324     }
325 
326     talloc_zfree(srv_opts->max_sudo_value);
327     srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);

==35602== Process terminating with default action of signal 11 (SIGSEGV)
==35602==  General Protection Fault
==35602==    at 0x3EB8447E2C: vfprintf (vfprintf.c:1641)
==35602==    by 0x3EB84FFB0F: __vsnprintf_chk (vsnprintf_chk.c:65)
==35602==    by 0x3EBE405402: talloc_vasprintf (stdio2.h:78)
==35602==    by 0x3EBE402BDC: talloc_log (talloc.c:288)
==35602==    by 0x3EBE402B13: _talloc_free (talloc.c:356)
==35602==    by 0xD038168: sdap_sudo_refresh_done (sdap_async_sudo.c:326)
==35602==    by 0xD037BCE: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:195)
==35602==    by 0xD005516: generic_ext_search_handler (sdap_async.c:1677)
==35602==    by 0xD009AA2: sdap_get_generic_op_finished (sdap_async.c:1603)
==35602==    by 0xD00AD0E: sdap_process_result (sdap_async.c:352)
==35602==    by 0x3EC1007C90: tevent_common_loop_timer_delay (tevent_timed.c:341)
==35602==    by 0x3EC1008CBA: epoll_event_loop_once (tevent_epoll.c:916)
--35602-- Discarding syms at 0xd8c41f0-0xd8cc648 in /lib64/libnss_files-2.12.so due to munmap()
--35602-- Discarding syms at 0xdad1000-0xdad4388 in /lib64/libnss_dns-2.12.so due to munmap()
--35602-- Discarding syms at 0xf06c760-0xf0717f8 in /lib64/libnss_sss.so.2 due to munmap()

LS

[Pavel Březina]

On 12/14/2015 04:09 PM, Lukas Slebodnik wrote:
> On (14/12/15 13:34), Lukas Slebodnik wrote:
> > On (14/12/15 12:38), Pavel Březina wrote:
> > > On 12/14/2015 12:05 PM, Lukas Slebodnik wrote:
> > > > On (11/12/15 15:56), Pavel Březina wrote:
> > > > > On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > > > > >  From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > > > > > Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> > > > > >
> > > > > > LGTM, not tested yet. One question inline...
> > > > > >
> > > > > > >
> > > > > > > This removes old sudo timer and simplyfies code a lot. It also
> > > > > > > allows to manage offline/online state.
> > > > > > >
> > > > > > > - Full and smart refresh are disabled when offline.
> > > > > > > - Full refresh is run immediately when sssd is back online.
> > > > > > > - Smart refresh is scheduled normally when sssd is back online.
> > > > > > >
> > > > > > > Resolves:
> > > > > > > https://fedorahosted.org/sssd/ticket/1943
> > > > > >
> > > > > > I agree with the logic, but it's different from the original code, so is
> > > > > > the change intended?
> > > > >
> > > > > Yes, the functionality remains the same but we handle online/offline
> > > > > transition much better here.
> > > > > > >  From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > > > > > Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > > > > > >   sdap_sudo_refresh.c
> > > > > >
> > > > > > LGTM, I wonder why git doesn't show the changes as move since such a
> > > > > > substantial part of sdap_sudo.c changed?
> > > > >
> > > > > Dunno.
> > > > > > >  From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > > > > > Subject: [PATCH 03/35] SUDO: move offline check to handler
> > > > > > >
> > > > > > > We let sdap_id_op decide if we are offline or not here but we
> > > > > > > should not get to this code since ptask is disabled and we will
> > > > > > > not get through sudo handler if offline.
> > > > > >
> > > > > > LGTM, the connection error would still take us offline, which is what we
> > > > > > want.
> > > > > >
> > > > > > >  From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > > > > > Subject: [PATCH 04/35] SUDO: simplify error handling
> > > > > >
> > > > > > LGTM
> > > > > >
> > > > > > >  From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > > > > > Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> > > > > >
> > > > > > LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > > > > > fixed in another patch. If you end up changing this patchset, it might
> > > > > > also be better to fix the "i" iterators to use size_t instead of int,
> > > > > > but if you don't change them, it's fine.
> > > > >
> > > > > Ok, new patch is attached to the patch set. Thanks.
> > > > > > >  From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > > > > > Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > > > > > >
> > > > > > > This fix huge violation of tevent coding style.
> > > > > >
> > > > > > LGTM
> > > > > >
> > > > > > >  From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > > > > > Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > > > > > >
> > > > > > > The old search base iterator was difficult to read since its logic
> > > > > > > spread through all functions. This patch also shorten names.
> > > > > >
> > > > > > LGTM
> > > > > >
> > > > > > >  From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > > > > > Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > > > > >
> > > > > > > Reduce code duplication.
> > > > > > > ---
> > > > > > >   src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > > > > > >   src/providers/ldap/sdap_sudo.h         |  2 +-
> > > > > > >   src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > > > > > >   3 files changed, 49 insertions(+), 60 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > > > > > index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > > > > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > > > > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > > > > > @@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > > >
> > > > > > >       /* Empty sudoers? Done. */
> > > > > > >       if (rules_count == 0 || rules == NULL) {
> > > > > > > +        *_usn = NULL;
> > > > > > >           return EOK;
> > > > > > >       }
> > > > > > >
> > > > > > > @@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > > >       return EOK;
> > > > > > >   }
> > > > > > >
> > > > > > > +static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > > > > > +{
> > > > > > > +    unsigned int usn_number;
> > > > > > > +    char *endptr = NULL;
> > > > > > > +
> > > > > > > +    if (usn == NULL) {
> > > > > > > +        DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    if (srv_opts == NULL) {
> > > > > > > +        DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    talloc_zfree(srv_opts->max_sudo_value);
> > > > > > > +    srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > > > > > +
> > > > > > > +    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;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > > > > > > +                             srv_opts->max_sudo_value);
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > What is the logic behind this function? Why do we store the USN both as
> > > > > > a string unconditionally and as a number conditionally? Wouldn't it make
> > > > > > sense to just store the data in one format?
> > > > >
> > > > > max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> > > > > It's string since it is not needed to be a number. last_usn is the largers
> > > > > value on the whole server, it needs to be compared.
> > > > >
> > > > > max_sudo_value can be a number also, but it would be inconsistent with other
> > > > > providers.
> > > > > > >  From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > > > > > Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > > > > > >   request
> > > > > > >
> > > > > > > Preparation for:
> > > > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > > >
> > > > > > LGTM
> > > > > >
> > > > > > >  From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > > > > > Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > > > > > >
> > > > > > > USN should never be unknown when we run smart refresh so this is
> > > > > > > more a safety condition for a code bug. Nevertheless, we should
> > > > > > > not immitate full refresh here under those conditions.
> > > > > >
> > > > > > LGTM
> > > > > >
> > > > > > >  From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > > > > > Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> > > > > >
> > > > > > LGTM
> > > > > >
> > > > > > >  From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > > > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > > Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > > > > > Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > > > > > >
> > > > > > > Resolves:
> > > > > > > https://fedorahosted.org/sssd/ticket/2672
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > +static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > > > > > +{
> > > > > > > +    struct sdap_sudo_ctx *sudo_ctx;
> > > > > > > +    struct sdap_sudo_refresh_state *state;
> > > > > > > +    struct tevent_req *req;
> > > > > > > +    errno_t ret;
> > > > > > > +
> > > > > > > +    req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > > > > +    state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > > > > > +
> > > > > > > +    sudo_ctx = state->sudo_ctx;
> > > > > > > +
> > > > > > > +    ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > > > > > > +                                      &sudo_ctx->ip_addr);
> > > > > > > +    talloc_zfree(subreq);
> > > > > > > +    if (ret != EOK) {
> > > > > > > +        DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > > > > > > +                                 "host filter will be disabled [%d]: %s\n",
> > > > > > > +                                 ret, sss_strerror(ret));
> > > > > > > +        sudo_ctx->use_host_filter = false;
> > > > > > > +    }
> > > > > >
> > > > > > I think you forgot an else here..
> > > > >
> > > > > Yes. thanks.
> > > >
> > > > We had a discussion on IRC with Pavel and Jakub
> > > > and patch set caused a bug. The smart refresh was not executed
> > > > if USN was NULL. (full refresh did not found any sudo rules)
> > > >
> > > > e.g.
> > > > install freeIPA (sssd is started)
> > > > add sudo rules
> > > > sudo rules will be downloaded after full refresh
> > > >  (which is 1 day by default or after restarting sssd)
> > > >
> > > > LS
> > >
> > > I have changed the patch 0009 in a way so we do not imitate full refresh in
> > > smart refresh but still be able to run smart refresh if no rules were found.
> >
> > Here is a diff:
> > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > index 9f66d6c..13824c5 100644
> > --- a/src/providers/ldap/sdap_async_sudo.c
> > +++ b/src/providers/ldap/sdap_async_sudo.c
> > @@ -305,13 +305,21 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> >      unsigned int usn_number;
> >      char *endptr = NULL;
> > if (usn == NULL) {
> >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > if (srv_opts == NULL) {
> >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> >    return;
> > }
> > if (srv_opts == NULL) {
> >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > if (usn == NULL) {
> >    /* If the USN value is unknown and we don't have max_sudo_value set
> >     * (possibly first full refresh which did not find any rule) we will
> >     * set zero so smart refresh can pick up. */
> >    if (srv_opts->max_sudo_value == NULL) {
> >        srv_opts->max_sudo_value = "0";
>
> I'm sorry that I didn't noticed it earlier.
> You assign static allocated constant here.
> But there is zfree :-) It was not visible from diff
>
> 313     if (usn == NULL) {
> 314         /* If the USN value is unknown and we don't have max_sudo_value set
> 315          * (possibly first full refresh which did not find any rule) we will
> 316          * set zero so smart refresh can pick up. */
> 317         if (srv_opts->max_sudo_value == NULL) {
> 318             srv_opts->max_sudo_value = "0";
> 319             return;
> 320         }
> 321
> 322         DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> 323         return;
> 324     }
> 325
> 326     talloc_zfree(srv_opts->max_sudo_value);
> 327     srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
>
> ==35602== Process terminating with default action of signal 11 (SIGSEGV)
> ==35602==  General Protection Fault
> ==35602==    at 0x3EB8447E2C: vfprintf (vfprintf.c:1641)
> ==35602==    by 0x3EB84FFB0F: __vsnprintf_chk (vsnprintf_chk.c:65)
> ==35602==    by 0x3EBE405402: talloc_vasprintf (stdio2.h:78)
> ==35602==    by 0x3EBE402BDC: talloc_log (talloc.c:288)
> ==35602==    by 0x3EBE402B13: _talloc_free (talloc.c:356)
> ==35602==    by 0xD038168: sdap_sudo_refresh_done (sdap_async_sudo.c:326)
> ==35602==    by 0xD037BCE: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:195)
> ==35602==    by 0xD005516: generic_ext_search_handler (sdap_async.c:1677)
> ==35602==    by 0xD009AA2: sdap_get_generic_op_finished (sdap_async.c:1603)
> ==35602==    by 0xD00AD0E: sdap_process_result (sdap_async.c:352)
> ==35602==    by 0x3EC1007C90: tevent_common_loop_timer_delay (tevent_timed.c:341)
> ==35602==    by 0x3EC1008CBA: epoll_event_loop_once (tevent_epoll.c:916)
> --35602-- Discarding syms at 0xd8c41f0-0xd8cc648 in /lib64/libnss_files-2.12.so due to munmap()
> --35602-- Discarding syms at 0xdad1000-0xdad4388 in /lib64/libnss_dns-2.12.so due to munmap()
> --35602-- Discarding syms at 0xf06c760-0xf0717f8 in /lib64/libnss_sss.so.2 due to munmap()
>
> LS

Thanks. I should pay more attention when doing a quick fix, I was 
already overcoded though. I hope this patch set is the one :-)

[Lukas Slebodnik]

On (15/12/15 11:21), Pavel Březina wrote:
> On 12/14/2015 04:09 PM, Lukas Slebodnik wrote:
> > On (14/12/15 13:34), Lukas Slebodnik wrote:
> > > On (14/12/15 12:38), Pavel Březina wrote:
> > > > On 12/14/2015 12:05 PM, Lukas Slebodnik wrote:
> > > > > On (11/12/15 15:56), Pavel Březina wrote:
> > > > > > On 12/11/2015 03:07 PM, Jakub Hrozek wrote:
> > > > > > >> From 2479352011f96a8f64c403d1278b791c73df703a Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Fri, 6 Nov 2015 13:00:47 +0100
> > > > > > >>Subject: [PATCH 01/35] SUDO: convert periodical refreshes to be_ptask
> > > > > > >
> > > > > > >LGTM, not tested yet. One question inline...
> > > > > > >
> > > > > > >>
> > > > > > >>This removes old sudo timer and simplyfies code a lot. It also
> > > > > > >>allows to manage offline/online state.
> > > > > > >>
> > > > > > >>- Full and smart refresh are disabled when offline.
> > > > > > >>- Full refresh is run immediately when sssd is back online.
> > > > > > >>- Smart refresh is scheduled normally when sssd is back online.
> > > > > > >>
> > > > > > >>Resolves:
> > > > > > >>https://fedorahosted.org/sssd/ticket/1943
> > > > > > >
> > > > > > >I agree with the logic, but it's different from the original code, so is
> > > > > > >the change intended?
> > > > > >
> > > > > > Yes, the functionality remains the same but we handle online/offline
> > > > > > transition much better here.
> > > > > >
> > > > > > >> From d17b4d1c63f1b85a14a6fe8b2400e2c29158cd43 Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Fri, 6 Nov 2015 13:32:34 +0100
> > > > > > >>Subject: [PATCH 02/35] SUDO: move refreshes from sdap_sudo.c to
> > > > > > >>  sdap_sudo_refresh.c
> > > > > > >
> > > > > > >LGTM, I wonder why git doesn't show the changes as move since such a
> > > > > > >substantial part of sdap_sudo.c changed?
> > > > > >
> > > > > > Dunno.
> > > > > >
> > > > > > >> From 633d0de6e3edab4d5b988506426800cf35845702 Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Mon, 9 Nov 2015 12:26:07 +0100
> > > > > > >>Subject: [PATCH 03/35] SUDO: move offline check to handler
> > > > > > >>
> > > > > > >>We let sdap_id_op decide if we are offline or not here but we
> > > > > > >>should not get to this code since ptask is disabled and we will
> > > > > > >>not get through sudo handler if offline.
> > > > > > >
> > > > > > >LGTM, the connection error would still take us offline, which is what we
> > > > > > >want.
> > > > > > >
> > > > > > >> From 3dc1bbb6fa4353f0bb2cc2b9417900a6d4695240 Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Mon, 9 Nov 2015 12:44:13 +0100
> > > > > > >>Subject: [PATCH 04/35] SUDO: simplify error handling
> > > > > > >
> > > > > > >LGTM
> > > > > > >
> > > > > > >> From 2e85798a9d1f856b549cb733b6a7f353bec05e6e Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Mon, 9 Nov 2015 12:52:35 +0100
> > > > > > >>Subject: [PATCH 05/35] SUDO: fix sdap_id_op logic
> > > > > > >
> > > > > > >LGTM, the fail/immediate confusion in sdap_sudo_load_sudoers_send is
> > > > > > >fixed in another patch. If you end up changing this patchset, it might
> > > > > > >also be better to fix the "i" iterators to use size_t instead of int,
> > > > > > >but if you don't change them, it's fine.
> > > > > >
> > > > > > Ok, new patch is attached to the patch set. Thanks.
> > > > > >
> > > > > > >
> > > > > > >> From 03bcc4d6b5c1324644bab7b98a10bfacaf9096c7 Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Mon, 9 Nov 2015 13:34:45 +0100
> > > > > > >>Subject: [PATCH 07/35] SUDO: fix sdap_sudo_smart_refresh_recv()
> > > > > > >>
> > > > > > >>This fix huge violation of tevent coding style.
> > > > > > >
> > > > > > >LGTM
> > > > > > >
> > > > > > >> From 0093a630eac9a745512c643b1389506c3839a56e Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Mon, 9 Nov 2015 13:59:48 +0100
> > > > > > >>Subject: [PATCH 08/35] SUDO: sdap_sudo_load_sudoers improve iterator
> > > > > > >>
> > > > > > >>The old search base iterator was difficult to read since its logic
> > > > > > >>spread through all functions. This patch also shorten names.
> > > > > > >
> > > > > > >LGTM
> > > > > > >
> > > > > > >> From a7d3e06ac32b74e2586d87f9671b5a49eda7cafe Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Tue, 10 Nov 2015 10:34:41 +0100
> > > > > > >>Subject: [PATCH 09/35] SUDO: set USN inside sdap_sudo_refresh request
> > > > > > >>
> > > > > > >>Reduce code duplication.
> > > > > > >>---
> > > > > > >>  src/providers/ldap/sdap_async_sudo.c   | 48 +++++++++++++++++++++------
> > > > > > >>  src/providers/ldap/sdap_sudo.h         |  2 +-
> > > > > > >>  src/providers/ldap/sdap_sudo_refresh.c | 59 ++++++----------------------------
> > > > > > >>  3 files changed, 49 insertions(+), 60 deletions(-)
> > > > > > >>
> > > > > > >>diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > > > > >>index 71e1b3322a557beaf82b51316967b3147fb61f76..7a90ccde9cbd2efc0d0509f21e072cd745c57844 100644
> > > > > > >>--- a/src/providers/ldap/sdap_async_sudo.c
> > > > > > >>+++ b/src/providers/ldap/sdap_async_sudo.c
> > > > > > >>@@ -283,6 +283,7 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > > >>
> > > > > > >>      /* Empty sudoers? Done. */
> > > > > > >>      if (rules_count == 0 || rules == NULL) {
> > > > > > >>+        *_usn = NULL;
> > > > > > >>          return EOK;
> > > > > > >>      }
> > > > > > >>
> > > > > > >>@@ -299,8 +300,37 @@ static int sdap_sudo_store_sudoers(TALLOC_CTX *mem_ctx,
> > > > > > >>      return EOK;
> > > > > > >>  }
> > > > > > >>
> > > > > > >>+static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > > > > > >>+{
> > > > > > >>+    unsigned int usn_number;
> > > > > > >>+    char *endptr = NULL;
> > > > > > >>+
> > > > > > >>+    if (usn == NULL) {
> > > > > > >>+        DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > > > > >>+        return;
> > > > > > >>+    }
> > > > > > >>+
> > > > > > >>+    if (srv_opts == NULL) {
> > > > > > >>+        DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > > > > >>+        return;
> > > > > > >>+    }
> > > > > > >>+
> > > > > > >>+    talloc_zfree(srv_opts->max_sudo_value);
> > > > > > >>+    srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> > > > > > >>+
> > > > > > >>+    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;
> > > > > > >>+    }
> > > > > > >>+
> > > > > > >>+    DEBUG(SSSDBG_FUNC_DATA, "SUDO higher USN value: [%s]\n",
> > > > > > >>+                             srv_opts->max_sudo_value);
> > > > > > >>+}
> > > > > > >>+
> > > > > > >
> > > > > > >What is the logic behind this function? Why do we store the USN both as
> > > > > > >a string unconditionally and as a number conditionally? Wouldn't it make
> > > > > > >sense to just store the data in one format?
> > > > > >
> > > > > > max_sudo_value is internal to sudo, it is max value found in sudo subtree.
> > > > > > It's string since it is not needed to be a number. last_usn is the largers
> > > > > > value on the whole server, it needs to be compared.
> > > > > >
> > > > > > max_sudo_value can be a number also, but it would be inconsistent with other
> > > > > > providers.
> > > > > >
> > > > > > >
> > > > > > >> From 1ca17bd4110960f3d9b6834446cec83dfe9a1ca8 Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Tue, 10 Nov 2015 11:31:50 +0100
> > > > > > >>Subject: [PATCH 10/35] SUDO: built host filter inside sdap_sudo_refresh
> > > > > > >>  request
> > > > > > >>
> > > > > > >>Preparation for:
> > > > > > >>https://fedorahosted.org/sssd/ticket/2672
> > > > > > >
> > > > > > >LGTM
> > > > > > >
> > > > > > >> From ebe26c922ad72c97f53d5ca0f5eeef1034c4a470 Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Tue, 10 Nov 2015 11:34:14 +0100
> > > > > > >>Subject: [PATCH 11/35] SUDO: fail if usn is unknown in smart refresh
> > > > > > >>
> > > > > > >>USN should never be unknown when we run smart refresh so this is
> > > > > > >>more a safety condition for a code bug. Nevertheless, we should
> > > > > > >>not immitate full refresh here under those conditions.
> > > > > > >
> > > > > > >LGTM
> > > > > > >
> > > > > > >> From aa06528d2a1fdd6b5df5de629d2da5297cbcc90d Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Tue, 10 Nov 2015 12:34:11 +0100
> > > > > > >>Subject: [PATCH 12/35] SUDO: fix potential memory leak in sdap_sudo_init
> > > > > > >
> > > > > > >LGTM
> > > > > > >
> > > > > > >> From 897f68a3379f8fab70eae106f5fa71b4fe4d809a Mon Sep 17 00:00:00 2001
> > > > > > >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > > > >>Date: Tue, 10 Nov 2015 12:32:16 +0100
> > > > > > >>Subject: [PATCH 13/35] SUDO: obtain host information when going online
> > > > > > >>
> > > > > > >>Resolves:
> > > > > > >>https://fedorahosted.org/sssd/ticket/2672
> > > > > > >
> > > > > > >[...]
> > > > > > >
> > > > > > >>+static void sdap_sudo_refresh_hostinfo_done(struct tevent_req *subreq)
> > > > > > >>+{
> > > > > > >>+    struct sdap_sudo_ctx *sudo_ctx;
> > > > > > >>+    struct sdap_sudo_refresh_state *state;
> > > > > > >>+    struct tevent_req *req;
> > > > > > >>+    errno_t ret;
> > > > > > >>+
> > > > > > >>+    req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > > > >>+    state = tevent_req_data(req, struct sdap_sudo_refresh_state);
> > > > > > >>+
> > > > > > >>+    sudo_ctx = state->sudo_ctx;
> > > > > > >>+
> > > > > > >>+    ret = sdap_sudo_get_hostinfo_recv(sudo_ctx, subreq, &sudo_ctx->hostnames,
> > > > > > >>+                                      &sudo_ctx->ip_addr);
> > > > > > >>+    talloc_zfree(subreq);
> > > > > > >>+    if (ret != EOK) {
> > > > > > >>+        DEBUG(SSSDBG_OP_FAILURE, "Unable to retrieve host information, "
> > > > > > >>+                                 "host filter will be disabled [%d]: %s\n",
> > > > > > >>+                                 ret, sss_strerror(ret));
> > > > > > >>+        sudo_ctx->use_host_filter = false;
> > > > > > >>+    }
> > > > > > >
> > > > > > >I think you forgot an else here..
> > > > > > >
> > > > > >
> > > > > > Yes. thanks.
> > > > >
> > > > > We had a discussion on IRC with Pavel and Jakub
> > > > > and patch set caused a bug. The smart refresh was not executed
> > > > > if USN was NULL. (full refresh did not found any sudo rules)
> > > > >
> > > > > e.g.
> > > > > install freeIPA (sssd is started)
> > > > > add sudo rules
> > > > > sudo rules will be downloaded after full refresh
> > > > > (which is 1 day by default or after restarting sssd)
> > > > >
> > > > > LS
> > > >
> > > > I have changed the patch 0009 in a way so we do not imitate full refresh in
> > > > smart refresh but still be able to run smart refresh if no rules were found.
> > >
> > > Here is a diff:
> > > diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> > > index 9f66d6c..13824c5 100644
> > > --- a/src/providers/ldap/sdap_async_sudo.c
> > > +++ b/src/providers/ldap/sdap_async_sudo.c
> > > @@ -305,13 +305,21 @@ static void sdap_sudo_set_usn(struct sdap_server_opts *srv_opts, char *usn)
> > >     unsigned int usn_number;
> > >     char *endptr = NULL;
> > > if (usn == NULL) {
> > >    DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > > if (srv_opts == NULL) {
> > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > >   return;
> > > }
> > > if (srv_opts == NULL) {
> > >    DEBUG(SSSDBG_TRACE_FUNC, "Bug: srv_opts is NULL\n");
> > > if (usn == NULL) {
> > >    /* If the USN value is unknown and we don't have max_sudo_value set
> > >     * (possibly first full refresh which did not find any rule) we will
> > >     * set zero so smart refresh can pick up. */
> > >    if (srv_opts->max_sudo_value == NULL) {
> > >        srv_opts->max_sudo_value = "0";
> >
> > I'm sorry that I didn't noticed it earlier.
> > You assign static allocated constant here.
> > But there is zfree :-) It was not visible from diff
> >
> > 313     if (usn == NULL) {
> > 314         /* If the USN value is unknown and we don't have max_sudo_value set
> > 315          * (possibly first full refresh which did not find any rule) we will
> > 316          * set zero so smart refresh can pick up. */
> > 317         if (srv_opts->max_sudo_value == NULL) {
> > 318             srv_opts->max_sudo_value = "0";
> > 319             return;
> > 320         }
> > 321
> > 322         DEBUG(SSSDBG_TRACE_FUNC, "Empty USN, ignoring\n");
> > 323         return;
> > 324     }
> > 325
> > 326     talloc_zfree(srv_opts->max_sudo_value);
> > 327     srv_opts->max_sudo_value = talloc_steal(srv_opts, usn);
> >
> > ==35602== Process terminating with default action of signal 11 (SIGSEGV)
> > ==35602==  General Protection Fault
> > ==35602==    at 0x3EB8447E2C: vfprintf (vfprintf.c:1641)
> > ==35602==    by 0x3EB84FFB0F: __vsnprintf_chk (vsnprintf_chk.c:65)
> > ==35602==    by 0x3EBE405402: talloc_vasprintf (stdio2.h:78)
> > ==35602==    by 0x3EBE402BDC: talloc_log (talloc.c:288)
> > ==35602==    by 0x3EBE402B13: _talloc_free (talloc.c:356)
> > ==35602==    by 0xD038168: sdap_sudo_refresh_done (sdap_async_sudo.c:326)
> > ==35602==    by 0xD037BCE: sdap_sudo_load_sudoers_done (sdap_async_sudo.c:195)
> > ==35602==    by 0xD005516: generic_ext_search_handler (sdap_async.c:1677)
> > ==35602==    by 0xD009AA2: sdap_get_generic_op_finished (sdap_async.c:1603)
> > ==35602==    by 0xD00AD0E: sdap_process_result (sdap_async.c:352)
> > ==35602==    by 0x3EC1007C90: tevent_common_loop_timer_delay (tevent_timed.c:341)
> > ==35602==    by 0x3EC1008CBA: epoll_event_loop_once (tevent_epoll.c:916)
> > --35602-- Discarding syms at 0xd8c41f0-0xd8cc648 in /lib64/libnss_files-2.12.so due to munmap()
> > --35602-- Discarding syms at 0xdad1000-0xdad4388 in /lib64/libnss_dns-2.12.so due to munmap()
> > --35602-- Discarding syms at 0xf06c760-0xf0717f8 in /lib64/libnss_sss.so.2 due to munmap()
> >
> > LS
>
> Thanks. I should pay more attention when doing a quick fix, I was already
> overcoded though. I hope this patch set is the one :-)

ACK to patch set

http://sssd-ci.duckdns.org/logs/job/34/76/summary.html

LS

[Lukas Slebodnik]

On (15/12/15 16:29), Lukas Slebodnik wrote:
> On (15/12/15 11:21), Pavel Březina wrote:
> > Thanks. I should pay more attention when doing a quick fix, I was already
> > overcoded though. I hope this patch set is the one :-)
>
> ACK to patch set
>
> http://sssd-ci.duckdns.org/logs/job/34/76/summary.html

master:
* 6b83f562fbd67cf61a7167c6057764fd08146241
* 15ebeedaad83cc5dcf896cfcdea850227fdc46b5
* 895b8d884d0f5277e181fe1212ec0c0daaf3977d
* 38262a2622af9fe71ca336799da6e88d91be0d81
* cb235ec146f1ba81c211f8506736edea436be28a
* 556801ec367543a8d534e55ecd11a977642bcee6
* c0000a8cc9eccdf5cd8dd72fd6e9bc09d8c7cf00
* 1ab2b07c71da6c19c3855e390d10156d598c06a2
* a00c89f23bd50d4fd9cf24aa09037c997781b8c9
* d103c2e4a704b1dfffd39fea2b601c2f337d06d5
* efa19bb588ce1dc6c3f4b94b94464886ad764d09
* 24eac34a8c1f0a284cb697e8d5c09ff049181691
* 7e0158f9fdb1d299ab2d018e9d81cc71eed98c15
* fc19031212369d69a9693ac8777ce1e61a16fe93
* 81f135f9e83031c4a021a3d19009b2bc179c8468
* 00fea5c2aaa0277bea522d2f61de75699ee2ed49
* a13cf3d295a4a6654dfa7e4193c0a2bc8bb78e92

sssd-1-13:
* 76170b96e485fe7e50e7fb2b5121e27cad5b56e8
* 74354462881737933e55eab3765608c5e6411059
* 32c7b5c1c8c358b33350b264bbf74f86e9cb1815
* bbfbada8a95bd68de952d8b80943ec365c15b3ef
* cf6f8b46cd6147fcee0534fab1c278318df58852
* bbeaec95e9814909a0bad62b31333c132558f320
* 69993374865057cf6578a69ed431dcb74d857042
* 62f97308c83378e1e82abb34894e4bddd7820fe9
* 2099434ccfec74828d37ea310fcb4482792b1aa1
* a789a6dae1ffbfb3d92901c35d28f829cfe0d6ec
* 5021ac65dbb912bd92d893c6b6888b020c5eed34
* 9ec4295869c4b0bbc9a9f7c95fb410b645ddb854
* be1121afa04c26c44899054690c52ca87971e133
* af3b1a9efa41d9c164ee2955f9acbfe813c27f16
* 1079a7c602eb44a43bd92dfbdbe224b55a8f4e5d
* 03c2affe789c9b566d3dccc03f3f704bc1df5cce
* 39843ca39c3c9d2fdb58c0d1884ba34cf8600fb1

LS