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

2015-10-13 Thread Lukas Slebodnik 
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 
>>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 
>>+
>>+#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 
>>+
>>+#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

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

2015-10-13 Thread Lukas Slebodnik 
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 
>>>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 
>>>+
>>>+#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 
>>>+
>>>+#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'?
>
If it is required then it should be included otherwise no.
ATM we do not have any

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

2015-10-13 Thread Pavel Reichl 



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 
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 
+
+#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 
+
+#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'?


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.


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

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

2015-10-13 Thread Lukas Slebodnik 
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 
>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 
>+
>+#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 
>+
>+#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

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

2015-10-13 Thread Pavel Reichl 



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 
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 
+
+#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 
+
+#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'?


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

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

2015-10-13 Thread Michal Židek 

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 
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 
+
+#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 
+
+#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

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

2015-10-13 Thread Pavel Reichl 



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 
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 
+
+#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 
+
+#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'?


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

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

2015-10-13 Thread Lukas Slebodnik 
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 
>>>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 
>>>+
>>>+#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 
>>>+
>>>+#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.
>>

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

2015-10-12 Thread Pavel Reichl 



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 
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 
+
+#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 
+
+#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.
>From acfdeb192ac361037b9d9f58e75563129af53a41 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Sun, 11 Oct 2015 22:29:44 +0200
Subject: [PATCH] SDAP: add missing header files

---
 src/providers/ldap/ldap_auth.h   | 2 +-
 src/providers/ldap/sdap_async_enum.h | 2 ++
 src/providers/ldap/sdap_autofs.h | 2 ++
 src/providers/ldap/sdap_id_op.h  | 4 
 src/providers/ldap/sdap_sudo.h   | 2 ++
 src/providers/ldap/sdap_users.h  | 2 +-
 6 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/providers/ldap/ldap_auth.h b/src/providers/ldap/ldap_auth.h
index 5fbddd7087dc65ab8bd1df5fb57492d2fc26d0bb..6c6a3cb957058f6747d0e44d5a365d28bf1d59ec 100644
--- a/src/providers/ldap/ldap_auth.h
+++ b/src/providers/ldap/ldap_auth.h
@@ -20,7 +20,7 @@
 #ifndef _LDAP_AUTH_H_
 #define _LDAP_AUTH_H_
 
-#include "config.h"
+#include "util/util.h"
 
 enum pwexpire {
 PWEXPIRE_NONE = 0,
diff --git a/src/providers/ldap/sdap_async_enum.h b/src/providers/ldap/sdap_async_enum.h
index 2da38f988913fa0d6f252697925e50e05eb794a6..72cadce5e54b892877fd24cb2b73d31af864db36 100644
--- a/src/providers/ldap/sdap_async_enum.h
+++ b/src/providers/ldap/sdap_async_enum.h
@@ -26,6 +26,8 @@
 #ifndef _SDAP_ASYNC_ENUM_H_
 #define _SDAP_ASYNC_ENUM_H_
 
+#include "util/util.h"
+
 struct tevent_req *
 sdap_dom_enum_ex_send(TALLOC_CTX *memctx,
   struct tevent_context *ev,
diff --git a/src/providers/ldap/sdap_autofs.h b/src/providers/ldap/sdap_autofs.h
index 0369e2645db556d10fd8b937347a34a2f77ae5f2..c367fd38e7131a947b095584e1b114a4185e8478 100644
--- a/src/providers/ldap/sdap_autofs.h
+++ b/src/providers/ldap/sdap_autofs.h
@@ -25,6 +25,8 @@
 #ifndef _SDAP_AUTOFS_H_
 #define _SDAP_AUTOFS_H_
 
+#include "util/util.h"
+
 int sdap_autofs_init(struct be_ctx *be_ctx,
  struct sdap_id_ctx *id_ctx,
  struct bet_ops **ops,
diff --git a/src/providers/ldap/sdap_id_op.h b/src/providers/ldap/sdap_id_op.h
index b808dd89aebb096b7163c10df39784a54b7e0b03..2448e60c7ea9b7a68fb905f93738500af1ed068f 100644
--- a/src/providers/ldap/sdap_id_op.h
+++ b/src/providers/ldap/sdap_id_op.h
@@ -25,6 +25,10 @@
 #ifndef _SDAP_ID_OP_H_
 #define _SDAP_ID_OP_H_
 
+#include "config.h"
+
+#include 
+
 struct sdap_id_ctx;
 struct sdap_id_conn_ctx;
 
diff --git a/src/providers/ldap/sdap_sudo.h b/src/providers/ldap/sdap_sudo.h
index e2764b90c91470ed64e4c07a792206fa034646a6..1ac222d11062ad7769bf5e69e526c390a54519f3 100644
--- a/src/providers/ldap/sdap_sudo.h
+++ b/src/providers/ldap/sdap_sudo.h
@@ -21,6 +21,8 @@
 #ifndef _SDAP_SUDO_H_
 #define _SDAP_SUDO_H_
 
+#include "util/util.h"
+
 struct sdap_sudo_ctx {
 struct

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

2015-10-12 Thread Lukas Slebodnik 
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 
>>>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 
>>>+
>>>+#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 
>>>+
>>>+#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
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

2015-10-12 Thread Pavel Reichl 



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 
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 
+
+#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 
+
+#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'?

Thanks!



LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

2015-10-12 Thread Pavel Reichl 



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 
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 
+
+#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 
+
+#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?





___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

2015-10-12 Thread Lukas Slebodnik 
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 
>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 
>+
>+#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 
>+
>+#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.
But it's not cleaner which header file need to be included and why?

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, ...)

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

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

2015-10-11 Thread Lukas Slebodnik 
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 
>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 
>+
>+#include "util/util.h"
>+
util.h already includes talloc.h.
Could you explain why do we need to include both of them?



> 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 
>+
>+#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?

@see
src/util/util.h
#ifndef __SSSD_UTIL_H__
#define __SSSD_UTIL_H__

#include "config.h"
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#include 
#include 
#include 
#include 


LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel