[PATCH] correct order of include files to include replace.h first
Stefan (metze) Metzmacher
metze at samba.org
Thu Jul 16 22:48:27 UTC 2015
Hi Jeremy,
>> Attached pls find fixed patch set. Changes are:
>> - Split the second patch into several patches, one patch per subdir
>> - Remove some included sys/xxx files
>> - Also fix vfs_btrfs.c include order. The main problem was that when
>> config.h is included correctly, then replace/system/filesys.h includes
>> <sys/mount.h>, and that clashes with <linux/fs.h>. Because vfs_btrfs.c
>> is linux-specific, I opted to removing system/filesys.h.
>> - To prevent this error from recurring, instead of changing the
>> generated config.h by waf - change replace.h
>>
>> v2 1/8 - same as v1 1/3
>> v2 2/8 to 6/8 - a split of v1 2/3, also removing some system include files
>> v2 7/8 - fix vfs_btrfs.c
>> v2 8/8 - change replace.h to prevent such errors in the future
>
> Reviewed-by: Jeremy Allison <jra at samba.org>
>
> Can I get a second Team reviewer ?
See my comments below...
>> On Thu, Jul 9, 2015 at 12:55 AM, Stefan (metze) Metzmacher
>> <metze at samba.org> wrote:
>>> Hi Uri,
>>>
>>>> This is a re-submission of a patch series, because the previous
>>>> submission was part of a thread and may have gone unnoticed.
>>>> The patches change order of include files to make sure replace.h is
>>>> included first. I am told this is the convention.
>>>>
>>>> 1/3 - This is an actual bug fix, causing fcntl locking not to work in
>>>> 32-bit x86.
>>>
>>> Reviewed-by: me
>>>
>>>> 2/3 - Fix order of includes in other files, to comply with the
>>>> convention, but with no actual known bug that it fixes
>>>
>>> Can you please split this patch? One commit per sub directory please.
>>> This makes potential backporting easier. In some places you could
>>> also try to remove the direct include of system headers, we should
>>> try to use "system/*.h" instead of possible.
>>>
>>>> 3/3 - a change to the build system to catch all those files in 2/3 -
>>>> This is a change to waf so I don't really expect it to be pushed
>>>> as-is, it's just an idea of what can be done to automate this check.
>>>
>>> Why don't you fix source3/modules/vfs_btrfs.c
>>> similar to the 2nd patch.
>>>
>>> Instead of modifying third_party/waf/wafadmin/Tools/config_c.py,
>>> I guess we can add somthing like this to lib/replace/replace.h.
>>>
>>> metze
>>>
>
>> From 1ea7554ed87b622801e7d0602c659ae1b4c74921 Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Thu, 2 Jul 2015 20:09:02 +0300
>> Subject: [PATCH v2 1/8] util.c: fix order of inclusion to correctly consume
>> config.h
>>
>> replace.h has to be the first file included in order to correctly act
>> upon the definitions in config.h.
>>
>> Specifically, this change fixes 32-bit i686 builds, which depend upon
>> _FILE_OFFSET_BITS=64 to be set before any standard library file is
>> included.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> lib/util/util.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/util/util.c b/lib/util/util.c
>> index 4f0e67f..393d83d 100644
>> --- a/lib/util/util.c
>> +++ b/lib/util/util.c
>> @@ -22,8 +22,8 @@
>> along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <talloc.h>
>> #include "replace.h"
>> +#include <talloc.h>
>> #include "system/network.h"
>> #include "system/filesys.h"
>> #include "system/locale.h"
>> --
>> 1.9.1
>>
>>
>> From 1461dd5caf08c40d89ea65d6e401ef1b53768345 Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 09:29:13 +0300
>> Subject: [PATCH v2 2/8] tdbrestore: include config.h before any glibc headers
>>
>> config.h may have some flags which affect glibc behavior, e.g.
>> _FILE_OFFSET_BITS=64. To make sure these flags have the desired
>> effect, config.h must be included before any glibc header files.
>>
>> This commit does not fix a specific known bug. It changes the code to
>> comply with coding conventions.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> lib/tdb/tools/tdbrestore.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/tdb/tools/tdbrestore.c b/lib/tdb/tools/tdbrestore.c
>> index f65b36f..81c986c 100644
>> --- a/lib/tdb/tools/tdbrestore.c
>> +++ b/lib/tdb/tools/tdbrestore.c
>> @@ -17,8 +17,8 @@
>> along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <assert.h>
>> #include "replace.h"
>> +#include <assert.h>
>> #include "system/locale.h"
>> #include "system/time.h"
>> #include "system/filesys.h"
>> --
>> 1.9.1
>>
>>
>> From 04af42173b75f3592e043de0406171a85b502756 Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 09:30:36 +0300
>> Subject: [PATCH v2 3/8] lib/util: include config.h before any glibc headers
>>
>> config.h may have some flags which affect glibc behavior, e.g.
>> _FILE_OFFSET_BITS=64. To make sure these flags have the desired
>> effect, config.h must be included before any glibc header files.
>>
>> This commit does not fix a specific known bug. It changes the code to
>> comply with coding conventions.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> lib/util/debug.c | 2 +-
>> lib/util/idtree.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/util/debug.c b/lib/util/debug.c
>> index 726c682..f2a445b 100644
>> --- a/lib/util/debug.c
>> +++ b/lib/util/debug.c
>> @@ -19,8 +19,8 @@
>> along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <talloc.h>
>> #include "replace.h"
>> +#include <talloc.h>
>> #include "system/filesys.h"
>> #include "system/syslog.h"
>> #include "system/locale.h"
>> diff --git a/lib/util/idtree.c b/lib/util/idtree.c
>> index 0056c09..2104c74 100644
>> --- a/lib/util/idtree.c
>> +++ b/lib/util/idtree.c
>> @@ -32,8 +32,8 @@
>> * @file
>> */
>>
>> -#include <talloc.h>
>> #include "replace.h"
>> +#include <talloc.h>
>> #include "debug.h"
>> #include "idtree.h"
>>
>> --
>> 1.9.1
>>
>>
>> From 8f27e3b042f510a37baab4250103028d6fd9f0bc Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 09:31:52 +0300
>> Subject: [PATCH v2 4/8] source3/lib: include config.h before any glibc headers
>>
>> config.h may have some flags which affect glibc behavior, e.g.
>> _FILE_OFFSET_BITS=64. To make sure these flags have the desired
>> effect, config.h must be included before any glibc header files.
>>
>> This commit does not fix a specific known bug. It changes the code to
>> comply with coding conventions.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> source3/lib/messages_dgm_ref.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/source3/lib/messages_dgm_ref.c b/source3/lib/messages_dgm_ref.c
>> index b4511e0..0a6cbf7 100644
>> --- a/source3/lib/messages_dgm_ref.c
>> +++ b/source3/lib/messages_dgm_ref.c
>> @@ -17,8 +17,8 @@
>> * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <talloc.h>
>> #include "replace.h"
>> +#include <talloc.h>
>> #include "messages_dgm.h"
>> #include "messages_dgm_ref.h"
>> #include "lib/util/debug.h"
>> --
>> 1.9.1
>>
>>
>> From a7ae4631a64b4c6786890a8671c0e6c337fb6bda Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 09:36:46 +0300
>> Subject: [PATCH v2 5/8] fssd: include config.h before any glibc headers
>>
>> config.h may have some flags which affect glibc behavior, e.g.
>> _FILE_OFFSET_BITS=64. To make sure these flags have the desired
>> effect, config.h must be included before any glibc header files.
>>
>> This commit does not fix a specific known bug. It changes the code to
>> comply with coding conventions.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> source3/rpc_server/fss/srv_fss_state.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/source3/rpc_server/fss/srv_fss_state.c b/source3/rpc_server/fss/srv_fss_state.c
>> index 97fd0cb..97604f3 100644
>> --- a/source3/rpc_server/fss/srv_fss_state.c
>> +++ b/source3/rpc_server/fss/srv_fss_state.c
>> @@ -17,8 +17,8 @@
>> * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <fcntl.h>
>> #include "source3/include/includes.h"
>> +#include <fcntl.h>
>> #include "source3/include/util_tdb.h"
>> #include "lib/dbwrap/dbwrap.h"
>> #include "lib/dbwrap/dbwrap_open.h"
>> --
>> 1.9.1
>>
>>
>> From 68ddb73dd032473e551f10f0dd050ee29b2e50bf Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 09:38:01 +0300
>> Subject: [PATCH v2 6/8] torture: include config.h before any glibc headers
>>
>> config.h may have some flags which affect glibc behavior, e.g.
>> _FILE_OFFSET_BITS=64. To make sure these flags have the desired
>> effect, config.h must be included before any glibc header files.
>>
>> Also remove inclusion of some system files, relying on
>> replace/system/*.h instead.
>>
>> This commit does not fix a specific known bug. It changes the code to
>> comply with coding conventions.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> source4/torture/local/fsrvp_state.c | 4 +---
>> source4/torture/local/verif_trailer.c | 4 +---
>> 2 files changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/source4/torture/local/fsrvp_state.c b/source4/torture/local/fsrvp_state.c
>> index f9c0f0f..3a08543 100644
>> --- a/source4/torture/local/fsrvp_state.c
>> +++ b/source4/torture/local/fsrvp_state.c
>> @@ -17,11 +17,9 @@
>> along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <sys/types.h>
>> -#include <sys/stat.h>
>> +#include "includes.h"
>> #include <unistd.h>
>>
>> -#include "includes.h"
>> #include "librpc/gen_ndr/security.h"
>> #include "lib/param/param.h"
>> #include "lib/util/dlinklist.h"
>> diff --git a/source4/torture/local/verif_trailer.c b/source4/torture/local/verif_trailer.c
>> index eb95b21..efb2ac7 100644
>> --- a/source4/torture/local/verif_trailer.c
>> +++ b/source4/torture/local/verif_trailer.c
>> @@ -19,11 +19,9 @@
>> along with this program. If not, see <http://www.gnu.org/licenses/>.
>> */
>>
>> -#include <sys/types.h>
>> -#include <sys/stat.h>
>> +#include "includes.h"
>> #include <unistd.h>
>>
>> -#include "includes.h"
>> #include "librpc/gen_ndr/security.h"
>> #include "lib/param/param.h"
>> #include "lib/util/dlinklist.h"
>> --
>> 1.9.1
>>
>>
>> From 2fe336a614e8379bb12b378b47f2330d44f4131b Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 11:19:49 +0300
>> Subject: [PATCH v2 7/8] vfs_btrfs: include config.h before glibc headers
>>
>> Comply with the convention of including config.h before any
>> glibc header file, to make config.h definitions such as
>> _FILE_OFFSET_BITS affect glibc.
>>
>> This commit does not fix a specific known bug. It changes the code to
>> comply with coding conventions.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> source3/modules/vfs_btrfs.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/source3/modules/vfs_btrfs.c b/source3/modules/vfs_btrfs.c
>> index bd95637..3799757 100644
>> --- a/source3/modules/vfs_btrfs.c
>> +++ b/source3/modules/vfs_btrfs.c
>> @@ -19,13 +19,12 @@
>>
>> #include <linux/ioctl.h>
>> #include <linux/fs.h>
>> +#include "includes.h"
>> #include <sys/ioctl.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <dirent.h>
>> #include <libgen.h>
>> -#include "system/filesys.h"
>> -#include "includes.h"
This looks still wrong "includes.h" should be the first one
and "system/filesys.h" should be kept as 2nd, some of the other
might not be needed anymore then.
>> #include "smbd/smbd.h"
>> #include "librpc/gen_ndr/smbXsrv.h"
>> #include "librpc/gen_ndr/ioctl.h"
>> --
>> 1.9.1
>>
>>
>> From ff39fa49497a8e66c0467e8f7be7e47d22979b88 Mon Sep 17 00:00:00 2001
>> From: Uri Simchoni <urisimchoni at gmail.com>
>> Date: Sun, 12 Jul 2015 10:00:27 +0300
>> Subject: [PATCH v2 8/8] replace.h: verify that config.h is included first
>>
>> Add a check before #including config.h that no glibc header has
>> already been included. The importance for this is that some definitions
>> in config.h affect glibc (e.g. _FILE_OFFSET_BITS) and if any glibc header
>> is already included then it's already too late.
>>
>> Exempted from this check are files which also include Python devel
>> headers, because otherwise we have lots of "redefined" warnings, and
>> Python has its own config.h with definitions such as _FILE_OFFSET_BITS.
>>
>> Signed-off-by: Uri Simchoni <urisimchoni at gmail.com>
>> ---
>> lib/replace/replace.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/replace/replace.h b/lib/replace/replace.h
>> index 3ff4e36..6f407e9 100644
>> --- a/lib/replace/replace.h
>> +++ b/lib/replace/replace.h
>> @@ -29,6 +29,10 @@
>> #define _LIBREPLACE_REPLACE_H
>>
>> #ifndef NO_CONFIG_H
>> +#if defined(_FEATURES_H) && !defined(Py_PYCONFIG_H)
>> +#error "config.h must be included before any glibc header file"
>> +#endif
>> +
#error "config.h must be included before any glibc header file"
should be something like this:
#error "config.h via replace.h must be included before any glibc header
file"
in order to indicate that this check comes from replace.h.
Otherwise Reviewed-by: Stefan Metzmacher <metze at samba.org>
metze
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150717/25e43d89/signature.sig>
More information about the samba-technical
mailing list