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?= <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.
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?= <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?
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?= <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.
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?= <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.
You are right, it should say or.