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

From: Al Viro
Date: Mon Sep 29 2014 - 11:47:20 EST

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.


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

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.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at