On Sat, Sep 14, 2013 at 04:05:21AM +0100, Al Viro wrote: > Again, the normal return value of ->show() is 0 and that includes the case of > overflow. THE ONLY reason to check for overflow early is when subsequent > output of ->show() takes long to generate and we want to skip that and > have seq_read() do realloc-and-call-show-again immediately. And in that > case the right fix is often to get saner iterator and stop shoving everything > into a single ->show() call...
Actually, let's take a look at the suckers. * a bunch in arch/arm/plat-pxa/dma.c:dbg_show_requester_chan(): all buggy; shouldn't care about pos at all and return 0. Additionally, dbg_show_descriptors() is buggy _and_ dumb - max_show shouldn't exist at all and it shouldn't be using single_open(); it's iterating over a linked list, for fsck sake... * cris fasttimer: apparently valid "skip pointless work if we'd overflown" uses; not sure if it's really needed here. * cris show_cpuinfo(): buggy, should return 0 regardless. * microblaze show_cpuinfo(): pointless sum is calculated and never used. * openrisc show_cpuinfo(): buggy, should return 0 regardless. * s390 pci_perf_show(): ditto. * mtrr_seq_show(): pointless sum is calculated and never used. * print_wakeup_source_stats(): buggy, should return 0 regardless. * i8k_proc_show(): ditto. * 3 ->show() instances in impi: ditto. * i2o_report_query_status(): ditto. * drivers/mfd/ab8500-debugfs.c:ab8500_registers_print(): legimitate use, a side effect of lousy iterator, unfortunately complicated by the same thing used for printk as well. * same file, in ab8500_print_modem_registers() - return value of seq_printf() is stored in local variable and immediately overwritten. * same file, a metric arseload of return seq_printf(....): buggy, should return 0. * doc3g: ditto. * drivers/parisc callers: pointless sums calculated and never used. * drivers/regulator/dbx500-prcmu.c: somebody got religious about reporting !!!scary!!! overflows. Even though debugging printks are completely pointless there. * drivers/rtc: return value passed to caller and discarded there. If it ever stops being discarded, we'll get a bug. drivers/s390/cio/blacklist.c: buggy, should return 0. lustre: a bunch "should return 0" cases, AFAICS all of them are of that kind. rtl8192*: a couple of such cases. * drivers/usb/gadget/goku_udc.c: apparently valid uses, this time with what looks like a damn good reason to skip extra work - it's reading from IO ports and that can be slow. * drivers/usb/gadget/pxa27x_udc.c: pointless sums discarded. * debugfs_print_regs32(): should return 0. The only caller ignores its return value, fortunately, otherwise we'd have bugs. * fs/dlm: valid, but very likely to be begging for better iterators. * fs/proc children_seq_show(): buggy, should return 0. * sysvipc_proc_show() and stuff it's calling (i.e. all show_... in ipc/*): buggy, should return 0. Enough for now... Overall: I suspect that Joe might be right. The very few callers that use the return value and use it correctly can bloody well call seq_overflow(), preferably with a detailed comment about the reasons for doing so. Anything that really wants the length of output (if we have such places at all) can use %n or see Figure 1. I haven't crawled through lib/*, net/* and sound/* yet, but that's how the things look so far. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
