[PATCH v5] xtables: Add an interval option for xtables lock wait

[PATCH v5] xtables: Add an interval option for xtables lock wait

[Subash Abhinov Kasiviswanathan]

ip[6]tables currently waits for 1 second for the xtables lock to be
freed if the -w option is used. We have seen that the lock is held
much less than that resulting in unnecessary delay when trying to
acquire the lock. This problem is even severe in case of latency
sensitive applications.

Introduce a new option 'W' to specify the wait interval in decimal
format [seconds.microseconds]. If this option is not specified,
the command sleeps for 1 second by default.

v1->v2: Change behavior to take millisecond sleep as an argument to
-w as suggested by Pablo. Also maintain current behavior for -w to
sleep for 1 second as mentioned by Liping.

v2->v3: Move the millisecond behavior to a new option as suggested
by Pablo.

v3->v4: Use select instead of usleep. Sleep every iteration for
the time specified in the "-W" argument. Update man page.

v4->v5: Fix compilation error when enabling nftables

Cc: Liping Zhang <zlpnobody@gmail.com>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 iptables/ip6tables.c   | 26 ++++++++++++++++++++++++--
 iptables/iptables.8.in |  7 +++++++
 iptables/iptables.c    | 26 ++++++++++++++++++++++++--
 iptables/xshared.c     | 31 +++++++++++++++++++++++--------
 iptables/xshared.h     |  2 +-
 iptables/xtables.c     | 25 ++++++++++++++++++++++++-
 6 files changed, 103 insertions(+), 14 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index c48ddf9..9629403 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -103,6 +103,7 @@ static struct option original_opts[] = {
 	{.name = "out-interface", .has_arg = 1, .val = 'o'},
 	{.name = "verbose",       .has_arg = 0, .val = 'v'},
 	{.name = "wait",          .has_arg = 2, .val = 'w'},
+	{.name = "wait-interval", .has_arg = 2, .val = 'W'},
 	{.name = "exact",         .has_arg = 0, .val = 'x'},
 	{.name = "version",       .has_arg = 0, .val = 'V'},
 	{.name = "help",          .has_arg = 2, .val = 'h'},
@@ -260,6 +261,9 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "  --table	-t table	table to manipulate (default: `filter')\n"
 "  --verbose	-v		verbose mode\n"
 "  --wait	-w [seconds]	wait for the xtables lock\n"
+"  --wait-interval	-W [seconds.microseconds]\n"
+"				interval to wait for xtables lock\n"
+"				default is 1 second\n"
 "  --line-numbers		print line numbers when listing\n"
 "  --exact	-x		expand numbers (display exact values)\n"
 /*"[!] --fragment	-f		match second or further fragments only\n"*/
@@ -1325,6 +1329,7 @@ int do_command6(int argc, char *argv[], char **table,
 
 	int verbose = 0;
 	int wait = 0;
+	double wait_interval = 1;
 	const char *chain = NULL;
 	const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
 	const char *policy = NULL, *newname = NULL;
@@ -1360,7 +1365,7 @@ int do_command6(int argc, char *argv[], char **table,
 
 	opts = xt_params->orig_opts;
 	while ((cs.c = getopt_long(argc, argv,
-	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:bvw::nt:m:xc:g:46",
+	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:bvwW::nt:m:xc:g:46",
 					   opts, NULL)) != -1) {
 		switch (cs.c) {
 			/*
@@ -1616,6 +1621,23 @@ int do_command6(int argc, char *argv[], char **table,
 						"wait seconds not numeric");
 			break;
 
+		case 'W':
+			if (restore) {
+				xtables_error(PARAMETER_PROBLEM,
+					      "You cannot use `-W' from "
+					      "ip6tables-restore");
+			}
+			if (optarg) {
+				if (sscanf(optarg, "%lf", &wait_interval) != 1)
+					xtables_error(PARAMETER_PROBLEM,
+						"wait interval not numeric");
+			} else if (optind < argc && argv[optind][0] != '-'
+						 && argv[optind][0] != '!')
+				if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
+					xtables_error(PARAMETER_PROBLEM,
+						"wait interval not numeric");
+			break;
+
 		case 'm':
 			command_match(&cs);
 			break;
@@ -1770,7 +1792,7 @@ int do_command6(int argc, char *argv[], char **table,
 	generic_opt_check(command, cs.options);
 
 	/* Attempt to acquire the xtables lock */
-	if (!restore && !xtables_lock(wait)) {
+	if (!restore && !xtables_lock(wait, wait_interval)) {
 		fprintf(stderr, "Another app is currently holding the xtables lock. ");
 		if (wait == 0)
 			fprintf(stderr, "Perhaps you want to use the -w option?\n");
diff --git a/iptables/iptables.8.in b/iptables/iptables.8.in
index 5a8c7ae..adc9fb3 100644
--- a/iptables/iptables.8.in
+++ b/iptables/iptables.8.in
@@ -370,6 +370,13 @@ the program will exit if the lock cannot be obtained.  This option will
 make the program wait (indefinitely or for optional \fIseconds\fP) until
 the exclusive lock can be obtained.
 .TP
+\fB\-W\fP, \fB\-\-wait-interval\fP [\fIseconds.microseconds\fP]
+Interval to wait per each iteration.
+When running latency sensitive applications, waiting for the xtables lock
+for extended durations may not be acceptable. This option will make each
+iteration take the amount of time specified. The default interval is
+1 second.
+.TP
 \fB\-n\fP, \fB\-\-numeric\fP
 Numeric output.
 IP addresses and port numbers will be printed in numeric format.
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 91617c2..1b26d61 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -100,6 +100,7 @@ static struct option original_opts[] = {
 	{.name = "out-interface", .has_arg = 1, .val = 'o'},
 	{.name = "verbose",       .has_arg = 0, .val = 'v'},
 	{.name = "wait",          .has_arg = 2, .val = 'w'},
+	{.name = "wait-interval", .has_arg = 2, .val = 'W'},
 	{.name = "exact",         .has_arg = 0, .val = 'x'},
 	{.name = "fragments",     .has_arg = 0, .val = 'f'},
 	{.name = "version",       .has_arg = 0, .val = 'V'},
@@ -254,6 +255,9 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "  --table	-t table	table to manipulate (default: `filter')\n"
 "  --verbose	-v		verbose mode\n"
 "  --wait	-w [seconds]	wait for the xtables lock\n"
+"  --wait-interval	-W [seconds.microseconds]\n"
+"				interval to wait for xtables lock\n"
+"				default is 1 second\n"
 "  --line-numbers		print line numbers when listing\n"
 "  --exact	-x		expand numbers (display exact values)\n"
 "[!] --fragment	-f		match second or further fragments only\n"
@@ -1321,6 +1325,7 @@ int do_command4(int argc, char *argv[], char **table,
 
 	int verbose = 0;
 	int wait = 0;
+	double wait_interval = 1;
 	const char *chain = NULL;
 	const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
 	const char *policy = NULL, *newname = NULL;
@@ -1355,7 +1360,7 @@ int do_command4(int argc, char *argv[], char **table,
 	opterr = 0;
 	opts = xt_params->orig_opts;
 	while ((cs.c = getopt_long(argc, argv,
-	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvw::nt:m:xc:g:46",
+	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvwW::nt:m:xc:g:46",
 					   opts, NULL)) != -1) {
 		switch (cs.c) {
 			/*
@@ -1609,6 +1614,23 @@ int do_command4(int argc, char *argv[], char **table,
 						"wait seconds not numeric");
 			break;
 
+		case 'W':
+			if (restore) {
+				xtables_error(PARAMETER_PROBLEM,
+					      "You cannot use `-W' from "
+					      "iptables-restore");
+			}
+			if (optarg) {
+				if (sscanf(optarg, "%lf", &wait_interval) != 1)
+					xtables_error(PARAMETER_PROBLEM,
+						"wait interval not numeric");
+			} else if (optind < argc && argv[optind][0] != '-'
+						 && argv[optind][0] != '!')
+				if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
+					xtables_error(PARAMETER_PROBLEM,
+						"wait interval not numeric");
+			break;
+
 		case 'm':
 			command_match(&cs);
 			break;
@@ -1759,7 +1781,7 @@ int do_command4(int argc, char *argv[], char **table,
 	generic_opt_check(command, cs.options);
 
 	/* Attempt to acquire the xtables lock */
-	if (!restore && !xtables_lock(wait)) {
+	if (!restore && !xtables_lock(wait, wait_interval)) {
 		fprintf(stderr, "Another app is currently holding the xtables lock. ");
 		if (wait == 0)
 			fprintf(stderr, "Perhaps you want to use the -w option?\n");
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 81c2581..d810c98 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -9,12 +9,15 @@
 #include <sys/file.h>
 #include <sys/socket.h>
 #include <sys/un.h>
+#include <sys/time.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <xtables.h>
+#include <math.h>
 #include "xshared.h"
 
 #define XT_LOCK_NAME	"/run/xtables.lock"
+#define BASE_MICROSECONDS	100000
 
 /*
  * Print out any special helps. A user might like to be able to add a --help
@@ -244,9 +247,17 @@ void xs_init_match(struct xtables_match *match)
 		match->init(match->m);
 }
 
-bool xtables_lock(int wait)
+bool xtables_lock(int wait, double wait_interval)
 {
-	int fd, waited = 0, i = 0;
+	int fd, i = 0;
+	double wait_int, wait_fract;
+	struct timeval total_time, wait_time, waited_time;
+
+	wait_fract = modf(wait_interval, &wait_int);
+	total_time.tv_sec = wait;
+	total_time.tv_usec = 0;
+	waited_time.tv_sec = 0;
+	waited_time.tv_usec = 0;
 
 	fd = open(XT_LOCK_NAME, O_CREAT, 0600);
 	if (fd < 0)
@@ -255,12 +266,16 @@ bool xtables_lock(int wait)
 	while (1) {
 		if (flock(fd, LOCK_EX | LOCK_NB) == 0)
 			return true;
-		else if (wait >= 0 && waited >= wait)
-			return false;
-		if (++i % 2 == 0)
+		if (++i % 10 == 0)
 			fprintf(stderr, "Another app is currently holding the xtables lock; "
-				"waiting (%ds) for it to exit...\n", waited);
-		waited++;
-		sleep(1);
+				"waiting (%lds %ldus) for it to exit...\n", waited_time.tv_sec, waited_time.tv_usec);
+		wait_time.tv_sec = wait_int;
+		wait_time.tv_usec = wait_fract*BASE_MICROSECONDS;
+		if (wait != -1)
+			timersub(&total_time, &wait_time, &total_time);
+		timeradd(&waited_time, &wait_time, &waited_time);
+		if (!timerisset(&total_time))
+			return false;
+		select(0, NULL, NULL, NULL, &wait_time);
 	}
 }
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 40dd915..964cd4c 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -85,7 +85,7 @@ extern struct xtables_match *load_proto(struct iptables_command_state *);
 extern int subcmd_main(int, char **, const struct subcommand *);
 extern void xs_init_target(struct xtables_target *);
 extern void xs_init_match(struct xtables_match *);
-extern bool xtables_lock(int wait);
+bool xtables_lock(int wait, double wait_interval);
 
 extern const struct xtables_afinfo *afinfo;
 
diff --git a/iptables/xtables.c b/iptables/xtables.c
index ecd577c..baeb472 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -86,6 +86,7 @@ static struct option original_opts[] = {
 	{.name = "out-interface", .has_arg = 1, .val = 'o'},
 	{.name = "verbose",	  .has_arg = 0, .val = 'v'},
 	{.name = "wait",	  .has_arg = 2, .val = 'w'},
+	{.name = "wait-interval", .has_arg = 2, .val = 'W'},
 	{.name = "exact",	  .has_arg = 0, .val = 'x'},
 	{.name = "fragments",	  .has_arg = 0, .val = 'f'},
 	{.name = "version",	  .has_arg = 0, .val = 'V'},
@@ -239,6 +240,10 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "				network interface name ([+] for wildcard)\n"
 "  --table	-t table	table to manipulate (default: `filter')\n"
 "  --verbose	-v		verbose mode\n"
+"  --wait	-w [seconds]	wait for the xtables lock\n"
+"  --wait-interval	-W [seconds.microseconds]\n"
+"				interval to wait for xtables lock\n"
+"				default is 1 second\n"
 "  --line-numbers		print line numbers when listing\n"
 "  --exact	-x		expand numbers (display exact values)\n"
 "[!] --fragment	-f		match second or further fragments only\n"
@@ -687,6 +692,7 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 	struct xtables_rule_match *matchp;
 	struct xtables_target *t;
 	int wait = 0;
+	double wait_interval = 1;
 
 	memset(cs, 0, sizeof(*cs));
 	cs->jumpto = "";
@@ -716,7 +722,7 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 
 	opts = xt_params->orig_opts;
 	while ((cs->c = getopt_long(argc, argv,
-	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvw::nt:m:xc:g:46",
+	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvwW::nt:m:xc:g:46",
 					   opts, NULL)) != -1) {
 		switch (cs->c) {
 			/*
@@ -1019,6 +1025,23 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 						      "wait seconds not numeric");
 			break;
 
+		case 'W':
+			if (p->restore) {
+				xtables_error(PARAMETER_PROBLEM,
+					      "You cannot use `-W' from "
+					      "iptables-restore");
+			}
+			if (optarg) {
+				if (sscanf(optarg, "%lf", &wait_interval) != 1)
+					xtables_error(PARAMETER_PROBLEM,
+						"wait interval not numeric");
+			} else if (optind < argc && argv[optind][0] != '-'
+						 && argv[optind][0] != '!')
+				if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
+					xtables_error(PARAMETER_PROBLEM,
+						"wait interval not numeric");
+			break;
+
 		case '0':
 			set_option(&cs->options, OPT_LINENUMBERS,
 				   &args->invflags, cs->invert);
-- 
1.9.1

Re: [PATCH v5] xtables: Add an interval option for xtables lock wait

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

Hi Subash,

On Thu, Jun 23, 2016 at 06:44:06PM -0600, Subash Abhinov Kasiviswanathan wrote:
> ip[6]tables currently waits for 1 second for the xtables lock to be
> freed if the -w option is used. We have seen that the lock is held
> much less than that resulting in unnecessary delay when trying to
> acquire the lock. This problem is even severe in case of latency
> sensitive applications.
> 
> Introduce a new option 'W' to specify the wait interval in decimal
> format [seconds.microseconds]. If this option is not specified,
> the command sleeps for 1 second by default.

I'm attaching a patch that applies on top of this.

That I can remember, you only needed to reduce the wait interval, not
to make it ever larger, so I'm simplifying this so -W only takes
microseconds.

I have only included some missing error check in case -W is specified
but -w is not.

I can collapse this patch to yours, unless you have any concern.

[-- Attachment #2: 0001-iptables-revisit-new-W-option.patch --]
[-- Type: text/x-diff, Size: 13013 bytes --]

>From 4644aaabdffaafc488c80b7b81b6be45a297e9e9 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 1 Jul 2016 12:11:59 +0200
Subject: [PATCH] iptables: revisit new -W option

* Simplify -W so it only takes the interval wait in microseconds.
* Bail out if -W is specific but -w is not.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 iptables/ip6tables.c | 35 +++++++++++++++++++-------------
 iptables/iptables.c  | 37 +++++++++++++++++++---------------
 iptables/xshared.c   | 57 +++++++++++++++++++++++++++++++++++++---------------
 iptables/xshared.h   |  4 +++-
 iptables/xtables.c   | 32 ++++++++++++++++-------------
 5 files changed, 104 insertions(+), 61 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 9629403..66df8e9 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -260,8 +260,8 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "				network interface name ([+] for wildcard)\n"
 "  --table	-t table	table to manipulate (default: `filter')\n"
 "  --verbose	-v		verbose mode\n"
-"  --wait	-w [seconds]	wait for the xtables lock\n"
-"  --wait-interval	-W [seconds.microseconds]\n"
+"  --wait	-w [seconds]	maximum wait to acquire xtables lock before give up\n"
+"  --wait-interval -W [usecs]	wait time to try to acquire xtables lock\n"
 "				interval to wait for xtables lock\n"
 "				default is 1 second\n"
 "  --line-numbers		print line numbers when listing\n"
@@ -1329,7 +1329,10 @@ int do_command6(int argc, char *argv[], char **table,
 
 	int verbose = 0;
 	int wait = 0;
-	double wait_interval = 1;
+	struct timeval wait_interval = {
+		.tv_sec	= 1,
+	};
+	bool wait_interval_set = false;
 	const char *chain = NULL;
 	const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
 	const char *policy = NULL, *newname = NULL;
@@ -1365,7 +1368,7 @@ int do_command6(int argc, char *argv[], char **table,
 
 	opts = xt_params->orig_opts;
 	while ((cs.c = getopt_long(argc, argv,
-	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:bvwW::nt:m:xc:g:46",
+	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:bvw::W::nt:m:xc:g:46",
 					   opts, NULL)) != -1) {
 		switch (cs.c) {
 			/*
@@ -1627,15 +1630,15 @@ int do_command6(int argc, char *argv[], char **table,
 					      "You cannot use `-W' from "
 					      "ip6tables-restore");
 			}
-			if (optarg) {
-				if (sscanf(optarg, "%lf", &wait_interval) != 1)
-					xtables_error(PARAMETER_PROBLEM,
-						"wait interval not numeric");
-			} else if (optind < argc && argv[optind][0] != '-'
-						 && argv[optind][0] != '!')
-				if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
-					xtables_error(PARAMETER_PROBLEM,
-						"wait interval not numeric");
+			if (optarg)
+				parse_wait_interval(optarg, &wait_interval);
+			else if (optind < argc &&
+				argv[optind][0] != '-' &&
+				argv[optind][0] != '!')
+				parse_wait_interval(argv[optind++],
+						    &wait_interval);
+
+			wait_interval_set = true;
 			break;
 
 		case 'm':
@@ -1742,6 +1745,10 @@ int do_command6(int argc, char *argv[], char **table,
 		cs.invert = FALSE;
 	}
 
+	if (!wait && wait_interval_set)
+		xtables_error(PARAMETER_PROBLEM,
+			      "--wait-interval only makes sense with --wait\n");
+
 	if (strcmp(*table, "nat") == 0 &&
 	    ((policy != NULL && strcmp(policy, "DROP") == 0) ||
 	    (cs.jumpto != NULL && strcmp(cs.jumpto, "DROP") == 0)))
@@ -1792,7 +1799,7 @@ int do_command6(int argc, char *argv[], char **table,
 	generic_opt_check(command, cs.options);
 
 	/* Attempt to acquire the xtables lock */
-	if (!restore && !xtables_lock(wait, wait_interval)) {
+	if (!restore && !xtables_lock(wait, &wait_interval)) {
 		fprintf(stderr, "Another app is currently holding the xtables lock. ");
 		if (wait == 0)
 			fprintf(stderr, "Perhaps you want to use the -w option?\n");
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 1b26d61..540d111 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -254,9 +254,8 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "				network interface name ([+] for wildcard)\n"
 "  --table	-t table	table to manipulate (default: `filter')\n"
 "  --verbose	-v		verbose mode\n"
-"  --wait	-w [seconds]	wait for the xtables lock\n"
-"  --wait-interval	-W [seconds.microseconds]\n"
-"				interval to wait for xtables lock\n"
+"  --wait	-w [seconds]	maximum wait to acquire xtables lock before give up\n"
+"  --wait-interval -W [usecs]	wait time to try to acquire xtables lock\n"
 "				default is 1 second\n"
 "  --line-numbers		print line numbers when listing\n"
 "  --exact	-x		expand numbers (display exact values)\n"
@@ -1322,10 +1321,12 @@ int do_command4(int argc, char *argv[], char **table,
 	unsigned int nsaddrs = 0, ndaddrs = 0;
 	struct in_addr *saddrs = NULL, *smasks = NULL;
 	struct in_addr *daddrs = NULL, *dmasks = NULL;
-
+	struct timeval wait_interval = {
+		.tv_sec = 1,
+	};
+	bool wait_interval_set = false;
 	int verbose = 0;
 	int wait = 0;
-	double wait_interval = 1;
 	const char *chain = NULL;
 	const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
 	const char *policy = NULL, *newname = NULL;
@@ -1360,7 +1361,7 @@ int do_command4(int argc, char *argv[], char **table,
 	opterr = 0;
 	opts = xt_params->orig_opts;
 	while ((cs.c = getopt_long(argc, argv,
-	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvwW::nt:m:xc:g:46",
+	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvw::W::nt:m:xc:g:46",
 					   opts, NULL)) != -1) {
 		switch (cs.c) {
 			/*
@@ -1620,15 +1621,15 @@ int do_command4(int argc, char *argv[], char **table,
 					      "You cannot use `-W' from "
 					      "iptables-restore");
 			}
-			if (optarg) {
-				if (sscanf(optarg, "%lf", &wait_interval) != 1)
-					xtables_error(PARAMETER_PROBLEM,
-						"wait interval not numeric");
-			} else if (optind < argc && argv[optind][0] != '-'
-						 && argv[optind][0] != '!')
-				if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
-					xtables_error(PARAMETER_PROBLEM,
-						"wait interval not numeric");
+			if (optarg)
+				parse_wait_interval(optarg, &wait_interval);
+			else if (optind < argc &&
+				 argv[optind][0] != '-' &&
+				 argv[optind][0] != '!')
+				parse_wait_interval(argv[optind++],
+						    &wait_interval);
+
+			wait_interval_set = true;
 			break;
 
 		case 'm':
@@ -1731,6 +1732,10 @@ int do_command4(int argc, char *argv[], char **table,
 		cs.invert = FALSE;
 	}
 
+	if (!wait && wait_interval_set)
+		xtables_error(PARAMETER_PROBLEM,
+			      "--wait-interval only makes sense with --wait\n");
+
 	if (strcmp(*table, "nat") == 0 &&
 	    ((policy != NULL && strcmp(policy, "DROP") == 0) ||
 	    (cs.jumpto != NULL && strcmp(cs.jumpto, "DROP") == 0)))
@@ -1781,7 +1786,7 @@ int do_command4(int argc, char *argv[], char **table,
 	generic_opt_check(command, cs.options);
 
 	/* Attempt to acquire the xtables lock */
-	if (!restore && !xtables_lock(wait, wait_interval)) {
+	if (!restore && !xtables_lock(wait, &wait_interval)) {
 		fprintf(stderr, "Another app is currently holding the xtables lock. ");
 		if (wait == 0)
 			fprintf(stderr, "Perhaps you want to use the -w option?\n");
diff --git a/iptables/xshared.c b/iptables/xshared.c
index d810c98..cccb8ae 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -247,15 +247,13 @@ void xs_init_match(struct xtables_match *match)
 		match->init(match->m);
 }
 
-bool xtables_lock(int wait, double wait_interval)
+bool xtables_lock(int wait, struct timeval *wait_interval)
 {
+	struct timeval time_left, wait_time, waited_time;
 	int fd, i = 0;
-	double wait_int, wait_fract;
-	struct timeval total_time, wait_time, waited_time;
 
-	wait_fract = modf(wait_interval, &wait_int);
-	total_time.tv_sec = wait;
-	total_time.tv_usec = 0;
+	time_left.tv_sec = wait;
+	time_left.tv_usec = 0;
 	waited_time.tv_sec = 0;
 	waited_time.tv_usec = 0;
 
@@ -266,16 +264,43 @@ bool xtables_lock(int wait, double wait_interval)
 	while (1) {
 		if (flock(fd, LOCK_EX | LOCK_NB) == 0)
 			return true;
-		if (++i % 10 == 0)
-			fprintf(stderr, "Another app is currently holding the xtables lock; "
-				"waiting (%lds %ldus) for it to exit...\n", waited_time.tv_sec, waited_time.tv_usec);
-		wait_time.tv_sec = wait_int;
-		wait_time.tv_usec = wait_fract*BASE_MICROSECONDS;
-		if (wait != -1)
-			timersub(&total_time, &wait_time, &total_time);
-		timeradd(&waited_time, &wait_time, &waited_time);
-		if (!timerisset(&total_time))
-			return false;
+		if (++i % 10 == 0) {
+			if (wait != -1)
+				fprintf(stderr, "Another app is currently holding the xtables lock; "
+					"still %lds %ldus time ahead to have a chance to grab the lock...\n",
+					time_left.tv_sec, time_left.tv_usec);
+			else
+				fprintf(stderr, "Another app is currently holding the xtables lock; "
+						"waiting for it to exit...\n");
+		}
+
+		wait_time = *wait_interval;
 		select(0, NULL, NULL, NULL, &wait_time);
+		if (wait == -1)
+			continue;
+
+		timeradd(&waited_time, wait_interval, &waited_time);
+		timersub(&time_left, wait_interval, &time_left);
+		if (!timerisset(&time_left))
+			return false;
+	}
+}
+
+void parse_wait_interval(const char *str, struct timeval *wait_interval)
+{
+	unsigned int usec;
+	int ret;
+
+	ret = sscanf(str, "%u", &usec);
+	if (ret == 1) {
+		if (usec > 999999)
+			xtables_error(PARAMETER_PROBLEM,
+				      "too long usec wait %u > 999999 usec",
+				      usec);
+
+		wait_interval->tv_sec = 0;
+		wait_interval->tv_usec = usec;
+		return;
 	}
+	xtables_error(PARAMETER_PROBLEM, "wait interval not numeric");
 }
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 964cd4c..6eb8eb8 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -85,7 +85,9 @@ extern struct xtables_match *load_proto(struct iptables_command_state *);
 extern int subcmd_main(int, char **, const struct subcommand *);
 extern void xs_init_target(struct xtables_target *);
 extern void xs_init_match(struct xtables_match *);
-bool xtables_lock(int wait, double wait_interval);
+bool xtables_lock(int wait, struct timeval *wait_interval);
+
+void parse_wait_interval(const char *str, struct timeval *wait_interval);
 
 extern const struct xtables_afinfo *afinfo;
 
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 84f9efd..15a84a6 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -240,9 +240,8 @@ exit_printhelp(const struct xtables_rule_match *matches)
 "				network interface name ([+] for wildcard)\n"
 "  --table	-t table	table to manipulate (default: `filter')\n"
 "  --verbose	-v		verbose mode\n"
-"  --wait	-w [seconds]	wait for the xtables lock\n"
-"  --wait-interval	-W [seconds.microseconds]\n"
-"				interval to wait for xtables lock\n"
+"  --wait	-w [seconds]	maximum wait to acquire xtables lock before give up\n"
+"  --wait-interval -W [usecs]	wait time to try to acquire xtables lock\n"
 "				default is 1 second\n"
 "  --line-numbers		print line numbers when listing\n"
 "  --exact	-x		expand numbers (display exact values)\n"
@@ -690,9 +689,10 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 {
 	struct xtables_match *m;
 	struct xtables_rule_match *matchp;
+	bool wait_interval_set = false;
+	struct timeval wait_interval;
 	struct xtables_target *t;
 	int wait = 0;
-	double wait_interval = 1;
 
 	memset(cs, 0, sizeof(*cs));
 	cs->jumpto = "";
@@ -722,7 +722,7 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 
 	opts = xt_params->orig_opts;
 	while ((cs->c = getopt_long(argc, argv,
-	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvwW::nt:m:xc:g:46",
+	   "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvw::W::nt:m:xc:g:46",
 					   opts, NULL)) != -1) {
 		switch (cs->c) {
 			/*
@@ -1032,15 +1032,15 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 					      "You cannot use `-W' from "
 					      "iptables-restore");
 			}
-			if (optarg) {
-				if (sscanf(optarg, "%lf", &wait_interval) != 1)
-					xtables_error(PARAMETER_PROBLEM,
-						"wait interval not numeric");
-			} else if (optind < argc && argv[optind][0] != '-'
-						 && argv[optind][0] != '!')
-				if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
-					xtables_error(PARAMETER_PROBLEM,
-						"wait interval not numeric");
+			if (optarg)
+				parse_wait_interval(optarg, &wait_interval);
+			else if (optind < argc &&
+				   argv[optind][0] != '-' &&
+				   argv[optind][0] != '!')
+				parse_wait_interval(argv[optind++],
+						    &wait_interval);
+
+			wait_interval_set = true;
 			break;
 
 		case '0':
@@ -1125,6 +1125,10 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 			"\nThe \"nat\" table is not intended for filtering, "
 			"the use of DROP is therefore inhibited.\n\n");
 
+	if (!wait && wait_interval_set)
+		xtables_error(PARAMETER_PROBLEM,
+			      "--wait-interval only makes sense with --wait\n");
+
 	for (matchp = cs->matches; matchp; matchp = matchp->next)
 		xtables_option_mfcall(matchp->match);
 	if (cs->target != NULL)
-- 
2.1.4

Re: [PATCH v5] xtables: Add an interval option for xtables lock wait

[subashab]

> I'm attaching a patch that applies on top of this.
> 
> That I can remember, you only needed to reduce the wait interval, not
> to make it ever larger, so I'm simplifying this so -W only takes
> microseconds.
> 
> I have only included some missing error check in case -W is specified
> but -w is not.
> 
> I can collapse this patch to yours, unless you have any concern.

Thanks Pablo. Sure, that is fine.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v5] xtables: Add an interval option for xtables lock wait

[Pablo Neira Ayuso]

On Fri, Jul 01, 2016 at 11:14:58AM -0600, subashab@codeaurora.org wrote:
> >I'm attaching a patch that applies on top of this.
> >
> >That I can remember, you only needed to reduce the wait interval, not
> >to make it ever larger, so I'm simplifying this so -W only takes
> >microseconds.
> >
> >I have only included some missing error check in case -W is specified
> >but -w is not.
> >
> >I can collapse this patch to yours, unless you have any concern.
> 
> Thanks Pablo. Sure, that is fine.

OK, I have applied this. Please give it another review and follow up
in case there's some fallout.

Thanks.