diff mbox

[RFA,2/8] netfilter: Remove return values for print_conntrack callbacks

Message ID 20141029220107.465008329@goodmis.org
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Steven Rostedt Oct. 29, 2014, 9:56 p.m. UTC
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

[ REQUEST FOR ACKS ]

The seq_printf() and friends are having their return values removed.
The print_conntrack() returns the result of seq_printf(), which is
meaningless when seq_printf() returns void. Might as well remove the
return values of print_conntrack() as well.

Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: netfilter-devel@vger.kernel.org
Cc: coreteam@netfilter.org
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/net/netfilter/nf_conntrack_l4proto.h          | 2 +-
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c | 5 ++++-
 net/netfilter/nf_conntrack_proto_dccp.c               | 4 ++--
 net/netfilter/nf_conntrack_proto_gre.c                | 8 ++++----
 net/netfilter/nf_conntrack_proto_sctp.c               | 4 ++--
 net/netfilter/nf_conntrack_proto_tcp.c                | 4 ++--
 net/netfilter/nf_conntrack_standalone.c               | 4 ++--
 7 files changed, 17 insertions(+), 14 deletions(-)

Comments

Florian Westphal Oct. 29, 2014, 10:16 p.m. UTC | #1
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> [ REQUEST FOR ACKS ]
> 
> The seq_printf() and friends are having their return values removed.
> The print_conntrack() returns the result of seq_printf(), which is
> meaningless when seq_printf() returns void. Might as well remove the
> return values of print_conntrack() as well.
[..]

> diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> index 4c48e434bb1f..91f207c2cb69 100644
> --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> @@ -147,7 +147,10 @@ static int ct_seq_show(struct seq_file *s, void *v)
>  		      ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
>  		goto release;
>  
> -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> +	if (l4proto->print_conntrack)
> +		l4proto->print_conntrack(s, ct);
> +
> +	if (seq_has_overflowed(s))
>  		goto release;

Its not obvious to me why nf_conntrack_l3proto_ipv4_compat now calls
seq_has_overflowed ...

> > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index cf65a1e040dd..348aa3602787 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -199,8 +199,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
>  		       ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
>  		goto release;
>  
> -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> -		goto release;
> +	if (l4proto->print_conntrack)
> +		l4proto->print_conntrack(s, ct);
>  
>  	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
>  			l3proto, l4proto))

... while nf_conntrack_standalone does not.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Oct. 29, 2014, 11:53 p.m. UTC | #2
On Wed, 29 Oct 2014 23:16:01 +0100
Florian Westphal <fw@strlen.de> wrote:

> Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > [ REQUEST FOR ACKS ]
> > 
> > The seq_printf() and friends are having their return values removed.
> > The print_conntrack() returns the result of seq_printf(), which is
> > meaningless when seq_printf() returns void. Might as well remove the
> > return values of print_conntrack() as well.
> [..]
> 
> > diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> > index 4c48e434bb1f..91f207c2cb69 100644
> > --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> > +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> > @@ -147,7 +147,10 @@ static int ct_seq_show(struct seq_file *s, void *v)
> >  		      ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
> >  		goto release;
> >  
> > -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> > +	if (l4proto->print_conntrack)
> > +		l4proto->print_conntrack(s, ct);
> > +
> > +	if (seq_has_overflowed(s))
> >  		goto release;
> 
> Its not obvious to me why nf_conntrack_l3proto_ipv4_compat now calls
> seq_has_overflowed ...
> 
> > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> > index cf65a1e040dd..348aa3602787 100644
> > --- a/net/netfilter/nf_conntrack_standalone.c
> > +++ b/net/netfilter/nf_conntrack_standalone.c
> > @@ -199,8 +199,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> >  		       ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
> >  		goto release;
> >  
> > -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> > -		goto release;
> > +	if (l4proto->print_conntrack)
> > +		l4proto->print_conntrack(s, ct);
> >  
> >  	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> >  			l3proto, l4proto))
> 
> ... while nf_conntrack_standalone does not.

Because I did those two changes separately at different times ;-)

I can add it here too to be consistent. The goto release logic is not
that critical, as it's really a micro optimization for a slow path.

If we were to remove all the goto release calls, what would happen is
if the buffer were to fill up, the rest of the code would be done in
vain, because it would do the work but the result will be thrown away.
The seq_file code would throw away the buffer completely (does this
regardless of the goto release call when the buffer fills up), and
would allocate a new buffer, and then run the code again. This time it
would hopefully have enough buffer space to hold the entire content,
otherwise, wash rinse repeat.

As this is to output text from a proc file, and the user will most
likely never notice this delay. It's probably done as more of a good
feeling that we didn't waste computer cycles and burn more green house
gasses than necessary, then anything that is remotely noticeable.
Although, I haven't run benchmarks to see what the difference is.

I'll make the change for consistency.

Thanks!

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Oct. 30, 2014, 1:06 a.m. UTC | #3
On Wed, 29 Oct 2014 23:16:01 +0100
Florian Westphal <fw@strlen.de> wrote:

> Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > [ REQUEST FOR ACKS ]
> > 
> > The seq_printf() and friends are having their return values removed.
> > The print_conntrack() returns the result of seq_printf(), which is
> > meaningless when seq_printf() returns void. Might as well remove the
> > return values of print_conntrack() as well.
> [..]
> 
> > diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> > index 4c48e434bb1f..91f207c2cb69 100644
> > --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> > +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> > @@ -147,7 +147,10 @@ static int ct_seq_show(struct seq_file *s, void *v)
> >  		      ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
> >  		goto release;
> >  
> > -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> > +	if (l4proto->print_conntrack)
> > +		l4proto->print_conntrack(s, ct);
> > +
> > +	if (seq_has_overflowed(s))
> >  		goto release;
> 
> Its not obvious to me why nf_conntrack_l3proto_ipv4_compat now calls
> seq_has_overflowed ...
> 
> > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> > index cf65a1e040dd..348aa3602787 100644
> > --- a/net/netfilter/nf_conntrack_standalone.c
> > +++ b/net/netfilter/nf_conntrack_standalone.c
> > @@ -199,8 +199,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> >  		       ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
> >  		goto release;
> >  
> > -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> > -		goto release;
> > +	if (l4proto->print_conntrack)
> > +		l4proto->print_conntrack(s, ct);
> >  
> >  	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> >  			l3proto, l4proto))
> 
> ... while nf_conntrack_standalone does not.

Oh, looks like it was added here in patch 3/8.

The reason for the two patches is that patch 3/8 is from Joe. I added
places that he missed, and put that patch before his (2/8).

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Nov. 4, 2014, 1:05 p.m. UTC | #4
On Wed, 29 Oct 2014 17:56:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> 
> [ REQUEST FOR ACKS ]

Can any of the netfilter folks give me an Acked-by for this?

Thanks!

-- Steve

> 
> The seq_printf() and friends are having their return values removed.
> The print_conntrack() returns the result of seq_printf(), which is
> meaningless when seq_printf() returns void. Might as well remove the
> return values of print_conntrack() as well.
> 
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> Cc: netfilter-devel@vger.kernel.org
> Cc: coreteam@netfilter.org
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/net/netfilter/nf_conntrack_l4proto.h          | 2 +-
>  net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c | 5 ++++-
>  net/netfilter/nf_conntrack_proto_dccp.c               | 4 ++--
>  net/netfilter/nf_conntrack_proto_gre.c                | 8 ++++----
>  net/netfilter/nf_conntrack_proto_sctp.c               | 4 ++--
>  net/netfilter/nf_conntrack_proto_tcp.c                | 4 ++--
>  net/netfilter/nf_conntrack_standalone.c               | 4 ++--
>  7 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
> index 4c8d573830b7..82e4ec002a39 100644
> --- a/include/net/netfilter/nf_conntrack_l4proto.h
> +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> @@ -60,7 +60,7 @@ struct nf_conntrack_l4proto {
>  			   const struct nf_conntrack_tuple *);
>  
>  	/* Print out the private part of the conntrack. */
> -	int (*print_conntrack)(struct seq_file *s, struct nf_conn *);
> +	void (*print_conntrack)(struct seq_file *s, struct nf_conn *);
>  
>  	/* Return the array of timeouts for this protocol. */
>  	unsigned int *(*get_timeouts)(struct net *net);
> diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> index 4c48e434bb1f..91f207c2cb69 100644
> --- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> +++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
> @@ -147,7 +147,10 @@ static int ct_seq_show(struct seq_file *s, void *v)
>  		      ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
>  		goto release;
>  
> -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> +	if (l4proto->print_conntrack)
> +		l4proto->print_conntrack(s, ct);
> +
> +	if (seq_has_overflowed(s))
>  		goto release;
>  
>  	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
> index cb372f96f10d..15971177470a 100644
> --- a/net/netfilter/nf_conntrack_proto_dccp.c
> +++ b/net/netfilter/nf_conntrack_proto_dccp.c
> @@ -626,9 +626,9 @@ static int dccp_print_tuple(struct seq_file *s,
>  			  ntohs(tuple->dst.u.dccp.port));
>  }
>  
> -static int dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> +static void dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
>  {
> -	return seq_printf(s, "%s ", dccp_state_names[ct->proto.dccp.state]);
> +	seq_printf(s, "%s ", dccp_state_names[ct->proto.dccp.state]);
>  }
>  
>  #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
> diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
> index d5665739e3b1..cba607ada069 100644
> --- a/net/netfilter/nf_conntrack_proto_gre.c
> +++ b/net/netfilter/nf_conntrack_proto_gre.c
> @@ -235,11 +235,11 @@ static int gre_print_tuple(struct seq_file *s,
>  }
>  
>  /* print private data for conntrack */
> -static int gre_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> +static void gre_print_conntrack(struct seq_file *s, struct nf_conn *ct)
>  {
> -	return seq_printf(s, "timeout=%u, stream_timeout=%u ",
> -			  (ct->proto.gre.timeout / HZ),
> -			  (ct->proto.gre.stream_timeout / HZ));
> +	seq_printf(s, "timeout=%u, stream_timeout=%u ",
> +		   (ct->proto.gre.timeout / HZ),
> +		   (ct->proto.gre.stream_timeout / HZ));
>  }
>  
>  static unsigned int *gre_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index 1314d33f6bcf..c61f4cd6407d 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -175,7 +175,7 @@ static int sctp_print_tuple(struct seq_file *s,
>  }
>  
>  /* Print out the private part of the conntrack. */
> -static int sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> +static void sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
>  {
>  	enum sctp_conntrack state;
>  
> @@ -183,7 +183,7 @@ static int sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
>  	state = ct->proto.sctp.state;
>  	spin_unlock_bh(&ct->lock);
>  
> -	return seq_printf(s, "%s ", sctp_conntrack_names[state]);
> +	seq_printf(s, "%s ", sctp_conntrack_names[state]);
>  }
>  
>  #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 44d1ea32570a..79668fd3db96 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -311,7 +311,7 @@ static int tcp_print_tuple(struct seq_file *s,
>  }
>  
>  /* Print out the private part of the conntrack. */
> -static int tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> +static void tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
>  {
>  	enum tcp_conntrack state;
>  
> @@ -319,7 +319,7 @@ static int tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
>  	state = ct->proto.tcp.state;
>  	spin_unlock_bh(&ct->lock);
>  
> -	return seq_printf(s, "%s ", tcp_conntrack_names[state]);
> +	seq_printf(s, "%s ", tcp_conntrack_names[state]);
>  }
>  
>  static unsigned int get_conntrack_index(const struct tcphdr *tcph)
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index cf65a1e040dd..348aa3602787 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -199,8 +199,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
>  		       ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
>  		goto release;
>  
> -	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> -		goto release;
> +	if (l4proto->print_conntrack)
> +		l4proto->print_conntrack(s, ct);
>  
>  	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
>  			l3proto, l4proto))

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Nov. 4, 2014, 2:11 p.m. UTC | #5
On Tue, 4 Nov 2014 08:05:35 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 29 Oct 2014 17:56:04 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > [ REQUEST FOR ACKS ]
> 
> Can any of the netfilter folks give me an Acked-by for this?
> 

I hope nobody expects Jozsef Kadlecsik to do this, as I'm getting "Mail
Delivery Failures" from him with this message:

>>> kadlec@blackhole.kfki.hu (after RCPT TO): 550 5.7.1 <kadlec@blackhole.kfki.hu>: Recipient address rejected: Access denied. Your site is banned because of the unsolicited mail messages received from it.  

Wow, seems they are banning all Time Warner Cable customers in my area!

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Nov. 4, 2014, 2:22 p.m. UTC | #6
On Tue, Nov 04, 2014 at 08:05:35AM -0500, Steven Rostedt wrote:
> On Wed, 29 Oct 2014 17:56:04 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > 
> > [ REQUEST FOR ACKS ]
> 
> Can any of the netfilter folks give me an Acked-by for this?

If Florian's concern were addressed, then:

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

to this patch and 4/8.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Nov. 4, 2014, 2:31 p.m. UTC | #7
On Tue, 4 Nov 2014 15:22:36 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Tue, Nov 04, 2014 at 08:05:35AM -0500, Steven Rostedt wrote:
> > On Wed, 29 Oct 2014 17:56:04 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > 
> > > [ REQUEST FOR ACKS ]
> > 
> > Can any of the netfilter folks give me an Acked-by for this?
> 
> If Florian's concern were addressed, then:

Yeah, the change he mentioned was done is 3/8. As that was written by
Joe Perches, I did some work that he missed and put it before his
patch, which showed a discrepancy between the two functions. After all
patches are applied, it should be consistent to his liking.

> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> to this patch and 4/8.

Thanks for this!

Can I get someone to ack 3/8?

Thanks again,

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Nov. 4, 2014, 2:46 p.m. UTC | #8
On Tue, 2014-11-04 at 09:31 -0500, Steven Rostedt wrote:
> On Tue, 4 Nov 2014 15:22:36 +0100
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Nov 04, 2014 at 08:05:35AM -0500, Steven Rostedt wrote:
> > > On Wed, 29 Oct 2014 17:56:04 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > > [ REQUEST FOR ACKS ]
> > > Can any of the netfilter folks give me an Acked-by for this?
> > If Florian's concern were addressed, then:
> Yeah, the change he mentioned was done is 3/8. As that was written by
> Joe Perches, I did some work that he missed and put it before his
> patch, which showed a discrepancy between the two functions. After all
> patches are applied, it should be consistent to his liking.

I think seq_has_overflowed does not need
to be used after every seq_<put/print> call.

It interrupts reading code flow and just
isn't alll that necessary as every operation
before it will be redone anyway.

It should be used before or after a large
blocks though.


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Nov. 4, 2014, 2:46 p.m. UTC | #9
On Tue, Nov 04, 2014 at 09:31:50AM -0500, Steven Rostedt wrote:
> On Tue, 4 Nov 2014 15:22:36 +0100
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > On Tue, Nov 04, 2014 at 08:05:35AM -0500, Steven Rostedt wrote:
> > > On Wed, 29 Oct 2014 17:56:04 -0400
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > > 
> > > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > > 
> > > > [ REQUEST FOR ACKS ]
> > > 
> > > Can any of the netfilter folks give me an Acked-by for this?
> > 
> > If Florian's concern were addressed, then:
> 
> Yeah, the change he mentioned was done is 3/8. As that was written by
> Joe Perches, I did some work that he missed and put it before his
> patch, which showed a discrepancy between the two functions. After all
> patches are applied, it should be consistent to his liking.
> 
> > 
> > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > to this patch and 4/8.
> 
> Thanks for this!
> 
> Can I get someone to ack 3/8?

I don't see any 3/8 in my inbox, only 2/8 and 4/8.

Let me know, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Nov. 4, 2014, 2:48 p.m. UTC | #10
On Tue, 4 Nov 2014 15:46:32 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:


> > Can I get someone to ack 3/8?
> 
> I don't see any 3/8 in my inbox, only 2/8 and 4/8.
> 
> Let me know, thanks.

Strange, you were on the Cc of that patch.

Let me go back and check to make sure that didn't bounce too.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Nov. 4, 2014, 2:52 p.m. UTC | #11
On Tue, 04 Nov 2014 06:46:19 -0800
Joe Perches <joe@perches.com> wrote:

> On Tue, 2014-11-04 at 09:31 -0500, Steven Rostedt wrote:
> > On Tue, 4 Nov 2014 15:22:36 +0100
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Tue, Nov 04, 2014 at 08:05:35AM -0500, Steven Rostedt wrote:
> > > > On Wed, 29 Oct 2014 17:56:04 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > > From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> > > > > [ REQUEST FOR ACKS ]
> > > > Can any of the netfilter folks give me an Acked-by for this?
> > > If Florian's concern were addressed, then:
> > Yeah, the change he mentioned was done is 3/8. As that was written by
> > Joe Perches, I did some work that he missed and put it before his
> > patch, which showed a discrepancy between the two functions. After all
> > patches are applied, it should be consistent to his liking.
> 
> I think seq_has_overflowed does not need
> to be used after every seq_<put/print> call.
> 
> It interrupts reading code flow and just
> isn't alll that necessary as every operation
> before it will be redone anyway.
> 
> It should be used before or after a large
> blocks though.
> 

It's not used in every occurrence. The problem that Florian had was that
there were two almost identical functions, and you changed one to have
the seq_has_overflowed() check, but the other one was left without it.

It wasn't about checking multiple times, it was about consistency
between two similar functions.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt Nov. 4, 2014, 7:59 p.m. UTC | #12
On Tue, 4 Nov 2014 15:46:32 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:


> > > 
> > > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > 
> > > to this patch and 4/8.
> > 
> > Thanks for this!
> > 
> > Can I get someone to ack 3/8?
> 
> I don't see any 3/8 in my inbox, only 2/8 and 4/8.
> 
> Let me know, thanks.

I replied to the patch. Did you get that reply?

Or did it somehow end up in your spam filter?

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
index 4c8d573830b7..82e4ec002a39 100644
--- a/include/net/netfilter/nf_conntrack_l4proto.h
+++ b/include/net/netfilter/nf_conntrack_l4proto.h
@@ -60,7 +60,7 @@  struct nf_conntrack_l4proto {
 			   const struct nf_conntrack_tuple *);
 
 	/* Print out the private part of the conntrack. */
-	int (*print_conntrack)(struct seq_file *s, struct nf_conn *);
+	void (*print_conntrack)(struct seq_file *s, struct nf_conn *);
 
 	/* Return the array of timeouts for this protocol. */
 	unsigned int *(*get_timeouts)(struct net *net);
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
index 4c48e434bb1f..91f207c2cb69 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
@@ -147,7 +147,10 @@  static int ct_seq_show(struct seq_file *s, void *v)
 		      ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
 		goto release;
 
-	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
+	if (l4proto->print_conntrack)
+		l4proto->print_conntrack(s, ct);
+
+	if (seq_has_overflowed(s))
 		goto release;
 
 	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
index cb372f96f10d..15971177470a 100644
--- a/net/netfilter/nf_conntrack_proto_dccp.c
+++ b/net/netfilter/nf_conntrack_proto_dccp.c
@@ -626,9 +626,9 @@  static int dccp_print_tuple(struct seq_file *s,
 			  ntohs(tuple->dst.u.dccp.port));
 }
 
-static int dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
+static void dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 {
-	return seq_printf(s, "%s ", dccp_state_names[ct->proto.dccp.state]);
+	seq_printf(s, "%s ", dccp_state_names[ct->proto.dccp.state]);
 }
 
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index d5665739e3b1..cba607ada069 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -235,11 +235,11 @@  static int gre_print_tuple(struct seq_file *s,
 }
 
 /* print private data for conntrack */
-static int gre_print_conntrack(struct seq_file *s, struct nf_conn *ct)
+static void gre_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 {
-	return seq_printf(s, "timeout=%u, stream_timeout=%u ",
-			  (ct->proto.gre.timeout / HZ),
-			  (ct->proto.gre.stream_timeout / HZ));
+	seq_printf(s, "timeout=%u, stream_timeout=%u ",
+		   (ct->proto.gre.timeout / HZ),
+		   (ct->proto.gre.stream_timeout / HZ));
 }
 
 static unsigned int *gre_get_timeouts(struct net *net)
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 1314d33f6bcf..c61f4cd6407d 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -175,7 +175,7 @@  static int sctp_print_tuple(struct seq_file *s,
 }
 
 /* Print out the private part of the conntrack. */
-static int sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
+static void sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 {
 	enum sctp_conntrack state;
 
@@ -183,7 +183,7 @@  static int sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 	state = ct->proto.sctp.state;
 	spin_unlock_bh(&ct->lock);
 
-	return seq_printf(s, "%s ", sctp_conntrack_names[state]);
+	seq_printf(s, "%s ", sctp_conntrack_names[state]);
 }
 
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 44d1ea32570a..79668fd3db96 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -311,7 +311,7 @@  static int tcp_print_tuple(struct seq_file *s,
 }
 
 /* Print out the private part of the conntrack. */
-static int tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
+static void tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 {
 	enum tcp_conntrack state;
 
@@ -319,7 +319,7 @@  static int tcp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 	state = ct->proto.tcp.state;
 	spin_unlock_bh(&ct->lock);
 
-	return seq_printf(s, "%s ", tcp_conntrack_names[state]);
+	seq_printf(s, "%s ", tcp_conntrack_names[state]);
 }
 
 static unsigned int get_conntrack_index(const struct tcphdr *tcph)
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index cf65a1e040dd..348aa3602787 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -199,8 +199,8 @@  static int ct_seq_show(struct seq_file *s, void *v)
 		       ? (long)(ct->timeout.expires - jiffies)/HZ : 0) != 0)
 		goto release;
 
-	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
-		goto release;
+	if (l4proto->print_conntrack)
+		l4proto->print_conntrack(s, ct);
 
 	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
 			l3proto, l4proto))