[CRIU] [PATCH 0/2] make: Add compile time testing for system headers

[Kir Kolyshkin]

On 05/08/2013 08:52 AM, Cyrill Gorcunov wrote:
> On Tue, May 07, 2013 at 01:37:43PM -0700, Kir Kolyshkin wrote:
>> The idea is the only reason such code should fail to compile is when
>> struct is undefined, or header file is not found (for which we can add
>> another test if needed).
>>
> Here is somewhat simplier approach for a while, without code duplication
> and explicit acceess to structure members.

This one looks already good enough to me. We can improve it as needed. 
Feel free to use

Acked-by: Kir Kolyshkin <kir at openvz.org>

Please see some minor nitpicks (which you can definitely ignore) below.

> 0001-make-Introduce-compile-time-include-config.h-generat.patch
>  From 3f561d760784e23d4976957d21cc15545e2ee876 Mon Sep 17 00:00:00 2001
> From: Cyrill Gorcunov<gorcunov at openvz.org>
> Date: Fri, 3 May 2013 19:14:55 +0400
> Subject: [PATCH] make: Introduce compile time include/config.h generation
>
> It's being reported that some systems (as Ubuntu 13.04) already
> have struct tcp_repair_opt definition in their system headers.
>
> | sk-tcp.c:25:8: error: redefinition of struct tcp_repair_opt
> | sk-tcp.c:31:2: error: redeclaration of enumerator TCP_NO_QUEUE
>
> So add a facility for compile time testing for reported entities
> to be present on a system. For this we generate include/config.h
> where all tested entries will lay and source code need to include
> it only in places where really needed.
>
> Reported-by: Vasily Averin<vvs at parallels.com>
> Signed-off-by: Cyrill Gorcunov<gorcunov at openvz.org>
> ---
>   Makefile                  |  7 +++++--
>   Makefile.config           | 15 +++++++++++++++
>   scripts/feature-tests.mak | 13 +++++++++++++
>   sk-tcp.c                  |  3 +++
>   4 files changed, 36 insertions(+), 2 deletions(-)
>   create mode 100644 Makefile.config
>   create mode 100644 scripts/feature-tests.mak
>
> diff --git a/Makefile b/Makefile
> index cffab50..c82d12d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -17,6 +17,7 @@ export VERSION_MAJOR VERSION_MINOR VERSION_SUBLEVEL VERSION_EXTRA VERSION_NAME
>   #MAKEFLAGS := -r -R
>   
>   include Makefile.inc
> +include Makefile.config
>   
>   #
>   # Common definitions
> @@ -112,13 +113,14 @@ build-crtools := -r -R -f scripts/Makefile.build makefile=Makefile.crtools obj
>   PROGRAM		:= criu
>   
>   .PHONY: all zdtm test rebuild clean distclean tags cscope	\
> -	docs help pie protobuf arch/$(ARCH) clean-built
> +	docs help pie protobuf arch/$(ARCH) clean-built		\
> +	config

I would not add config to phony targets here, it is not even in this file...

>   
>   ifeq ($(GCOV),1)
>   %.o $(PROGRAM): override CFLAGS += --coverage
>   endif
>   
> -all: pie $(VERSION_HEADER)
> +all: config pie $(VERSION_HEADER)
>   	$(Q) $(MAKE) $(PROGRAM)
>   
>   protobuf/%::
> @@ -158,6 +160,7 @@ clean-built:
>   	$(Q) $(MAKE) $(build)=pie clean
>   	$(Q) $(MAKE) $(build-crtools)=. clean
>   	$(Q) $(MAKE) -C Documentation clean
> +	$(Q) $(RM) ./include/config.h
>   	$(Q) $(RM) ./$(PROGRAM)
>   
>   rebuild: clean-built
> diff --git a/Makefile.config b/Makefile.config
> new file mode 100644
> index 0000000..993968a
> --- /dev/null
> +++ b/Makefile.config
> @@ -0,0 +1,15 @@
> +include scripts/utilities.mak
> +include scripts/feature-tests.mak
> +
> +CONFIG		:= include/config.h
> +
> +$(CONFIG): scripts/utilities.mak scripts/feature-tests.mak
> +	$(E) "  GEN     " $@
> +	$(Q) @echo '#ifndef __CR_CONFIG_H__' > $@
> +	$(Q) @echo '#define __CR_CONFIG_H__' >> $@
> +ifeq ($(call try-cc,$(TCP_REPAIR_TEST),,),y)
> +	$(Q) @echo '#define CONFIG_HAS_TCP_REPAIR' >> $@
> +endif
> +	$(Q) @echo '#endif /* __CR_CONFIG_H__ */' >> $@
> +
> +config: $(CONFIG)

... but rather right here where it is defined.

Otherwise it's complicated to track down if all the phony targets are 
marked as such.

> diff --git a/scripts/feature-tests.mak b/scripts/feature-tests.mak
> new file mode 100644
> index 0000000..c961153
> --- /dev/null
> +++ b/scripts/feature-tests.mak
> @@ -0,0 +1,13 @@
> +define TCP_REPAIR_TEST
> +
> +#include <netinet/tcp.h>
> +
> +int main(void)
> +{
> +	struct tcp_repair_opt opts;
> +	opts.opt_code = TCP_NO_QUEUE;
> +	opts.opt_val = 0;
> +
> +	return opts.opt_val;
> +}
> +endef
> diff --git a/sk-tcp.c b/sk-tcp.c
> index ff51783..4327c85 100644
> --- a/sk-tcp.c
> +++ b/sk-tcp.c
> @@ -18,10 +18,12 @@
>   #include "netfilter.h"
>   #include "image.h"
>   #include "namespaces.h"
> +#include "config.h"
>   
>   #include "protobuf.h"
>   #include "protobuf/tcp-stream.pb-c.h"
>   
> +#ifndef CONFIG_HAS_TCP_REPAIR

Here you could add a comment where you got these definitions from
(like /* Taken from netinet/in.h */ or whatever)
just in case if we would want to update this later.
>   struct tcp_repair_opt {
>   	u32	opt_code;
>   	u32	opt_val;
> @@ -33,6 +35,7 @@ enum {
>   	TCP_SEND_QUEUE,
>   	TCP_QUEUES_NR,
>   };
> +#endif
>   
>   #ifndef TCP_TIMESTAMP
>   #define TCP_TIMESTAMP	24
> -- 1.8.1.4

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20130508/c8aa14cc/attachment.html>