[PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void

[PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void

[Joe Perches]

Many uses of the return value of seq_printf/seq_puts/seq_putc are
incorrect.  Many assume that the return value is the number of
chars emitted into a buffer like printf/puts/putc.

It would be better to make the return value of these functions void
to avoid these misuses.

Start to do so.
Convert seq_overflow to a public function from a static function.

Remove the return uses of seq_printf/seq_puts/seq_putc from net.
Add a seq_overflow function call instead.

Joe Perches (3):
  seq: Add a seq_overflow test.
  batman-adv: Use seq_overflow
  netfilter: Use seq_overflow

 fs/seq_file.c                                      | 15 ++++----
 include/linux/seq_file.h                           |  2 +
 include/net/netfilter/nf_conntrack_acct.h          |  3 +-
 net/batman-adv/gateway_client.c                    | 25 ++++++------
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c     |  6 ++-
 .../netfilter/nf_conntrack_l3proto_ipv4_compat.c   | 42 +++++++++++++--------
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c     |  6 ++-
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c     | 10 +++--
 net/netfilter/nf_conntrack_acct.c                  | 11 +++---
 net/netfilter/nf_conntrack_expect.c                |  4 +-
 net/netfilter/nf_conntrack_proto_dccp.c            | 12 ++++--
 net/netfilter/nf_conntrack_proto_gre.c             | 15 +++++---
 net/netfilter/nf_conntrack_proto_sctp.c            | 12 ++++--
 net/netfilter/nf_conntrack_proto_tcp.c             | 11 ++++--
 net/netfilter/nf_conntrack_proto_udp.c             |  7 ++--
 net/netfilter/nf_conntrack_proto_udplite.c         |  7 ++--
 net/netfilter/nf_conntrack_standalone.c            | 44 +++++++++++++---------
 net/netfilter/nf_log.c                             | 26 ++++++-------
 net/netfilter/nfnetlink_log.c                      | 12 +++---
 net/netfilter/nfnetlink_queue_core.c               | 14 ++++---
 net/netfilter/x_tables.c                           |  8 ++--
 net/netfilter/xt_hashlimit.c                       | 34 +++++++++--------
 22 files changed, 191 insertions(+), 135 deletions(-)

-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH -next 1/3] seq: Add a seq_overflow test.

[Joe Perches]

seq_printf and seq_puts returns are often misused.

Instead of checking the seq_printf or seq_puts return,
add a new seq_overflow function to test if a seq_file has
overflowed the available buffer space.

This will eventually allow seq_printf and seq_puts to be
converted to have a void return instead of the int return
that is often assumed to have a size_t value instead of an
error/no-error value.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/seq_file.c            | 15 ++++++++-------
 include/linux/seq_file.h |  2 ++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb..aab0736 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -14,16 +14,17 @@
 #include <asm/uaccess.h>
 #include <asm/page.h>
 
-
-/*
- * seq_files have a buffer which can may overflow. When this happens a larger
- * buffer is reallocated and all the data will be printed again.
- * The overflow state is true when m->count == m->size.
+/**
+ * seq_overflow - test if a seq_file has overflowed the space available
+ * @m: the seq_file handle
+ *
+ * Returns -1 when an overflow has occurred, 0 otherwise.
  */
-static bool seq_overflow(struct seq_file *m)
+int seq_overflow(struct seq_file *m)
 {
-	return m->count == m->size;
+	return m->count == m->size ? -1 : 0;
 }
+EXPORT_SYMBOL(seq_overflow);
 
 static void seq_set_overflow(struct seq_file *m)
 {
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..f8f9dc0 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -107,6 +107,8 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
 __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
 __printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
 
+int seq_overflow(struct seq_file *m);
+
 int seq_path(struct seq_file *, const struct path *, const char *);
 int seq_dentry(struct seq_file *, struct dentry *, const char *);
 int seq_path_root(struct seq_file *m, const struct path *path,
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 10 Dec 2013 21:12:41 -0800

> Many uses of the return value of seq_printf/seq_puts/seq_putc are
> incorrect.  Many assume that the return value is the number of
> chars emitted into a buffer like printf/puts/putc.
> 
> It would be better to make the return value of these functions void
> to avoid these misuses.
> 
> Start to do so.
> Convert seq_overflow to a public function from a static function.
> 
> Remove the return uses of seq_printf/seq_puts/seq_putc from net.
> Add a seq_overflow function call instead.

I'm fine with this going in whatever tree is appropriate for
the seq_overflow un-static change:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH -next 1/3] seq: Add a seq_overflow test.

[Ryan Mallon]

On 11/12/13 16:12, Joe Perches wrote:
> seq_printf and seq_puts returns are often misused.
> 
> Instead of checking the seq_printf or seq_puts return,
> add a new seq_overflow function to test if a seq_file has
> overflowed the available buffer space.
> 
> This will eventually allow seq_printf and seq_puts to be
> converted to have a void return instead of the int return
> that is often assumed to have a size_t value instead of an
> error/no-error value.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/seq_file.c            | 15 ++++++++-------
>  include/linux/seq_file.h |  2 ++
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 1d641bb..aab0736 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -14,16 +14,17 @@
>  #include <asm/uaccess.h>
>  #include <asm/page.h>
>  
> -
> -/*
> - * seq_files have a buffer which can may overflow. When this happens a larger
> - * buffer is reallocated and all the data will be printed again.
> - * The overflow state is true when m->count == m->size.
> +/**
> + * seq_overflow - test if a seq_file has overflowed the space available
> + * @m: the seq_file handle
> + *
> + * Returns -1 when an overflow has occurred, 0 otherwise.
>   */
> -static bool seq_overflow(struct seq_file *m)
> +int seq_overflow(struct seq_file *m)
>  {
> -	return m->count == m->size;
> +	return m->count == m->size ? -1 : 0;
>  }
> +EXPORT_SYMBOL(seq_overflow);

What is the reasoning in making this return int instead of bool? Having
it return int encourages people to do:

	return seq_overflow(s);

When used in seq_file functions will return -EPERM (-1) to user-space,
which is confusing. It should probably return bool and let the caller
sort out the correct error to return.

~Ryan

Re: [PATCH -next 1/3] seq: Add a seq_overflow test.

[Joe Perches]

On Thu, 2013-12-12 at 10:27 +1100, Ryan Mallon wrote:
> On 11/12/13 16:12, Joe Perches wrote:
> > seq_printf and seq_puts returns are often misused.
> > 
> > Instead of checking the seq_printf or seq_puts return,
> > add a new seq_overflow function to test if a seq_file has
> > overflowed the available buffer space.
> > 
> > This will eventually allow seq_printf and seq_puts to be
> > converted to have a void return instead of the int return
> > that is often assumed to have a size_t value instead of an
> > error/no-error value.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  fs/seq_file.c            | 15 ++++++++-------
> >  include/linux/seq_file.h |  2 ++
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/seq_file.c b/fs/seq_file.c
> > index 1d641bb..aab0736 100644
> > --- a/fs/seq_file.c
> > +++ b/fs/seq_file.c
> > @@ -14,16 +14,17 @@
> >  #include <asm/uaccess.h>
> >  #include <asm/page.h>
> >  
> > -
> > -/*
> > - * seq_files have a buffer which can may overflow. When this happens a larger
> > - * buffer is reallocated and all the data will be printed again.
> > - * The overflow state is true when m->count == m->size.
> > +/**
> > + * seq_overflow - test if a seq_file has overflowed the space available
> > + * @m: the seq_file handle
> > + *
> > + * Returns -1 when an overflow has occurred, 0 otherwise.
> >   */
> > -static bool seq_overflow(struct seq_file *m)
> > +int seq_overflow(struct seq_file *m)
> >  {
> > -	return m->count == m->size;
> > +	return m->count == m->size ? -1 : 0;
> >  }
> > +EXPORT_SYMBOL(seq_overflow);
> 
> What is the reasoning in making this return int instead of bool? Having
> it return int encourages people to do:
> 
> 	return seq_overflow(s);
> 
> When used in seq_file functions will return -EPERM (-1) to user-space,
> which is confusing. It should probably return bool and let the caller
> sort out the correct error to return.

It was intended to make the return from seq_printf the same.

Anyway, the patch series is out of date (see Al's comments).
I'll update it later unless you or someone else does it first.