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?= <pbrezina(a)redhat.com>
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 <pbrezina(a)redhat.com>
+
+ 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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
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?= <pbrezina(a)redhat.com>
Date: Wed, 25 Nov 2015 13:21:49 +0100
Subject: [PATCH 15/28] SUDO: make sdap_sudo_handler static
---
ACK
LS