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?
On Wed, 4 Oct 2006 12:37:53 -0600
Matthew Wilcox <[email protected]> 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 ;)
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 ...
On Wed, 4 Oct 2006 13:25:37 -0600
Matthew Wilcox <[email protected]> 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)
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
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
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
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