[SSSD] [PATCH] SDAP: add missing header files

Lukas Slebodnik lslebodn at redhat.com
Wed Oct 14 05:54:46 UTC 2015


On (13/10/15 15:39), Michal Židek wrote:
>On 10/13/2015 01:43 PM, Pavel Reichl wrote:
>>
>>
>>On 10/13/2015 01:28 PM, Lukas Slebodnik wrote:
>>>On (13/10/15 12:36), Pavel Reichl wrote:
>>>>
>>>>
>>>>On 10/13/2015 12:22 PM, Lukas Slebodnik wrote:
>>>>>On (13/10/15 10:10), Pavel Reichl wrote:
>>>>>>On 10/13/2015 09:59 AM, Lukas Slebodnik wrote:
>>>>>>>On (12/10/15 14:45), Pavel Reichl wrote:
>>>>>>>>On 10/12/2015 02:36 PM, Lukas Slebodnik wrote:
>>>>>>>>>On (12/10/15 14:06), Pavel Reichl wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>On 10/12/2015 01:52 PM, Lukas Slebodnik wrote:
>>>>>>>>>>>On (12/10/15 13:05), Pavel Reichl wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>On 10/12/2015 07:32 AM, Lukas Slebodnik wrote:
>>>>>>>>>>>>>On (11/10/15 23:11), Pavel Reichl wrote:
>>>>>>>>>>>>>>Hello,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>while messing around with some code analyses tool I noticed
>>>>>>>>>>>>>>that some header files are not including directly header
>>>>>>>>>>>>>>files which they use and thus are dependent on being
>>>>>>>>>>>>>>included in particular order.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>Please see simple patch attached that fixes some of such
>>>>>>>>>>>>>>files (but definitely not all).
>>>>>>>>>>>>>
>>>>>>>>>>>>>>From 12c273af1c8ffe727efc526c9e993240904bcbf1 Mon Sep 17
>>>>>>>>>>>>>00:00:00 2001
>>>>>>>>>>>>>>From: Pavel Reichl <reichl.pavel at gmail.com>
>>>>>>>>>>>>>>Date: Sun, 11 Oct 2015 22:29:44 +0200
>>>>>>>>>>>>>>Subject: [PATCH 1/4] SDAP: add missing header files
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>---
>>>>>>>>>>>>>>src/providers/ldap/ldap_auth.h       | 4 ++++
>>>>>>>>>>>>>>src/providers/ldap/sdap_async_enum.h | 6 ++++++
>>>>>>>>>>>>>>src/providers/ldap/sdap_autofs.h     | 6 ++++++
>>>>>>>>>>>>>>src/providers/ldap/sdap_id_op.h      | 4 ++++
>>>>>>>>>>>>>>src/providers/ldap/sdap_sudo.h       | 6 ++++++
>>>>>>>>>>>>>>src/providers/ldap/sdap_users.h      | 4 ++++
>>>>>>>>>>>>>>6 files changed, 30 insertions(+)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>diff --git a/src/providers/ldap/ldap_auth.h
>>>>>>>>>>>>>>b/src/providers/ldap/ldap_auth.h
>>>>>>>>>>>>>>index
>>>>>>>>>>>>>>5fbddd7087dc65ab8bd1df5fb57492d2fc26d0bb..1848af5de19796c8287117b9afe5c6d3e1a248ec
>>>>>>>>>>>>>>100644
>>>>>>>>>>>>>>--- a/src/providers/ldap/ldap_auth.h
>>>>>>>>>>>>>>+++ b/src/providers/ldap/ldap_auth.h
>>>>>>>>>>>>>>@@ -22,6 +22,10 @@
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>#include "config.h"
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>+#include <talloc.h>
>>>>>>>>>>>>>>+
>>>>>>>>>>>>>>+#include "util/util.h"
>>>>>>>>>>>>>>+
>>>>>>>>>>>>>util.h already includes talloc.h.
>>>>>>>>>>>>>Could you explain why do we need to include both of them?
>>>>>>>>>>>>
>>>>>>>>>>>>Thanks for noticing that. This seems to be quite common in
>>>>>>>>>>>>our code base:
>>>>>>>>>>>>git grep -l 'util.h' -- '*.[ch]' | xargs grep 'talloc.h' | wc -l
>>>>>>>>>>>>88
>>>>>>>>>>>>
>>>>>>>>>>>>But it's glitch in the patch nonetheless.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>enum pwexpire {
>>>>>>>>>>>>>>     PWEXPIRE_NONE = 0,
>>>>>>>>>>>>>>     PWEXPIRE_LDAP_PASSWORD_POLICY,
>>>>>>>>>>>>>>diff --git a/src/providers/ldap/sdap_async_enum.h
>>>>>>>>>>>>>>b/src/providers/ldap/sdap_async_enum.h
>>>>>>>>>>>>>>index
>>>>>>>>>>>>>>2da38f988913fa0d6f252697925e50e05eb794a6..7d4ff82c5e003ffbd14aca76f321a8a0fc41b3d6
>>>>>>>>>>>>>>100644
>>>>>>>>>>>>>>--- a/src/providers/ldap/sdap_async_enum.h
>>>>>>>>>>>>>>+++ b/src/providers/ldap/sdap_async_enum.h
>>>>>>>>>>>>>>@@ -26,6 +26,12 @@
>>>>>>>>>>>>>>#ifndef _SDAP_ASYNC_ENUM_H_
>>>>>>>>>>>>>>#define _SDAP_ASYNC_ENUM_H_
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>+#include "config.h"
>>>>>>>>>>>>>>+
>>>>>>>>>>>>>>+#include <talloc.h>
>>>>>>>>>>>>>>+
>>>>>>>>>>>>>>+#include "util/util.h"
>>>>>>>>>>>>>>+
>>>>>>>>>>>>>
>>>>>>>>>>>>>util.h already includes config.h and talloc.h.
>>>>>>>>>>>>>Could you explain why we need to include all mentioned
>>>>>>>>>>>>>header files?
>>>>>>>>>>>>
>>>>>>>>>>>>Same as above. Thanks for noticing.
>>>>>>>>>>>>
>>>>>>>>>>>>git grep -l 'util.h' -- '*.[ch]' | xargs grep -l 'talloc.h' |
>>>>>>>>>>>>xargs grep 'config.h' | wc -l
>>>>>>>>>>>>23
>>>>>>>>>>>>
>>>>>>>>>>>>Please see updated patch.
>>>>>>>>>>>
>>>>>>>>>>>I'm sorry but I still miss an explanation why do we need to
>>>>>>>>>>>include these
>>>>>>>>>>>header files. You just updated patches but the explanation is
>>>>>>>>>>>neither in mail
>>>>>>>>>>>nor in commit message.
>>>>>>>>>>>
>>>>>>>>>>>LS
>>>>>>>>>>
>>>>>>>>>>In first mail I wrote:
>>>>>>>>>>
>>>>>>>>>>>>>>  I noticed that some header files are not including
>>>>>>>>>>>>>>directly header files which they use and thus are dependent
>>>>>>>>>>>>>>on being included in particular order.
>>>>>>>>>>
>>>>>>>>>>If we have header file A that uses declarations from other
>>>>>>>>>>header file B and
>>>>>>>>>>B is not directly included from A then compilation of source
>>>>>>>>>>file that
>>>>>>>>>>includes header file A will fail unless header file B is
>>>>>>>>>>included before
>>>>>>>>>>header A. This is not desired behavior. I don't think this is a
>>>>>>>>>>big problem
>>>>>>>>>>and I don't think we should spent time actively fixing this.
>>>>>>>>>>Still I hit this
>>>>>>>>>>problem and I fixed it in the files that failed for me. I don't
>>>>>>>>>>see any
>>>>>>>>>>reason why not to merge such improvements. Do you?
>>>>>>>>>>
>>>>>>>>>I'm not stupid I know why we need to include missing header file.
>>>>>>>>I'm sorry if I offended you. It was not my intention.
>>>>>>>>
>>>>>>>>>But it's not cleaner which header file need to be included and why?
>>>>>>>>
>>>>>>>>OK, I'll add explanation to commit messages.
>>>>>>>>>
>>>>>>>>>So could you be more concreate?
>>>>>>>>>
>>>>>>>>>e.g.: We need to include talloc.h because talloc memory context
>>>>>>>>>are used in
>>>>>>>>>header file.
>>>>>>>>>
>>>>>>>>>BTW the main problem with including util.h is that include many
>>>>>>>>>header files,
>>>>>>>>>which is not necessary in many cases. And header files should
>>>>>>>>>not include
>>>>>>>>>unnecessary header files. It significantly slow down compilation
>>>>>>>>>on slow
>>>>>>>>>processors (arm development boards e.g. raspberry pi, ...)
>>>>>>>>
>>>>>>>>Yes I agree, so if header file is missing just declaration of
>>>>>>>>errno_t then including just 'util/util_errors.h' is fine with you?
>>>>>>>>
>>>>>>>>BTW should every header file contain as first header file
>>>>>>>>'config.h'?
>>>>>>>>
>
>Just a note here. Header file 'config.h' should be somehow propagated
>to all source files. It will not cause warnings if the header file
>is missing somewhere but it may change behavior of code. If it is
>included as part of other header file it is OK, but IMO it is less
>error prone if it is included explicitly all the time.
>
>>>>>>>If it is required then it should be included otherwise no.
>>>>>>>ATM we do not have any compilation warnings caused by missing
>>>>>>>header files.
>>>>>>>So could you explain why we need to include them? It would make sense
>>>>>>
>>>>>>In my first post I wrote that I was playing around with some code
>>>>>>analysis tool. It didn't work because header files didn't include
>>>>>>necessary header files on their own. In latter discussion We both
>>>>>>agreed this is not optimal.
>>>>>>
>>>>>You didn't mentioned which tool.
>>>>>So it cannot be verified.
>>>>
>>>>If the header file contains errno_t or TALLOC_CTX and no include or
>>>>forward
>>>>declaration what so ever what's there to verify? But OK, I think I
>>>>see your
>>>>point now. I assume I could write a dummy.c and include the offending
>>>>headers
>>>>and show you the compiler errors.
>>>>
>>>>>
>>>>>>>to do it as part of refactoring.
>>>>>>>But there are higher priority things which could be done instead.
>>>>>>
>>>>>>Yes, there are - I already wrote the same myself. This patch was
>>>>>>just by-product of my messing around. I decided to share it as I
>>>>>>thought it improves SSSD a tiny bit. If you don't think it improves
>>>>>>SSSD at all then we can drop it.
>>>>>>
>>>>>The patch would make sense if we could prevent such problems in future.
>>>>>In another words: The tesst would be part of CI script.
>>>>>Otherwise such "potential problems" can be created in future.
>>>>
>>>>Generally the fact that you can't prevent something from happening in
>>>>future should not be a block for fixing it now.
>>>>
>>>It is the same use-case as with trailing whitespace.
>>>It's error-prone to rely on reviewer. If something is possible to
>>>automate then
>>>it should be automated. It does not make a sense to send patches every
>>>week
>>>which fixes triling whitespaces.
>>
>>Yes, it's the same use-case as with trailing whitespace. I don't think
>>that the test was worth the effort.
>>"Patches every week" - I don't think this is the case - AFAIR the
>>trailing spaces I fixed were old months and were quite few.
>>
>>>
>>>You mentioned that you played with code analysis tool. So you didn't
>>>check
>>>all files manually. This is exactly the purpose of static analyzers.
>>
>>Static analyzer just didn't work on those files (just the files I
>>randomly picked) as it could not compile them.
>>
>>>To do boring stuff instead of people. Could you explain why it connot
>>>be incorporated to the our CI script? or does it mean that you do not
>>
>>It didn't analyzed header file dependencies (the tool analyzed
>>cyclomatic complexity). If you want test for that then I suppose you can
>>do something like generate a source file for every header file and make
>>the source file to include the header file then try to compile it and
>>watch for relevant errors.
>>
>>>want to improve code quality of sssd?
>>Right, that's why I'm sending such patches at first place.
>>
>>
>>>
>>>LS
>
>The automatic detection of this would be 'nice to have', but
>probably not a 'must have'. I would proceed with review of this
>patch and the automatic detection can be done as a separate
>ticket (if ever).
>
I'm sorry but I have different opinion.
It is ideal task for automated test. It can tested directly or
indirectly by "static analysis tool" which we do not know the name yet.
If author does not want to share details (he could do in the 1st mail)
then he can maintain such trivial patch himself. I have many WIP patches
in may local git which are not suitable for upstream or does not have a test.

Summary:
A) We do not have any related compile time warning at the moment. So we cannot
   rely at the compiler. (If you know about such compiler warning we can enable
   it.)
B) We do not have test which could prevent such mistake in futures.
C) It should be tested with with enabled/disabled optional features.
   e.g. systemd,sysv nss,libcrypto...

As you can see from summary. It's ideal task for our CI script.
Moreover, we already have a precedent for similar feature in CI script.
We generate code coverage of unit tests (+ html report).
If code coverage is lower then expected level than CI script fails.
It failed on debian when we changed requirements for cmocka >= 1.0.

>Pavel can you please file a ticket with explanation how you
>detected these missing header files (how you hit the problem,
>what tools complaint about it etc.)
>and that it would nice to automate detection of similar issues.
>Maybe it will end up in deferred if we find it too difficult
>to do.
>
>Lukas, will you continue the review of this patch?
I will after we will have a reasonable way how to prevent such issues in
future.

LS


More information about the sssd-devel mailing list