diff mbox series

iptables: flush stdout after every verbose log.

Message ID 20200421081542.108296-1-zenczykowski@gmail.com
State Not Applicable
Headers show
Series iptables: flush stdout after every verbose log. | expand

Commit Message

Maciej Żenczykowski April 21, 2020, 8:15 a.m. UTC 
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(-)

Comments

Pablo Neira Ayuso April 28, 2020, 12:05 a.m. UTC | #1 
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.
Maciej Żenczykowski April 28, 2020, 12:14 a.m. UTC | #2 
> 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)
Pablo Neira Ayuso April 28, 2020, 10:30 p.m. UTC | #3 
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?
Maciej Żenczykowski May 9, 2020, 7:30 p.m. UTC | #4 
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?
Pablo Neira Ayuso May 11, 2020, 12:10 p.m. UTC | #5 
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.
Maciej Żenczykowski May 11, 2020, 3:40 p.m. UTC | #6 
> Applied, thanks for explaning.

You're welcome!
diff mbox series

Patch

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) &&