From: Randy Dunlap <rdunlap@xenotime.net>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@elte.hu>,
"Michael S. Tsirkin" <mst@mellanox.co.il>,
Thomas Gleixner <tglx@linutronix.de>,
Adrian Bunk <bunk@stusta.de>,
Andrew Morton <akpm@linux-foundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Michal Piotrowski <michal.k.k.piotrowski@gmail.com>,
Emil Karlson <jkarlson@cc.hut.fi>,
Soeren Sonnenburg <kernel@nn7.de>, Len Brown <lenb@kernel.org>
Subject: Re: [5/6] 2.6.21-rc2: known regressions
Date: Fri, 16 Mar 2007 08:18:11 -0700 [thread overview]
Message-ID: <20070316081811.1163058f.rdunlap@xenotime.net> (raw)
In-Reply-To: <Pine.LNX.4.64.0703060817060.5963@woody.linux-foundation.org>
On Tue, 6 Mar 2007 08:44:43 -0800 (PST) Linus Torvalds wrote:
>
> This is just a coding style thing, but I thought I should really point it
> out, because these kinds of things quite often result in nasty bugs simply
> because the source code is so hard to read properly:
>
> On Tue, 6 Mar 2007, Ingo Molnar wrote:
> >
> > -static void hrtimer_switch_to_hres(void)
> > +static int hrtimer_switch_to_hres(void)
>
> Ok, so here's the quiz: does this function return "true on success, false
> on failure", or does it return "zero on success, negative on failure"?
>
> > if (base->hres_active)
> > - return;
> > + return 1;
>
> Ahh, it must be "true on success", right?
>
> > local_irq_save(flags);
> >
> > if (tick_init_highres()) {
> > local_irq_restore(flags);
> > - return;
> > + return 0;
>
> Ohh-oh! This is clearly a failure schenario! And indeed,
> "tick_init_highres()" will do the "negative on failure, zero on success"
> thing.
>
> BUT! That means that you're testing the return value WRONG!
>
> A function that returns a negative error value should be tested with
>
> if (tick_init_highres() < 0) {
> local_irq_restore(flags);
> return 0;
> }
>
> because now you *see* that it's a failure.
>
> So here's the coding style:
>
> - "true on success, false on failure" should be tested by just doing the
> implicit test against zero (because that's how C booleans work!)
>
> Example:
>
> if (everything_is_done())
> return;
>
> Or:
>
> if (!something_worked_ok()) {
> printk("Aiee! Bug!\n");
> return;
> }
>
> - "negative error values" should preferably always be tested as such
>
> if (tick_init_highres() < 0) {
> printk("Aieee! Couldn't init!\n");
> return 0;
> }
>
> or, much better, actually use a temporary variable called "err" or
> "error" or something, at which point "!error" is suddenly readable
> again:
>
[1b]
> err = tick_init_highres();
> if (!err)
> return;
So this one above and [2] below lose the obvious "negative error"
information, and also prevent such functions from returning a
positive value (> 0), e.g., to indicate a successful amount of
work done (like bytes read or written). The second version above
[1b] also does not quite agree with your statement:
- "negative error values" should preferably always be tested as such
so in the interest of CodingStyle, can you be even clearer?
> I know this sounds stupid, but we've long since come to the point where
> source code readability on a *local* scale is damn important, simply
> because that's how people look at code: they may not always remember
> whether "zero is success" or "zero is false".
>
> In general, I would suggest:
>
> - ALWAYS use "negative means error". If you had done that in this case,
> then hrtimer_switch_to_hres() would have been a lot more readable,
> *and* it could actually have returned the error code that it got to the
> caller. In general, it's just more information when you see
>
[2]
> error = some_function();
> if (error)
> return error;
>
> because even if it may generate basically *exactly* same code as the
> reversed "positive" version:
>
> if (!some_version_is_true())
> return 0;
>
> it simply has more semantic information for *humans*.
>
> And when you do this, *test it as such*. Either use an explicit "< 0"
> so that you *see* that you're testing an error value, or use that
> "retval/error = xyzzy()" pattern that is already showing "it's more
> than just true/false"
>
> - use "true/false" only for things where it's *really* obvious that the
> answer is never an error, and always a "was it true"?
>
> Yeah, even so, the true/false kind of thing may be more common (especially
> with small helper functions that are literally *designed* to be used just
> as a conditional), but I think in this case, you really should have done
> it as a "returns error" function. Partly because now it was throwing away
> an error code, partly simply because in this case, it really wasn't about
> true/false as much as about "did something error out and keep it from
> succeeding?".
>
> Maybe I'm just getting anal in my old age. I at one time tried to make
> sparse check for these things, but there was no really sane thing I could
> come up with (way way WAAY too much manual annotation).
>
> I might have to break down and suggest people use
>
> bool somefunction(..)
> {
> if (... < 0)
> return false;
> ...
> return true;
> }
>
> just to (a) eventually have sparse check for these things but more
> importantly (b) have people see more at a glance whether a function is
> supposed to return "negative or success" or "true or false".
>
> I've not generally been a huge fan of "boolean", especially in the
> traditional C kind of sense (capital screaming letters, and really just an
> "int" with lipstick). But with modern C, and "bool" defined as really
> holding just 0/1 (in practice - "unsigned char"), we could actually check
> these things (and verify with sparse that you never assign any integer
> except for 0/1 to a boolean, and otherwise always have to use a real
> boolean construct).
>
> Thus endeth my overly long coding style rant.
>
> Linus
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
next prev parent reply other threads:[~2007-03-16 16:19 UTC|newest]
Thread overview: 187+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-02-28 5:16 Linux 2.6.21-rc2 Linus Torvalds
2007-02-28 5:50 ` Gabriel C
2007-02-28 7:13 ` [PATCH] affinity is not defined in non-smp kernels - i386 Fernando Luis Vázquez Cao
2007-02-28 7:16 ` [PATCH] affinity is not defined in non-smp kernels - i386 (v2) Fernando Luis Vázquez Cao
2007-02-28 7:24 ` [PATCH] affinity is not defined in non-smp kernels - i386 Eric W. Biederman
2007-02-28 17:31 ` Bill Davidsen
2007-02-28 18:21 ` Eric W. Biederman
2007-02-28 18:30 ` Linus Torvalds
2007-02-28 7:42 ` [PATCH] affinity is not defined in non-smp kernels - i386 (v2) Fernando Luis Vázquez Cao
2007-02-28 7:17 ` [PATCH] affinity is not defined in non-smp kernels - x86_64 Fernando Luis Vázquez Cao
2007-02-28 7:23 ` Linux 2.6.21-rc2 David Brown
2007-02-28 7:39 ` Brice Goglin
2007-02-28 13:09 ` Eric W. Biederman
2007-02-28 16:44 ` David Brown
2007-02-28 17:07 ` Randy Dunlap
2007-02-28 7:41 ` [PATCH] affinity is not defined in non-smp kernels - x86_64 Fernando Luis Vázquez Cao
2007-02-28 7:59 ` Linux 2.6.21-rc2 Damien Wyart
2007-03-05 1:50 ` [1/6] 2.6.21-rc2: known regressions Adrian Bunk
2007-03-05 2:26 ` Andrew Morton
2007-03-05 3:35 ` Greg KH
2007-03-06 0:55 ` Johannes Berg
2007-03-05 4:01 ` Mark Lord
2007-03-05 4:34 ` Greg KH
2007-03-05 12:42 ` Marcel Holtmann
2007-03-05 4:34 ` [BUG} usb regression in 2.6.21-rc2-git3 Mark Lord
2007-03-05 4:37 ` [BUG] sdhci regression in 2.6.21-rc2 Mark Lord
2007-03-05 5:36 ` Pierre Ossman
2007-03-05 14:25 ` Mark Lord
2007-03-05 15:19 ` Mark Lord
2007-03-06 4:17 ` Andrew Morton
2007-03-06 5:47 ` Pierre Ossman
2007-03-06 6:09 ` Andrew Morton
2007-03-06 7:23 ` Pierre Ossman
2007-03-05 15:20 ` Pierre Ossman
2007-03-05 15:23 ` Pierre Ossman
2007-03-05 15:35 ` Mark Lord
2007-03-05 16:00 ` Pierre Ossman
2007-03-05 16:18 ` Mark Lord
2007-03-05 4:43 ` [BUG} usb regression in 2.6.21-rc2-git3 Mark Lord
2007-03-12 14:56 ` [BUG} usb-serial " Mark Lord
2007-03-12 15:06 ` Oliver Neukum
2007-03-12 15:13 ` Mark Lord
2007-03-12 15:27 ` Oliver Neukum
2007-03-12 15:29 ` Greg KH
2007-03-12 15:38 ` Oliver Neukum
2007-03-12 16:03 ` Mark Lord
2007-03-12 16:10 ` Greg KH
2007-03-12 16:22 ` Mark Lord
2007-03-12 16:11 ` Mark Lord
2007-03-12 16:14 ` Mark Lord
2007-03-12 16:27 ` Mark Lord
2007-03-12 16:50 ` Mark Lord
2007-03-12 18:48 ` Oliver Neukum
2007-03-12 20:22 ` [PATCH] usb-serial regression (Oops) in 2.6.21-rc* Mark Lord
2007-03-12 20:33 ` Greg KH
2007-03-12 22:20 ` Mark Lord
2007-03-12 22:42 ` Jim Radford
2007-03-12 22:59 ` [PATCH] usb-serial regression fix Jim Radford
2007-03-13 0:18 ` Greg KH
2007-03-13 0:41 ` Jim Radford
2007-03-13 1:55 ` Mark Lord
2007-03-13 9:14 ` Jim Radford
2007-03-13 10:14 ` Oliver Neukum
2007-03-13 13:39 ` Mark Lord
2007-03-13 13:50 ` Oliver Neukum
2007-03-13 13:55 ` Mark Lord
2007-03-13 15:30 ` Jim Radford
2007-03-13 16:35 ` Mark Lord
2007-03-12 16:28 ` [BUG} usb-serial regression in 2.6.21-rc2-git3 Oliver Neukum
2007-03-12 15:31 ` Greg KH
2007-03-07 11:06 ` [1/6] 2.6.21-rc2: known regressions Jeff Garzik
2007-03-07 22:17 ` Albert Hopkins
2007-03-05 1:50 ` [2/6] " Adrian Bunk
2007-03-07 11:09 ` Jeff Garzik
2007-03-07 16:10 ` Linus Torvalds
2007-03-08 12:03 ` Ash Milsted
2007-03-08 12:31 ` Michael S. Tsirkin
2007-03-08 15:11 ` Jeff Chua
2007-03-08 18:01 ` Linus Torvalds
2007-03-08 19:06 ` Ingo Molnar
2007-03-08 19:10 ` Ingo Molnar
2007-03-08 19:47 ` Michael S. Tsirkin
2007-03-08 20:10 ` Ingo Molnar
2007-03-08 19:25 ` Ingo Molnar
2007-03-08 23:07 ` Ingo Molnar
2007-03-08 23:12 ` Ingo Molnar
2007-03-08 23:28 ` Ingo Molnar
2007-03-08 23:49 ` Linus Torvalds
2007-03-09 10:56 ` Ingo Molnar
2007-03-09 18:00 ` Linus Torvalds
2007-03-09 11:19 ` Pavel Machek
2007-03-18 16:07 ` Ingo Molnar
2007-03-18 16:40 ` [linux-pm] " Jim Gettys
2007-03-19 19:08 ` BSOD (was: [2/6] 2.6.21-rc2: known regressions) Pete Zaitcev
2007-03-19 19:38 ` BSOD David Miller
2007-03-19 19:54 ` BSOD Jesse Barnes
2007-03-19 20:05 ` BSOD David Miller
2007-03-19 20:20 ` BSOD Jesse Barnes
2007-03-19 20:20 ` BSOD Jim Gettys
2007-03-20 9:19 ` BSOD Paul Mackerras
2007-03-20 20:33 ` BSOD Jim Gettys
2007-03-19 20:33 ` [linux-pm] [2/6] 2.6.21-rc2: known regressions Bill Davidsen
2007-03-19 22:08 ` Jim Gettys
2007-03-20 14:44 ` Bill Davidsen
2007-03-09 17:48 ` Johannes Stezenbach
2007-03-09 23:35 ` Pavel Machek
2007-03-10 9:01 ` Ingo Molnar
2007-03-10 11:43 ` Stefan Seyfried
2007-03-10 13:53 ` Johannes Stezenbach
2007-03-10 15:18 ` Ingo Molnar
2007-03-10 22:08 ` Pavel Machek
2007-03-11 8:20 ` Ingo Molnar
2007-03-12 6:34 ` Stefan Seyfried
2007-03-10 22:04 ` s2ram (was Re: [2/6] 2.6.21-rc2: known regressions) Pavel Machek
2007-03-08 19:46 ` [2/6] 2.6.21-rc2: known regressions Michael S. Tsirkin
2007-03-08 19:57 ` Michael S. Tsirkin
[not found] ` <20070311120802.GA8823@elte.hu>
2007-03-12 20:20 ` Michael S. Tsirkin
2007-03-17 21:41 ` Michael S. Tsirkin
2007-03-17 22:33 ` Thomas Gleixner
2007-03-21 17:28 ` Michael S. Tsirkin
2007-03-05 1:50 ` [3/6] " Adrian Bunk
2007-03-05 3:58 ` Michal Jaegermann
2007-03-06 17:08 ` Alan Cox
2007-03-07 11:12 ` Jeff Garzik
2007-03-10 1:09 ` Mathieu Bérard
2007-03-10 4:11 ` and try remove another quirk on this computers " Sergio Monteiro Basto
2007-03-10 5:41 ` Linus Torvalds
2007-03-11 4:32 ` Sergio Monteiro Basto
2007-03-12 11:37 ` Tejun Heo
2007-03-13 12:31 ` Mathieu Bérard
2007-03-13 12:41 ` Tejun Heo
2007-03-13 20:56 ` Mathieu Bérard
2007-03-14 6:07 ` Tejun Heo
2007-03-14 10:49 ` Mathieu Bérard
2007-03-05 1:50 ` [4/6] " Adrian Bunk
2007-03-05 10:35 ` Antonino A. Daplas
2007-03-05 15:06 ` Andrew
2007-03-08 23:28 ` Len Brown
2007-03-09 19:25 ` Andrew
2007-03-05 12:21 ` Richard Purdie
2007-03-05 1:50 ` [5/6] " Adrian Bunk
2007-03-05 7:57 ` Ingo Molnar
2007-03-05 8:13 ` Andrew Morton
2007-03-05 15:25 ` Daniel Walker
2007-03-05 15:27 ` Ingo Molnar
2007-03-05 16:42 ` Daniel Walker
2007-03-05 19:30 ` Ingo Molnar
2007-03-05 16:14 ` Bill Davidsen
2007-03-05 16:21 ` Ingo Molnar
2007-03-05 23:12 ` Adrian Bunk
2007-03-05 23:43 ` Thomas Gleixner
2007-03-05 23:45 ` Linus Torvalds
2007-03-06 0:25 ` Thomas Gleixner
2007-03-06 6:49 ` Soeren Sonnenburg
2007-03-06 7:49 ` Soeren Sonnenburg
2007-03-06 0:38 ` Linus Torvalds
2007-03-06 1:02 ` Thomas Gleixner
2007-03-06 1:31 ` Linus Torvalds
2007-03-06 2:18 ` Linus Torvalds
2007-03-06 7:25 ` Ingo Molnar
2007-03-06 8:09 ` Thomas Gleixner
2007-03-06 10:33 ` Michael S. Tsirkin
2007-03-06 10:37 ` Ingo Molnar
2007-03-06 10:46 ` Michael S. Tsirkin
2007-03-06 11:32 ` Ingo Molnar
2007-03-06 12:20 ` Michael S. Tsirkin
2007-03-06 16:44 ` Linus Torvalds
2007-03-06 17:05 ` Ingo Molnar
2007-03-06 17:29 ` [PATCH] highres: do not run the TIMER_SOFTIRQ after switching to highres mode Thomas Gleixner
2007-03-06 17:41 ` Linus Torvalds
2007-03-16 15:18 ` Randy Dunlap [this message]
2007-03-06 11:36 ` [5/6] 2.6.21-rc2: known regressions Soeren Sonnenburg
2007-03-06 12:07 ` Ingo Molnar
2007-03-06 12:15 ` Michael S. Tsirkin
2007-03-06 12:51 ` Ingo Molnar
2007-03-06 12:55 ` Michael S. Tsirkin
2007-03-06 13:03 ` Ingo Molnar
2007-03-06 13:09 ` Thomas Gleixner
2007-03-06 12:09 ` Jeff Chua
2007-03-11 17:32 ` Pavel Machek
2007-03-06 10:33 ` Michael S. Tsirkin
2007-03-05 1:50 ` [6/6] " Adrian Bunk
2007-03-05 2:07 ` David Miller
2007-03-05 2:26 ` Adrian Bunk
2007-03-05 2:29 ` David Miller
2007-03-05 4:42 ` David Miller
2007-03-05 3:32 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20070316081811.1163058f.rdunlap@xenotime.net \
--to=rdunlap@xenotime.net \
--cc=akpm@linux-foundation.org \
--cc=bunk@stusta.de \
--cc=jkarlson@cc.hut.fi \
--cc=kernel@nn7.de \
--cc=lenb@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michal.k.k.piotrowski@gmail.com \
--cc=mingo@elte.hu \
--cc=mst@mellanox.co.il \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).