[PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

I noticed that seq_putc() is not consistent with the way seq_puts() is
when it comes to filling the buffer.

Lets look at the two functions doing basically the same thing:

	seq_putc('A');

and

	seq_puts("A");

Now if m->count is 1 less than m->size, the two do different things.
The seq_putc() will write the character, return success, but if a
seq_overflow() is called on the seq_file *m, it will return true.

For seq_puts(), the character is not written, count is set to size, it
returns failure (-1), and like seq_putc(), seq_overflow() will
also return true.

Note, technically, both should act the exact same way, because
seq_puts() does not add the ending null character:

int seq_puts(struct seq_file *m, const char *s)
{
        int len = strlen(s);
        if (m->count + len < m->size) {
                memcpy(m->buf + m->count, s, len);
                m->count += len;
                return 0;
        }
        seq_set_overflow(m);
        return -1;
}

len is just the number of characters in "A" and does not include the
nul terminator. The memcpy, would only copy one character for the
example above. But if m->count is 1 less than m->size, it would not do
the copy and would set the overflow and return failure.

Looking at seq_putc():

int seq_putc(struct seq_file *m, char c)
{
        if (m->count < m->size) {
                m->buf[m->count++] = c;
                return 0;
        }
        return -1;
}

If m->count is one less than m->size, the if condition will succeed,
the buffer will be updated, and success would be returned. But as
overflow is determined by m->count == m->size, that would be true.

Note, I also noticed that seq_puts() behaves slightly different than
other seq_*() functions, in that it does not fill the buffer when it
overflows, but just sets the m->count to size. I'm not sure if this is
incorrect or not as there's bogus data within the buffer that is
encompassed by m->count. This isn't that important as the seq_file()
which uses this code, on overflow, simply frees the entire contents of
m->buf, and updates the size by a power of two the next go around, and
it doesn't care if there's bogus data in an overflowed buffer or not.

But, I'm trying to write code that both the seq_file and trace_seq can
share and these issues will come to the surface and make a difference.

Anyway, this patch is just to make seq_putc() match the behavior of
seq_puts() which does have visibility to the other subsystems.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3857b720cb1b..4cdd900c6a0c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -686,10 +686,11 @@ EXPORT_SYMBOL(seq_open_private);
 
 int seq_putc(struct seq_file *m, char c)
 {
-	if (m->count < m->size) {
+	if (m->count + 1 < m->size) {
 		m->buf[m->count++] = c;
 		return 0;
 	}
+	seq_set_overflow(m);
 	return -1;
 }
 EXPORT_SYMBOL(seq_putc);

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Petr Mladek]

On Fri 2014-09-26 14:37:21, Steven Rostedt wrote:
> I noticed that seq_putc() is not consistent with the way seq_puts() is
> when it comes to filling the buffer.
> 
> Lets look at the two functions doing basically the same thing:
> 
> 	seq_putc('A');
> 
> and
> 
> 	seq_puts("A");
> 
> Now if m->count is 1 less than m->size, the two do different things.
> The seq_putc() will write the character, return success, but if a
> seq_overflow() is called on the seq_file *m, it will return true.
> 
> For seq_puts(), the character is not written, count is set to size, it
> returns failure (-1), and like seq_putc(), seq_overflow() will
> also return true.

Great catch!

> Note, technically, both should act the exact same way, because
> seq_puts() does not add the ending null character:
> 
> int seq_puts(struct seq_file *m, const char *s)
> {
>         int len = strlen(s);
>         if (m->count + len < m->size) {
>                 memcpy(m->buf + m->count, s, len);
>                 m->count += len;
>                 return 0;
>         }
>         seq_set_overflow(m);
>         return -1;
> }
> 
> len is just the number of characters in "A" and does not include the
> nul terminator. The memcpy, would only copy one character for the
> example above. But if m->count is 1 less than m->size, it would not do
> the copy and would set the overflow and return failure.
> 
> Looking at seq_putc():
> 
> int seq_putc(struct seq_file *m, char c)
> {
>         if (m->count < m->size) {
>                 m->buf[m->count++] = c;
>                 return 0;
>         }
>         return -1;
> }
> 
> If m->count is one less than m->size, the if condition will succeed,
> the buffer will be updated, and success would be returned. But as
> overflow is determined by m->count == m->size, that would be true.
> 
> Note, I also noticed that seq_puts() behaves slightly different than
> other seq_*() functions, in that it does not fill the buffer when it
> overflows, but just sets the m->count to size. I'm not sure if this is
> incorrect or not as there's bogus data within the buffer that is
> encompassed by m->count. This isn't that important as the seq_file()
> which uses this code, on overflow, simply frees the entire contents of
> m->buf, and updates the size by a power of two the next go around, and
> it doesn't care if there's bogus data in an overflowed buffer or not.

Hmm, this inconsistency seems to be in more functions. I would divide
it into three categories:

a) Functions that writes valid data until the end of the buffer
   and returns -1 when the operation makes it full (m->count ==
   m->size) or when they are not able to write at all:

	seq_bitmap()
	seq_bitmap_list()


b) Functions that writes the full buffer but they report -1 only
   when they cannot write at all:

	set_putc()


c) Functions that leave mess at the end of the buffer when they could
   not write all data; they mark it as full and return -1 when this happens:

	set_puts()
	seq_put_decimal_ull()
	seq_put_decimal_ll()
	seq_write()


This patch moves set_putc() from b) to c) because it leaves mess in the
last byte.


The semantic looks weird in all categories:

add a) They report error even when there was not a real overflow.
    Well, they do not know if there was overflow because
    bitmap_scnprintf() does not report if it shrunk
    the data or not.

add b) It does not report error even when seq_overflow() might later
    report overflow.

add c) Any overflow invalidates the whole buffer because there might
    be mess at the end. The overflow is not checked by read functions,
    so they allow to read the mess.


I think that reasonable semantic would be:

  + either signalize overflow another way (extra struct member and
    leave count to point to the valid data) or always fill valid
    data until the end of the buffer.

    The second variant might be problem if we add only half of
    an escape sequence.


  + always return error when seq_overflow() would return overflow;
    in fact, the full buffer means that the last write operation
    was most likely incomplete

    Also we might rename it from "overflow" to "full" or "filled"
    or "sealed" or "complete" that might describe the state more
    precisely because there is not a real overflow in some situations


Best Regards,
Petr

 
> But, I'm trying to write code that both the seq_file and trace_seq can
> share and these issues will come to the surface and make a difference.
> 
> Anyway, this patch is just to make seq_putc() match the behavior of
> seq_puts() which does have visibility to the other subsystems.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3857b720cb1b..4cdd900c6a0c 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -686,10 +686,11 @@ EXPORT_SYMBOL(seq_open_private);
>  
>  int seq_putc(struct seq_file *m, char c)
>  {
> -	if (m->count < m->size) {
> +	if (m->count + 1 < m->size) {
>
>  		m->buf[m->count++] = c;
>  		return 0;
>  	}
> +	seq_set_overflow(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_putc);

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

On Mon, 29 Sep 2014 16:41:22 +0200
Petr Mladek <pmladek@suse.cz> wrote:


> This patch moves set_putc() from b) to c) because it leaves mess in the
> last byte.

Which removes the "b" section, and makes seq_putc() and seq_puts()
function the same.

> 
> 
> The semantic looks weird in all categories:
> 
> add a) They report error even when there was not a real overflow.
>     Well, they do not know if there was overflow because
>     bitmap_scnprintf() does not report if it shrunk
>     the data or not.
> 
> add b) It does not report error even when seq_overflow() might later
>     report overflow.
> 
> add c) Any overflow invalidates the whole buffer because there might
>     be mess at the end. The overflow is not checked by read functions,
>     so they allow to read the mess.
> 
> 
> I think that reasonable semantic would be:
> 
>   + either signalize overflow another way (extra struct member and
>     leave count to point to the valid data) or always fill valid
>     data until the end of the buffer.
> 
>     The second variant might be problem if we add only half of
>     an escape sequence.
> 
> 
>   + always return error when seq_overflow() would return overflow;
>     in fact, the full buffer means that the last write operation
>     was most likely incomplete

I think this is probably the best option.

> 
>     Also we might rename it from "overflow" to "full" or "filled"
>     or "sealed" or "complete" that might describe the state more
>     precisely because there is not a real overflow in some situations

We could make overflow be m->count > m->size, which would mean that
m->count == m->size (full) isn't a problem. Although, I would have to
audit the kernel to see if anything just assumes that m->count is
always less than m->size.

This would mean overflow is the correct term, and wouldn't have to
change it.

Anyway, that should be a second patch, and we should get my current
patch (the one to make putc and puts the same) in first, as that
actually fixes the inconsistency between the two.

I'll post another patch to try to make seq_file operations a bit more
consistent. Perhaps we can have m->count be what would have been
written.

-- Steve

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Joe Perches]

On Mon, 2014-09-29 at 11:25 -0400, Steven Rostedt wrote:
> Anyway, that should be a second patch, and we should get my current
> patch (the one to make putc and puts the same) in first, as that
> actually fixes the inconsistency between the two.
> 
> I'll post another patch to try to make seq_file operations a bit more
> consistent. Perhaps we can have m->count be what would have been
> written.

https://lkml.org/lkml/2013/9/11/801

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

On Mon, 29 Sep 2014 08:46:50 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2014-09-29 at 11:25 -0400, Steven Rostedt wrote:
> > Anyway, that should be a second patch, and we should get my current
> > patch (the one to make putc and puts the same) in first, as that
> > actually fixes the inconsistency between the two.
> > 
> > I'll post another patch to try to make seq_file operations a bit more
> > consistent. Perhaps we can have m->count be what would have been
> > written.
> 
> https://lkml.org/lkml/2013/9/11/801
> 

That has nothing to do with what I'm working on. I'm not touching the
return value of the seq_*() functions. I may make them all return void
though.

I may just make m->count == m->size + 1 for all cases. The m->count ==
what would have been written is if seq_buf() needs it. Which I highly
doubt it would.

-- Steve

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Petr Mladek]

On Mon 2014-09-29 11:25:38, Steven Rostedt wrote:
> On Mon, 29 Sep 2014 16:41:22 +0200
> Petr Mladek <pmladek@suse.cz> wrote:
> 
> 
> > This patch moves set_putc() from b) to c) because it leaves mess in the
> > last byte.
> 
> Which removes the "b" section, and makes seq_putc() and seq_puts()
> function the same.
> 
> > 
> > 
> > The semantic looks weird in all categories:
> > 
> > add a) They report error even when there was not a real overflow.
> >     Well, they do not know if there was overflow because
> >     bitmap_scnprintf() does not report if it shrunk
> >     the data or not.
> > 
> > add b) It does not report error even when seq_overflow() might later
> >     report overflow.
> > 
> > add c) Any overflow invalidates the whole buffer because there might
> >     be mess at the end. The overflow is not checked by read functions,
> >     so they allow to read the mess.
> > 
> > 
> > I think that reasonable semantic would be:
> > 
> >   + either signalize overflow another way (extra struct member and
> >     leave count to point to the valid data) or always fill valid
> >     data until the end of the buffer.
> > 
> >     The second variant might be problem if we add only half of
> >     an escape sequence.

Do you prefer:

      + the extra struct member?
      + or always filling some valid data?
      + or invalidating the buffer when the overflow is set?

> > 
> >   + always return error when seq_overflow() would return overflow;
> >     in fact, the full buffer means that the last write operation
> >     was most likely incomplete
> 
> I think this is probably the best option.

> 
> > 
> >     Also we might rename it from "overflow" to "full" or "filled"
> >     or "sealed" or "complete" that might describe the state more
> >     precisely because there is not a real overflow in some situations
> 
> We could make overflow be m->count > m->size, which would mean that
> m->count == m->size (full) isn't a problem. Although, I would have to
> audit the kernel to see if anything just assumes that m->count is
> always less than m->size.

I am not sure if it is a good idea. I think that having
m->count > m->size is prone to buffer overflow errors in
the buffer reading code. I vote for adding extra flag to
the struct :-)

> This would mean overflow is the correct term, and wouldn't have to
> change it.
> 
> Anyway, that should be a second patch, and we should get my current
> patch (the one to make putc and puts the same) in first, as that
> actually fixes the inconsistency between the two.

Feel free to add

Acked-by: Petr Mladek <pmladek@suse.cz>

for this patch. The question is if it is really really needed in the
final patch set. It might get even partly reverted with the follow
up changes.

Best Regards,
Petr

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

On Mon, 29 Sep 2014 18:23:34 +0200
Petr Mladek <pmladek@suse.cz> wrote:
 
> Do you prefer:
> 
>       + the extra struct member?

I hate the extra member.

>       + or always filling some valid data?

Not needed.

>       + or invalidating the buffer when the overflow is set?

Not needed.

> 
> > > 
> > >   + always return error when seq_overflow() would return overflow;
> > >     in fact, the full buffer means that the last write operation
> > >     was most likely incomplete
> > 
> > I think this is probably the best option.
> 
> > 
> > > 
> > >     Also we might rename it from "overflow" to "full" or "filled"
> > >     or "sealed" or "complete" that might describe the state more
> > >     precisely because there is not a real overflow in some situations
> > 
> > We could make overflow be m->count > m->size, which would mean that
> > m->count == m->size (full) isn't a problem. Although, I would have to
> > audit the kernel to see if anything just assumes that m->count is
> > always less than m->size.
> 
> I am not sure if it is a good idea. I think that having
> m->count > m->size is prone to buffer overflow errors in
> the buffer reading code. I vote for adding extra flag to
> the struct :-)

Reading the seq_file code, it is really an all or nothing approach. It
even does the saving of the m->count and on overflow, will reset it to
what it did originally.

If an overflow happens, the buffer count should either be reset to what
it was (throw away anything that was written), or discarded completely,
which it sometimes does.

I'm going to write up some patches that converts overflow to be count =
size + 1, and then updating everything to be an all or nothing approach.

I'll take Joe's patches to convert them to void as well, with the
addition of a seq_is_full() helper for those places that wont waste time
adding more if it's just going to be discarded.

But first, I'm off to my doctor's appointment followed by more PT.

-- Steve

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Al Viro]

On Mon, Sep 29, 2014 at 04:41:22PM +0200, Petr Mladek wrote:

> Hmm, this inconsistency seems to be in more functions. I would divide
> it into three categories:
> 
> a) Functions that writes valid data until the end of the buffer
>    and returns -1 when the operation makes it full (m->count ==
>    m->size) or when they are not able to write at all:
> 
> 	seq_bitmap()
> 	seq_bitmap_list()

> b) Functions that writes the full buffer but they report -1 only
>    when they cannot write at all:
> 
> 	set_putc()

> c) Functions that leave mess at the end of the buffer when they could
>    not write all data; they mark it as full and return -1 when this happens:
> 
> 	set_puts()
> 	seq_put_decimal_ull()
> 	seq_put_decimal_ll()
> 	seq_write()

... and they really should not return *anything*.

>   + always return error when seq_overflow() would return overflow;
>     in fact, the full buffer means that the last write operation
>     was most likely incomplete

No.  _Any_ caller that decides to report that error to its caller is fucking
broken.  We had some cases like that.

<greps>

Oh, lovely - notify_fdinfo() is broken.  Exactly that way - "we get an error,
better report it to caller".  Bad idea - overflow is *NOT* something ->show()
must report to seq_read().  In that case you just return 0.  Returning -1
means something very different - "have read(2) fail with EPERM".

fanotify_fdinfo(): ditto.

seq_puts() ones: drivers/parisc/ccio-dma.c:ccio_proc_bitmap_info() - bogus,
but harmless (it assumes for some reason that seq_puts() returns the number
of characters written; return values are added up and ignored).

drivers/regulator/dbx500-prcmu.c:
        err = seq_puts(s, "ux500-regulator status:\n");
        if (err < 0)
                dev_err(dev, "seq_puts overflow\n");
No, you don't - it's not an error.

drivers/watchdog/bcm_kona_wdt.c: EPERM-from-read() bug.

fs/dlm/debug_fs.c: ditto.

drivers/usb/gadget/udc/goku_udc.c: unique case of seq_puts() return value
used correctly.  I.e. "if it's already overflown, don't bother with
producing the rest of output, you'll be called again on bigger buffer
anyway; just return 0 and be done with it" hint.  And yes, it is the
only place in the tree that looks at return value of seq_puts() and uses
it correctly.

<greps for seq_printf>

arch/arm/plat-pxa/dma.c:59:     pos += seq_printf(s, "DMA channel %d requesters list :\n", chan);
Bogus.  seq_printf() returns 0 or -1, again with the same kind of semantics
("don't bother with more output, you've already overflown" hint).

arch/microblaze/kernel/cpu/mb.c: same bogosity, but there it at least
ignores the sum and just returns 0 in the end.  Why the hell add them
up is a mystery...

drivers/base/power/wakeup.c: called in a helper, returned to caller, which
promptly ignores it.

drivers/mtd/devices/docg3.c: bogus, overflow leads to read(2) returning
more or less random error.

drivers/parisc/ccio-dma.c: added up and ignored.
drivers/parisc/sba_iommu.c: added up and ignored.

conntrack ct_seq_show() and friends: used properly.

net/netfilter/nf_log.c: bogus, EPERM-from-read() kind.

OK, I'm convinced - we do need to make those suckers return nothing at all,
preventing the well-intentioned bugs of that sort.  There had been a discussion
of that a while ago, but it hadn't gone anywhere.  Time to end that
depravity, let's bury the body...

What we need is a helper along the lines of seq_already_overflown() that
could be used by the few places that really want that hint.  As in
	if (seq_already_overflown(m))
		return 0; // we'll be called again with bigger buffer anyway

And let's make seq_printf and friends return void.  Any breakage we miss
on grep will be caught by compiler.  Enough is enough.

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Joe Perches]

On Mon, 2014-09-29 at 16:47 +0100, Al Viro wrote:
> On Mon, Sep 29, 2014 at 04:41:22PM +0200, Petr Mladek wrote:
> > Hmm, this inconsistency seems to be in more functions. I would divide
> > it into three categories:
[]
> No.  _Any_ caller that decides to report that error to its caller is fucking
> broken.  We had some cases like that.
[]
> And let's make seq_printf and friends return void.  Any breakage we miss
> on grep will be caught by compiler.  Enough is enough.

https://lkml.org/lkml/2013/12/11/8

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

On Mon, 29 Sep 2014 09:08:01 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2014-09-29 at 16:47 +0100, Al Viro wrote:
> > On Mon, Sep 29, 2014 at 04:41:22PM +0200, Petr Mladek wrote:
> > > Hmm, this inconsistency seems to be in more functions. I would divide
> > > it into three categories:
> []
> > No.  _Any_ caller that decides to report that error to its caller is fucking
> > broken.  We had some cases like that.
> []
> > And let's make seq_printf and friends return void.  Any breakage we miss
> > on grep will be caught by compiler.  Enough is enough.
> 
> https://lkml.org/lkml/2013/12/11/8
> 

Since you like posting links:

https://lkml.org/lkml/2013/12/11/76

If you want to resurrect your patches, go ahead. I'll pull them into my
list. Probably should rename seq_overflow() to seq_is_full(), which
makes it sound less of an error.

And as Al noted, only fix what's there (just the places that check the
return values). Don't add anything else.

-- Steve

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Joe Perches]

On Mon, 2014-09-29 at 12:15 -0400, Steven Rostedt wrote:
> On Mon, 29 Sep 2014 09:08:01 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Mon, 2014-09-29 at 16:47 +0100, Al Viro wrote:
> > > On Mon, Sep 29, 2014 at 04:41:22PM +0200, Petr Mladek wrote:
> > > > Hmm, this inconsistency seems to be in more functions. I would divide
> > > > it into three categories:
> > []
> > > No.  _Any_ caller that decides to report that error to its caller is fucking
> > > broken.  We had some cases like that.
> > []
> > > And let's make seq_printf and friends return void.  Any breakage we miss
> > > on grep will be caught by compiler.  Enough is enough.
> > 
> > https://lkml.org/lkml/2013/12/11/8
> > 
> 
> Since you like posting links:
> 
> https://lkml.org/lkml/2013/12/11/76
> 
> If you want to resurrect your patches, go ahead. I'll pull them into my
> list. Probably should rename seq_overflow() to seq_is_full(), which
> makes it sound less of an error.
> 
> And as Al noted, only fix what's there (just the places that check the
> return values). Don't add anything else.

I did that too

https://lkml.org/lkml/2013/12/11/713

Then I gave up.

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

On Mon, 29 Sep 2014 09:30:35 -0700
Joe Perches <joe@perches.com> wrote:


> > Since you like posting links:
> > 
> > https://lkml.org/lkml/2013/12/11/76
> > 
> > If you want to resurrect your patches, go ahead. I'll pull them into my
> > list. Probably should rename seq_overflow() to seq_is_full(), which
> > makes it sound less of an error.
> > 
> > And as Al noted, only fix what's there (just the places that check the
> > return values). Don't add anything else.
> 
> I did that too
> 
> https://lkml.org/lkml/2013/12/11/713
> 
> Then I gave up.

Although I like seq_is_full() over seq_is_buf_full(), I'll take either.
If Al would ack those patches I'd be happy to take them in.

-- Steve

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Joe Perches]

On Mon, 2014-09-29 at 12:42 -0400, Steven Rostedt wrote:
> Although I like seq_is_full() over seq_is_buf_full(), I'll take either.
> If Al would ack those patches I'd be happy to take them in.

seq_is_full is fine with me too.

[PATCH 0/7] seq_printf cleanups

[Joe Perches]

seq_printf should return void.

Add a public bool seq_is_full function that can be used to shortcut
unnecesary seq_printf/seq_puts calls when the seq buffer is full.

Start removing the misuses of the seq_printf/seq_puts return value.

Patchset brought forward from an unreplied to set of changes from
back in December 2013.

https://lkml.org/lkml/2013/12/11/713

Renamed seq_is_buf_full to seq_is_full.

Joe Perches (7):
  seq_file: Rename static bool seq_overflow to public bool seq_is_full
  netfilter: Convert print_tuple functions to return void
  dlm: Use seq_is_full - remove seq_printf returns
  dlm: Use seq_puts, remove unnecessary trailing spaces
  fs: Convert show_fdinfo functions to void
  debugfs: Fix misuse of seq_printf return value
  docg3: Fix mixuse of seq_printf return value

 Documentation/filesystems/seq_file.txt       |  28 +--
 Documentation/filesystems/vfs.txt            |   2 +-
 drivers/mtd/devices/docg3.c                  | 112 ++++++------
 drivers/net/tun.c                            |   4 +-
 fs/debugfs/file.c                            |  14 +-
 fs/dlm/debug_fs.c                            | 260 +++++++++++++--------------
 fs/eventfd.c                                 |  15 +-
 fs/eventpoll.c                               |  19 +-
 fs/notify/fdinfo.c                           |  76 ++++----
 fs/notify/fdinfo.h                           |   4 +-
 fs/proc/fd.c                                 |   2 +-
 fs/seq_file.c                                |  28 +--
 fs/signalfd.c                                |  10 +-
 fs/timerfd.c                                 |  27 +--
 include/linux/fs.h                           |   2 +-
 include/linux/seq_file.h                     |   8 +
 include/net/netfilter/nf_conntrack_core.h    |   2 +-
 include/net/netfilter/nf_conntrack_l3proto.h |   4 +-
 include/net/netfilter/nf_conntrack_l4proto.h |   4 +-
 net/netfilter/nf_conntrack_l3proto_generic.c |   5 +-
 net/netfilter/nf_conntrack_proto_dccp.c      |  10 +-
 net/netfilter/nf_conntrack_proto_generic.c   |   5 +-
 net/netfilter/nf_conntrack_proto_gre.c       |  10 +-
 net/netfilter/nf_conntrack_proto_sctp.c      |  10 +-
 net/netfilter/nf_conntrack_proto_tcp.c       |  10 +-
 net/netfilter/nf_conntrack_proto_udp.c       |  10 +-
 net/netfilter/nf_conntrack_proto_udplite.c   |  10 +-
 net/netfilter/nf_conntrack_standalone.c      |  15 +-
 28 files changed, 333 insertions(+), 373 deletions(-)

-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 1/7] seq_file: Rename static bool seq_overflow to public bool seq_is_full

[Joe Perches]

The return values of seq_printf/puts/putc are frequently misused.

Start down a path to remove all the return value uses of these
functions.

Make the static bool seq_overflow public along with a rename of
the function to seq_is_full.  Rename the still static
seq_set_overflow to seq_set_full.

Update the documentation to not show return types for seq_printf
et al.  Add a description of seq_is_full.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/filesystems/seq_file.txt | 28 ++++++++++++++++------------
 fs/seq_file.c                          | 28 ++++++++++++++--------------
 include/linux/seq_file.h               |  8 ++++++++
 3 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
index 8ea3e90..e19c636 100644
--- a/Documentation/filesystems/seq_file.txt
+++ b/Documentation/filesystems/seq_file.txt
@@ -180,27 +180,23 @@ output must be passed to the seq_file code. Some utility functions have
 been defined which make this task easy.
 
 Most code will simply use seq_printf(), which works pretty much like
-printk(), but which requires the seq_file pointer as an argument. It is
-common to ignore the return value from seq_printf(), but a function
-producing complicated output may want to check that value and quit if
-something non-zero is returned; an error return means that the seq_file
-buffer has been filled and further output will be discarded.
+printk(), but which requires the seq_file pointer as an argument.
 
 For straight character output, the following functions may be used:
 
-	int seq_putc(struct seq_file *m, char c);
-	int seq_puts(struct seq_file *m, const char *s);
-	int seq_escape(struct seq_file *m, const char *s, const char *esc);
+	seq_putc(struct seq_file *m, char c);
+	seq_puts(struct seq_file *m, const char *s);
+	seq_escape(struct seq_file *m, const char *s, const char *esc);
 
 The first two output a single character and a string, just like one would
 expect. seq_escape() is like seq_puts(), except that any character in s
 which is in the string esc will be represented in octal form in the output.
 
-There is also a pair of functions for printing filenames:
+There are also a pair of functions for printing filenames:
 
-	int seq_path(struct seq_file *m, struct path *path, char *esc);
-	int seq_path_root(struct seq_file *m, struct path *path,
-			  struct path *root, char *esc)
+	seq_path(struct seq_file *m, struct path *path, char *esc);
+	seq_path_root(struct seq_file *m, struct path *path,
+		      struct path *root, char *esc)
 
 Here, path indicates the file of interest, and esc is a set of characters
 which should be escaped in the output.  A call to seq_path() will output
@@ -209,6 +205,14 @@ root is desired, it can be used with seq_path_root().  Note that, if it
 turns out that path cannot be reached from root, the value of root will be
 changed in seq_file_root() to a root which *does* work.
 
+A function producing complicated output may want to check
+	bool seq_is_full(struct seq_file *m);
+and avoid further seq_<output> calls if true is returned.
+
+A true return from seq_is_full means that the seq_file buffer is full
+and further output will be discarded.  The seq_show function will attempt
+to allocate a larger buffer and retry printing.
+
 
 Making it all work
 
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3857b72..555aed6 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -16,18 +16,18 @@
 #include <asm/uaccess.h>
 #include <asm/page.h>
 
-
 /*
- * seq_files have a buffer which can may overflow. When this happens a larger
+ * seq_files have a buffer which 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.
  */
-static bool seq_overflow(struct seq_file *m)
+bool seq_is_full(struct seq_file *m)
 {
 	return m->count == m->size;
 }
+EXPORT_SYMBOL(seq_is_full);
 
-static void seq_set_overflow(struct seq_file *m)
+static void seq_set_full(struct seq_file *m)
 {
 	m->count = m->size;
 }
@@ -124,7 +124,7 @@ static int traverse(struct seq_file *m, loff_t offset)
 			error = 0;
 			m->count = 0;
 		}
-		if (seq_overflow(m))
+		if (seq_is_full(m))
 			goto Eoverflow;
 		if (pos + m->count > offset) {
 			m->from = offset - pos;
@@ -267,7 +267,7 @@ Fill:
 			break;
 		}
 		err = m->op->show(m, p);
-		if (seq_overflow(m) || err) {
+		if (seq_is_full(m) || err) {
 			m->count = offs;
 			if (likely(err <= 0))
 				break;
@@ -396,7 +396,7 @@ int seq_escape(struct seq_file *m, const char *s, const char *esc)
 			*p++ = '0' + (c & 07);
 			continue;
 		}
-		seq_set_overflow(m);
+		seq_set_full(m);
 		return -1;
         }
 	m->count = p - m->buf;
@@ -415,7 +415,7 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
 			return 0;
 		}
 	}
-	seq_set_overflow(m);
+	seq_set_full(m);
 	return -1;
 }
 EXPORT_SYMBOL(seq_vprintf);
@@ -557,7 +557,7 @@ int seq_bitmap(struct seq_file *m, const unsigned long *bits,
 			return 0;
 		}
 	}
-	seq_set_overflow(m);
+	seq_set_full(m);
 	return -1;
 }
 EXPORT_SYMBOL(seq_bitmap);
@@ -573,7 +573,7 @@ int seq_bitmap_list(struct seq_file *m, const unsigned long *bits,
 			return 0;
 		}
 	}
-	seq_set_overflow(m);
+	seq_set_full(m);
 	return -1;
 }
 EXPORT_SYMBOL(seq_bitmap_list);
@@ -702,7 +702,7 @@ int seq_puts(struct seq_file *m, const char *s)
 		m->count += len;
 		return 0;
 	}
-	seq_set_overflow(m);
+	seq_set_full(m);
 	return -1;
 }
 EXPORT_SYMBOL(seq_puts);
@@ -736,7 +736,7 @@ int seq_put_decimal_ull(struct seq_file *m, char delimiter,
 	m->count += len;
 	return 0;
 overflow:
-	seq_set_overflow(m);
+	seq_set_full(m);
 	return -1;
 }
 EXPORT_SYMBOL(seq_put_decimal_ull);
@@ -746,7 +746,7 @@ int seq_put_decimal_ll(struct seq_file *m, char delimiter,
 {
 	if (num < 0) {
 		if (m->count + 3 >= m->size) {
-			seq_set_overflow(m);
+			seq_set_full(m);
 			return -1;
 		}
 		if (delimiter)
@@ -774,7 +774,7 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
 		seq->count += len;
 		return 0;
 	}
-	seq_set_overflow(seq);
+	seq_set_full(seq);
 	return -1;
 }
 EXPORT_SYMBOL(seq_write);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..1f36da8 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -43,6 +43,14 @@ struct seq_operations {
 #define SEQ_SKIP 1
 
 /**
+ * seq_is_full - check if the buffer associated to seq_file is full
+ * @m: the seq_file handle
+ *
+ * Returns true if the buffer is full
+ */
+bool seq_is_full(struct seq_file *m);
+
+/**
  * seq_get_buf - get buffer to write arbitrary data to
  * @m: the seq_file handle
  * @bufp: the beginning of the buffer is stored here
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 1/7] seq_file: Rename static bool seq_overflow to public bool seq_is_full

[Steven Rostedt]

On Mon, 29 Sep 2014 16:08:21 -0700
Joe Perches <joe@perches.com> wrote:

> The return values of seq_printf/puts/putc are frequently misused.
> 
> Start down a path to remove all the return value uses of these
> functions.
> 
> Make the static bool seq_overflow public along with a rename of
> the function to seq_is_full.  Rename the still static
> seq_set_overflow to seq_set_full.
> 
> Update the documentation to not show return types for seq_printf
> et al.  Add a description of seq_is_full.

Actually, can you make a separate function that's public that is
seq_is_full(), where m->count >= m->size, and leave seq_overflow()
alone.

That's because I'm working on making seq_overflow be
m->count > m->size, and allow m->count == m->size not be flagged as an
overflow. I'm looking at places that hard code writing into the buffer
(outside of seq_file.c) and they too can fill the buffer exactly.

Thus, all that is needed is the seq_is_full() function added, and you
don't need to modify the seq_set_overflow() either. It makes sense for
external functions to know test for seq_is_full() as if
m->count == m->size, they can't write anymore either.

Thanks!

-- Steve


> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  Documentation/filesystems/seq_file.txt | 28 ++++++++++++++++------------
>  fs/seq_file.c                          | 28 ++++++++++++++--------------
>  include/linux/seq_file.h               |  8 ++++++++
>  3 files changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
> index 8ea3e90..e19c636 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -180,27 +180,23 @@ output must be passed to the seq_file code. Some utility functions have
>  been defined which make this task easy.
>  
>  Most code will simply use seq_printf(), which works pretty much like
> -printk(), but which requires the seq_file pointer as an argument. It is
> -common to ignore the return value from seq_printf(), but a function
> -producing complicated output may want to check that value and quit if
> -something non-zero is returned; an error return means that the seq_file
> -buffer has been filled and further output will be discarded.
> +printk(), but which requires the seq_file pointer as an argument.
>  
>  For straight character output, the following functions may be used:
>  
> -	int seq_putc(struct seq_file *m, char c);
> -	int seq_puts(struct seq_file *m, const char *s);
> -	int seq_escape(struct seq_file *m, const char *s, const char *esc);
> +	seq_putc(struct seq_file *m, char c);
> +	seq_puts(struct seq_file *m, const char *s);
> +	seq_escape(struct seq_file *m, const char *s, const char *esc);
>  
>  The first two output a single character and a string, just like one would
>  expect. seq_escape() is like seq_puts(), except that any character in s
>  which is in the string esc will be represented in octal form in the output.
>  
> -There is also a pair of functions for printing filenames:
> +There are also a pair of functions for printing filenames:
>  
> -	int seq_path(struct seq_file *m, struct path *path, char *esc);
> -	int seq_path_root(struct seq_file *m, struct path *path,
> -			  struct path *root, char *esc)
> +	seq_path(struct seq_file *m, struct path *path, char *esc);
> +	seq_path_root(struct seq_file *m, struct path *path,
> +		      struct path *root, char *esc)
>  
>  Here, path indicates the file of interest, and esc is a set of characters
>  which should be escaped in the output.  A call to seq_path() will output
> @@ -209,6 +205,14 @@ root is desired, it can be used with seq_path_root().  Note that, if it
>  turns out that path cannot be reached from root, the value of root will be
>  changed in seq_file_root() to a root which *does* work.
>  
> +A function producing complicated output may want to check
> +	bool seq_is_full(struct seq_file *m);
> +and avoid further seq_<output> calls if true is returned.
> +
> +A true return from seq_is_full means that the seq_file buffer is full
> +and further output will be discarded.  The seq_show function will attempt
> +to allocate a larger buffer and retry printing.
> +
>  
>  Making it all work
>  
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3857b72..555aed6 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -16,18 +16,18 @@
>  #include <asm/uaccess.h>
>  #include <asm/page.h>
>  
> -
>  /*
> - * seq_files have a buffer which can may overflow. When this happens a larger
> + * seq_files have a buffer which 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.
>   */
> -static bool seq_overflow(struct seq_file *m)
> +bool seq_is_full(struct seq_file *m)
>  {
>  	return m->count == m->size;
>  }
> +EXPORT_SYMBOL(seq_is_full);
>  
> -static void seq_set_overflow(struct seq_file *m)
> +static void seq_set_full(struct seq_file *m)
>  {
>  	m->count = m->size;
>  }
> @@ -124,7 +124,7 @@ static int traverse(struct seq_file *m, loff_t offset)
>  			error = 0;
>  			m->count = 0;
>  		}
> -		if (seq_overflow(m))
> +		if (seq_is_full(m))
>  			goto Eoverflow;
>  		if (pos + m->count > offset) {
>  			m->from = offset - pos;
> @@ -267,7 +267,7 @@ Fill:
>  			break;
>  		}
>  		err = m->op->show(m, p);
> -		if (seq_overflow(m) || err) {
> +		if (seq_is_full(m) || err) {
>  			m->count = offs;
>  			if (likely(err <= 0))
>  				break;
> @@ -396,7 +396,7 @@ int seq_escape(struct seq_file *m, const char *s, const char *esc)
>  			*p++ = '0' + (c & 07);
>  			continue;
>  		}
> -		seq_set_overflow(m);
> +		seq_set_full(m);
>  		return -1;
>          }
>  	m->count = p - m->buf;
> @@ -415,7 +415,7 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
>  			return 0;
>  		}
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_vprintf);
> @@ -557,7 +557,7 @@ int seq_bitmap(struct seq_file *m, const unsigned long *bits,
>  			return 0;
>  		}
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_bitmap);
> @@ -573,7 +573,7 @@ int seq_bitmap_list(struct seq_file *m, const unsigned long *bits,
>  			return 0;
>  		}
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_bitmap_list);
> @@ -702,7 +702,7 @@ int seq_puts(struct seq_file *m, const char *s)
>  		m->count += len;
>  		return 0;
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_puts);
> @@ -736,7 +736,7 @@ int seq_put_decimal_ull(struct seq_file *m, char delimiter,
>  	m->count += len;
>  	return 0;
>  overflow:
> -	seq_set_overflow(m);
> +	seq_set_full(m);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_put_decimal_ull);
> @@ -746,7 +746,7 @@ int seq_put_decimal_ll(struct seq_file *m, char delimiter,
>  {
>  	if (num < 0) {
>  		if (m->count + 3 >= m->size) {
> -			seq_set_overflow(m);
> +			seq_set_full(m);
>  			return -1;
>  		}
>  		if (delimiter)
> @@ -774,7 +774,7 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
>  		seq->count += len;
>  		return 0;
>  	}
> -	seq_set_overflow(seq);
> +	seq_set_full(seq);
>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_write);
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 52e0097..1f36da8 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -43,6 +43,14 @@ struct seq_operations {
>  #define SEQ_SKIP 1
>  
>  /**
> + * seq_is_full - check if the buffer associated to seq_file is full
> + * @m: the seq_file handle
> + *
> + * Returns true if the buffer is full
> + */
> +bool seq_is_full(struct seq_file *m);
> +
> +/**
>   * seq_get_buf - get buffer to write arbitrary data to
>   * @m: the seq_file handle
>   * @bufp: the beginning of the buffer is stored here

Re: [PATCH 1/7] seq_file: Rename static bool seq_overflow to public bool seq_is_full

[Joe Perches]

On Mon, 2014-09-29 at 19:44 -0400, Steven Rostedt wrote:
> On Mon, 29 Sep 2014 16:08:21 -0700 Joe Perches <joe@perches.com> wrote:
> > The return values of seq_printf/puts/putc are frequently misused.
> > 
> > Start down a path to remove all the return value uses of these
> > functions.
[]
> Actually, can you make a separate function that's public that is
> seq_is_full(), where m->count >= m->size, and leave seq_overflow()
> alone.

Change the first patch to suit your taste.

The rest of the series should not need change.

Re: [PATCH 1/7] seq_file: Rename static bool seq_overflow to public bool seq_is_full

[Petr Mladek]

On Mon 29-09-14 16:08:21, Joe Perches wrote:
> The return values of seq_printf/puts/putc are frequently misused.
> 
> Start down a path to remove all the return value uses of these
> functions.
> 
> Make the static bool seq_overflow public along with a rename of
> the function to seq_is_full.  Rename the still static
> seq_set_overflow to seq_set_full.
> 
> Update the documentation to not show return types for seq_printf
> et al.  Add a description of seq_is_full.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  Documentation/filesystems/seq_file.txt | 28 ++++++++++++++++------------
>  fs/seq_file.c                          | 28 ++++++++++++++--------------
>  include/linux/seq_file.h               |  8 ++++++++
>  3 files changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
> index 8ea3e90..e19c636 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -180,27 +180,23 @@ output must be passed to the seq_file code. Some utility functions have
>  been defined which make this task easy.
>  
>  Most code will simply use seq_printf(), which works pretty much like
> -printk(), but which requires the seq_file pointer as an argument. It is
> -common to ignore the return value from seq_printf(), but a function
> -producing complicated output may want to check that value and quit if
> -something non-zero is returned; an error return means that the seq_file
> -buffer has been filled and further output will be discarded.
> +printk(), but which requires the seq_file pointer as an argument.
>  
>  For straight character output, the following functions may be used:
>  
> -	int seq_putc(struct seq_file *m, char c);
> -	int seq_puts(struct seq_file *m, const char *s);
> -	int seq_escape(struct seq_file *m, const char *s, const char *esc);
> +	seq_putc(struct seq_file *m, char c);
> +	seq_puts(struct seq_file *m, const char *s);
> +	seq_escape(struct seq_file *m, const char *s, const char *esc);
>  
>  The first two output a single character and a string, just like one would
>  expect. seq_escape() is like seq_puts(), except that any character in s
>  which is in the string esc will be represented in octal form in the output.
>  
> -There is also a pair of functions for printing filenames:
> +There are also a pair of functions for printing filenames:
>  
> -	int seq_path(struct seq_file *m, struct path *path, char *esc);
> -	int seq_path_root(struct seq_file *m, struct path *path,
> -			  struct path *root, char *esc)
> +	seq_path(struct seq_file *m, struct path *path, char *esc);
> +	seq_path_root(struct seq_file *m, struct path *path,
> +		      struct path *root, char *esc)
>  
>  Here, path indicates the file of interest, and esc is a set of characters
>  which should be escaped in the output.  A call to seq_path() will output
> @@ -209,6 +205,14 @@ root is desired, it can be used with seq_path_root().  Note that, if it
>  turns out that path cannot be reached from root, the value of root will be
>  changed in seq_file_root() to a root which *does* work.
>  
> +A function producing complicated output may want to check
> +	bool seq_is_full(struct seq_file *m);
> +and avoid further seq_<output> calls if true is returned.
> +
> +A true return from seq_is_full means that the seq_file buffer is full
> +and further output will be discarded.  The seq_show function will attempt
> +to allocate a larger buffer and retry printing.
> +
>  
>  Making it all work
>  
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 3857b72..555aed6 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -16,18 +16,18 @@
>  #include <asm/uaccess.h>
>  #include <asm/page.h>
>  
> -
>  /*
> - * seq_files have a buffer which can may overflow. When this happens a larger
> + * seq_files have a buffer which 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.
>   */
> -static bool seq_overflow(struct seq_file *m)
> +bool seq_is_full(struct seq_file *m)
>  {
>  	return m->count == m->size;
>  }
> +EXPORT_SYMBOL(seq_is_full);
>  
> -static void seq_set_overflow(struct seq_file *m)
> +static void seq_set_full(struct seq_file *m)
>  {
>  	m->count = m->size;
>  }
> @@ -124,7 +124,7 @@ static int traverse(struct seq_file *m, loff_t offset)
>  			error = 0;
>  			m->count = 0;
>  		}
> -		if (seq_overflow(m))
> +		if (seq_is_full(m))
>  			goto Eoverflow;

I would keep seq_overflow() here. seq_is_full() should mean that the
data still fit into the buffer.

>  		if (pos + m->count > offset) {
>  			m->from = offset - pos;
> @@ -267,7 +267,7 @@ Fill:
>  			break;
>  		}
>  		err = m->op->show(m, p);
> -		if (seq_overflow(m) || err) {
> +		if (seq_is_full(m) || err) {

same here

>  			m->count = offs;
>  			if (likely(err <= 0))
>  				break;
> @@ -396,7 +396,7 @@ int seq_escape(struct seq_file *m, const char *s, const char *esc)
>  			*p++ = '0' + (c & 07);
>  			continue;
>  		}
> -		seq_set_overflow(m);
> +		seq_set_full(m);

I would keep seq_set_overflow() here because the data did not fit in.

>  		return -1;
>          }
>  	m->count = p - m->buf;
> @@ -415,7 +415,7 @@ int seq_vprintf(struct seq_file *m, const char *f, va_list args)
>  			return 0;
>  		}
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);

Hmm, we do not know if the data are shrunken by vsnprintf(). I would
keep seq_set_overflow() here to be on the safe side.

>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_vprintf);
> @@ -557,7 +557,7 @@ int seq_bitmap(struct seq_file *m, const unsigned long *bits,
>  			return 0;
>  		}
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);

same here

>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_bitmap);
> @@ -573,7 +573,7 @@ int seq_bitmap_list(struct seq_file *m, const unsigned long *bits,
>  			return 0;
>  		}
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);

and here

>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_bitmap_list);
> @@ -702,7 +702,7 @@ int seq_puts(struct seq_file *m, const char *s)
>  		m->count += len;
>  		return 0;
>  	}
> -	seq_set_overflow(m);
> +	seq_set_full(m);

and here

>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_puts);
> @@ -736,7 +736,7 @@ int seq_put_decimal_ull(struct seq_file *m, char delimiter,
>  	m->count += len;
>  	return 0;
>  overflow:
> -	seq_set_overflow(m);
> +	seq_set_full(m);

We should change one above contition from

	if (m->count + 2 >= m->size) /* we'll write 2 bytes at least */
		goto overflow;

to

	if (m->count + 2 > m->size) /* we'll write 2 bytes at least */
		goto overflow;

>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_put_decimal_ull);
> @@ -746,7 +746,7 @@ int seq_put_decimal_ll(struct seq_file *m, char delimiter,
>  {
>  	if (num < 0) {
>  		if (m->count + 3 >= m->size) {
> -			seq_set_overflow(m);
> +			seq_set_full(m);
>  			return -1;

We should change this to:

		if (m->count + 3 > m->size) {
			seq_set_overflow(m);


>  		}
>  		if (delimiter)
> @@ -774,7 +774,7 @@ int seq_write(struct seq_file *seq, const void *data, size_t len)
>  		seq->count += len;
>  		return 0;
>  	}
> -	seq_set_overflow(seq);
> +	seq_set_full(seq);

IMHO, the whole function should be:

void seq_write(struct seq_file *seq, const void *data, size_t len)
{
	if (seq->count + len <= seq->size) {
		memcpy(seq->buf + seq->count, data, len);
		seq->count += len;
		return 0;
	}
	seq_set_overflow(seq);
}

I mean that the overflow should be set only when there is a real overflow.


All in all, this patch should depend on the Steven's work and most of
it will be unused.

Best Regards,
Petr

>  	return -1;
>  }
>  EXPORT_SYMBOL(seq_write);
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index 52e0097..1f36da8 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -43,6 +43,14 @@ struct seq_operations {
>  #define SEQ_SKIP 1
>  
>  /**
> + * seq_is_full - check if the buffer associated to seq_file is full
> + * @m: the seq_file handle
> + *
> + * Returns true if the buffer is full
> + */
> +bool seq_is_full(struct seq_file *m);
> +
> +/**
>   * seq_get_buf - get buffer to write arbitrary data to
>   * @m: the seq_file handle
>   * @bufp: the beginning of the buffer is stored here
> -- 
> 1.8.1.2.459.gbcd45b4.dirty
> 

[PATCH 2/7] netfilter: Convert print_tuple functions to return void

[Joe Perches]

Since adding a new function to seq_file (seq_is_full)
there isn't any value for functions called from seq_show to
return anything.   Remove the int returns of the various
print_tuple/<foo>_print_tuple functions.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/net/netfilter/nf_conntrack_core.h    |  2 +-
 include/net/netfilter/nf_conntrack_l3proto.h |  4 ++--
 include/net/netfilter/nf_conntrack_l4proto.h |  4 ++--
 net/netfilter/nf_conntrack_l3proto_generic.c |  5 ++---
 net/netfilter/nf_conntrack_proto_dccp.c      | 10 +++++-----
 net/netfilter/nf_conntrack_proto_generic.c   |  5 ++---
 net/netfilter/nf_conntrack_proto_gre.c       | 10 +++++-----
 net/netfilter/nf_conntrack_proto_sctp.c      | 10 +++++-----
 net/netfilter/nf_conntrack_proto_tcp.c       | 10 +++++-----
 net/netfilter/nf_conntrack_proto_udp.c       | 10 +++++-----
 net/netfilter/nf_conntrack_proto_udplite.c   | 10 +++++-----
 net/netfilter/nf_conntrack_standalone.c      | 15 +++++++--------
 12 files changed, 46 insertions(+), 49 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index cc0c188..f2f0fa3 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -72,7 +72,7 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
 	return ret;
 }
 
-int
+void
 print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
             const struct nf_conntrack_l3proto *l3proto,
             const struct nf_conntrack_l4proto *proto);
diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
index adc1fa3..cdc920b 100644
--- a/include/net/netfilter/nf_conntrack_l3proto.h
+++ b/include/net/netfilter/nf_conntrack_l3proto.h
@@ -38,8 +38,8 @@ struct nf_conntrack_l3proto {
 			     const struct nf_conntrack_tuple *orig);
 
 	/* Print out the per-protocol part of the tuple. */
-	int (*print_tuple)(struct seq_file *s,
-			   const struct nf_conntrack_tuple *);
+	void (*print_tuple)(struct seq_file *s,
+			    const struct nf_conntrack_tuple *);
 
 	/*
 	 * Called before tracking. 
diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
index 4c8d573..fead8ee 100644
--- a/include/net/netfilter/nf_conntrack_l4proto.h
+++ b/include/net/netfilter/nf_conntrack_l4proto.h
@@ -56,8 +56,8 @@ struct nf_conntrack_l4proto {
 		     u_int8_t pf, unsigned int hooknum);
 
 	/* Print out the per-protocol part of the tuple. Return like seq_* */
-	int (*print_tuple)(struct seq_file *s,
-			   const struct nf_conntrack_tuple *);
+	void (*print_tuple)(struct seq_file *s,
+			    const struct nf_conntrack_tuple *);
 
 	/* Print out the private part of the conntrack. */
 	int (*print_conntrack)(struct seq_file *s, struct nf_conn *);
diff --git a/net/netfilter/nf_conntrack_l3proto_generic.c b/net/netfilter/nf_conntrack_l3proto_generic.c
index e7eb807..cf9ace7 100644
--- a/net/netfilter/nf_conntrack_l3proto_generic.c
+++ b/net/netfilter/nf_conntrack_l3proto_generic.c
@@ -49,10 +49,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
 	return true;
 }
 
-static int generic_print_tuple(struct seq_file *s,
-			    const struct nf_conntrack_tuple *tuple)
+static void generic_print_tuple(struct seq_file *s,
+				const struct nf_conntrack_tuple *tuple)
 {
-	return 0;
 }
 
 static int generic_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
index cb372f9..40d96f9 100644
--- a/net/netfilter/nf_conntrack_proto_dccp.c
+++ b/net/netfilter/nf_conntrack_proto_dccp.c
@@ -618,12 +618,12 @@ out_invalid:
 	return -NF_ACCEPT;
 }
 
-static int dccp_print_tuple(struct seq_file *s,
-			    const struct nf_conntrack_tuple *tuple)
+static void dccp_print_tuple(struct seq_file *s,
+			     const struct nf_conntrack_tuple *tuple)
 {
-	return seq_printf(s, "sport=%hu dport=%hu ",
-			  ntohs(tuple->src.u.dccp.port),
-			  ntohs(tuple->dst.u.dccp.port));
+	seq_printf(s, "sport=%hu dport=%hu ",
+		   ntohs(tuple->src.u.dccp.port),
+		   ntohs(tuple->dst.u.dccp.port));
 }
 
 static int dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
index d25f293..0a3ded1 100644
--- a/net/netfilter/nf_conntrack_proto_generic.c
+++ b/net/netfilter/nf_conntrack_proto_generic.c
@@ -39,10 +39,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
 }
 
 /* Print out the per-protocol part of the tuple. */
-static int generic_print_tuple(struct seq_file *s,
-			       const struct nf_conntrack_tuple *tuple)
+static void generic_print_tuple(struct seq_file *s,
+				const struct nf_conntrack_tuple *tuple)
 {
-	return 0;
 }
 
 static unsigned int *generic_get_timeouts(struct net *net)
diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index d566573..cd85336 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -226,12 +226,12 @@ static bool gre_pkt_to_tuple(const struct sk_buff *skb, unsigned int dataoff,
 }
 
 /* print gre part of tuple */
-static int gre_print_tuple(struct seq_file *s,
-			   const struct nf_conntrack_tuple *tuple)
+static void gre_print_tuple(struct seq_file *s,
+			    const struct nf_conntrack_tuple *tuple)
 {
-	return seq_printf(s, "srckey=0x%x dstkey=0x%x ",
-			  ntohs(tuple->src.u.gre.key),
-			  ntohs(tuple->dst.u.gre.key));
+	seq_printf(s, "srckey=0x%x dstkey=0x%x ",
+		   ntohs(tuple->src.u.gre.key),
+		   ntohs(tuple->dst.u.gre.key));
 }
 
 /* print private data for conntrack */
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 1314d33..1b1d0e9 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -166,12 +166,12 @@ static bool sctp_invert_tuple(struct nf_conntrack_tuple *tuple,
 }
 
 /* Print out the per-protocol part of the tuple. */
-static int sctp_print_tuple(struct seq_file *s,
-			    const struct nf_conntrack_tuple *tuple)
+static void sctp_print_tuple(struct seq_file *s,
+			     const struct nf_conntrack_tuple *tuple)
 {
-	return seq_printf(s, "sport=%hu dport=%hu ",
-			  ntohs(tuple->src.u.sctp.port),
-			  ntohs(tuple->dst.u.sctp.port));
+	seq_printf(s, "sport=%hu dport=%hu ",
+		   ntohs(tuple->src.u.sctp.port),
+		   ntohs(tuple->dst.u.sctp.port));
 }
 
 /* Print out the private part of the conntrack. */
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 44d1ea3..c2693e78 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -302,12 +302,12 @@ static bool tcp_invert_tuple(struct nf_conntrack_tuple *tuple,
 }
 
 /* Print out the per-protocol part of the tuple. */
-static int tcp_print_tuple(struct seq_file *s,
-			   const struct nf_conntrack_tuple *tuple)
+static void tcp_print_tuple(struct seq_file *s,
+			    const struct nf_conntrack_tuple *tuple)
 {
-	return seq_printf(s, "sport=%hu dport=%hu ",
-			  ntohs(tuple->src.u.tcp.port),
-			  ntohs(tuple->dst.u.tcp.port));
+	seq_printf(s, "sport=%hu dport=%hu ",
+		   ntohs(tuple->src.u.tcp.port),
+		   ntohs(tuple->dst.u.tcp.port));
 }
 
 /* Print out the private part of the conntrack. */
diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
index 9d7721c..6957281 100644
--- a/net/netfilter/nf_conntrack_proto_udp.c
+++ b/net/netfilter/nf_conntrack_proto_udp.c
@@ -63,12 +63,12 @@ static bool udp_invert_tuple(struct nf_conntrack_tuple *tuple,
 }
 
 /* Print out the per-protocol part of the tuple. */
-static int udp_print_tuple(struct seq_file *s,
-			   const struct nf_conntrack_tuple *tuple)
+static void udp_print_tuple(struct seq_file *s,
+			    const struct nf_conntrack_tuple *tuple)
 {
-	return seq_printf(s, "sport=%hu dport=%hu ",
-			  ntohs(tuple->src.u.udp.port),
-			  ntohs(tuple->dst.u.udp.port));
+	seq_printf(s, "sport=%hu dport=%hu ",
+		   ntohs(tuple->src.u.udp.port),
+		   ntohs(tuple->dst.u.udp.port));
 }
 
 static unsigned int *udp_get_timeouts(struct net *net)
diff --git a/net/netfilter/nf_conntrack_proto_udplite.c b/net/netfilter/nf_conntrack_proto_udplite.c
index 2750e6c..c5903d1 100644
--- a/net/netfilter/nf_conntrack_proto_udplite.c
+++ b/net/netfilter/nf_conntrack_proto_udplite.c
@@ -71,12 +71,12 @@ static bool udplite_invert_tuple(struct nf_conntrack_tuple *tuple,
 }
 
 /* Print out the per-protocol part of the tuple. */
-static int udplite_print_tuple(struct seq_file *s,
-			       const struct nf_conntrack_tuple *tuple)
+static void udplite_print_tuple(struct seq_file *s,
+				const struct nf_conntrack_tuple *tuple)
 {
-	return seq_printf(s, "sport=%hu dport=%hu ",
-			  ntohs(tuple->src.u.udp.port),
-			  ntohs(tuple->dst.u.udp.port));
+	seq_printf(s, "sport=%hu dport=%hu ",
+		   ntohs(tuple->src.u.udp.port),
+		   ntohs(tuple->dst.u.udp.port));
 }
 
 static unsigned int *udplite_get_timeouts(struct net *net)
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index cf65a1e..3c2ce5c 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -36,12 +36,13 @@
 MODULE_LICENSE("GPL");
 
 #ifdef CONFIG_NF_CONNTRACK_PROCFS
-int
+void
 print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
             const struct nf_conntrack_l3proto *l3proto,
             const struct nf_conntrack_l4proto *l4proto)
 {
-	return l3proto->print_tuple(s, tuple) || l4proto->print_tuple(s, tuple);
+	l3proto->print_tuple(s, tuple);
+	l4proto->print_tuple(s, tuple);
 }
 EXPORT_SYMBOL_GPL(print_tuple);
 
@@ -202,9 +203,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
 	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
 		goto release;
 
-	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
-			l3proto, l4proto))
-		goto release;
+	print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
+		    l3proto, l4proto);
 
 	if (seq_print_acct(s, ct, IP_CT_DIR_ORIGINAL))
 		goto release;
@@ -213,9 +213,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
 		if (seq_printf(s, "[UNREPLIED] "))
 			goto release;
 
-	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
-			l3proto, l4proto))
-		goto release;
+	print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
+		    l3proto, l4proto);
 
 	if (seq_print_acct(s, ct, IP_CT_DIR_REPLY))
 		goto release;
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 2/7] netfilter: Convert print_tuple functions to return void

[Petr Mladek]

On Mon 29-09-14 16:08:22, Joe Perches wrote:
> Since adding a new function to seq_file (seq_is_full)
> there isn't any value for functions called from seq_show to
> return anything.   Remove the int returns of the various
> print_tuple/<foo>_print_tuple functions.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  include/net/netfilter/nf_conntrack_core.h    |  2 +-
>  include/net/netfilter/nf_conntrack_l3proto.h |  4 ++--
>  include/net/netfilter/nf_conntrack_l4proto.h |  4 ++--
>  net/netfilter/nf_conntrack_l3proto_generic.c |  5 ++---
>  net/netfilter/nf_conntrack_proto_dccp.c      | 10 +++++-----
>  net/netfilter/nf_conntrack_proto_generic.c   |  5 ++---
>  net/netfilter/nf_conntrack_proto_gre.c       | 10 +++++-----
>  net/netfilter/nf_conntrack_proto_sctp.c      | 10 +++++-----
>  net/netfilter/nf_conntrack_proto_tcp.c       | 10 +++++-----
>  net/netfilter/nf_conntrack_proto_udp.c       | 10 +++++-----
>  net/netfilter/nf_conntrack_proto_udplite.c   | 10 +++++-----
>  net/netfilter/nf_conntrack_standalone.c      | 15 +++++++--------
>  12 files changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
> index cc0c188..f2f0fa3 100644
> --- a/include/net/netfilter/nf_conntrack_core.h
> +++ b/include/net/netfilter/nf_conntrack_core.h
> @@ -72,7 +72,7 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
>  	return ret;
>  }
>  
> -int
> +void
>  print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
>              const struct nf_conntrack_l3proto *l3proto,
>              const struct nf_conntrack_l4proto *proto);
> diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
> index adc1fa3..cdc920b 100644
> --- a/include/net/netfilter/nf_conntrack_l3proto.h
> +++ b/include/net/netfilter/nf_conntrack_l3proto.h
> @@ -38,8 +38,8 @@ struct nf_conntrack_l3proto {
>  			     const struct nf_conntrack_tuple *orig);
>  
>  	/* Print out the per-protocol part of the tuple. */
> -	int (*print_tuple)(struct seq_file *s,
> -			   const struct nf_conntrack_tuple *);
> +	void (*print_tuple)(struct seq_file *s,
> +			    const struct nf_conntrack_tuple *);
>  
>  	/*
>  	 * Called before tracking. 
> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
> index 4c8d573..fead8ee 100644
> --- a/include/net/netfilter/nf_conntrack_l4proto.h
> +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> @@ -56,8 +56,8 @@ struct nf_conntrack_l4proto {
>  		     u_int8_t pf, unsigned int hooknum);
>  
>  	/* Print out the per-protocol part of the tuple. Return like seq_* */
> -	int (*print_tuple)(struct seq_file *s,
> -			   const struct nf_conntrack_tuple *);
> +	void (*print_tuple)(struct seq_file *s,
> +			    const struct nf_conntrack_tuple *);
>  
>  	/* Print out the private part of the conntrack. */
>  	int (*print_conntrack)(struct seq_file *s, struct nf_conn *);
> diff --git a/net/netfilter/nf_conntrack_l3proto_generic.c b/net/netfilter/nf_conntrack_l3proto_generic.c
> index e7eb807..cf9ace7 100644
> --- a/net/netfilter/nf_conntrack_l3proto_generic.c
> +++ b/net/netfilter/nf_conntrack_l3proto_generic.c
> @@ -49,10 +49,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
>  	return true;
>  }
>  
> -static int generic_print_tuple(struct seq_file *s,
> -			    const struct nf_conntrack_tuple *tuple)
> +static void generic_print_tuple(struct seq_file *s,
> +				const struct nf_conntrack_tuple *tuple)
>  {
> -	return 0;
>  }
>  
>  static int generic_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
> diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
> index cb372f9..40d96f9 100644
> --- a/net/netfilter/nf_conntrack_proto_dccp.c
> +++ b/net/netfilter/nf_conntrack_proto_dccp.c
> @@ -618,12 +618,12 @@ out_invalid:
>  	return -NF_ACCEPT;
>  }
>  
> -static int dccp_print_tuple(struct seq_file *s,
> -			    const struct nf_conntrack_tuple *tuple)
> +static void dccp_print_tuple(struct seq_file *s,
> +			     const struct nf_conntrack_tuple *tuple)
>  {
> -	return seq_printf(s, "sport=%hu dport=%hu ",
> -			  ntohs(tuple->src.u.dccp.port),
> -			  ntohs(tuple->dst.u.dccp.port));
> +	seq_printf(s, "sport=%hu dport=%hu ",
> +		   ntohs(tuple->src.u.dccp.port),
> +		   ntohs(tuple->dst.u.dccp.port));
>  }
>  
>  static int dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
> index d25f293..0a3ded1 100644
> --- a/net/netfilter/nf_conntrack_proto_generic.c
> +++ b/net/netfilter/nf_conntrack_proto_generic.c
> @@ -39,10 +39,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
>  }
>  
>  /* Print out the per-protocol part of the tuple. */
> -static int generic_print_tuple(struct seq_file *s,
> -			       const struct nf_conntrack_tuple *tuple)
> +static void generic_print_tuple(struct seq_file *s,
> +				const struct nf_conntrack_tuple *tuple)
>  {
> -	return 0;
>  }
>  
>  static unsigned int *generic_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
> index d566573..cd85336 100644
> --- a/net/netfilter/nf_conntrack_proto_gre.c
> +++ b/net/netfilter/nf_conntrack_proto_gre.c
> @@ -226,12 +226,12 @@ static bool gre_pkt_to_tuple(const struct sk_buff *skb, unsigned int dataoff,
>  }
>  
>  /* print gre part of tuple */
> -static int gre_print_tuple(struct seq_file *s,
> -			   const struct nf_conntrack_tuple *tuple)
> +static void gre_print_tuple(struct seq_file *s,
> +			    const struct nf_conntrack_tuple *tuple)
>  {
> -	return seq_printf(s, "srckey=0x%x dstkey=0x%x ",
> -			  ntohs(tuple->src.u.gre.key),
> -			  ntohs(tuple->dst.u.gre.key));
> +	seq_printf(s, "srckey=0x%x dstkey=0x%x ",
> +		   ntohs(tuple->src.u.gre.key),
> +		   ntohs(tuple->dst.u.gre.key));
>  }
>  
>  /* print private data for conntrack */
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index 1314d33..1b1d0e9 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -166,12 +166,12 @@ static bool sctp_invert_tuple(struct nf_conntrack_tuple *tuple,
>  }
>  
>  /* Print out the per-protocol part of the tuple. */
> -static int sctp_print_tuple(struct seq_file *s,
> -			    const struct nf_conntrack_tuple *tuple)
> +static void sctp_print_tuple(struct seq_file *s,
> +			     const struct nf_conntrack_tuple *tuple)
>  {
> -	return seq_printf(s, "sport=%hu dport=%hu ",
> -			  ntohs(tuple->src.u.sctp.port),
> -			  ntohs(tuple->dst.u.sctp.port));
> +	seq_printf(s, "sport=%hu dport=%hu ",
> +		   ntohs(tuple->src.u.sctp.port),
> +		   ntohs(tuple->dst.u.sctp.port));
>  }
>  
>  /* Print out the private part of the conntrack. */
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 44d1ea3..c2693e78 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -302,12 +302,12 @@ static bool tcp_invert_tuple(struct nf_conntrack_tuple *tuple,
>  }
>  
>  /* Print out the per-protocol part of the tuple. */
> -static int tcp_print_tuple(struct seq_file *s,
> -			   const struct nf_conntrack_tuple *tuple)
> +static void tcp_print_tuple(struct seq_file *s,
> +			    const struct nf_conntrack_tuple *tuple)
>  {
> -	return seq_printf(s, "sport=%hu dport=%hu ",
> -			  ntohs(tuple->src.u.tcp.port),
> -			  ntohs(tuple->dst.u.tcp.port));
> +	seq_printf(s, "sport=%hu dport=%hu ",
> +		   ntohs(tuple->src.u.tcp.port),
> +		   ntohs(tuple->dst.u.tcp.port));
>  }
>  
>  /* Print out the private part of the conntrack. */
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 9d7721c..6957281 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -63,12 +63,12 @@ static bool udp_invert_tuple(struct nf_conntrack_tuple *tuple,
>  }
>  
>  /* Print out the per-protocol part of the tuple. */
> -static int udp_print_tuple(struct seq_file *s,
> -			   const struct nf_conntrack_tuple *tuple)
> +static void udp_print_tuple(struct seq_file *s,
> +			    const struct nf_conntrack_tuple *tuple)
>  {
> -	return seq_printf(s, "sport=%hu dport=%hu ",
> -			  ntohs(tuple->src.u.udp.port),
> -			  ntohs(tuple->dst.u.udp.port));
> +	seq_printf(s, "sport=%hu dport=%hu ",
> +		   ntohs(tuple->src.u.udp.port),
> +		   ntohs(tuple->dst.u.udp.port));
>  }
>  
>  static unsigned int *udp_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_udplite.c b/net/netfilter/nf_conntrack_proto_udplite.c
> index 2750e6c..c5903d1 100644
> --- a/net/netfilter/nf_conntrack_proto_udplite.c
> +++ b/net/netfilter/nf_conntrack_proto_udplite.c
> @@ -71,12 +71,12 @@ static bool udplite_invert_tuple(struct nf_conntrack_tuple *tuple,
>  }
>  
>  /* Print out the per-protocol part of the tuple. */
> -static int udplite_print_tuple(struct seq_file *s,
> -			       const struct nf_conntrack_tuple *tuple)
> +static void udplite_print_tuple(struct seq_file *s,
> +				const struct nf_conntrack_tuple *tuple)
>  {
> -	return seq_printf(s, "sport=%hu dport=%hu ",
> -			  ntohs(tuple->src.u.udp.port),
> -			  ntohs(tuple->dst.u.udp.port));
> +	seq_printf(s, "sport=%hu dport=%hu ",
> +		   ntohs(tuple->src.u.udp.port),
> +		   ntohs(tuple->dst.u.udp.port));
>  }
>  
>  static unsigned int *udplite_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index cf65a1e..3c2ce5c 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -36,12 +36,13 @@
>  MODULE_LICENSE("GPL");
>  
>  #ifdef CONFIG_NF_CONNTRACK_PROCFS
> -int
> +void
>  print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
>              const struct nf_conntrack_l3proto *l3proto,
>              const struct nf_conntrack_l4proto *l4proto)
>  {
> -	return l3proto->print_tuple(s, tuple) || l4proto->print_tuple(s, tuple);
> +	l3proto->print_tuple(s, tuple);
> +	l4proto->print_tuple(s, tuple);
>  }
>  EXPORT_SYMBOL_GPL(print_tuple);
>  
> @@ -202,9 +203,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
>  	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
>  		goto release;
>  
> -	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> -			l3proto, l4proto))
> -		goto release;
> +	print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> +		    l3proto, l4proto);

To be precise, we should add:

	if (seq_overflow(s))
		goto release;
  
>  	if (seq_print_acct(s, ct, IP_CT_DIR_ORIGINAL))
>  		goto release;
> @@ -213,9 +213,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
>  		if (seq_printf(s, "[UNREPLIED] "))
>  			goto release;
>  
> -	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> -			l3proto, l4proto))
> -		goto release;
> +	print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> +		    l3proto, l4proto);

same here

>  	if (seq_print_acct(s, ct, IP_CT_DIR_REPLY))
>  		goto release;
> -- 
> 1.8.1.2.459.gbcd45b4.dirty
> 

Best Regards,
Petr

Re: [PATCH 2/7] netfilter: Convert print_tuple functions to return void

[Joe Perches]

On Tue, 2014-09-30 at 12:22 +0200, Petr Mladek wrote:
> On Mon 29-09-14 16:08:22, Joe Perches wrote:
> > Since adding a new function to seq_file (seq_is_full)
> > there isn't any value for functions called from seq_show to
> > return anything.   Remove the int returns of the various
> > print_tuple/<foo>_print_tuple functions.

[a bunch of quoted stuff]

Please remember to cut out from your replies the unnecessary old stuff.
It can take quite awhile to scan through looking for your comments.

> > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
[]
> > @@ -202,9 +203,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> >  	if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> >  		goto release;
> >  
> > -	if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> > -			l3proto, l4proto))
> > -		goto release;
> > +	print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> > +		    l3proto, l4proto);
> 
> To be precise, we should add:
> 
> 	if (seq_overflow(s))
> 		goto release;

Precision isn't all that useful when checking seq_<output>.

There really isn't much value in checking each possible
overflow site.  A periodic check prior to or in the middle
of a more costly/longish operation should be acceptable.

The entire block that precedes any seq buffer full test will
be redone when the buffer is expanded.

[PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns

[Joe Perches]

The seq_printf return should be ignored and the
seq_is_full function should be tested instead.

Convert functions returning int to void where
seq_printf is used.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/dlm/debug_fs.c | 248 +++++++++++++++++++++++++-----------------------------
 1 file changed, 116 insertions(+), 132 deletions(-)

diff --git a/fs/dlm/debug_fs.c b/fs/dlm/debug_fs.c
index 1323c56..27c8335 100644
--- a/fs/dlm/debug_fs.c
+++ b/fs/dlm/debug_fs.c
@@ -48,8 +48,8 @@ static char *print_lockmode(int mode)
 	}
 }
 
-static int print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
-			      struct dlm_rsb *res)
+static void print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
+			       struct dlm_rsb *res)
 {
 	seq_printf(s, "%08x %s", lkb->lkb_id, print_lockmode(lkb->lkb_grmode));
 
@@ -68,21 +68,17 @@ static int print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
 	if (lkb->lkb_wait_type)
 		seq_printf(s, " wait_type: %d", lkb->lkb_wait_type);
 
-	return seq_puts(s, "\n");
+	seq_puts(s, "\n");
 }
 
-static int print_format1(struct dlm_rsb *res, struct seq_file *s)
+static void print_format1(struct dlm_rsb *res, struct seq_file *s)
 {
 	struct dlm_lkb *lkb;
 	int i, lvblen = res->res_ls->ls_lvblen, recover_list, root_list;
-	int rv;
 
 	lock_rsb(res);
 
-	rv = seq_printf(s, "\nResource %p Name (len=%d) \"",
-			res, res->res_length);
-	if (rv)
-		goto out;
+	seq_printf(s, "\nResource %p Name (len=%d) \"", res, res->res_length);
 
 	for (i = 0; i < res->res_length; i++) {
 		if (isprint(res->res_name[i]))
@@ -92,17 +88,16 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
 	}
 
 	if (res->res_nodeid > 0)
-		rv = seq_printf(s, "\"\nLocal Copy, Master is node %d\n",
-				res->res_nodeid);
+		seq_printf(s, "\"\nLocal Copy, Master is node %d\n",
+			   res->res_nodeid);
 	else if (res->res_nodeid == 0)
-		rv = seq_puts(s, "\"\nMaster Copy\n");
+		seq_puts(s, "\"\nMaster Copy\n");
 	else if (res->res_nodeid == -1)
-		rv = seq_printf(s, "\"\nLooking up master (lkid %x)\n",
-			   	res->res_first_lkid);
+		seq_printf(s, "\"\nLooking up master (lkid %x)\n",
+			   res->res_first_lkid);
 	else
-		rv = seq_printf(s, "\"\nInvalid master %d\n",
-				res->res_nodeid);
-	if (rv)
+		seq_printf(s, "\"\nInvalid master %d\n", res->res_nodeid);
+	if (seq_is_full(s))
 		goto out;
 
 	/* Print the LVB: */
@@ -116,8 +111,8 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
 		}
 		if (rsb_flag(res, RSB_VALNOTVALID))
 			seq_puts(s, " (INVALID)");
-		rv = seq_puts(s, "\n");
-		if (rv)
+		seq_puts(s, "\n");
+		if (seq_is_full(s))
 			goto out;
 	}
 
@@ -125,32 +120,30 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
 	recover_list = !list_empty(&res->res_recover_list);
 
 	if (root_list || recover_list) {
-		rv = seq_printf(s, "Recovery: root %d recover %d flags %lx "
-				"count %d\n", root_list, recover_list,
-			   	res->res_flags, res->res_recover_locks_count);
-		if (rv)
-			goto out;
+		seq_printf(s, "Recovery: root %d recover %d flags %lx count %d\n",
+			   root_list, recover_list,
+			   res->res_flags, res->res_recover_locks_count);
 	}
 
 	/* Print the locks attached to this resource */
 	seq_puts(s, "Granted Queue\n");
 	list_for_each_entry(lkb, &res->res_grantqueue, lkb_statequeue) {
-		rv = print_format1_lock(s, lkb, res);
-		if (rv)
+		print_format1_lock(s, lkb, res);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	seq_puts(s, "Conversion Queue\n");
 	list_for_each_entry(lkb, &res->res_convertqueue, lkb_statequeue) {
-		rv = print_format1_lock(s, lkb, res);
-		if (rv)
+		print_format1_lock(s, lkb, res);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	seq_puts(s, "Waiting Queue\n");
 	list_for_each_entry(lkb, &res->res_waitqueue, lkb_statequeue) {
-		rv = print_format1_lock(s, lkb, res);
-		if (rv)
+		print_format1_lock(s, lkb, res);
+		if (seq_is_full(s))
 			goto out;
 	}
 
@@ -159,23 +152,23 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
 
 	seq_puts(s, "Lookup Queue\n");
 	list_for_each_entry(lkb, &res->res_lookup, lkb_rsb_lookup) {
-		rv = seq_printf(s, "%08x %s", lkb->lkb_id,
-				print_lockmode(lkb->lkb_rqmode));
+		seq_printf(s, "%08x %s",
+			   lkb->lkb_id, print_lockmode(lkb->lkb_rqmode));
 		if (lkb->lkb_wait_type)
 			seq_printf(s, " wait_type: %d", lkb->lkb_wait_type);
-		rv = seq_puts(s, "\n");
+		seq_puts(s, "\n");
+		if (seq_is_full(s))
+			goto out;
 	}
  out:
 	unlock_rsb(res);
-	return rv;
 }
 
-static int print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
-			      struct dlm_rsb *r)
+static void print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
+			       struct dlm_rsb *r)
 {
 	u64 xid = 0;
 	u64 us;
-	int rv;
 
 	if (lkb->lkb_flags & DLM_IFL_USER) {
 		if (lkb->lkb_ua)
@@ -188,103 +181,97 @@ static int print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
 	/* id nodeid remid pid xid exflags flags sts grmode rqmode time_us
 	   r_nodeid r_len r_name */
 
-	rv = seq_printf(s, "%x %d %x %u %llu %x %x %d %d %d %llu %u %d \"%s\"\n",
-			lkb->lkb_id,
-			lkb->lkb_nodeid,
-			lkb->lkb_remid,
-			lkb->lkb_ownpid,
-			(unsigned long long)xid,
-			lkb->lkb_exflags,
-			lkb->lkb_flags,
-			lkb->lkb_status,
-			lkb->lkb_grmode,
-			lkb->lkb_rqmode,
-			(unsigned long long)us,
-			r->res_nodeid,
-			r->res_length,
-			r->res_name);
-	return rv;
+	seq_printf(s, "%x %d %x %u %llu %x %x %d %d %d %llu %u %d \"%s\"\n",
+		   lkb->lkb_id,
+		   lkb->lkb_nodeid,
+		   lkb->lkb_remid,
+		   lkb->lkb_ownpid,
+		   (unsigned long long)xid,
+		   lkb->lkb_exflags,
+		   lkb->lkb_flags,
+		   lkb->lkb_status,
+		   lkb->lkb_grmode,
+		   lkb->lkb_rqmode,
+		   (unsigned long long)us,
+		   r->res_nodeid,
+		   r->res_length,
+		   r->res_name);
 }
 
-static int print_format2(struct dlm_rsb *r, struct seq_file *s)
+static void print_format2(struct dlm_rsb *r, struct seq_file *s)
 {
 	struct dlm_lkb *lkb;
-	int rv = 0;
 
 	lock_rsb(r);
 
 	list_for_each_entry(lkb, &r->res_grantqueue, lkb_statequeue) {
-		rv = print_format2_lock(s, lkb, r);
-		if (rv)
+		print_format2_lock(s, lkb, r);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	list_for_each_entry(lkb, &r->res_convertqueue, lkb_statequeue) {
-		rv = print_format2_lock(s, lkb, r);
-		if (rv)
+		print_format2_lock(s, lkb, r);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	list_for_each_entry(lkb, &r->res_waitqueue, lkb_statequeue) {
-		rv = print_format2_lock(s, lkb, r);
-		if (rv)
+		print_format2_lock(s, lkb, r);
+		if (seq_is_full(s))
 			goto out;
 	}
  out:
 	unlock_rsb(r);
-	return rv;
 }
 
-static int print_format3_lock(struct seq_file *s, struct dlm_lkb *lkb,
+static void print_format3_lock(struct seq_file *s, struct dlm_lkb *lkb,
 			      int rsb_lookup)
 {
 	u64 xid = 0;
-	int rv;
 
 	if (lkb->lkb_flags & DLM_IFL_USER) {
 		if (lkb->lkb_ua)
 			xid = lkb->lkb_ua->xid;
 	}
 
-	rv = seq_printf(s, "lkb %x %d %x %u %llu %x %x %d %d %d %d %d %d %u %llu %llu\n",
-			lkb->lkb_id,
-			lkb->lkb_nodeid,
-			lkb->lkb_remid,
-			lkb->lkb_ownpid,
-			(unsigned long long)xid,
-			lkb->lkb_exflags,
-			lkb->lkb_flags,
-			lkb->lkb_status,
-			lkb->lkb_grmode,
-			lkb->lkb_rqmode,
-			lkb->lkb_last_bast.mode,
-			rsb_lookup,
-			lkb->lkb_wait_type,
-			lkb->lkb_lvbseq,
-			(unsigned long long)ktime_to_ns(lkb->lkb_timestamp),
-			(unsigned long long)ktime_to_ns(lkb->lkb_last_bast_time));
-	return rv;
+	seq_printf(s, "lkb %x %d %x %u %llu %x %x %d %d %d %d %d %d %u %llu %llu\n",
+		   lkb->lkb_id,
+		   lkb->lkb_nodeid,
+		   lkb->lkb_remid,
+		   lkb->lkb_ownpid,
+		   (unsigned long long)xid,
+		   lkb->lkb_exflags,
+		   lkb->lkb_flags,
+		   lkb->lkb_status,
+		   lkb->lkb_grmode,
+		   lkb->lkb_rqmode,
+		   lkb->lkb_last_bast.mode,
+		   rsb_lookup,
+		   lkb->lkb_wait_type,
+		   lkb->lkb_lvbseq,
+		   (unsigned long long)ktime_to_ns(lkb->lkb_timestamp),
+		   (unsigned long long)ktime_to_ns(lkb->lkb_last_bast_time));
 }
 
-static int print_format3(struct dlm_rsb *r, struct seq_file *s)
+static void print_format3(struct dlm_rsb *r, struct seq_file *s)
 {
 	struct dlm_lkb *lkb;
 	int i, lvblen = r->res_ls->ls_lvblen;
 	int print_name = 1;
-	int rv;
 
 	lock_rsb(r);
 
-	rv = seq_printf(s, "rsb %p %d %x %lx %d %d %u %d ",
-			r,
-			r->res_nodeid,
-			r->res_first_lkid,
-			r->res_flags,
-			!list_empty(&r->res_root_list),
-			!list_empty(&r->res_recover_list),
-			r->res_recover_locks_count,
-			r->res_length);
-	if (rv)
+	seq_printf(s, "rsb %p %d %x %lx %d %d %u %d ",
+		   r,
+		   r->res_nodeid,
+		   r->res_first_lkid,
+		   r->res_flags,
+		   !list_empty(&r->res_root_list),
+		   !list_empty(&r->res_recover_list),
+		   r->res_recover_locks_count,
+		   r->res_length);
+	if (seq_is_full(s))
 		goto out;
 
 	for (i = 0; i < r->res_length; i++) {
@@ -300,8 +287,8 @@ static int print_format3(struct dlm_rsb *r, struct seq_file *s)
 		else
 			seq_printf(s, " %02x", (unsigned char)r->res_name[i]);
 	}
-	rv = seq_puts(s, "\n");
-	if (rv)
+	seq_puts(s, "\n");
+	if (seq_is_full(s))
 		goto out;
 
 	if (!r->res_lvbptr)
@@ -311,58 +298,55 @@ static int print_format3(struct dlm_rsb *r, struct seq_file *s)
 
 	for (i = 0; i < lvblen; i++)
 		seq_printf(s, " %02x", (unsigned char)r->res_lvbptr[i]);
-	rv = seq_puts(s, "\n");
-	if (rv)
+	seq_puts(s, "\n");
+	if (seq_is_full(s))
 		goto out;
 
  do_locks:
 	list_for_each_entry(lkb, &r->res_grantqueue, lkb_statequeue) {
-		rv = print_format3_lock(s, lkb, 0);
-		if (rv)
+		print_format3_lock(s, lkb, 0);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	list_for_each_entry(lkb, &r->res_convertqueue, lkb_statequeue) {
-		rv = print_format3_lock(s, lkb, 0);
-		if (rv)
+		print_format3_lock(s, lkb, 0);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	list_for_each_entry(lkb, &r->res_waitqueue, lkb_statequeue) {
-		rv = print_format3_lock(s, lkb, 0);
-		if (rv)
+		print_format3_lock(s, lkb, 0);
+		if (seq_is_full(s))
 			goto out;
 	}
 
 	list_for_each_entry(lkb, &r->res_lookup, lkb_rsb_lookup) {
-		rv = print_format3_lock(s, lkb, 1);
-		if (rv)
+		print_format3_lock(s, lkb, 1);
+		if (seq_is_full(s))
 			goto out;
 	}
  out:
 	unlock_rsb(r);
-	return rv;
 }
 
-static int print_format4(struct dlm_rsb *r, struct seq_file *s)
+static void print_format4(struct dlm_rsb *r, struct seq_file *s)
 {
 	int our_nodeid = dlm_our_nodeid();
 	int print_name = 1;
-	int i, rv;
+	int i;
 
 	lock_rsb(r);
 
-	rv = seq_printf(s, "rsb %p %d %d %d %d %lu %lx %d ",
-			r,
-			r->res_nodeid,
-			r->res_master_nodeid,
-			r->res_dir_nodeid,
-			our_nodeid,
-			r->res_toss_time,
-			r->res_flags,
-			r->res_length);
-	if (rv)
-		goto out;
+	seq_printf(s, "rsb %p %d %d %d %d %lu %lx %d ",
+		   r,
+		   r->res_nodeid,
+		   r->res_master_nodeid,
+		   r->res_dir_nodeid,
+		   our_nodeid,
+		   r->res_toss_time,
+		   r->res_flags,
+		   r->res_length);
 
 	for (i = 0; i < r->res_length; i++) {
 		if (!isascii(r->res_name[i]) || !isprint(r->res_name[i]))
@@ -377,10 +361,9 @@ static int print_format4(struct dlm_rsb *r, struct seq_file *s)
 		else
 			seq_printf(s, " %02x", (unsigned char)r->res_name[i]);
 	}
-	rv = seq_puts(s, "\n");
- out:
+	seq_puts(s, "\n");
+
 	unlock_rsb(r);
-	return rv;
 }
 
 struct rsbtbl_iter {
@@ -390,11 +373,12 @@ struct rsbtbl_iter {
 	int header;
 };
 
-/* seq_printf returns -1 if the buffer is full, and 0 otherwise.
-   If the buffer is full, seq_printf can be called again, but it
-   does nothing and just returns -1.  So, the these printing routines
-   periodically check the return value to avoid wasting too much time
-   trying to print to a full buffer. */
+/*
+ * If the buffer is full, seq_printf can be called again, but it
+ * does nothing.  So, the these printing routines periodically check
+ * seq_is_full to avoid wasting too much time trying to print to
+ * a full buffer.
+ */
 
 static int table_seq_show(struct seq_file *seq, void *iter_ptr)
 {
@@ -403,7 +387,7 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
 
 	switch (ri->format) {
 	case 1:
-		rv = print_format1(ri->rsb, seq);
+		print_format1(ri->rsb, seq);
 		break;
 	case 2:
 		if (ri->header) {
@@ -412,21 +396,21 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
 					"r_nodeid r_len r_name\n");
 			ri->header = 0;
 		}
-		rv = print_format2(ri->rsb, seq);
+		print_format2(ri->rsb, seq);
 		break;
 	case 3:
 		if (ri->header) {
 			seq_printf(seq, "version rsb 1.1 lvb 1.1 lkb 1.1\n");
 			ri->header = 0;
 		}
-		rv = print_format3(ri->rsb, seq);
+		print_format3(ri->rsb, seq);
 		break;
 	case 4:
 		if (ri->header) {
 			seq_printf(seq, "version 4 rsb 2\n");
 			ri->header = 0;
 		}
-		rv = print_format4(ri->rsb, seq);
+		print_format4(ri->rsb, seq);
 		break;
 	}
 
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns

[Petr Mladek]

On Mon 29-09-14 16:08:23, Joe Perches wrote:
> The seq_printf return should be ignored and the
> seq_is_full function should be tested instead.
> 
> Convert functions returning int to void where
> seq_printf is used.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/dlm/debug_fs.c | 248 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 116 insertions(+), 132 deletions(-)
> 
> diff --git a/fs/dlm/debug_fs.c b/fs/dlm/debug_fs.c
> index 1323c56..27c8335 100644
> --- a/fs/dlm/debug_fs.c
> +++ b/fs/dlm/debug_fs.c
> @@ -48,8 +48,8 @@ static char *print_lockmode(int mode)
>  	}
>  }
>  
> -static int print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
> -			      struct dlm_rsb *res)
> +static void print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
> +			       struct dlm_rsb *res)
>  {
>  	seq_printf(s, "%08x %s", lkb->lkb_id, print_lockmode(lkb->lkb_grmode));
>  
> @@ -68,21 +68,17 @@ static int print_format1_lock(struct seq_file *s, struct dlm_lkb *lkb,
>  	if (lkb->lkb_wait_type)
>  		seq_printf(s, " wait_type: %d", lkb->lkb_wait_type);
>  
> -	return seq_puts(s, "\n");
> +	seq_puts(s, "\n");
>  }
>  
> -static int print_format1(struct dlm_rsb *res, struct seq_file *s)
> +static void print_format1(struct dlm_rsb *res, struct seq_file *s)
>  {
>  	struct dlm_lkb *lkb;
>  	int i, lvblen = res->res_ls->ls_lvblen, recover_list, root_list;
> -	int rv;
>  
>  	lock_rsb(res);
>  
> -	rv = seq_printf(s, "\nResource %p Name (len=%d) \"",
> -			res, res->res_length);
> -	if (rv)
> -		goto out;
> +	seq_printf(s, "\nResource %p Name (len=%d) \"", res, res->res_length);

To keep the functionality, we should add

	if(seq_overflow(s))
		goto out;

  
>  	for (i = 0; i < res->res_length; i++) {
>  		if (isprint(res->res_name[i]))
> @@ -92,17 +88,16 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
>  	}
>  
>  	if (res->res_nodeid > 0)
> -		rv = seq_printf(s, "\"\nLocal Copy, Master is node %d\n",
> -				res->res_nodeid);
> +		seq_printf(s, "\"\nLocal Copy, Master is node %d\n",
> +			   res->res_nodeid);
>  	else if (res->res_nodeid == 0)
> -		rv = seq_puts(s, "\"\nMaster Copy\n");
> +		seq_puts(s, "\"\nMaster Copy\n");
>  	else if (res->res_nodeid == -1)
> -		rv = seq_printf(s, "\"\nLooking up master (lkid %x)\n",
> -			   	res->res_first_lkid);
> +		seq_printf(s, "\"\nLooking up master (lkid %x)\n",
> +			   res->res_first_lkid);
>  	else
> -		rv = seq_printf(s, "\"\nInvalid master %d\n",
> -				res->res_nodeid);
> -	if (rv)
> +		seq_printf(s, "\"\nInvalid master %d\n", res->res_nodeid);
> +	if (seq_is_full(s))
>  		goto out;

I would check for seq_overflow()

Etc. There are needed many more changes if we agree on introducing
seq_is_full() and seq_overflow().

Well, we will need to touch most of these locations once again when
Steven removes return value from all the seq_* functions.

Best Regards,
Petr

>  	/* Print the LVB: */
> @@ -116,8 +111,8 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
>  		}
>  		if (rsb_flag(res, RSB_VALNOTVALID))
>  			seq_puts(s, " (INVALID)");
> -		rv = seq_puts(s, "\n");
> -		if (rv)
> +		seq_puts(s, "\n");
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
> @@ -125,32 +120,30 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
>  	recover_list = !list_empty(&res->res_recover_list);
>  
>  	if (root_list || recover_list) {
> -		rv = seq_printf(s, "Recovery: root %d recover %d flags %lx "
> -				"count %d\n", root_list, recover_list,
> -			   	res->res_flags, res->res_recover_locks_count);
> -		if (rv)
> -			goto out;
> +		seq_printf(s, "Recovery: root %d recover %d flags %lx count %d\n",
> +			   root_list, recover_list,
> +			   res->res_flags, res->res_recover_locks_count);
>  	}
>  
>  	/* Print the locks attached to this resource */
>  	seq_puts(s, "Granted Queue\n");
>  	list_for_each_entry(lkb, &res->res_grantqueue, lkb_statequeue) {
> -		rv = print_format1_lock(s, lkb, res);
> -		if (rv)
> +		print_format1_lock(s, lkb, res);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	seq_puts(s, "Conversion Queue\n");
>  	list_for_each_entry(lkb, &res->res_convertqueue, lkb_statequeue) {
> -		rv = print_format1_lock(s, lkb, res);
> -		if (rv)
> +		print_format1_lock(s, lkb, res);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	seq_puts(s, "Waiting Queue\n");
>  	list_for_each_entry(lkb, &res->res_waitqueue, lkb_statequeue) {
> -		rv = print_format1_lock(s, lkb, res);
> -		if (rv)
> +		print_format1_lock(s, lkb, res);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
> @@ -159,23 +152,23 @@ static int print_format1(struct dlm_rsb *res, struct seq_file *s)
>  
>  	seq_puts(s, "Lookup Queue\n");
>  	list_for_each_entry(lkb, &res->res_lookup, lkb_rsb_lookup) {
> -		rv = seq_printf(s, "%08x %s", lkb->lkb_id,
> -				print_lockmode(lkb->lkb_rqmode));
> +		seq_printf(s, "%08x %s",
> +			   lkb->lkb_id, print_lockmode(lkb->lkb_rqmode));
>  		if (lkb->lkb_wait_type)
>  			seq_printf(s, " wait_type: %d", lkb->lkb_wait_type);
> -		rv = seq_puts(s, "\n");
> +		seq_puts(s, "\n");
> +		if (seq_is_full(s))
> +			goto out;
>  	}
>   out:
>  	unlock_rsb(res);
> -	return rv;
>  }
>  
> -static int print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
> -			      struct dlm_rsb *r)
> +static void print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
> +			       struct dlm_rsb *r)
>  {
>  	u64 xid = 0;
>  	u64 us;
> -	int rv;
>  
>  	if (lkb->lkb_flags & DLM_IFL_USER) {
>  		if (lkb->lkb_ua)
> @@ -188,103 +181,97 @@ static int print_format2_lock(struct seq_file *s, struct dlm_lkb *lkb,
>  	/* id nodeid remid pid xid exflags flags sts grmode rqmode time_us
>  	   r_nodeid r_len r_name */
>  
> -	rv = seq_printf(s, "%x %d %x %u %llu %x %x %d %d %d %llu %u %d \"%s\"\n",
> -			lkb->lkb_id,
> -			lkb->lkb_nodeid,
> -			lkb->lkb_remid,
> -			lkb->lkb_ownpid,
> -			(unsigned long long)xid,
> -			lkb->lkb_exflags,
> -			lkb->lkb_flags,
> -			lkb->lkb_status,
> -			lkb->lkb_grmode,
> -			lkb->lkb_rqmode,
> -			(unsigned long long)us,
> -			r->res_nodeid,
> -			r->res_length,
> -			r->res_name);
> -	return rv;
> +	seq_printf(s, "%x %d %x %u %llu %x %x %d %d %d %llu %u %d \"%s\"\n",
> +		   lkb->lkb_id,
> +		   lkb->lkb_nodeid,
> +		   lkb->lkb_remid,
> +		   lkb->lkb_ownpid,
> +		   (unsigned long long)xid,
> +		   lkb->lkb_exflags,
> +		   lkb->lkb_flags,
> +		   lkb->lkb_status,
> +		   lkb->lkb_grmode,
> +		   lkb->lkb_rqmode,
> +		   (unsigned long long)us,
> +		   r->res_nodeid,
> +		   r->res_length,
> +		   r->res_name);
>  }
>  
> -static int print_format2(struct dlm_rsb *r, struct seq_file *s)
> +static void print_format2(struct dlm_rsb *r, struct seq_file *s)
>  {
>  	struct dlm_lkb *lkb;
> -	int rv = 0;
>  
>  	lock_rsb(r);
>  
>  	list_for_each_entry(lkb, &r->res_grantqueue, lkb_statequeue) {
> -		rv = print_format2_lock(s, lkb, r);
> -		if (rv)
> +		print_format2_lock(s, lkb, r);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	list_for_each_entry(lkb, &r->res_convertqueue, lkb_statequeue) {
> -		rv = print_format2_lock(s, lkb, r);
> -		if (rv)
> +		print_format2_lock(s, lkb, r);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	list_for_each_entry(lkb, &r->res_waitqueue, lkb_statequeue) {
> -		rv = print_format2_lock(s, lkb, r);
> -		if (rv)
> +		print_format2_lock(s, lkb, r);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>   out:
>  	unlock_rsb(r);
> -	return rv;
>  }
>  
> -static int print_format3_lock(struct seq_file *s, struct dlm_lkb *lkb,
> +static void print_format3_lock(struct seq_file *s, struct dlm_lkb *lkb,
>  			      int rsb_lookup)
>  {
>  	u64 xid = 0;
> -	int rv;
>  
>  	if (lkb->lkb_flags & DLM_IFL_USER) {
>  		if (lkb->lkb_ua)
>  			xid = lkb->lkb_ua->xid;
>  	}
>  
> -	rv = seq_printf(s, "lkb %x %d %x %u %llu %x %x %d %d %d %d %d %d %u %llu %llu\n",
> -			lkb->lkb_id,
> -			lkb->lkb_nodeid,
> -			lkb->lkb_remid,
> -			lkb->lkb_ownpid,
> -			(unsigned long long)xid,
> -			lkb->lkb_exflags,
> -			lkb->lkb_flags,
> -			lkb->lkb_status,
> -			lkb->lkb_grmode,
> -			lkb->lkb_rqmode,
> -			lkb->lkb_last_bast.mode,
> -			rsb_lookup,
> -			lkb->lkb_wait_type,
> -			lkb->lkb_lvbseq,
> -			(unsigned long long)ktime_to_ns(lkb->lkb_timestamp),
> -			(unsigned long long)ktime_to_ns(lkb->lkb_last_bast_time));
> -	return rv;
> +	seq_printf(s, "lkb %x %d %x %u %llu %x %x %d %d %d %d %d %d %u %llu %llu\n",
> +		   lkb->lkb_id,
> +		   lkb->lkb_nodeid,
> +		   lkb->lkb_remid,
> +		   lkb->lkb_ownpid,
> +		   (unsigned long long)xid,
> +		   lkb->lkb_exflags,
> +		   lkb->lkb_flags,
> +		   lkb->lkb_status,
> +		   lkb->lkb_grmode,
> +		   lkb->lkb_rqmode,
> +		   lkb->lkb_last_bast.mode,
> +		   rsb_lookup,
> +		   lkb->lkb_wait_type,
> +		   lkb->lkb_lvbseq,
> +		   (unsigned long long)ktime_to_ns(lkb->lkb_timestamp),
> +		   (unsigned long long)ktime_to_ns(lkb->lkb_last_bast_time));
>  }
>  
> -static int print_format3(struct dlm_rsb *r, struct seq_file *s)
> +static void print_format3(struct dlm_rsb *r, struct seq_file *s)
>  {
>  	struct dlm_lkb *lkb;
>  	int i, lvblen = r->res_ls->ls_lvblen;
>  	int print_name = 1;
> -	int rv;
>  
>  	lock_rsb(r);
>  
> -	rv = seq_printf(s, "rsb %p %d %x %lx %d %d %u %d ",
> -			r,
> -			r->res_nodeid,
> -			r->res_first_lkid,
> -			r->res_flags,
> -			!list_empty(&r->res_root_list),
> -			!list_empty(&r->res_recover_list),
> -			r->res_recover_locks_count,
> -			r->res_length);
> -	if (rv)
> +	seq_printf(s, "rsb %p %d %x %lx %d %d %u %d ",
> +		   r,
> +		   r->res_nodeid,
> +		   r->res_first_lkid,
> +		   r->res_flags,
> +		   !list_empty(&r->res_root_list),
> +		   !list_empty(&r->res_recover_list),
> +		   r->res_recover_locks_count,
> +		   r->res_length);
> +	if (seq_is_full(s))
>  		goto out;
>  
>  	for (i = 0; i < r->res_length; i++) {
> @@ -300,8 +287,8 @@ static int print_format3(struct dlm_rsb *r, struct seq_file *s)
>  		else
>  			seq_printf(s, " %02x", (unsigned char)r->res_name[i]);
>  	}
> -	rv = seq_puts(s, "\n");
> -	if (rv)
> +	seq_puts(s, "\n");
> +	if (seq_is_full(s))
>  		goto out;
>  
>  	if (!r->res_lvbptr)
> @@ -311,58 +298,55 @@ static int print_format3(struct dlm_rsb *r, struct seq_file *s)
>  
>  	for (i = 0; i < lvblen; i++)
>  		seq_printf(s, " %02x", (unsigned char)r->res_lvbptr[i]);
> -	rv = seq_puts(s, "\n");
> -	if (rv)
> +	seq_puts(s, "\n");
> +	if (seq_is_full(s))
>  		goto out;
>  
>   do_locks:
>  	list_for_each_entry(lkb, &r->res_grantqueue, lkb_statequeue) {
> -		rv = print_format3_lock(s, lkb, 0);
> -		if (rv)
> +		print_format3_lock(s, lkb, 0);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	list_for_each_entry(lkb, &r->res_convertqueue, lkb_statequeue) {
> -		rv = print_format3_lock(s, lkb, 0);
> -		if (rv)
> +		print_format3_lock(s, lkb, 0);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	list_for_each_entry(lkb, &r->res_waitqueue, lkb_statequeue) {
> -		rv = print_format3_lock(s, lkb, 0);
> -		if (rv)
> +		print_format3_lock(s, lkb, 0);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>  
>  	list_for_each_entry(lkb, &r->res_lookup, lkb_rsb_lookup) {
> -		rv = print_format3_lock(s, lkb, 1);
> -		if (rv)
> +		print_format3_lock(s, lkb, 1);
> +		if (seq_is_full(s))
>  			goto out;
>  	}
>   out:
>  	unlock_rsb(r);
> -	return rv;
>  }
>  
> -static int print_format4(struct dlm_rsb *r, struct seq_file *s)
> +static void print_format4(struct dlm_rsb *r, struct seq_file *s)
>  {
>  	int our_nodeid = dlm_our_nodeid();
>  	int print_name = 1;
> -	int i, rv;
> +	int i;
>  
>  	lock_rsb(r);
>  
> -	rv = seq_printf(s, "rsb %p %d %d %d %d %lu %lx %d ",
> -			r,
> -			r->res_nodeid,
> -			r->res_master_nodeid,
> -			r->res_dir_nodeid,
> -			our_nodeid,
> -			r->res_toss_time,
> -			r->res_flags,
> -			r->res_length);
> -	if (rv)
> -		goto out;
> +	seq_printf(s, "rsb %p %d %d %d %d %lu %lx %d ",
> +		   r,
> +		   r->res_nodeid,
> +		   r->res_master_nodeid,
> +		   r->res_dir_nodeid,
> +		   our_nodeid,
> +		   r->res_toss_time,
> +		   r->res_flags,
> +		   r->res_length);
>  
>  	for (i = 0; i < r->res_length; i++) {
>  		if (!isascii(r->res_name[i]) || !isprint(r->res_name[i]))
> @@ -377,10 +361,9 @@ static int print_format4(struct dlm_rsb *r, struct seq_file *s)
>  		else
>  			seq_printf(s, " %02x", (unsigned char)r->res_name[i]);
>  	}
> -	rv = seq_puts(s, "\n");
> - out:
> +	seq_puts(s, "\n");
> +
>  	unlock_rsb(r);
> -	return rv;
>  }
>  
>  struct rsbtbl_iter {
> @@ -390,11 +373,12 @@ struct rsbtbl_iter {
>  	int header;
>  };
>  
> -/* seq_printf returns -1 if the buffer is full, and 0 otherwise.
> -   If the buffer is full, seq_printf can be called again, but it
> -   does nothing and just returns -1.  So, the these printing routines
> -   periodically check the return value to avoid wasting too much time
> -   trying to print to a full buffer. */
> +/*
> + * If the buffer is full, seq_printf can be called again, but it
> + * does nothing.  So, the these printing routines periodically check
> + * seq_is_full to avoid wasting too much time trying to print to
> + * a full buffer.
> + */
>  
>  static int table_seq_show(struct seq_file *seq, void *iter_ptr)
>  {
> @@ -403,7 +387,7 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
>  
>  	switch (ri->format) {
>  	case 1:
> -		rv = print_format1(ri->rsb, seq);
> +		print_format1(ri->rsb, seq);
>  		break;
>  	case 2:
>  		if (ri->header) {
> @@ -412,21 +396,21 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
>  					"r_nodeid r_len r_name\n");
>  			ri->header = 0;
>  		}
> -		rv = print_format2(ri->rsb, seq);
> +		print_format2(ri->rsb, seq);
>  		break;
>  	case 3:
>  		if (ri->header) {
>  			seq_printf(seq, "version rsb 1.1 lvb 1.1 lkb 1.1\n");
>  			ri->header = 0;
>  		}
> -		rv = print_format3(ri->rsb, seq);
> +		print_format3(ri->rsb, seq);
>  		break;
>  	case 4:
>  		if (ri->header) {
>  			seq_printf(seq, "version 4 rsb 2\n");
>  			ri->header = 0;
>  		}
> -		rv = print_format4(ri->rsb, seq);
> +		print_format4(ri->rsb, seq);
>  		break;
>  	}
>  
> -- 
> 1.8.1.2.459.gbcd45b4.dirty
> 

Re: [PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns

[Steven Rostedt]

Note, I've started with Joe's patches and I'm massaging them for
something I can work with.

On Tue, 30 Sep 2014 12:34:35 +0200
Petr Mladek <pmladek@suse.cz> wrote:


> > -		rv = seq_printf(s, "\"\nInvalid master %d\n",
> > -				res->res_nodeid);
> > -	if (rv)
> > +		seq_printf(s, "\"\nInvalid master %d\n", res->res_nodeid);
> > +	if (seq_is_full(s))
> >  		goto out;
> 
> I would check for seq_overflow()
> 
> Etc. There are needed many more changes if we agree on introducing
> seq_is_full() and seq_overflow().

As I'm looking at this code, I'm thinking that we never
really care about seq_is_full(). We only really care if
seq_overflowed(), in which the contents will be discarded.

Rational? Because if we break when seq_is_full(), my new logic wont
throw away the result. If we break out of the function when it's full
and not when it has overflowed, then we may never print out the rest of
the content, as the seq_file code will still use a full buffer that
hasn't overflowed.

I'm thinking of switching everything to use seq_has_overflowed() and
try again.

Thoughts?

-- Steve

Re: [PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns

[Joe Perches]

On Mon, 2014-10-27 at 16:17 -0400, Steven Rostedt wrote:
> Note, I've started with Joe's patches and I'm massaging them for
> something I can work with.
[]
> I'm thinking of switching everything to use seq_has_overflowed() and
> try again.

Fine by me.

Re: [PATCH 3/7] dlm: Use seq_is_full - remove seq_printf returns

[Petr Mladek]

On Mon 2014-10-27 16:17:24, Steven Rostedt wrote:
> Note, I've started with Joe's patches and I'm massaging them for
> something I can work with.
> 
> On Tue, 30 Sep 2014 12:34:35 +0200
> Petr Mladek <pmladek@suse.cz> wrote:
> 
> 
> > > -		rv = seq_printf(s, "\"\nInvalid master %d\n",
> > > -				res->res_nodeid);
> > > -	if (rv)
> > > +		seq_printf(s, "\"\nInvalid master %d\n", res->res_nodeid);
> > > +	if (seq_is_full(s))
> > >  		goto out;
> > 
> > I would check for seq_overflow()
> > 
> > Etc. There are needed many more changes if we agree on introducing
> > seq_is_full() and seq_overflow().
> 
> As I'm looking at this code, I'm thinking that we never
> really care about seq_is_full(). We only really care if
> seq_overflowed(), in which the contents will be discarded.
> 
> Rational? Because if we break when seq_is_full(), my new logic wont
> throw away the result. If we break out of the function when it's full
> and not when it has overflowed, then we may never print out the rest of
> the content, as the seq_file code will still use a full buffer that
> hasn't overflowed.
> 
> I'm thinking of switching everything to use seq_has_overflowed() and
> try again.
> 
> Thoughts?

Sounds good to me.

Best Regards,
Petr

[PATCH 4/7] dlm: Use seq_puts, remove unnecessary trailing spaces

[Joe Perches]

Convert the seq_printf output with constant strings to seq_puts.
Remove unnecessary trailing spaces from seq_(printf/puts) output.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/dlm/debug_fs.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/dlm/debug_fs.c b/fs/dlm/debug_fs.c
index 27c8335..c69a766 100644
--- a/fs/dlm/debug_fs.c
+++ b/fs/dlm/debug_fs.c
@@ -279,7 +279,7 @@ static void print_format3(struct dlm_rsb *r, struct seq_file *s)
 			print_name = 0;
 	}
 
-	seq_printf(s, "%s", print_name ? "str " : "hex");
+	seq_puts(s, print_name ? "str " : "hex");
 
 	for (i = 0; i < r->res_length; i++) {
 		if (print_name)
@@ -353,7 +353,7 @@ static void print_format4(struct dlm_rsb *r, struct seq_file *s)
 			print_name = 0;
 	}
 
-	seq_printf(s, "%s", print_name ? "str " : "hex");
+	seq_puts(s, print_name ? "str " : "hex");
 
 	for (i = 0; i < r->res_length; i++) {
 		if (print_name)
@@ -391,23 +391,21 @@ static int table_seq_show(struct seq_file *seq, void *iter_ptr)
 		break;
 	case 2:
 		if (ri->header) {
-			seq_printf(seq, "id nodeid remid pid xid exflags "
-					"flags sts grmode rqmode time_ms "
-					"r_nodeid r_len r_name\n");
+			seq_puts(seq, "id nodeid remid pid xid exflags flags sts grmode rqmode time_ms r_nodeid r_len r_name\n");
 			ri->header = 0;
 		}
 		print_format2(ri->rsb, seq);
 		break;
 	case 3:
 		if (ri->header) {
-			seq_printf(seq, "version rsb 1.1 lvb 1.1 lkb 1.1\n");
+			seq_puts(seq, "version rsb 1.1 lvb 1.1 lkb 1.1\n");
 			ri->header = 0;
 		}
 		print_format3(ri->rsb, seq);
 		break;
 	case 4:
 		if (ri->header) {
-			seq_printf(seq, "version 4 rsb 2\n");
+			seq_puts(seq, "version 4 rsb 2\n");
 			ri->header = 0;
 		}
 		print_format4(ri->rsb, seq);
-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 5/7] fs: Convert show_fdinfo functions to void

[Joe Perches]

seq_printf functions shouldn't really check the return value.
Checking seq_is_full occasionally is used instead.

Update vfs documentation.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/filesystems/vfs.txt |  2 +-
 drivers/net/tun.c                 |  4 +--
 fs/eventfd.c                      | 15 +++-----
 fs/eventpoll.c                    | 19 ++++------
 fs/notify/fdinfo.c                | 76 +++++++++++++++++----------------------
 fs/notify/fdinfo.h                |  4 +--
 fs/proc/fd.c                      |  2 +-
 fs/signalfd.c                     | 10 ++----
 fs/timerfd.c                      | 27 +++++++-------
 include/linux/fs.h                |  2 +-
 10 files changed, 68 insertions(+), 93 deletions(-)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index fceff7c..af1dbc1 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -828,7 +828,7 @@ struct file_operations {
 	ssize_t (*splice_read)(struct file *, struct pipe_inode_info *, size_t, unsigned int);
 	int (*setlease)(struct file *, long arg, struct file_lock **, void **);
 	long (*fallocate)(struct file *, int mode, loff_t offset, loff_t len);
-	int (*show_fdinfo)(struct seq_file *m, struct file *f);
+	void (*show_fdinfo)(struct seq_file *m, struct file *f);
 };
 
 Again, all methods are called without any locks being held, unless
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 186ce54..a3420e0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2209,7 +2209,7 @@ static int tun_chr_close(struct inode *inode, struct file *file)
 }
 
 #ifdef CONFIG_PROC_FS
-static int tun_chr_show_fdinfo(struct seq_file *m, struct file *f)
+static void tun_chr_show_fdinfo(struct seq_file *m, struct file *f)
 {
 	struct tun_struct *tun;
 	struct ifreq ifr;
@@ -2225,7 +2225,7 @@ static int tun_chr_show_fdinfo(struct seq_file *m, struct file *f)
 	if (tun)
 		tun_put(tun);
 
-	return seq_printf(m, "iff:\t%s\n", ifr.ifr_name);
+	seq_printf(m, "iff:\t%s\n", ifr.ifr_name);
 }
 #endif
 
diff --git a/fs/eventfd.c b/fs/eventfd.c
index d6a88e7..abcb25d 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -286,25 +286,20 @@ static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t c
 	return res;
 }
 
-#ifdef CONFIG_PROC_FS
-static int eventfd_show_fdinfo(struct seq_file *m, struct file *f)
+static void eventfd_show_fdinfo(struct seq_file *m, struct file *f)
 {
+#ifdef CONFIG_PROC_FS
 	struct eventfd_ctx *ctx = f->private_data;
-	int ret;
 
 	spin_lock_irq(&ctx->wqh.lock);
-	ret = seq_printf(m, "eventfd-count: %16llx\n",
-			 (unsigned long long)ctx->count);
+	seq_printf(m, "eventfd-count: %16llx\n",
+		   (unsigned long long)ctx->count);
 	spin_unlock_irq(&ctx->wqh.lock);
-
-	return ret;
-}
 #endif
+}
 
 static const struct file_operations eventfd_fops = {
-#ifdef CONFIG_PROC_FS
 	.show_fdinfo	= eventfd_show_fdinfo,
-#endif
 	.release	= eventfd_release,
 	.poll		= eventfd_poll,
 	.read		= eventfd_read,
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 7bcfff9..4e742b6 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -869,34 +869,29 @@ static unsigned int ep_eventpoll_poll(struct file *file, poll_table *wait)
 	return pollflags != -1 ? pollflags : 0;
 }
 
-#ifdef CONFIG_PROC_FS
-static int ep_show_fdinfo(struct seq_file *m, struct file *f)
+static void ep_show_fdinfo(struct seq_file *m, struct file *f)
 {
+#ifdef CONFIG_PROC_FS
 	struct eventpoll *ep = f->private_data;
 	struct rb_node *rbp;
-	int ret = 0;
 
 	mutex_lock(&ep->mtx);
 	for (rbp = rb_first(&ep->rbr); rbp; rbp = rb_next(rbp)) {
 		struct epitem *epi = rb_entry(rbp, struct epitem, rbn);
 
-		ret = seq_printf(m, "tfd: %8d events: %8x data: %16llx\n",
-				 epi->ffd.fd, epi->event.events,
-				 (long long)epi->event.data);
-		if (ret)
+		seq_printf(m, "tfd: %8d events: %8x data: %16llx\n",
+			   epi->ffd.fd, epi->event.events,
+			   (long long)epi->event.data);
+		if (seq_is_full(m))
 			break;
 	}
 	mutex_unlock(&ep->mtx);
-
-	return ret;
-}
 #endif
+}
 
 /* File callbacks that implement the eventpoll file behaviour */
 static const struct file_operations eventpoll_fops = {
-#ifdef CONFIG_PROC_FS
 	.show_fdinfo	= ep_show_fdinfo,
-#endif
 	.release	= ep_eventpoll_release,
 	.poll		= ep_eventpoll_poll,
 	.llseek		= noop_llseek,
diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 9d7e2b9..0bb10b7 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -20,25 +20,24 @@
 
 #if defined(CONFIG_INOTIFY_USER) || defined(CONFIG_FANOTIFY)
 
-static int show_fdinfo(struct seq_file *m, struct file *f,
-		       int (*show)(struct seq_file *m, struct fsnotify_mark *mark))
+static void show_fdinfo(struct seq_file *m, struct file *f,
+			void (*show)(struct seq_file *m,
+				     struct fsnotify_mark *mark))
 {
 	struct fsnotify_group *group = f->private_data;
 	struct fsnotify_mark *mark;
-	int ret = 0;
 
 	mutex_lock(&group->mark_mutex);
 	list_for_each_entry(mark, &group->marks_list, g_list) {
-		ret = show(m, mark);
-		if (ret)
+		show(m, mark);
+		if (seq_is_full(m))
 			break;
 	}
 	mutex_unlock(&group->mark_mutex);
-	return ret;
 }
 
 #if defined(CONFIG_EXPORTFS)
-static int show_mark_fhandle(struct seq_file *m, struct inode *inode)
+static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 {
 	struct {
 		struct file_handle handle;
@@ -52,71 +51,62 @@ static int show_mark_fhandle(struct seq_file *m, struct inode *inode)
 	ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, 0);
 	if ((ret == FILEID_INVALID) || (ret < 0)) {
 		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
-		return 0;
+		return;
 	}
 
 	f.handle.handle_type = ret;
 	f.handle.handle_bytes = size * sizeof(u32);
 
-	ret = seq_printf(m, "fhandle-bytes:%x fhandle-type:%x f_handle:",
-			 f.handle.handle_bytes, f.handle.handle_type);
+	seq_printf(m, "fhandle-bytes:%x fhandle-type:%x f_handle:",
+		   f.handle.handle_bytes, f.handle.handle_type);
 
 	for (i = 0; i < f.handle.handle_bytes; i++)
-		ret |= seq_printf(m, "%02x", (int)f.handle.f_handle[i]);
-
-	return ret;
+		seq_printf(m, "%02x", (int)f.handle.f_handle[i]);
 }
 #else
-static int show_mark_fhandle(struct seq_file *m, struct inode *inode)
+static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 {
-	return 0;
 }
 #endif
 
 #ifdef CONFIG_INOTIFY_USER
 
-static int inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
+static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 {
 	struct inotify_inode_mark *inode_mark;
 	struct inode *inode;
-	int ret = 0;
 
 	if (!(mark->flags & (FSNOTIFY_MARK_FLAG_ALIVE | FSNOTIFY_MARK_FLAG_INODE)))
-		return 0;
+		return;
 
 	inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark);
 	inode = igrab(mark->i.inode);
 	if (inode) {
-		ret = seq_printf(m, "inotify wd:%x ino:%lx sdev:%x "
-				 "mask:%x ignored_mask:%x ",
-				 inode_mark->wd, inode->i_ino,
-				 inode->i_sb->s_dev,
-				 mark->mask, mark->ignored_mask);
-		ret |= show_mark_fhandle(m, inode);
-		ret |= seq_putc(m, '\n');
+		seq_printf(m, "inotify wd:%x ino:%lx sdev:%x mask:%x ignored_mask:%x ",
+			   inode_mark->wd, inode->i_ino, inode->i_sb->s_dev,
+			   mark->mask, mark->ignored_mask);
+		show_mark_fhandle(m, inode);
+		seq_putc(m, '\n');
 		iput(inode);
 	}
-
-	return ret;
 }
 
-int inotify_show_fdinfo(struct seq_file *m, struct file *f)
+void inotify_show_fdinfo(struct seq_file *m, struct file *f)
 {
-	return show_fdinfo(m, f, inotify_fdinfo);
+	show_fdinfo(m, f, inotify_fdinfo);
 }
 
 #endif /* CONFIG_INOTIFY_USER */
 
 #ifdef CONFIG_FANOTIFY
 
-static int fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
+static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 {
 	unsigned int mflags = 0;
 	struct inode *inode;
-	int ret = 0;
 
 	if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE))
-		return 0;
+		return;
 
 	if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
 		mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
@@ -125,25 +115,23 @@ static int fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		inode = igrab(mark->i.inode);
 		if (!inode)
 			goto out;
-		ret = seq_printf(m, "fanotify ino:%lx sdev:%x "
-				 "mflags:%x mask:%x ignored_mask:%x ",
-				 inode->i_ino, inode->i_sb->s_dev,
-				 mflags, mark->mask, mark->ignored_mask);
-		ret |= show_mark_fhandle(m, inode);
-		ret |= seq_putc(m, '\n');
+		seq_printf(m, "fanotify ino:%lx sdev:%x mflags:%x mask:%x ignored_mask:%x ",
+			   inode->i_ino, inode->i_sb->s_dev,
+			   mflags, mark->mask, mark->ignored_mask);
+		show_mark_fhandle(m, inode);
+		seq_putc(m, '\n');
 		iput(inode);
 	} else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT) {
 		struct mount *mnt = real_mount(mark->m.mnt);
 
-		ret = seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x "
-				 "ignored_mask:%x\n", mnt->mnt_id, mflags,
-				 mark->mask, mark->ignored_mask);
+		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
+			   mnt->mnt_id, mflags, mark->mask, mark->ignored_mask);
 	}
 out:
-	return ret;
+	;
 }
 
-int fanotify_show_fdinfo(struct seq_file *m, struct file *f)
+void fanotify_show_fdinfo(struct seq_file *m, struct file *f)
 {
 	struct fsnotify_group *group = f->private_data;
 	unsigned int flags = 0;
@@ -169,7 +157,7 @@ int fanotify_show_fdinfo(struct seq_file *m, struct file *f)
 	seq_printf(m, "fanotify flags:%x event-flags:%x\n",
 		   flags, group->fanotify_data.f_flags);
 
-	return show_fdinfo(m, f, fanotify_fdinfo);
+	show_fdinfo(m, f, fanotify_fdinfo);
 }
 
 #endif /* CONFIG_FANOTIFY */
diff --git a/fs/notify/fdinfo.h b/fs/notify/fdinfo.h
index 556afda..9664c49 100644
--- a/fs/notify/fdinfo.h
+++ b/fs/notify/fdinfo.h
@@ -10,11 +10,11 @@ struct file;
 #ifdef CONFIG_PROC_FS
 
 #ifdef CONFIG_INOTIFY_USER
-extern int inotify_show_fdinfo(struct seq_file *m, struct file *f);
+void inotify_show_fdinfo(struct seq_file *m, struct file *f);
 #endif
 
 #ifdef CONFIG_FANOTIFY
-extern int fanotify_show_fdinfo(struct seq_file *m, struct file *f);
+void fanotify_show_fdinfo(struct seq_file *m, struct file *f);
 #endif
 
 #else /* CONFIG_PROC_FS */
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index e11d7c5..4c3c253 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -53,7 +53,7 @@ static int seq_show(struct seq_file *m, void *v)
 			   (long long)file->f_pos, f_flags,
 			   real_mount(file->f_path.mnt)->mnt_id);
 		if (file->f_op->show_fdinfo)
-			ret = file->f_op->show_fdinfo(m, file);
+			file->f_op->show_fdinfo(m, file);
 		fput(file);
 	}
 
diff --git a/fs/signalfd.c b/fs/signalfd.c
index 424b7b6..06bd5a2 100644
--- a/fs/signalfd.c
+++ b/fs/signalfd.c
@@ -229,24 +229,20 @@ static ssize_t signalfd_read(struct file *file, char __user *buf, size_t count,
 	return total ? total: ret;
 }
 
-#ifdef CONFIG_PROC_FS
-static int signalfd_show_fdinfo(struct seq_file *m, struct file *f)
+static void signalfd_show_fdinfo(struct seq_file *m, struct file *f)
 {
+#ifdef CONFIG_PROC_FS
 	struct signalfd_ctx *ctx = f->private_data;
 	sigset_t sigmask;
 
 	sigmask = ctx->sigmask;
 	signotset(&sigmask);
 	render_sigset_t(m, "sigmask:\t", &sigmask);
-
-	return 0;
-}
 #endif
+}
 
 static const struct file_operations signalfd_fops = {
-#ifdef CONFIG_PROC_FS
 	.show_fdinfo	= signalfd_show_fdinfo,
-#endif
 	.release	= signalfd_release,
 	.poll		= signalfd_poll,
 	.read		= signalfd_read,
diff --git a/fs/timerfd.c b/fs/timerfd.c
index b46ffa9..b94fa6c 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -288,7 +288,7 @@ static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
 }
 
 #ifdef CONFIG_PROC_FS
-static int timerfd_show(struct seq_file *m, struct file *file)
+static void timerfd_show(struct seq_file *m, struct file *file)
 {
 	struct timerfd_ctx *ctx = file->private_data;
 	struct itimerspec t;
@@ -298,18 +298,19 @@ static int timerfd_show(struct seq_file *m, struct file *file)
 	t.it_interval = ktime_to_timespec(ctx->tintv);
 	spin_unlock_irq(&ctx->wqh.lock);
 
-	return seq_printf(m,
-			  "clockid: %d\n"
-			  "ticks: %llu\n"
-			  "settime flags: 0%o\n"
-			  "it_value: (%llu, %llu)\n"
-			  "it_interval: (%llu, %llu)\n",
-			  ctx->clockid, (unsigned long long)ctx->ticks,
-			  ctx->settime_flags,
-			  (unsigned long long)t.it_value.tv_sec,
-			  (unsigned long long)t.it_value.tv_nsec,
-			  (unsigned long long)t.it_interval.tv_sec,
-			  (unsigned long long)t.it_interval.tv_nsec);
+	seq_printf(m,
+		   "clockid: %d\n"
+		   "ticks: %llu\n"
+		   "settime flags: 0%o\n"
+		   "it_value: (%llu, %llu)\n"
+		   "it_interval: (%llu, %llu)\n",
+		   ctx->clockid,
+		   (unsigned long long)ctx->ticks,
+		   ctx->settime_flags,
+		   (unsigned long long)t.it_value.tv_sec,
+		   (unsigned long long)t.it_value.tv_nsec,
+		   (unsigned long long)t.it_interval.tv_sec,
+		   (unsigned long long)t.it_interval.tv_nsec);
 }
 #else
 #define timerfd_show NULL
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a4ce5bae..e2f67f0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1494,7 +1494,7 @@ struct file_operations {
 	int (*setlease)(struct file *, long, struct file_lock **, void **);
 	long (*fallocate)(struct file *file, int mode, loff_t offset,
 			  loff_t len);
-	int (*show_fdinfo)(struct seq_file *m, struct file *f);
+	void (*show_fdinfo)(struct seq_file *m, struct file *f);
 };
 
 struct inode_operations {
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 5/7] fs: Convert show_fdinfo functions to void

[Steven Rostedt]

On Mon, 29 Sep 2014 16:08:25 -0700
Joe Perches <joe@perches.com> wrote:

> seq_printf functions shouldn't really check the return value.
> Checking seq_is_full occasionally is used instead.
> 
> Update vfs documentation.
> 
> Signed-off-by: Joe Perches <joe@perches.com>


>  
> -#ifdef CONFIG_PROC_FS
> -static int eventfd_show_fdinfo(struct seq_file *m, struct file *f)
> +static void eventfd_show_fdinfo(struct seq_file *m, struct file *f)
>  {
> +#ifdef CONFIG_PROC_FS
>  	struct eventfd_ctx *ctx = f->private_data;
> -	int ret;
>  
>  	spin_lock_irq(&ctx->wqh.lock);
> -	ret = seq_printf(m, "eventfd-count: %16llx\n",
> -			 (unsigned long long)ctx->count);
> +	seq_printf(m, "eventfd-count: %16llx\n",
> +		   (unsigned long long)ctx->count);
>  	spin_unlock_irq(&ctx->wqh.lock);
> -
> -	return ret;
> -}
>  #endif
> +}
>  
>  static const struct file_operations eventfd_fops = {
> -#ifdef CONFIG_PROC_FS
>  	.show_fdinfo	= eventfd_show_fdinfo,
> -#endif

I wouldn't change logic on this. There's no reason to call this
function if it isn't doing anything.

I'll change this to just do the update and not change logic like this.

-- Steve

>  	.release	= eventfd_release,
>  	.poll		= eventfd_poll,
>  	.read		= eventfd_read,



> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index e11d7c5..4c3c253 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -53,7 +53,7 @@ static int seq_show(struct seq_file *m, void *v)
>  			   (long long)file->f_pos, f_flags,
>  			   real_mount(file->f_path.mnt)->mnt_id);
>  		if (file->f_op->show_fdinfo)
> -			ret = file->f_op->show_fdinfo(m, file);
> +			file->f_op->show_fdinfo(m, file);
>  		fput(file);
>  	}
>  

Re: [PATCH 5/7] fs: Convert show_fdinfo functions to void

[Joe Perches]

On Tue, 2014-10-28 at 10:11 -0400, Steven Rostedt wrote:
> On Mon, 29 Sep 2014 16:08:25 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > seq_printf functions shouldn't really check the return value.
> > Checking seq_is_full occasionally is used instead.
> > 
> > Update vfs documentation.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> 
> >  
> > -#ifdef CONFIG_PROC_FS
> > -static int eventfd_show_fdinfo(struct seq_file *m, struct file *f)
> > +static void eventfd_show_fdinfo(struct seq_file *m, struct file *f)
> >  {
> > +#ifdef CONFIG_PROC_FS
> >  	struct eventfd_ctx *ctx = f->private_data;
> > -	int ret;
> >  
> >  	spin_lock_irq(&ctx->wqh.lock);
> > -	ret = seq_printf(m, "eventfd-count: %16llx\n",
> > -			 (unsigned long long)ctx->count);
> > +	seq_printf(m, "eventfd-count: %16llx\n",
> > +		   (unsigned long long)ctx->count);
> >  	spin_unlock_irq(&ctx->wqh.lock);
> > -
> > -	return ret;
> > -}
> >  #endif
> > +}
> >  
> >  static const struct file_operations eventfd_fops = {
> > -#ifdef CONFIG_PROC_FS
> >  	.show_fdinfo	= eventfd_show_fdinfo,
> > -#endif
> 
> I wouldn't change logic on this. There's no reason to call this
> function if it isn't doing anything.
> 
> I'll change this to just do the update and not change logic like this.

Fewer #ifdefs

Re: [PATCH 5/7] fs: Convert show_fdinfo functions to void

[Steven Rostedt]

On Tue, 28 Oct 2014 07:31:32 -0700
Joe Perches <joe@perches.com> wrote:

 
> > I wouldn't change logic on this. There's no reason to call this
> > function if it isn't doing anything.
> > 
> > I'll change this to just do the update and not change logic like this.
> 
> Fewer #ifdefs
> 

And there's other ways to fix it (like using an #else), but that is
off-topic to the current change set. In other words, that change should
be separate, as I don't want discussions on what's the best use of
#ifdefs distracting from getting in the "void seq_printf()" changes.

-- Steve

[PATCH 6/7] debugfs: Fix misuse of seq_printf return value

[Joe Perches]

Adding repeated -1 to the return is not correct.

Use seq_is_full to test for unnecessary seq_printf uses
and always return 0.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/debugfs/file.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 76c08c2..c24e578 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -695,15 +695,17 @@ EXPORT_SYMBOL_GPL(debugfs_create_u32_array);
 int debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
 			   int nregs, void __iomem *base, char *prefix)
 {
-	int i, ret = 0;
+	int i;
 
 	for (i = 0; i < nregs; i++, regs++) {
-		if (prefix)
-			ret += seq_printf(s, "%s", prefix);
-		ret += seq_printf(s, "%s = 0x%08x\n", regs->name,
-				  readl(base + regs->offset));
+		seq_printf(s, "%s%s = 0x%08x\n",
+			   prefix ? prefix : "",
+			   regs->name, readl(base + regs->offset));
+		if (seq_is_full(s))
+			break;
 	}
-	return ret;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(debugfs_print_regs32);
 
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 6/7] debugfs: Fix misuse of seq_printf return value

[Steven Rostedt]

On Mon, 29 Sep 2014 16:08:26 -0700
Joe Perches <joe@perches.com> wrote:

> Adding repeated -1 to the return is not correct.
> 
> Use seq_is_full to test for unnecessary seq_printf uses
> and always return 0.

Actually, debugfs_print_regs32() should return void as well.
I'll update that.

Thanks,

-- Steve

> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/debugfs/file.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 76c08c2..c24e578 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -695,15 +695,17 @@ EXPORT_SYMBOL_GPL(debugfs_create_u32_array);
>  int debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
>  			   int nregs, void __iomem *base, char *prefix)
>  {
> -	int i, ret = 0;
> +	int i;
>  
>  	for (i = 0; i < nregs; i++, regs++) {
> -		if (prefix)
> -			ret += seq_printf(s, "%s", prefix);
> -		ret += seq_printf(s, "%s = 0x%08x\n", regs->name,
> -				  readl(base + regs->offset));
> +		seq_printf(s, "%s%s = 0x%08x\n",
> +			   prefix ? prefix : "",
> +			   regs->name, readl(base + regs->offset));
> +		if (seq_is_full(s))
> +			break;
>  	}
> -	return ret;
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(debugfs_print_regs32);
>  

Re: [PATCH 6/7] debugfs: Fix misuse of seq_printf return value

[Joe Perches]

On Tue, 2014-10-28 at 10:58 -0400, Steven Rostedt wrote:
> On Mon, 29 Sep 2014 16:08:26 -0700
> Joe Perches <joe@perches.com> wrote:
> > Adding repeated -1 to the return is not correct.
> > Use seq_is_full to test for unnecessary seq_printf uses
> > and always return 0.
[]
> Actually, debugfs_print_regs32() should return void as well.
> I'll update that.

Great though this should possibly be another patch.

I didn't want to update the documentation.

Maybe remove the prototype and make it static too.

Re: [PATCH 6/7] debugfs: Fix misuse of seq_printf return value

[Steven Rostedt]

On Tue, 28 Oct 2014 08:25:51 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2014-10-28 at 10:58 -0400, Steven Rostedt wrote:
> > On Mon, 29 Sep 2014 16:08:26 -0700
> > Joe Perches <joe@perches.com> wrote:
> > > Adding repeated -1 to the return is not correct.
> > > Use seq_is_full to test for unnecessary seq_printf uses
> > > and always return 0.
> []
> > Actually, debugfs_print_regs32() should return void as well.
> > I'll update that.
> 
> Great though this should possibly be another patch.

Could have, but other patches that had functions returning the value of
seq_printf() also changed their output with the change. Don't worry
about being blamed, I update the change log to show that I modified it
too. ;-)

> 
> I didn't want to update the documentation.

It was a one liner, that didn't really change the content.

> 
> Maybe remove the prototype and make it static too.
> 

Now that can be a separate patch set if you want. I'm not too worried
about that as it doesn't affect the updates I want to do with
seq_file.

-- Steve

Re: [PATCH 6/7] debugfs: Fix misuse of seq_printf return value

[Joe Perches]

On Tue, 2014-10-28 at 11:42 -0400, Steven Rostedt wrote:
> On Tue, 28 Oct 2014 08:25:51 -0700 Joe Perches <joe@perches.com> wrote:
[]
> > Maybe remove the prototype and make it static too.
> Now that can be a separate patch set if you want.

No worries, it could happen later.
Thanks for taking this on btw.

Re: [PATCH 6/7] debugfs: Fix misuse of seq_printf return value

[Greg Kroah-Hartman]

On Mon, Sep 29, 2014 at 04:08:26PM -0700, Joe Perches wrote:
> Adding repeated -1 to the return is not correct.
> 
> Use seq_is_full to test for unnecessary seq_printf uses
> and always return 0.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Joe Perches]

seq_printf doesn't return a useful value, so remove
these misuses.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/mtd/devices/docg3.c | 112 ++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 60 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 21cc4b6..68ff83c 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1655,22 +1655,21 @@ static int dbg_flashctrl_show(struct seq_file *s, void *p)
 {
 	struct docg3 *docg3 = (struct docg3 *)s->private;
 
-	int pos = 0;
 	u8 fctrl;
 
 	mutex_lock(&docg3->cascade->lock);
 	fctrl = doc_register_readb(docg3, DOC_FLASHCONTROL);
 	mutex_unlock(&docg3->cascade->lock);
 
-	pos += seq_printf(s,
-		 "FlashControl : 0x%02x (%s,CE# %s,%s,%s,flash %s)\n",
-		 fctrl,
-		 fctrl & DOC_CTRL_VIOLATION ? "protocol violation" : "-",
-		 fctrl & DOC_CTRL_CE ? "active" : "inactive",
-		 fctrl & DOC_CTRL_PROTECTION_ERROR ? "protection error" : "-",
-		 fctrl & DOC_CTRL_SEQUENCE_ERROR ? "sequence error" : "-",
-		 fctrl & DOC_CTRL_FLASHREADY ? "ready" : "not ready");
-	return pos;
+	seq_printf(s, "FlashControl : 0x%02x (%s,CE# %s,%s,%s,flash %s)\n",
+		   fctrl,
+		   fctrl & DOC_CTRL_VIOLATION ? "protocol violation" : "-",
+		   fctrl & DOC_CTRL_CE ? "active" : "inactive",
+		   fctrl & DOC_CTRL_PROTECTION_ERROR ? "protection error" : "-",
+		   fctrl & DOC_CTRL_SEQUENCE_ERROR ? "sequence error" : "-",
+		   fctrl & DOC_CTRL_FLASHREADY ? "ready" : "not ready");
+
+	return 0;
 }
 DEBUGFS_RO_ATTR(flashcontrol, dbg_flashctrl_show);
 
@@ -1678,58 +1677,56 @@ static int dbg_asicmode_show(struct seq_file *s, void *p)
 {
 	struct docg3 *docg3 = (struct docg3 *)s->private;
 
-	int pos = 0, pctrl, mode;
+	int pctrl, mode;
 
 	mutex_lock(&docg3->cascade->lock);
 	pctrl = doc_register_readb(docg3, DOC_ASICMODE);
 	mode = pctrl & 0x03;
 	mutex_unlock(&docg3->cascade->lock);
 
-	pos += seq_printf(s,
-			 "%04x : RAM_WE=%d,RSTIN_RESET=%d,BDETCT_RESET=%d,WRITE_ENABLE=%d,POWERDOWN=%d,MODE=%d%d (",
-			 pctrl,
-			 pctrl & DOC_ASICMODE_RAM_WE ? 1 : 0,
-			 pctrl & DOC_ASICMODE_RSTIN_RESET ? 1 : 0,
-			 pctrl & DOC_ASICMODE_BDETCT_RESET ? 1 : 0,
-			 pctrl & DOC_ASICMODE_MDWREN ? 1 : 0,
-			 pctrl & DOC_ASICMODE_POWERDOWN ? 1 : 0,
-			 mode >> 1, mode & 0x1);
+	seq_printf(s,
+		   "%04x : RAM_WE=%d,RSTIN_RESET=%d,BDETCT_RESET=%d,WRITE_ENABLE=%d,POWERDOWN=%d,MODE=%d%d (",
+		   pctrl,
+		   pctrl & DOC_ASICMODE_RAM_WE ? 1 : 0,
+		   pctrl & DOC_ASICMODE_RSTIN_RESET ? 1 : 0,
+		   pctrl & DOC_ASICMODE_BDETCT_RESET ? 1 : 0,
+		   pctrl & DOC_ASICMODE_MDWREN ? 1 : 0,
+		   pctrl & DOC_ASICMODE_POWERDOWN ? 1 : 0,
+		   mode >> 1, mode & 0x1);
 
 	switch (mode) {
 	case DOC_ASICMODE_RESET:
-		pos += seq_puts(s, "reset");
+		seq_puts(s, "reset");
 		break;
 	case DOC_ASICMODE_NORMAL:
-		pos += seq_puts(s, "normal");
+		seq_puts(s, "normal");
 		break;
 	case DOC_ASICMODE_POWERDOWN:
-		pos += seq_puts(s, "powerdown");
+		seq_puts(s, "powerdown");
 		break;
 	}
-	pos += seq_puts(s, ")\n");
-	return pos;
+	seq_puts(s, ")\n");
+	return 0;
 }
 DEBUGFS_RO_ATTR(asic_mode, dbg_asicmode_show);
 
 static int dbg_device_id_show(struct seq_file *s, void *p)
 {
 	struct docg3 *docg3 = (struct docg3 *)s->private;
-	int pos = 0;
 	int id;
 
 	mutex_lock(&docg3->cascade->lock);
 	id = doc_register_readb(docg3, DOC_DEVICESELECT);
 	mutex_unlock(&docg3->cascade->lock);
 
-	pos += seq_printf(s, "DeviceId = %d\n", id);
-	return pos;
+	seq_printf(s, "DeviceId = %d\n", id);
+	return 0;
 }
 DEBUGFS_RO_ATTR(device_id, dbg_device_id_show);
 
 static int dbg_protection_show(struct seq_file *s, void *p)
 {
 	struct docg3 *docg3 = (struct docg3 *)s->private;
-	int pos = 0;
 	int protect, dps0, dps0_low, dps0_high, dps1, dps1_low, dps1_high;
 
 	mutex_lock(&docg3->cascade->lock);
@@ -1742,45 +1739,40 @@ static int dbg_protection_show(struct seq_file *s, void *p)
 	dps1_high = doc_register_readw(docg3, DOC_DPS1_ADDRHIGH);
 	mutex_unlock(&docg3->cascade->lock);
 
-	pos += seq_printf(s, "Protection = 0x%02x (",
-			 protect);
+	seq_printf(s, "Protection = 0x%02x (", protect);
 	if (protect & DOC_PROTECT_FOUNDRY_OTP_LOCK)
-		pos += seq_puts(s, "FOUNDRY_OTP_LOCK,");
+		seq_puts(s, "FOUNDRY_OTP_LOCK,");
 	if (protect & DOC_PROTECT_CUSTOMER_OTP_LOCK)
-		pos += seq_puts(s, "CUSTOMER_OTP_LOCK,");
+		seq_puts(s, "CUSTOMER_OTP_LOCK,");
 	if (protect & DOC_PROTECT_LOCK_INPUT)
-		pos += seq_puts(s, "LOCK_INPUT,");
+		seq_puts(s, "LOCK_INPUT,");
 	if (protect & DOC_PROTECT_STICKY_LOCK)
-		pos += seq_puts(s, "STICKY_LOCK,");
+		seq_puts(s, "STICKY_LOCK,");
 	if (protect & DOC_PROTECT_PROTECTION_ENABLED)
-		pos += seq_puts(s, "PROTECTION ON,");
+		seq_puts(s, "PROTECTION ON,");
 	if (protect & DOC_PROTECT_IPL_DOWNLOAD_LOCK)
-		pos += seq_puts(s, "IPL_DOWNLOAD_LOCK,");
+		seq_puts(s, "IPL_DOWNLOAD_LOCK,");
 	if (protect & DOC_PROTECT_PROTECTION_ERROR)
-		pos += seq_puts(s, "PROTECT_ERR,");
+		seq_puts(s, "PROTECT_ERR,");
 	else
-		pos += seq_puts(s, "NO_PROTECT_ERR");
-	pos += seq_puts(s, ")\n");
-
-	pos += seq_printf(s, "DPS0 = 0x%02x : "
-			 "Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, "
-			 "WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
-			 dps0, dps0_low, dps0_high,
-			 !!(dps0 & DOC_DPS_OTP_PROTECTED),
-			 !!(dps0 & DOC_DPS_READ_PROTECTED),
-			 !!(dps0 & DOC_DPS_WRITE_PROTECTED),
-			 !!(dps0 & DOC_DPS_HW_LOCK_ENABLED),
-			 !!(dps0 & DOC_DPS_KEY_OK));
-	pos += seq_printf(s, "DPS1 = 0x%02x : "
-			 "Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, "
-			 "WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
-			 dps1, dps1_low, dps1_high,
-			 !!(dps1 & DOC_DPS_OTP_PROTECTED),
-			 !!(dps1 & DOC_DPS_READ_PROTECTED),
-			 !!(dps1 & DOC_DPS_WRITE_PROTECTED),
-			 !!(dps1 & DOC_DPS_HW_LOCK_ENABLED),
-			 !!(dps1 & DOC_DPS_KEY_OK));
-	return pos;
+		seq_puts(s, "NO_PROTECT_ERR");
+	seq_puts(s, ")\n");
+
+	seq_printf(s, "DPS0 = 0x%02x : Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
+		   dps0, dps0_low, dps0_high,
+		   !!(dps0 & DOC_DPS_OTP_PROTECTED),
+		   !!(dps0 & DOC_DPS_READ_PROTECTED),
+		   !!(dps0 & DOC_DPS_WRITE_PROTECTED),
+		   !!(dps0 & DOC_DPS_HW_LOCK_ENABLED),
+		   !!(dps0 & DOC_DPS_KEY_OK));
+	seq_printf(s, "DPS1 = 0x%02x : Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
+		   dps1, dps1_low, dps1_high,
+		   !!(dps1 & DOC_DPS_OTP_PROTECTED),
+		   !!(dps1 & DOC_DPS_READ_PROTECTED),
+		   !!(dps1 & DOC_DPS_WRITE_PROTECTED),
+		   !!(dps1 & DOC_DPS_HW_LOCK_ENABLED),
+		   !!(dps1 & DOC_DPS_KEY_OK));
+	return 0;
 }
 DEBUGFS_RO_ATTR(protection, dbg_protection_show);
 
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Brian Norris]

On Mon, Sep 29, 2014 at 04:08:27PM -0700, Joe Perches wrote:
> seq_printf doesn't return a useful value, so remove
> these misuses.

Good catch. So it looks like this driver always had some form of
wrongness (returning a character count) in its debugfs callbacks, but
nobody noticed.

Applied to l2-mtd.git. Thanks!

Brian

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/mtd/devices/docg3.c | 112 ++++++++++++++++++++------------------------
>  1 file changed, 52 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 21cc4b6..68ff83c 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -1655,22 +1655,21 @@ static int dbg_flashctrl_show(struct seq_file *s, void *p)
>  {
>  	struct docg3 *docg3 = (struct docg3 *)s->private;
>  
> -	int pos = 0;
>  	u8 fctrl;
>  
>  	mutex_lock(&docg3->cascade->lock);
>  	fctrl = doc_register_readb(docg3, DOC_FLASHCONTROL);
>  	mutex_unlock(&docg3->cascade->lock);
>  
> -	pos += seq_printf(s,
> -		 "FlashControl : 0x%02x (%s,CE# %s,%s,%s,flash %s)\n",
> -		 fctrl,
> -		 fctrl & DOC_CTRL_VIOLATION ? "protocol violation" : "-",
> -		 fctrl & DOC_CTRL_CE ? "active" : "inactive",
> -		 fctrl & DOC_CTRL_PROTECTION_ERROR ? "protection error" : "-",
> -		 fctrl & DOC_CTRL_SEQUENCE_ERROR ? "sequence error" : "-",
> -		 fctrl & DOC_CTRL_FLASHREADY ? "ready" : "not ready");
> -	return pos;
> +	seq_printf(s, "FlashControl : 0x%02x (%s,CE# %s,%s,%s,flash %s)\n",
> +		   fctrl,
> +		   fctrl & DOC_CTRL_VIOLATION ? "protocol violation" : "-",
> +		   fctrl & DOC_CTRL_CE ? "active" : "inactive",
> +		   fctrl & DOC_CTRL_PROTECTION_ERROR ? "protection error" : "-",
> +		   fctrl & DOC_CTRL_SEQUENCE_ERROR ? "sequence error" : "-",
> +		   fctrl & DOC_CTRL_FLASHREADY ? "ready" : "not ready");
> +
> +	return 0;
>  }
>  DEBUGFS_RO_ATTR(flashcontrol, dbg_flashctrl_show);
>  
> @@ -1678,58 +1677,56 @@ static int dbg_asicmode_show(struct seq_file *s, void *p)
>  {
>  	struct docg3 *docg3 = (struct docg3 *)s->private;
>  
> -	int pos = 0, pctrl, mode;
> +	int pctrl, mode;
>  
>  	mutex_lock(&docg3->cascade->lock);
>  	pctrl = doc_register_readb(docg3, DOC_ASICMODE);
>  	mode = pctrl & 0x03;
>  	mutex_unlock(&docg3->cascade->lock);
>  
> -	pos += seq_printf(s,
> -			 "%04x : RAM_WE=%d,RSTIN_RESET=%d,BDETCT_RESET=%d,WRITE_ENABLE=%d,POWERDOWN=%d,MODE=%d%d (",
> -			 pctrl,
> -			 pctrl & DOC_ASICMODE_RAM_WE ? 1 : 0,
> -			 pctrl & DOC_ASICMODE_RSTIN_RESET ? 1 : 0,
> -			 pctrl & DOC_ASICMODE_BDETCT_RESET ? 1 : 0,
> -			 pctrl & DOC_ASICMODE_MDWREN ? 1 : 0,
> -			 pctrl & DOC_ASICMODE_POWERDOWN ? 1 : 0,
> -			 mode >> 1, mode & 0x1);
> +	seq_printf(s,
> +		   "%04x : RAM_WE=%d,RSTIN_RESET=%d,BDETCT_RESET=%d,WRITE_ENABLE=%d,POWERDOWN=%d,MODE=%d%d (",
> +		   pctrl,
> +		   pctrl & DOC_ASICMODE_RAM_WE ? 1 : 0,
> +		   pctrl & DOC_ASICMODE_RSTIN_RESET ? 1 : 0,
> +		   pctrl & DOC_ASICMODE_BDETCT_RESET ? 1 : 0,
> +		   pctrl & DOC_ASICMODE_MDWREN ? 1 : 0,
> +		   pctrl & DOC_ASICMODE_POWERDOWN ? 1 : 0,
> +		   mode >> 1, mode & 0x1);
>  
>  	switch (mode) {
>  	case DOC_ASICMODE_RESET:
> -		pos += seq_puts(s, "reset");
> +		seq_puts(s, "reset");
>  		break;
>  	case DOC_ASICMODE_NORMAL:
> -		pos += seq_puts(s, "normal");
> +		seq_puts(s, "normal");
>  		break;
>  	case DOC_ASICMODE_POWERDOWN:
> -		pos += seq_puts(s, "powerdown");
> +		seq_puts(s, "powerdown");
>  		break;
>  	}
> -	pos += seq_puts(s, ")\n");
> -	return pos;
> +	seq_puts(s, ")\n");
> +	return 0;
>  }
>  DEBUGFS_RO_ATTR(asic_mode, dbg_asicmode_show);
>  
>  static int dbg_device_id_show(struct seq_file *s, void *p)
>  {
>  	struct docg3 *docg3 = (struct docg3 *)s->private;
> -	int pos = 0;
>  	int id;
>  
>  	mutex_lock(&docg3->cascade->lock);
>  	id = doc_register_readb(docg3, DOC_DEVICESELECT);
>  	mutex_unlock(&docg3->cascade->lock);
>  
> -	pos += seq_printf(s, "DeviceId = %d\n", id);
> -	return pos;
> +	seq_printf(s, "DeviceId = %d\n", id);
> +	return 0;
>  }
>  DEBUGFS_RO_ATTR(device_id, dbg_device_id_show);
>  
>  static int dbg_protection_show(struct seq_file *s, void *p)
>  {
>  	struct docg3 *docg3 = (struct docg3 *)s->private;
> -	int pos = 0;
>  	int protect, dps0, dps0_low, dps0_high, dps1, dps1_low, dps1_high;
>  
>  	mutex_lock(&docg3->cascade->lock);
> @@ -1742,45 +1739,40 @@ static int dbg_protection_show(struct seq_file *s, void *p)
>  	dps1_high = doc_register_readw(docg3, DOC_DPS1_ADDRHIGH);
>  	mutex_unlock(&docg3->cascade->lock);
>  
> -	pos += seq_printf(s, "Protection = 0x%02x (",
> -			 protect);
> +	seq_printf(s, "Protection = 0x%02x (", protect);
>  	if (protect & DOC_PROTECT_FOUNDRY_OTP_LOCK)
> -		pos += seq_puts(s, "FOUNDRY_OTP_LOCK,");
> +		seq_puts(s, "FOUNDRY_OTP_LOCK,");
>  	if (protect & DOC_PROTECT_CUSTOMER_OTP_LOCK)
> -		pos += seq_puts(s, "CUSTOMER_OTP_LOCK,");
> +		seq_puts(s, "CUSTOMER_OTP_LOCK,");
>  	if (protect & DOC_PROTECT_LOCK_INPUT)
> -		pos += seq_puts(s, "LOCK_INPUT,");
> +		seq_puts(s, "LOCK_INPUT,");
>  	if (protect & DOC_PROTECT_STICKY_LOCK)
> -		pos += seq_puts(s, "STICKY_LOCK,");
> +		seq_puts(s, "STICKY_LOCK,");
>  	if (protect & DOC_PROTECT_PROTECTION_ENABLED)
> -		pos += seq_puts(s, "PROTECTION ON,");
> +		seq_puts(s, "PROTECTION ON,");
>  	if (protect & DOC_PROTECT_IPL_DOWNLOAD_LOCK)
> -		pos += seq_puts(s, "IPL_DOWNLOAD_LOCK,");
> +		seq_puts(s, "IPL_DOWNLOAD_LOCK,");
>  	if (protect & DOC_PROTECT_PROTECTION_ERROR)
> -		pos += seq_puts(s, "PROTECT_ERR,");
> +		seq_puts(s, "PROTECT_ERR,");
>  	else
> -		pos += seq_puts(s, "NO_PROTECT_ERR");
> -	pos += seq_puts(s, ")\n");
> -
> -	pos += seq_printf(s, "DPS0 = 0x%02x : "
> -			 "Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, "
> -			 "WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
> -			 dps0, dps0_low, dps0_high,
> -			 !!(dps0 & DOC_DPS_OTP_PROTECTED),
> -			 !!(dps0 & DOC_DPS_READ_PROTECTED),
> -			 !!(dps0 & DOC_DPS_WRITE_PROTECTED),
> -			 !!(dps0 & DOC_DPS_HW_LOCK_ENABLED),
> -			 !!(dps0 & DOC_DPS_KEY_OK));
> -	pos += seq_printf(s, "DPS1 = 0x%02x : "
> -			 "Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, "
> -			 "WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
> -			 dps1, dps1_low, dps1_high,
> -			 !!(dps1 & DOC_DPS_OTP_PROTECTED),
> -			 !!(dps1 & DOC_DPS_READ_PROTECTED),
> -			 !!(dps1 & DOC_DPS_WRITE_PROTECTED),
> -			 !!(dps1 & DOC_DPS_HW_LOCK_ENABLED),
> -			 !!(dps1 & DOC_DPS_KEY_OK));
> -	return pos;
> +		seq_puts(s, "NO_PROTECT_ERR");
> +	seq_puts(s, ")\n");
> +
> +	seq_printf(s, "DPS0 = 0x%02x : Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
> +		   dps0, dps0_low, dps0_high,
> +		   !!(dps0 & DOC_DPS_OTP_PROTECTED),
> +		   !!(dps0 & DOC_DPS_READ_PROTECTED),
> +		   !!(dps0 & DOC_DPS_WRITE_PROTECTED),
> +		   !!(dps0 & DOC_DPS_HW_LOCK_ENABLED),
> +		   !!(dps0 & DOC_DPS_KEY_OK));
> +	seq_printf(s, "DPS1 = 0x%02x : Protected area [0x%x - 0x%x] : OTP=%d, READ=%d, WRITE=%d, HW_LOCK=%d, KEY_OK=%d\n",
> +		   dps1, dps1_low, dps1_high,
> +		   !!(dps1 & DOC_DPS_OTP_PROTECTED),
> +		   !!(dps1 & DOC_DPS_READ_PROTECTED),
> +		   !!(dps1 & DOC_DPS_WRITE_PROTECTED),
> +		   !!(dps1 & DOC_DPS_HW_LOCK_ENABLED),
> +		   !!(dps1 & DOC_DPS_KEY_OK));
> +	return 0;
>  }
>  DEBUGFS_RO_ATTR(protection, dbg_protection_show);
>  

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Steven Rostedt]

On Wed, 22 Oct 2014 01:29:16 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Mon, Sep 29, 2014 at 04:08:27PM -0700, Joe Perches wrote:
> > seq_printf doesn't return a useful value, so remove
> > these misuses.
> 
> Good catch. So it looks like this driver always had some form of
> wrongness (returning a character count) in its debugfs callbacks, but
> nobody noticed.
> 
> Applied to l2-mtd.git. Thanks!
> 

Note, I'm going to be working on changes to remove the return value of
seq_printf() and friends. I need this change as well. If you
already applied it to your git tree, if you can still rebase, can you
make a separate branch off of Linus's tree that I can pull to do work
on?

Or I can take this patch as well, with your Acked-by.

Thanks!

-- Steve

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Joe Perches]

On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
> I'm going to be working on changes to remove the return value of
> seq_printf() and friends.

I'm sure you know all of this, but for anyone else:

This doesn't need to be done in a single pass.

The seq_<foo> functions themselves can wait until
all the uses are converted.

Here are files that likely need to change:

$ git grep --name-only -E "[\(=]\s*seq_(vprintf|printf|puts|putc|escape|write|put_decimal_\w+)\s*\("
arch/arm/plat-pxa/dma.c
arch/cris/arch-v10/kernel/fasttimer.c
arch/cris/arch-v32/kernel/fasttimer.c
arch/microblaze/kernel/cpu/mb.c
arch/x86/kernel/cpu/mtrr/if.c
drivers/base/power/wakeup.c
drivers/mfd/ab8500-debugfs.c
drivers/net/wireless/brcm80211/brcmfmac/dhd_sdio.c
drivers/parisc/ccio-dma.c
drivers/parisc/sba_iommu.c
drivers/regulator/dbx500-prcmu.c
drivers/staging/lustre/lustre/fid/lproc_fid.c
drivers/staging/lustre/lustre/llite/lproc_llite.c
drivers/staging/lustre/lustre/mdc/lproc_mdc.c
drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
drivers/staging/lustre/lustre/osc/lproc_osc.c
drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
drivers/usb/gadget/udc/goku_udc.c
drivers/usb/gadget/udc/pxa27x_udc.c
drivers/watchdog/bcm_kona_wdt.c
fs/debugfs/file.c
fs/dlm/debug_fs.c
fs/eventfd.c
fs/eventpoll.c
fs/notify/fdinfo.c
fs/proc/base.c
fs/seq_file.c
kernel/trace/trace_seq.c
net/batman-adv/gateway_client.c
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
net/netfilter/nf_conntrack_standalone.c
net/netfilter/nf_log.c
net/netfilter/xt_hashlimit.c

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Steven Rostedt]

On Tue, 28 Oct 2014 08:57:57 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
> > I'm going to be working on changes to remove the return value of
> > seq_printf() and friends.
> 
> I'm sure you know all of this, but for anyone else:
> 
> This doesn't need to be done in a single pass.

Yeah, I took a look at all the places, and I've decided to take it off
in chunks. I'm starting with the ones you posted, and will try to get
acks for them. And then go after other chunks as I have time.

I would like to get this done before I do my merge of trace_seq and
seq_file, but I'm thinking I may have to do that in parallel.

-- Steve

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Brian Norris]

On Tue, Oct 28, 2014 at 12:05:52PM -0400, Steven Rostedt wrote:
> On Tue, 28 Oct 2014 08:57:57 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
> > > I'm going to be working on changes to remove the return value of
> > > seq_printf() and friends.
> > 
> > I'm sure you know all of this, but for anyone else:
> > 
> > This doesn't need to be done in a single pass.
> 
> Yeah, I took a look at all the places, and I've decided to take it off
> in chunks. I'm starting with the ones you posted, and will try to get
> acks for them. And then go after other chunks as I have time.

I'll keep this in my tree as-is for now. Let me know if you still need
something changed. I can give my 'Ack' if you really want to pull this
into a separate branch.

BTW, I just noticed the $subject has a typo: s/miuse/misuse/

Brian

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Steven Rostedt]

On Tue, 28 Oct 2014 10:14:09 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Tue, Oct 28, 2014 at 12:05:52PM -0400, Steven Rostedt wrote:
> > On Tue, 28 Oct 2014 08:57:57 -0700
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
> > > > I'm going to be working on changes to remove the return value of
> > > > seq_printf() and friends.
> > > 
> > > I'm sure you know all of this, but for anyone else:
> > > 
> > > This doesn't need to be done in a single pass.
> > 
> > Yeah, I took a look at all the places, and I've decided to take it off
> > in chunks. I'm starting with the ones you posted, and will try to get
> > acks for them. And then go after other chunks as I have time.
> 
> I'll keep this in my tree as-is for now. Let me know if you still need
> something changed. I can give my 'Ack' if you really want to pull this
> into a separate branch.

If I get to a point where I can change the seq_printf() to return void,
then I'll want it, otherwise without it, it will cause my branch to fail
to compile on that code.

That's why I would like it. If we keep it in your tree and have that for
the next release, it will matter which tree goes into Linus's tree
first. If mine goes in first, then it will break his build.

Now, that's assuming I get this ready for 3.19, as I'll need quite a
few Acked-by's for the changes that I'll be making. If I don't get it
by 3.19, then it wont be an issue if that change goes in with your tree.

-- Steve

> 
> BTW, I just noticed the $subject has a typo: s/miuse/misuse/

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Joe Perches]

On Tue, 2014-10-28 at 12:05 -0400, Steven Rostedt wrote:
> On Tue, 28 Oct 2014 08:57:57 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
> > > I'm going to be working on changes to remove the return value of
> > > seq_printf() and friends.
> > 
> > I'm sure you know all of this, but for anyone else:
> > 
> > This doesn't need to be done in a single pass.
> 
> Yeah, I took a look at all the places, and I've decided to take it off
> in chunks. I'm starting with the ones you posted, and will try to get
> acks for them. And then go after other chunks as I have time.
> 
> I would like to get this done before I do my merge of trace_seq and
> seq_file, but I'm thinking I may have to do that in parallel.

I think the most important thing is to get the
seq_is_overflown (or seq_has_overflowed or whatever
other name is chosen) function added then the rest
of the patches can be applied whenever maintainers
(or Andrew or trivial or ...) pick them up.

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Steven Rostedt]

On Tue, 28 Oct 2014 10:27:40 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2014-10-28 at 12:05 -0400, Steven Rostedt wrote:
> > On Tue, 28 Oct 2014 08:57:57 -0700
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
> > > > I'm going to be working on changes to remove the return value of
> > > > seq_printf() and friends.
> > > 
> > > I'm sure you know all of this, but for anyone else:
> > > 
> > > This doesn't need to be done in a single pass.
> > 
> > Yeah, I took a look at all the places, and I've decided to take it off
> > in chunks. I'm starting with the ones you posted, and will try to get
> > acks for them. And then go after other chunks as I have time.
> > 
> > I would like to get this done before I do my merge of trace_seq and
> > seq_file, but I'm thinking I may have to do that in parallel.
> 
> I think the most important thing is to get the
> seq_is_overflown (or seq_has_overflowed or whatever
> other name is chosen) function added then the rest
> of the patches can be applied whenever maintainers
> (or Andrew or trivial or ...) pick them up.

I can get that in without much of an issue. But the merge of trace_seq
and seq_file would be easier if I didn't have to worry about return
values. Which is why I want to get this in quickly.

-- Steve
 

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Brian Norris]

On Tue, Oct 28, 2014 at 10:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 28 Oct 2014 10:27:40 -0700
> Joe Perches <joe@perches.com> wrote:
>> On Tue, 2014-10-28 at 12:05 -0400, Steven Rostedt wrote:
>> > On Tue, 28 Oct 2014 08:57:57 -0700
>> > Joe Perches <joe@perches.com> wrote:
>> >
>> > > On Tue, 2014-10-28 at 11:13 -0400, Steven Rostedt wrote:
>> > > > I'm going to be working on changes to remove the return value of
>> > > > seq_printf() and friends.
>> > >
>> > > I'm sure you know all of this, but for anyone else:
>> > >
>> > > This doesn't need to be done in a single pass.
>> >
>> > Yeah, I took a look at all the places, and I've decided to take it off
>> > in chunks. I'm starting with the ones you posted, and will try to get
>> > acks for them. And then go after other chunks as I have time.
>> >
>> > I would like to get this done before I do my merge of trace_seq and
>> > seq_file, but I'm thinking I may have to do that in parallel.
>>
>> I think the most important thing is to get the
>> seq_is_overflown (or seq_has_overflowed or whatever
>> other name is chosen) function added then the rest
>> of the patches can be applied whenever maintainers
>> (or Andrew or trivial or ...) pick them up.
>
> I can get that in without much of an issue. But the merge of trace_seq
> and seq_file would be easier if I didn't have to worry about return
> values. Which is why I want to get this in quickly.

Whatever you'd like. Please, have my ack and keep the patch in your own branch!

Acked-by: Brian Norris <computersforpeace@gmail.com>

It's no problem if we both have the patch, is it? Or if I see it in
linux-next, then I may drop it from my tree.

Regards,
Brian

Re: [PATCH 7/7] docg3: Fix miuse of seq_printf return value

[Joe Perches]

On Tue, 2014-10-28 at 13:32 -0400, Steven Rostedt wrote:
> On Tue, 28 Oct 2014 10:27:40 -0700 Joe Perches <joe@perches.com> wrote:
> > On Tue, 2014-10-28 at 12:05 -0400, Steven Rostedt wrote:
> > > I would like to get this done before I do my merge of trace_seq and
> > > seq_file, but I'm thinking I may have to do that in parallel.
> > 
> > I think the most important thing is to get the
> > seq_is_overflown (or seq_has_overflowed or whatever
> > other name is chosen) function added then the rest
> > of the patches can be applied whenever maintainers
> > (or Andrew or trivial or ...) pick them up.
> 
> I can get that in without much of an issue. But the merge of trace_seq
> and seq_file would be easier if I didn't have to worry about return
> values. Which is why I want to get this in quickly.

What is the issue there?

Re: [PATCH 0/7] seq_printf cleanups

[Steven Rostedt]

Joe,

If you haven't already done so, can you update checkpatch.pl to
complain if someone checks the return value of seq_printf(),
seq_puts(), or seq_putc().

It should state that those functions will soon be returning void.

Thanks!

-- Steve

Re: [PATCH 0/7] seq_printf cleanups

[Joe Perches]

On Tue, 2014-10-28 at 11:32 -0400, Steven Rostedt wrote:
> If you haven't already done so, can you update checkpatch.pl to
> complain if someone checks the return value of seq_printf(),
> seq_puts(), or seq_putc().

I'm not sure that matters much as a rule because I
hope soon the compiler will bleat when that happens.

There are several more too:

	seq_vprintf
	seq_escape
	seq_write
	seq_bitmap
	seq_cpumask/seq_nodemask (and _list variants),
	seq_put_decimal_xxx

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Joe Perches]

On Mon, 2014-09-29 at 09:08 -0700, Joe Perches wrote:
> On Mon, 2014-09-29 at 16:47 +0100, Al Viro wrote:
> > On Mon, Sep 29, 2014 at 04:41:22PM +0200, Petr Mladek wrote:
> > > Hmm, this inconsistency seems to be in more functions. I would divide
> > > it into three categories:
> []
> > No.  _Any_ caller that decides to report that error to its caller is fucking
> > broken.  We had some cases like that.
> []
> > And let's make seq_printf and friends return void.  Any breakage we miss
> > on grep will be caught by compiler.  Enough is enough.
> 
> https://lkml.org/lkml/2013/12/11/8

Hey Al, if you really want this to happen there are
a couple hundred uses of the return value that could
be inspected/converted.

I've cc'd you a couple times now on a few patches
that start that conversion.

If you're serious about changing the return type for
the next release,  it'd be useful if you'd ack/nack
the approach.

https://lkml.org/lkml/2014/9/29/709

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Steven Rostedt]

On Mon, 29 Sep 2014 16:47:12 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> What we need is a helper along the lines of seq_already_overflown() that
> could be used by the few places that really want that hint.  As in
> 	if (seq_already_overflown(m))
> 		return 0; // we'll be called again with bigger buffer anyway

We already have that, it's called seq_overflow(), but it's a static
function in seq_file.c. We can easily move that to seq_file.h. If you
want to rename it to seq_already_overflown() that would be fine.

> 
> And let's make seq_printf and friends return void.  Any breakage we miss
> on grep will be caught by compiler.  Enough is enough.

I'm good with this.

Note, I'm trying to make a seq_buf.c file that consolidates the code in
trace_seq.c and seq_file.c. This will extend slightly the use cases of
the seq_buf functions, but I think all the seq_*() functions from
seq_file.c can be turned into functions that return void.

I'm thinking of making m->count > m->size be what returns overflow as
true, as m->count == m->size can have all the data, that is, nothing
lost.

I'll start writing up some patches.

-- Steve

Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

[Al Viro]

On Mon, Sep 29, 2014 at 12:09:27PM -0400, Steven Rostedt wrote:

> Note, I'm trying to make a seq_buf.c file that consolidates the code in
> trace_seq.c and seq_file.c. This will extend slightly the use cases of
> the seq_buf functions, but I think all the seq_*() functions from
> seq_file.c can be turned into functions that return void.

Umm...  seq_path() and friends might want to return real errors.
And seq_path_root() needs to be able to return SEQ_SKIP.