<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 05/08/2013 08:52 AM, Cyrill Gorcunov
wrote:<br>
</div>
<blockquote cite="mid:20130508155214.GD1791@moon" type="cite">
<pre wrap="">On Tue, May 07, 2013 at 01:37:43PM -0700, Kir Kolyshkin wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
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).
</pre>
</blockquote>
<pre wrap="">
Here is somewhat simplier approach for a while, without code duplication
and explicit acceess to structure members.
</pre>
</blockquote>
<br>
This one looks already good enough to me. We can improve it as
needed. Feel free to use<br>
<br>
Acked-by: Kir Kolyshkin <a class="moz-txt-link-rfc2396E" href="mailto:kir@openvz.org"><kir@openvz.org></a><br>
<br>
Please see some minor nitpicks (which you can definitely ignore)
below.<br>
<br>
<blockquote type="cite">0001-make-Introduce-compile-time-include-config.h-generat.patch<br>
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">From 3f561d760784e23d4976957d21cc15545e2ee876 Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <a class="moz-txt-link-rfc2396E" href="mailto:gorcunov@openvz.org"><gorcunov@openvz.org></a>
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 <a class="moz-txt-link-rfc2396E" href="mailto:vvs@parallels.com"><vvs@parallels.com></a>
Signed-off-by: Cyrill Gorcunov <a class="moz-txt-link-rfc2396E" href="mailto:gorcunov@openvz.org"><gorcunov@openvz.org></a>
---
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</pre>
</div>
</blockquote>
<br>
I would not add config to phony targets here, it is not even in this
file...<br>
<br>
<blockquote type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
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)</pre>
</div>
</blockquote>
<br>
... but rather right here where it is defined.<br>
<br>
Otherwise it's complicated to track down if all the phony targets
are marked as such.<br>
<br>
<blockquote type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
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</pre>
</div>
</blockquote>
<br>
Here you could add a comment where you got these definitions from<br>
(like /* Taken from netinet/in.h */ or whatever)<br>
just in case if we would want to update this later.<br>
<blockquote type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
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
<div class="moz-txt-sig">--
1.8.1.4
</div></pre>
</div>
</blockquote>
<br>
</body>
</html>