[conntrack-tools PATCH r7363 0/5] rbtree alarm review

[conntrack-tools PATCH r7363 0/5] rbtree alarm review

[Max Kellermann]

Hi Pablo,

I have finally been able to take a little time and review your rbtree
based alarm implementation.  I looks good, I haven't found major
showstoppers here.  Here is a small patch set which optimizes the code
a little.

Max

---

Max Kellermann (5):
      minor whitespace cleanup
      use list_for_each_entry()
      make alarm_run_queue a local variable
      use "for" loop instead of "while"
      eliminate duplicate initialization


 include/queue.h |    4 ++--
 src/alarm.c     |   21 +++++++++------------
 src/main.c      |    6 +++---
 src/netlink.c   |   14 +++++++-------
 src/run.c       |   12 ++++++------
 5 files changed, 27 insertions(+), 30 deletions(-)

[conntrack-tools PATCH r7363 2/5] use "for" loop instead of "while"

[Max Kellermann]

---

 src/alarm.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)


diff --git a/src/alarm.c b/src/alarm.c
index 470efdd..a3bdbe2 100644
--- a/src/alarm.c
+++ b/src/alarm.c
@@ -128,15 +128,12 @@ do_alarm_run(struct timeval *next_run)
 
 	gettimeofday(&tv, NULL);
 
-	node = rb_first(&alarm_root);
-	while (node) {
+	for (node = rb_first(&alarm_root); node; node = rb_next(node)) {
 		this = container_of(node, struct alarm_block, node);
 
 		if (timercmp(&this->tv, &tv, >))
 			break;
 
-		node = rb_next(node);
-
 		list_add(&this->list, &alarm_run_queue);
 	}
 

Re: [conntrack-tools PATCH r7363 2/5] use "for" loop instead of "while"

[Pablo Neira Ayuso]

Max Kellermann wrote:
> ---
> 
>  src/alarm.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)

Applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[conntrack-tools PATCH r7363 1/5] eliminate duplicate initialization

[Max Kellermann]

---

 src/alarm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/src/alarm.c b/src/alarm.c
index 5013735..470efdd 100644
--- a/src/alarm.c
+++ b/src/alarm.c
@@ -105,7 +105,7 @@ calculate_next_run(struct timeval *cand,
 struct timeval *
 get_next_alarm_run(struct timeval *next_run)
 {
-	struct rb_node *node = alarm_root.rb_node;
+	struct rb_node *node;
 	struct timeval tv;
 
 	gettimeofday(&tv, NULL);
@@ -122,7 +122,7 @@ get_next_alarm_run(struct timeval *next_run)
 struct timeval *
 do_alarm_run(struct timeval *next_run)
 {
-	struct rb_node *node = alarm_root.rb_node;
+	struct rb_node *node;
 	struct alarm_block *this, *tmp;
 	struct timeval tv;
 

Re: [conntrack-tools PATCH r7363 1/5] eliminate duplicate initialization

[Pablo Neira Ayuso]

Max Kellermann wrote:
> ---
> 
>  src/alarm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Applied. Thanks Max.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[conntrack-tools PATCH r7363 3/5] make alarm_run_queue a local variable

[Max Kellermann]

---

 src/alarm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/src/alarm.c b/src/alarm.c
index a3bdbe2..c86ce44 100644
--- a/src/alarm.c
+++ b/src/alarm.c
@@ -21,7 +21,6 @@
 #include <limits.h>
 
 static struct rb_root alarm_root = RB_ROOT;
-static LIST_HEAD(alarm_run_queue);
 
 void init_alarm(struct alarm_block *t,
 		void *data,
@@ -122,12 +121,14 @@ get_next_alarm_run(struct timeval *next_run)
 struct timeval *
 do_alarm_run(struct timeval *next_run)
 {
+	struct list_head alarm_run_queue;
 	struct rb_node *node;
 	struct alarm_block *this, *tmp;
 	struct timeval tv;
 
 	gettimeofday(&tv, NULL);
 
+	INIT_LIST_HEAD(&alarm_run_queue);
 	for (node = rb_first(&alarm_root); node; node = rb_next(node)) {
 		this = container_of(node, struct alarm_block, node);
 

Re: [conntrack-tools PATCH r7363 3/5] make alarm_run_queue a local variable

[Pablo Neira Ayuso]

Max Kellermann wrote:
> ---
> 
>  src/alarm.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Also applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[conntrack-tools PATCH r7363 4/5] use list_for_each_entry()

[Max Kellermann]

since alarm_run_queue is now a local variable, we can use the faster
list_for_each_entry() instead of list_for_each_entry_safe() /
list_del().
---

 src/alarm.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)


diff --git a/src/alarm.c b/src/alarm.c
index c86ce44..8056ee6 100644
--- a/src/alarm.c
+++ b/src/alarm.c
@@ -123,7 +123,7 @@ do_alarm_run(struct timeval *next_run)
 {
 	struct list_head alarm_run_queue;
 	struct rb_node *node;
-	struct alarm_block *this, *tmp;
+	struct alarm_block *this;
 	struct timeval tv;
 
 	gettimeofday(&tv, NULL);
@@ -138,8 +138,7 @@ do_alarm_run(struct timeval *next_run)
 		list_add(&this->list, &alarm_run_queue);
 	}
 
-	list_for_each_entry_safe(this, tmp, &alarm_run_queue, list) {
-		list_del(&this->list);
+	list_for_each_entry(this, &alarm_run_queue, list) {
 		rb_erase(&this->node, &alarm_root);
 		RB_CLEAR_NODE(&this->node);
 		this->function(this, this->data);

Re: [conntrack-tools PATCH r7363 4/5] use list_for_each_entry()

[Pablo Neira Ayuso]

Max Kellermann wrote:
> since alarm_run_queue is now a local variable, we can use the faster
> list_for_each_entry() instead of list_for_each_entry_safe() /
> list_del().

Applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[conntrack-tools PATCH r7363 5/5] minor whitespace cleanup

[Max Kellermann]

---

 include/queue.h |    4 ++--
 src/alarm.c     |    4 ++--
 src/main.c      |    6 +++---
 src/netlink.c   |   14 +++++++-------
 src/run.c       |   12 ++++++------
 5 files changed, 20 insertions(+), 20 deletions(-)


diff --git a/include/queue.h b/include/queue.h
index 9a5d7b8..5a9cf39 100644
--- a/include/queue.h
+++ b/include/queue.h
@@ -21,8 +21,8 @@ void queue_destroy(struct queue *b);
 unsigned int queue_len(const struct queue *b);
 int queue_add(struct queue *b, const void *data, size_t size);
 void queue_del(struct queue *b, void *data);
-void queue_iterate(struct queue *b, 
-		   const void *data, 
+void queue_iterate(struct queue *b,
+		   const void *data,
 		   int (*iterate)(void *data1, const void *data2));
 
 #endif
diff --git a/src/alarm.c b/src/alarm.c
index 8056ee6..91ee2ca 100644
--- a/src/alarm.c
+++ b/src/alarm.c
@@ -1,6 +1,6 @@
 /*
  * (C) 2006-2008 by Pablo Neira Ayuso <pablo@netfilter.org>
- * 
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -85,7 +85,7 @@ int alarm_pending(struct alarm_block *alarm)
 
 static struct timeval *
 calculate_next_run(struct timeval *cand,
-		   struct timeval *tv, 
+		   struct timeval *tv,
 		   struct timeval *next_run)
 {
 	if (cand->tv_sec != LONG_MAX) {
diff --git a/src/main.c b/src/main.c
index 8221564..b6011f0 100644
--- a/src/main.c
+++ b/src/main.c
@@ -1,6 +1,6 @@
 /*
  * (C) 2006-2007 by Pablo Neira Ayuso <pablo@netfilter.org>
- * 
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -34,7 +34,7 @@ static const char usage_daemon_commands[] =
 	"Daemon mode commands:\n"
 	"  -d [options]\t\tRun in daemon mode\n";
 
-static const char usage_client_commands[] = 
+static const char usage_client_commands[] =
 	"Client mode commands:\n"
 	"  -c, commit external cache to conntrack table\n"
 	"  -f, flush internal and external cache\n"
@@ -244,7 +244,7 @@ int main(int argc, char *argv[])
 			exit(EXIT_FAILURE);
 		} else if (pid)
 			exit(EXIT_SUCCESS);
-		
+
 		setsid();
 
 		close(STDOUT_FILENO);
diff --git a/src/netlink.c b/src/netlink.c
index bb94001..f6a2378 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -1,6 +1,6 @@
 /*
  * (C) 2006 by Pablo Neira Ayuso <pablo@netfilter.org>
- * 
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -35,7 +35,7 @@ int ignore_conntrack(struct nf_conntrack *ct)
 		return 0;
 	}
 
-        /* Accept SNAT'ed traffic: not really coming to the local machine */
+	/* Accept SNAT'ed traffic: not really coming to the local machine */
 	if (nfct_getobjopt(ct, NFCT_GOPT_IS_SNAT)) {
 		debug_ct(ct, "SNAT");
 		return 0;
@@ -54,7 +54,7 @@ static int event_handler(enum nf_conntrack_msg_type type,
 			 struct nf_conntrack *ct,
 			 void *data)
 {
-	/* 
+	/*
 	 * Ignore this conntrack: it talks about a
 	 * connection that is not interesting for us.
 	 */
@@ -94,7 +94,7 @@ int nl_init_event_handler(void)
 
 	/* set up socket buffer size */
 	if (CONFIG(netlink_buffer_size))
-		nfnl_rcvbufsiz(nfct_nfnlh(STATE(event)), 
+		nfnl_rcvbufsiz(nfct_nfnlh(STATE(event)),
 			       CONFIG(netlink_buffer_size));
 	else {
 		socklen_t socklen = sizeof(unsigned int);
@@ -109,7 +109,7 @@ int nl_init_event_handler(void)
 
 	/* ensure that maximum grown size is >= than maximum size */
 	if (CONFIG(netlink_buffer_size_max_grown) < CONFIG(netlink_buffer_size))
-		CONFIG(netlink_buffer_size_max_grown) = 
+		CONFIG(netlink_buffer_size_max_grown) =
 					CONFIG(netlink_buffer_size);
 
 	/* register callback for events */
@@ -122,7 +122,7 @@ static int dump_handler(enum nf_conntrack_msg_type type,
 			struct nf_conntrack *ct,
 			void *data)
 {
-	/* 
+	/*
 	 * Ignore this conntrack: it talks about a
 	 * connection that is not interesting for us.
 	 */
@@ -167,7 +167,7 @@ void nl_resize_socket_buffer(struct nfct_handle *h)
 		return;
 
 	if (s > CONFIG(netlink_buffer_size_max_grown)) {
-		dlog(LOG_WARNING, 
+		dlog(LOG_WARNING,
 		     "maximum netlink socket buffer "
 		     "size has been reached. We are likely to "
 		     "be losing events, this may lead to "
diff --git a/src/run.c b/src/run.c
index f5832bc..6cf259d 100644
--- a/src/run.c
+++ b/src/run.c
@@ -48,7 +48,7 @@ void killer(int foo)
 
 	sigprocmask(SIG_UNBLOCK, &STATE(block), NULL);
 
-	exit(0);			
+	exit(0);
 }
 
 static void child(int foo)
@@ -115,7 +115,7 @@ init(void)
 	}
 
 	if (nl_init_event_handler() == -1) {
-		dlog(LOG_ERR, "can't open netlink handler: %s", 
+		dlog(LOG_ERR, "can't open netlink handler: %s",
 		     strerror(errno));
 		dlog(LOG_ERR, "no ctnetlink kernel support?");
 		return -1;
@@ -128,7 +128,7 @@ init(void)
 		return -1;
 	}
 
-        /* Signals handling */
+	/* Signals handling */
 	sigemptyset(&STATE(block));
 	sigaddset(&STATE(block), SIGTERM);
 	sigaddset(&STATE(block), SIGINT);
@@ -177,7 +177,7 @@ static void __run(struct timeval *next_alarm)
 	}
 
 	/* signals are racy */
-	sigprocmask(SIG_BLOCK, &STATE(block), NULL);		
+	sigprocmask(SIG_BLOCK, &STATE(block), NULL);
 
 	/* order received via UNIX socket */
 	if (FD_ISSET(STATE(local).fd, &readfds))
@@ -189,8 +189,8 @@ static void __run(struct timeval *next_alarm)
 		if (ret == -1) {
 			switch(errno) {
 			case ENOBUFS:
-                		/*
-		 		 * It seems that ctnetlink can't back off,
+				/*
+				 * It seems that ctnetlink can't back off,
 				 * it's likely that we're losing events.
 				 * Solution: duplicate the socket buffer
 				 * size and resync with master conntrack table.

Re: [conntrack-tools PATCH r7363 5/5] minor whitespace cleanup

[Pablo Neira Ayuso]

Max Kellermann wrote:
> ---
> 
>  include/queue.h |    4 ++--
>  src/alarm.c     |    4 ++--
>  src/main.c      |    6 +++---
>  src/netlink.c   |   14 +++++++-------
>  src/run.c       |   12 ++++++------
>  5 files changed, 20 insertions(+), 20 deletions(-)

Applied. Thanks.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [conntrack-tools PATCH r7363 0/5] rbtree alarm review

[Pablo Neira Ayuso]

Hi Max,

Max Kellermann wrote:
> I have finally been able to take a little time and review your rbtree
> based alarm implementation.  I looks good, I haven't found major
> showstoppers here.  Here is a small patch set which optimizes the code
> a little.

Again, thanks for the review and the patches.

Cheers,
Pablo

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers