Complaint about return code convention in queue_work() etc.

Complaint about return code convention in queue_work() etc.

[Alan Stern]

I'd like to lodge a bitter complaint about the return codes used by 
queue_work() and related functions:

	Why do the damn things return 0 for error and 1 for success???
	Why don't they use negative error codes for failure, like 
	everything else in the kernel?!!

I've tripped over this at least twice, and on each occasion spent a
considerable length of time trying to track down the problem.

If nobody objects, I'll write a patch to change the convention for the
return values.  It doesn't matter how many places those routines are
called from; it'll be worth it.

Alan Stern

Re: Complaint about return code convention in queue_work() etc.

[Jeff Garzik]

Alan Stern wrote:
> I'd like to lodge a bitter complaint about the return codes used by 
> queue_work() and related functions:
> 
> 	Why do the damn things return 0 for error and 1 for success???
> 	Why don't they use negative error codes for failure, like 
> 	everything else in the kernel?!!

It's a standard programming idiom:  return false (0) for failure, true 
(non-zero) for success.  Boolean.

Certainly the kernel often uses the -errno convention, but it's not a rule.

	Jeff

Re: Complaint about return code convention in queue_work() etc.

[Andrew Morton]

On Fri, 18 Aug 2006 17:43:18 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Stern wrote:
> > I'd like to lodge a bitter complaint about the return codes used by 
> > queue_work() and related functions:
> > 
> > 	Why do the damn things return 0 for error and 1 for success???
> > 	Why don't they use negative error codes for failure, like 
> > 	everything else in the kernel?!!
> 
> It's a standard programming idiom:  return false (0) for failure, true 
> (non-zero) for success.  Boolean.
> 
> Certainly the kernel often uses the -errno convention, but it's not a rule.
> 

The predominant convention in the kernel is 0==success and I do think that
the change which Alan suggests would be kinder to the
principle-of-least-surprise.

But if you're going to change the function's return conventions, please
also rename the function.

Re: Complaint about return code convention in queue_work() etc.

[Alexey Dobriyan]

On Fri, Aug 18, 2006 at 05:43:18PM -0400, Jeff Garzik wrote:
> Alan Stern wrote:
> >I'd like to lodge a bitter complaint about the return codes used by
> >queue_work() and related functions:
> >
> >	Why do the damn things return 0 for error and 1 for success???
> >	Why don't they use negative error codes for failure, like
> >	everything else in the kernel?!!
>
> It's a standard programming idiom:  return false (0) for failure, true
> (non-zero) for success.  Boolean.

There are at least 3 idioms:

1) return 0 on success, -E on fail¹.

	rv = foo();
	if (rv < 0)
		...

2) return 1 on YES, 0 on NO.
3) return valid pointer on OK, NULL on fail.

	p = kmalloc();
	if (!p)
		...

#2 should only be used if condition in question is spelled nice:

	if (license_is_gpl_compatible())
		...
	else
		ATI_you_can_fuck_off_too();

The question is into which category queue_work() fails.

> Certainly the kernel often uses the -errno convention, but it's not a rule.

¹ BSD returns E* where E* is negative and thus avoids "return E*;" bugs (where E
  is positive).

Re: Complaint about return code convention in queue_work() etc.

[Alan Stern]

On Sat, 19 Aug 2006, Alexey Dobriyan wrote:

> On Fri, Aug 18, 2006 at 05:43:18PM -0400, Jeff Garzik wrote:
> > Alan Stern wrote:
> > >I'd like to lodge a bitter complaint about the return codes used by
> > >queue_work() and related functions:
> > >
> > >	Why do the damn things return 0 for error and 1 for success???
> > >	Why don't they use negative error codes for failure, like
> > >	everything else in the kernel?!!
> >
> > It's a standard programming idiom:  return false (0) for failure, true
> > (non-zero) for success.  Boolean.
> 
> There are at least 3 idioms:
> 
> 1) return 0 on success, -E on fail¹.
> 
> 	rv = foo();
> 	if (rv < 0)
> 		...
> 
> 2) return 1 on YES, 0 on NO.
> 3) return valid pointer on OK, NULL on fail.
> 
> 	p = kmalloc();
> 	if (!p)
> 		...
> 
> #2 should only be used if condition in question is spelled nice:
> 
> 	if (license_is_gpl_compatible())
> 		...
> 	else
> 		ATI_you_can_fuck_off_too();
> 
> The question is into which category queue_work() fails.

Here's a general discussion...

Functions can return values of many different kinds, and one of the most
common is a value indicating whether the function succeeded or failed.  
Such a value can be represented as a "status" integer (0 = success, -Exxx
= failure) or a "succeeded" boolean (1 = success, 0 = failure).

Mixing up these two sorts of representations is a fertile source of
difficult-to-find bugs.  If the C language included a strong distinction
between integers and booleans then the compiler would find these mistakes
for us... but it doesn't.  To help prevent such errors, I suggest that the
following convention be implemented and added to
Documentation/CodingStyle:

	If the name of a function is an action or an imperative command,
	the function should return a "status" integer.  If the name is a
	predicate, the function should return a "succeeded" boolean.

	All EXPORTed functions must follow this rule, and all public
	functions should follow it.  Private (static) functions need
	not, but it is recommended that they do.

	Functions whose return value is an actual result of a computation, 
	rather than an indication of whether the computation succeeded, 
	are not subject to this rule.  Generally they indicate failure by
	returning some out-of-range result.  Typical examples would be
	functions that return pointers; they use NULL or the ERR_PTR
	mechanism to report failure.

queue_work() and friends flagrantly violate this convention.  The name 
"queue_work" clearly indicates an action or a command.  If the name were 
changed to "queue_work_succeeded" then it would be a predicate, and the 
1=success representation would be appropriate.  However this would be a 
bizarre name, since it would place more emphasis on the success of the 
operation than on the operation itself.

Other violators are down_read_trylock() and down_write_trylock().  Again,
the names suggest an action ("try to lock the rw-semaphore") and not a
predicate.  In addition, the return value meanings are opposite those
of down_trylock()!  Talk about potential for confusion...

No doubt there are other violators as well, but I can't think of any 
offhand.  People are invited to contribute a list of offenders.


On Fri, 18 Aug 2006, Andrew Morton wrote:

> The predominant convention in the kernel is 0==success and I do think that
> the change which Alan suggests would be kinder to the
> principle-of-least-surprise.
> 
> But if you're going to change the function's return conventions, please
> also rename the function.

Will do.

Alan Stern

Re: Complaint about return code convention in queue_work() etc.

[Jan Engelhardt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1284 bytes --]

>> > >I'd like to lodge a bitter complaint about the return codes used by
>> > >queue_work() and related functions:
>> > >
>> > >	Why do the damn things return 0 for error and 1 for success???
>> > >	Why don't they use negative error codes for failure, like
>> > >	everything else in the kernel?!!
>> >
>> > It's a standard programming idiom:  return false (0) for failure, true
>> > (non-zero) for success.  Boolean.
>> 
>> There are at least 3 idioms:
>> 
>> 1) return 0 on success, -E on fail¹.
>> 2) return 1 on YES, 0 on NO.
>> 3) return valid pointer on OK, NULL on fail.

I wrote something up some time ago,
http://svn.sourceforge.net/viewvc/vitalnix/trunk/src/doc/extra-aee.php?revision=1

>Functions can return values of many different kinds, and one of the most
>common is a value indicating whether the function succeeded or failed.  
>Such a value can be represented as a "status" integer (0 = success, -Exxx
>= failure) or a "succeeded" boolean (1 = success, 0 = failure).
>
>Mixing up these two sorts of representations is a fertile source of
>difficult-to-find bugs.  If the C language included a strong distinction
>between integers and booleans then the compiler would find these mistakes
>for us... but it doesn't.

Recently introduced "bool".



Jan Engelhardt
-- 

Re: Complaint about return code convention in queue_work() etc.

[Alan Stern]

On Sun, 20 Aug 2006, Jan Engelhardt wrote:

> >Mixing up these two sorts of representations is a fertile source of
> >difficult-to-find bugs.  If the C language included a strong distinction
> >between integers and booleans then the compiler would find these mistakes
> >for us... but it doesn't.
> 
> Recently introduced "bool".

I haven't seen the new definition of "bool", but it can't possibly provide 
a strong distinction between integers and booleans.  That is, if x is 
declared as an integer rather than as a bool, the compiler won't complain 
about "if (x) ...".


On Sun, 20 Aug 2006, Ingo Molnar wrote:

> yeah, lets just flip the logic over, but combined with a rename so that
> we dont surprise not-yet-in-tree code [and documentation/books].
> queue_work() -> add_work() or something like that.

How about add_work_to_q() instead of queue_work() and add_work() instead
of schedule_work()?

Alan Stern

Re: Complaint about return code convention in queue_work() etc.

[Jan Engelhardt]

>> >Mixing up these two sorts of representations is a fertile source of
>> >difficult-to-find bugs.  If the C language included a strong distinction
>> >between integers and booleans then the compiler would find these mistakes
>> >for us... but it doesn't.
>> 
>> Recently introduced "bool".
>
>I haven't seen the new definition of "bool", but it can't possibly provide 
>a strong distinction between integers and booleans.  That is, if x is 
>declared as an integer rather than as a bool, the compiler won't complain 
>about "if (x) ...".

Only Java will get you this distinction. I would be comfortable with a 
feature where conditionals (like if() and ?:) enforce a bool showing 
up in C/C++, but it's not easy to get into the mainline gcc.


Jan Engelhardt
-- 

Re: Complaint about return code convention in queue_work() etc.

[Alan Stern]

On Mon, 21 Aug 2006, Jan Engelhardt wrote:

> >> Recently introduced "bool".
> >
> >I haven't seen the new definition of "bool", but it can't possibly provide 
> >a strong distinction between integers and booleans.  That is, if x is 
> >declared as an integer rather than as a bool, the compiler won't complain 
> >about "if (x) ...".
> 
> Only Java will get you this distinction.

Not true.  It exists in Ruby.  :-)

> I would be comfortable with a 
> feature where conditionals (like if() and ?:) enforce a bool showing 
> up in C/C++, but it's not easy to get into the mainline gcc.

I think relying on an agreed-upon convention is the best we can do.

Alan Stern

Re: Complaint about return code convention in queue_work() etc.

[Ingo Molnar]

On Fri, 2006-08-18 at 17:39 -0400, Alan Stern wrote:
>         Why do the damn things return 0 for error and 1 for success???
>         Why don't they use negative error codes for failure, like 
>         everything else in the kernel?!!
> 
> I've tripped over this at least twice, and on each occasion spent a
> considerable length of time trying to track down the problem. 

yeah, lets just flip the logic over, but combined with a rename so that
we dont surprise not-yet-in-tree code [and documentation/books].
queue_work() -> add_work() or something like that.

	Ingo