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