Must check what?

Must check what?

[Matthew Wilcox]

I'd like to propose that anyone adding __must_check markers in the
future be forced to *WRITE SOME FUCKING DOCUMENTATION* about exactly
what it is the caller is supposed to be checking.

extern int __must_check bus_register(struct bus_type * bus);

Why, thank you.  Does it return 0 for success, or 1 on success?  Does it
return an errno?

Re: Must check what?

[Andrew Morton]

On Wed, 4 Oct 2006 12:37:53 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> 
> I'd like to propose that anyone adding __must_check markers in the
> future be forced to *WRITE SOME FUCKING DOCUMENTATION* about exactly
> what it is the caller is supposed to be checking.
> 
> extern int __must_check bus_register(struct bus_type * bus);
> 

I blame kernel-doc.  It should have a slot for documenting the return value,
but it doesn't, so nobody documents return values.

It should have a slot for documenting caller-provided locking requirements
too.  And for permissible calling-contexts.  They're all part of the
caller-provided environment, and these two tend to be a heck of a lot more
subtle than the function's formal arguments.

> Why, thank you.  Does it return 0 for success, or 1 on success?  Does it
> return an errno?

yes, no, yes ;)

Re: Must check what?

[Matthew Wilcox]

On Wed, Oct 04, 2006 at 12:02:42PM -0700, Andrew Morton wrote:
> I blame kernel-doc.  It should have a slot for documenting the return value,
> but it doesn't, so nobody documents return values.

There's also the question about where the documentation should go.  By
the function prototype in the header?  That's the easy place for people
using the function to find it.  By the code?  That's the place where it
stands the most chance (about 10%) of somebody bothering to update it
when they change the code.

> It should have a slot for documenting caller-provided locking requirements
> too.  And for permissible calling-contexts.  They're all part of the
> caller-provided environment, and these two tend to be a heck of a lot more
> subtle than the function's formal arguments.

Indeed.  And reference count assumptions.  It's almost like we want a
pre-condition assertion ...

Re: Must check what?

[Andrew Morton]

On Wed, 4 Oct 2006 13:25:37 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Oct 04, 2006 at 12:02:42PM -0700, Andrew Morton wrote:
> > I blame kernel-doc.  It should have a slot for documenting the return value,
> > but it doesn't, so nobody documents return values.
> 
> There's also the question about where the documentation should go.  By
> the function prototype in the header?  That's the easy place for people
> using the function to find it.  By the code?  That's the place where it
> stands the most chance (about 10%) of somebody bothering to update it
> when they change the code.

yes, by the code, if it's C.  In .h if it's C++.

> > It should have a slot for documenting caller-provided locking requirements
> > too.  And for permissible calling-contexts.  They're all part of the
> > caller-provided environment, and these two tend to be a heck of a lot more
> > subtle than the function's formal arguments.
> 
> Indeed.  And reference count assumptions.  It's almost like we want a
> pre-condition assertion ...

We have might_sleep(), assert_spin_locked(), BUG_ON(!irqs_disabled()), etc.

I like assertions personally.  If we had something like:

void foo(args)
{
	locals;

	assert_irqs_enabled();
	assert_spin_locked(some_lock);
	assert_in_atomic();
	assert_mutex_locked(some_mutex);

then we get documentation which is (optionally) checked at runtime - best
of both worlds.  Better than doing it in kernel-doc.  Automatically
self-updating (otherwise kernels go BUG).

But we'd need to sell the idea ;)

And we still need to document those return values in English.

(Or we do

	return assert_zero_or_errno(ret);

which is a bit ug, but gets us there)

Re: Must check what?

[Vadim Lobanov]

On Wednesday 04 October 2006 12:43, Andrew Morton wrote:
> > > It should have a slot for documenting caller-provided locking
> > > requirements too.  And for permissible calling-contexts.  They're all
> > > part of the caller-provided environment, and these two tend to be a
> > > heck of a lot more subtle than the function's formal arguments.
> >
> > Indeed.  And reference count assumptions.  It's almost like we want a
> > pre-condition assertion ...
>
> We have might_sleep(), assert_spin_locked(), BUG_ON(!irqs_disabled()), etc.
>
> I like assertions personally.  If we had something like:
>
> void foo(args)
> {
> 	locals;
>
> 	assert_irqs_enabled();
> 	assert_spin_locked(some_lock);
> 	assert_in_atomic();
> 	assert_mutex_locked(some_mutex);
>
> then we get documentation which is (optionally) checked at runtime - best
> of both worlds.  Better than doing it in kernel-doc.  Automatically
> self-updating (otherwise kernels go BUG).

Uhoh! How much is that going to hurt runtime? :) It actually seems to me like 
this should be doable by static code analysis tools without terribly much 
pain (in the relative sense of the term). Or am I wrong on this thought?

> And we still need to document those return values in English.

Definitely.

-- Vadim Lobanov

Re: Must check what?

[Helge Hafting]

Vadim Lobanov wrote:
> On Wednesday 04 October 2006 12:43, Andrew Morton wrote:
>   
>> I like assertions personally.  If we had something like:
>>
>> void foo(args)
>> {
>> 	locals;
>>
>> 	assert_irqs_enabled();
>> 	assert_spin_locked(some_lock);
>> 	assert_in_atomic();
>> 	assert_mutex_locked(some_mutex);
>>
>> then we get documentation which is (optionally) checked at runtime - best
>> of both worlds.  Better than doing it in kernel-doc.  Automatically
>> self-updating (otherwise kernels go BUG).
>>     
>
> Uhoh! How much is that going to hurt runtime? :) It actually seems to me like 
> this should be doable by static code analysis tools without terribly much 
> pain (in the relative sense of the term). Or am I wrong on this thought?
>   
Surely, any debugging that hurts will only really be there
if you enable CONFIG_DEBUG_something

The kind of stuff you ask people to turn on when they report
strange crashes.

Helge Hafting

Re: Must check what?

[Kyle Moffett]

On Oct 04, 2006, at 15:43:10, Andrew Morton wrote:
> (Or we do
>
> 	return assert_zero_or_errno(ret);
>
> which is a bit ug, but gets us there)

I'm personally a big fan of:

   typedef int kerr_t;
   # define kreterr_t kerr_t __must_check

Then:

   kreterr_t some_device_function(void *data)
   {
   	kerr_t result;
   	if (!data)
   		return -EINVAL;
   	result = other_device_function(data + sizeof(struct foo));
   	[...]
   	return result;
   }

You could even tag __bitwise on kerr_t except that wouldn't work  
nicely with returning a negative error code.  Is there any way to  
tell sparse that:

   -ESOMECODE => -((__force kerr_t)3)

is ok when kerr_t is a bitwise type, without allowing any other math  
operations?

Alternatively you could:

   #ifdef __KERNEL__
   # define KENOENT ((__force kerr_t) -ENOENT)
   # define KENOMEM ((__force kerr_t) -ENOMEM)
   [...]
   #endif

But that's a _lot_ of code churn for fairly minimal gain.  It might  
be easier just to patch sparse to add an errcode type attribute with  
the desired behavior.  There also might be some way to do it with  
enums but I'm not familiar enough with the exact GCC behavior to  
comment.

Cheers,
Kyle Moffett

Re: Must check what?

[Randy Dunlap]

On Wed, 4 Oct 2006 13:25:37 -0600 Matthew Wilcox wrote:

> On Wed, Oct 04, 2006 at 12:02:42PM -0700, Andrew Morton wrote:
> > I blame kernel-doc.  It should have a slot for documenting the return value,
> > but it doesn't, so nobody documents return values.

Anyone can add what kernel-doc sees as a section.  Just use:

 * Returns:
 * and describe the return values.

> There's also the question about where the documentation should go.  By
> the function prototype in the header?  That's the easy place for people
> using the function to find it.  By the code?  That's the place where it
> stands the most chance (about 10%) of somebody bothering to update it
> when they change the code.

Good questions.  Jury is still out, I suppose.

> > It should have a slot for documenting caller-provided locking requirements
> > too.  And for permissible calling-contexts.  They're all part of the
> > caller-provided environment, and these two tend to be a heck of a lot more
> > subtle than the function's formal arguments.
> 
> Indeed.  And reference count assumptions.  It's almost like we want a
> pre-condition assertion ...

I want context documentation:
 * Context:
 * Interrupt or process or bh/softirq etc. (or Any)


---
~Randy