Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iptables: flush stdout after every verbose log.
@ 2020-04-21  8:15 Maciej Żenczykowski
  2020-04-28  0:05 ` Pablo Neira Ayuso
  2020-05-11 12:10 ` Pablo Neira Ayuso
  0 siblings, 2 replies; 7+ messages in thread
From: Maciej Żenczykowski @ 2020-04-21  8:15 UTC (permalink / raw)
  To: Maciej Żenczykowski, Pablo Neira Ayuso, Florian Westphal
  Cc: Linux Network Development Mailing List,
	Netfilter Development Mailing List

From: Maciej Żenczykowski <maze@google.com>

Ensures that each logged line is flushed to stdout after it's
written, and not held in any buffer.

Places to modify found via:
  git grep -C5 'fputs[(]buffer, stdout[)];'

On Android iptables-restore -v is run as netd daemon's child process
and fed actions via pipe.  '#PING' is used to verify the child
is still responsive, and thus needs to be unbuffered.

Luckily if you're running iptables-restore in verbose mode you
probably either don't care about performance or - like Android
- actually need this.

Test: builds, required on Android for ip6?tables-restore netd
  subprocess health monitoring.
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 iptables/iptables-restore.c | 4 +++-
 iptables/xtables-restore.c  | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index b0a51d49..fea04842 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -178,8 +178,10 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
 		if (buffer[0] == '\n')
 			continue;
 		else if (buffer[0] == '#') {
-			if (verbose)
+			if (verbose) {
 				fputs(buffer, stdout);
+				fflush(stdout);
+			}
 			continue;
 		} else if ((strcmp(buffer, "COMMIT\n") == 0) && (in_table)) {
 			if (!testing) {
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index c472ac9b..8c25e5b2 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -85,8 +85,10 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 	if (buffer[0] == '\n')
 		return;
 	else if (buffer[0] == '#') {
-		if (verbose)
+		if (verbose) {
 			fputs(buffer, stdout);
+			fflush(stdout);
+		}
 		return;
 	} else if (state->in_table &&
 		   (strncmp(buffer, "COMMIT", 6) == 0) &&
-- 
2.26.1.301.g55bc3eb7cb9-goog


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iptables: flush stdout after every verbose log.
  2020-04-21  8:15 [PATCH] iptables: flush stdout after every verbose log Maciej Żenczykowski
@ 2020-04-28  0:05 ` Pablo Neira Ayuso
  2020-04-28  0:14   ` Maciej Żenczykowski
  2020-05-11 12:10 ` Pablo Neira Ayuso
  1 sibling, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2020-04-28  0:05 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Maciej Żenczykowski, Florian Westphal,
	Linux Network Development Mailing List,
	Netfilter Development Mailing List

On Tue, Apr 21, 2020 at 01:15:42AM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Ensures that each logged line is flushed to stdout after it's
> written, and not held in any buffer.
> 
> Places to modify found via:
>   git grep -C5 'fputs[(]buffer, stdout[)];'
> 
> On Android iptables-restore -v is run as netd daemon's child process
> and fed actions via pipe.  '#PING' is used to verify the child
> is still responsive, and thus needs to be unbuffered.
> 
> Luckily if you're running iptables-restore in verbose mode you
> probably either don't care about performance or - like Android
> - actually need this.

Could you check if this slows down iptables-restore?

Thank you.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iptables: flush stdout after every verbose log.
  2020-04-28  0:05 ` Pablo Neira Ayuso
@ 2020-04-28  0:14   ` Maciej Żenczykowski
  2020-04-28 22:30     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Maciej Żenczykowski @ 2020-04-28  0:14 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Florian Westphal, Linux Network Development Mailing List,
	Netfilter Development Mailing List

> Could you check if this slows down iptables-restore?

per the iptables-restore man page
       -v, --verbose
              Print additional debug info during ruleset processing.

Well, if you run it with verbose mode enabled you probably don't care
about performance all that much...
And it only does anything if you're feeding in comments, which again -
is unlikely.

iptables-save produces two comment lines per table, so you're likely
to have a grand total of 8 of these lines
(if you have raw/nat/mangle/filter tables all listed), and if you feed
that in to iptables-restore -v then
you end up calling flush 8 times, which triggers 8 extra write()
syscalls - which aren't exactly expensive to begin with...

So I think this is a non issue.

(I could change it so that only "#PING" flushes, which is the actual
comment netd sends for healthchecking,
but I just don't think it matters)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iptables: flush stdout after every verbose log.
  2020-04-28  0:14   ` Maciej Żenczykowski
@ 2020-04-28 22:30     ` Pablo Neira Ayuso
  2020-05-09 19:30       ` Maciej Żenczykowski
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2020-04-28 22:30 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Florian Westphal, Linux Network Development Mailing List,
	Netfilter Development Mailing List

On Mon, Apr 27, 2020 at 05:14:24PM -0700, Maciej Żenczykowski wrote:
> > Could you check if this slows down iptables-restore?
> 
> per the iptables-restore man page
>        -v, --verbose
>               Print additional debug info during ruleset processing.
> 
> Well, if you run it with verbose mode enabled you probably don't care
> about performance all that much...

Thanks for explaining.

How long has this been broken? I mean, netd has been there for quite a
while interacting with iptables. However, the existing behaviour was
not a problem? Or a recent bug?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iptables: flush stdout after every verbose log.
  2020-04-28 22:30     ` Pablo Neira Ayuso
@ 2020-05-09 19:30       ` Maciej Żenczykowski
  0 siblings, 0 replies; 7+ messages in thread
From: Maciej Żenczykowski @ 2020-05-09 19:30 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Florian Westphal, Linux Network Development Mailing List,
	Netfilter Development Mailing List

I don't think it's ever been broken per say since Android has it's own
copy of everything,
and there are local changes to iptables (that I'm trying to
cleanup/drop/upstream),
so we've carried this patch for years.

On Tue, Apr 28, 2020 at 3:30 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Mon, Apr 27, 2020 at 05:14:24PM -0700, Maciej Żenczykowski wrote:
> > > Could you check if this slows down iptables-restore?
> >
> > per the iptables-restore man page
> >        -v, --verbose
> >               Print additional debug info during ruleset processing.
> >
> > Well, if you run it with verbose mode enabled you probably don't care
> > about performance all that much...
>
> Thanks for explaining.
>
> How long has this been broken? I mean, netd has been there for quite a
> while interacting with iptables. However, the existing behaviour was
> not a problem? Or a recent bug?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iptables: flush stdout after every verbose log.
  2020-04-21  8:15 [PATCH] iptables: flush stdout after every verbose log Maciej Żenczykowski
  2020-04-28  0:05 ` Pablo Neira Ayuso
@ 2020-05-11 12:10 ` Pablo Neira Ayuso
  2020-05-11 15:40   ` Maciej Żenczykowski
  1 sibling, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2020-05-11 12:10 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Maciej Żenczykowski, Florian Westphal,
	Linux Network Development Mailing List,
	Netfilter Development Mailing List

On Tue, Apr 21, 2020 at 01:15:42AM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Ensures that each logged line is flushed to stdout after it's
> written, and not held in any buffer.
> 
> Places to modify found via:
>   git grep -C5 'fputs[(]buffer, stdout[)];'
> 
> On Android iptables-restore -v is run as netd daemon's child process
> and fed actions via pipe.  '#PING' is used to verify the child
> is still responsive, and thus needs to be unbuffered.
> 
> Luckily if you're running iptables-restore in verbose mode you
> probably either don't care about performance or - like Android
> - actually need this.

Applied, thanks for explaning.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iptables: flush stdout after every verbose log.
  2020-05-11 12:10 ` Pablo Neira Ayuso
@ 2020-05-11 15:40   ` Maciej Żenczykowski
  0 siblings, 0 replies; 7+ messages in thread
From: Maciej Żenczykowski @ 2020-05-11 15:40 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Florian Westphal, Linux Network Development Mailing List,
	Netfilter Development Mailing List

> Applied, thanks for explaning.

You're welcome!

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21  8:15 [PATCH] iptables: flush stdout after every verbose log Maciej Żenczykowski
2020-04-28  0:05 ` Pablo Neira Ayuso
2020-04-28  0:14   ` Maciej Żenczykowski
2020-04-28 22:30     ` Pablo Neira Ayuso
2020-05-09 19:30       ` Maciej Żenczykowski
2020-05-11 12:10 ` Pablo Neira Ayuso
2020-05-11 15:40   ` Maciej Żenczykowski

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git