[PATCH] Illustration of warning explosion silliness

[PATCH] Illustration of warning explosion silliness

[Jeff Garzik]

The following patch (DO NOT APPLY) illustrates why
device_for_each_child() should not be marked with __must_check.

The function returns the return value of the actor function, and ceases
iteration upon error.

However, _every_ case in drivers/scsi has a hardcoded return value,
illustrating how it is quite valid to not check the return value of this
function.

Not-signed-off-by: Jeff Garzik <jeff@garzik.org>


diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d6743b9..4816c42 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2166,11 +2166,13 @@ target_block(struct device *dev, void *d
 void
 scsi_target_block(struct device *dev)
 {
+	int dummy;
+
 	if (scsi_is_target_device(dev))
 		starget_for_each_device(to_scsi_target(dev), NULL,
 					device_block);
 	else
-		device_for_each_child(dev, NULL, target_block);
+		dummy = device_for_each_child(dev, NULL, target_block);
 }
 EXPORT_SYMBOL_GPL(scsi_target_block);
 
@@ -2192,11 +2194,13 @@ target_unblock(struct device *dev, void 
 void
 scsi_target_unblock(struct device *dev)
 {
+	int dummy;
+
 	if (scsi_is_target_device(dev))
 		starget_for_each_device(to_scsi_target(dev), NULL,
 					device_unblock);
 	else
-		device_for_each_child(dev, NULL, target_unblock);
+		dummy = device_for_each_child(dev, NULL, target_unblock);
 }
 EXPORT_SYMBOL_GPL(scsi_target_unblock);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index e7fe565..520ec13 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -803,6 +803,7 @@ static int __remove_child (struct device
 void scsi_remove_target(struct device *dev)
 {
 	struct device *rdev;
+	int dummy;
 
 	if (scsi_is_target_device(dev)) {
 		__scsi_remove_target(to_scsi_target(dev));
@@ -810,7 +811,7 @@ void scsi_remove_target(struct device *d
 	}
 
 	rdev = get_device(dev);
-	device_for_each_child(dev, NULL, __remove_child);
+	dummy = device_for_each_child(dev, NULL, __remove_child);
 	put_device(rdev);
 }
 EXPORT_SYMBOL(scsi_remove_target);
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index b5b0c2c..4370244 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -211,8 +211,10 @@ static int do_sas_phy_delete(struct devi
  */
 void sas_remove_children(struct device *dev)
 {
-	device_for_each_child(dev, (void *)0, do_sas_phy_delete);
-	device_for_each_child(dev, (void *)1, do_sas_phy_delete);
+	int dummy;
+
+	dummy = device_for_each_child(dev, (void *)0, do_sas_phy_delete);
+	dummy = device_for_each_child(dev, (void *)1, do_sas_phy_delete);
 }
 EXPORT_SYMBOL(sas_remove_children);
 
diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index 9f070f0..6cc79e5 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -372,8 +372,9 @@ static ssize_t
 store_spi_revalidate(struct class_device *cdev, const char *buf, size_t count)
 {
 	struct scsi_target *starget = transport_class_to_starget(cdev);
+	int dummy;
 
-	device_for_each_child(&starget->dev, NULL, child_iter);
+	dummy = device_for_each_child(&starget->dev, NULL, child_iter);
 	return count;
 }
 static CLASS_DEVICE_ATTR(revalidate, S_IWUSR, NULL, store_spi_revalidate);

Re: [PATCH] Illustration of warning explosion silliness

[Andrew Morton]

On Wed, 27 Sep 2006 20:58:30 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> The following patch (DO NOT APPLY) illustrates why
> device_for_each_child() should not be marked with __must_check.
> 
> The function returns the return value of the actor function, and ceases
> iteration upon error.
> 
> However, _every_ case in drivers/scsi has a hardcoded return value,
> illustrating how it is quite valid to not check the return value of this
> function.
> 

What does "has a hardcoded return value" mean?

AFICT the problem here is that (for example) (going up the call stack in
the callee->caller direction):

scsi_internal_device_block() returns an error code

but device_block() drops that on the floor

so target_block() drops it on the floor too

so scsi_target_block() drops it on the floor too


It's a small matter of (correct kernel) programming to correctly propagate
the scsi_internal_device_block() error code all the way back out of
scsi_target_block().

It all looks rather sloppy?

Re: [PATCH] Illustration of warning explosion silliness

[Jeff Garzik]

Andrew Morton wrote:
> On Wed, 27 Sep 2006 20:58:30 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> The following patch (DO NOT APPLY) illustrates why
>> device_for_each_child() should not be marked with __must_check.
>>
>> The function returns the return value of the actor function, and ceases
>> iteration upon error.
>>
>> However, _every_ case in drivers/scsi has a hardcoded return value,
>> illustrating how it is quite valid to not check the return value of this
>> function.
>>
> 
> What does "has a hardcoded return value" mean?

Reference the sentence before that.  The return value of the actor 
passed to device_for_each_child() is always either zero (for some 
actors) or one (for another actor).  In all cases, it is never variable.


> AFICT the problem here is that (for example) (going up the call stack in
> the callee->caller direction):
> 
> scsi_internal_device_block() returns an error code
> 
> but device_block() drops that on the floor
> 
> so target_block() drops it on the floor too
> 
> so scsi_target_block() drops it on the floor too
> 
> 
> It's a small matter of (correct kernel) programming to correctly propagate
> the scsi_internal_device_block() error code all the way back out of
> scsi_target_block().
> 
> It all looks rather sloppy?

Quite sloppy.  But that doesn't change the fact that 
device_for_each_child()'s actor _may_ hardcode the return value.  It's a 
valid usage model for that function.

If you are doing a simple collection of data -- adding items to a 
preallocating list or bitmap -- or doing a search, as with 
__remove_child() in drivers/scsi/scsi_sysfs.c, the return value can be 
quite useless.

The usage model should not be _forced_ upon the caller, since it might 
not be needed.

	Jeff

Re: [PATCH] Illustration of warning explosion silliness

[Andrew Morton]

On Wed, 27 Sep 2006 21:48:42 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> The usage model should not be _forced_ upon the caller, since it might 
> not be needed.

IMO: tough titty.

This isn't some random crappy perl script.  Nor is it even some random
crappy sound card driver.  This is our scsi stack.  The heart of our
MissionCriticalEnterpriseReadyCoreOfAThirtyBillionDollarIndustry operating
system.  Picture yourself telling a Fortune 100 CTO why his kernel is
cheerily discarding errors (and hence his reliability and possibly data)
all over the place.  Take a peek at spi_dv_device() and its callees...	

I was astonished at the number of ignored errors all over the
sysfs/driver-model code.  And that's only there to detect programming
errors.  That's nothing compared to these bugs.

Discarding already-detected hardware or software errors in the storage
stack is toe-curlingly lame, and completely trumps the inconvenience of
developers seeing a few warnings, or having to put artificial warning
shutter-uppers in a few places.


Now I'm sure I'm about to be flooded with long-winded explanations about
why all of this can never happen.  But y'know what?  I don't care. 
Hardware errors can sometimes happen.  As can programming errors, as can
memory-corruption and dropped-bit errors and all the other things we
regularly see.  The kernel should be robust in the presence of unexpected
events.  *Particularly* those parts which are handling storage.  Any
void-returning function in a driver or a mid-layer is a huge red flag.

And it's not sufficient to say "gee, I can't think of any reason why this
handler would return an error, so I'll design its callers to assume that". 
It is _much_ better to design the callers to assume that callees _can_
fail, and to stick the `return 0;' into the terminal callee.  Because
things can change.


There, I feel better now.  If you want to see the other warnings, set
CONFIG_ENABLE_MUST_CHECK=n.

Re: [PATCH] Illustration of warning explosion silliness

[Jeff Garzik]

Andrew Morton wrote:
> And it's not sufficient to say "gee, I can't think of any reason why this
> handler would return an error, so I'll design its callers to assume that". 
> It is _much_ better to design the callers to assume that callees _can_
> fail, and to stick the `return 0;' into the terminal callee.  Because
> things can change.

huh?  You're going off on a tangent.  I agree with the above, just like 
I already agreed that SCSI needs better error checking.

You're ignoring the API issue at hand.  Let me say it again for the 
cheap seats:  "search"  You search a list, and stick a pointer somewhere 
when found.  No hardware touched.  No allocations.  Real world.  There 
is an example of usage in the kernel today.

Yes, SCSI needs better error checking.  Yes, device_for_each_child() 
actors _may_ return errors.  No, that doesn't imply 
device_for_each_child() actors must be FORCED BY DESIGN to return error 
codes.  It's just walking a list.  The current implementation and API is 
fine... save for the "__must_check" marker itself.  The actor CAN return 
an error code via the current API.

CAN, not MUST.  (using RFC language)

	Jeff

Re: [PATCH] Illustration of warning explosion silliness

[Andrew Morton]

On Thu, 28 Sep 2006 00:19:36 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Andrew Morton wrote:
> > And it's not sufficient to say "gee, I can't think of any reason why this
> > handler would return an error, so I'll design its callers to assume that". 
> > It is _much_ better to design the callers to assume that callees _can_
> > fail, and to stick the `return 0;' into the terminal callee.  Because
> > things can change.
> 
> huh?  You're going off on a tangent.  I agree with the above, just like 
> I already agreed that SCSI needs better error checking.

No I'm not.  I'm saying that the bugs which this exposed are a far, far
more serious matter than a few false-positive warnings which need
workarounds.

> You're ignoring the API issue at hand.  Let me say it again for the 
> cheap seats:  "search"  You search a list, and stick a pointer somewhere 
> when found.  No hardware touched.  No allocations.  Real world.  There 
> is an example of usage in the kernel today.

If it's called in that fashion then the caller should still check the
device_for_each_child() return value to find out if it actually got a
match.

Now it could be that the mysterious caller to which you refer uses the
non-NULLness of some pointer to work out if a match occurred.  Well shrug -
add a BUG_ON(!device_for_each_child_return_value) or something.

Or write a new version of device_for_each_child() which returns void and
don't tell anyone about it.

But let's not encourage error-ignoring.

Re: [PATCH] Illustration of warning explosion silliness

[Jeff Garzik]

Andrew Morton wrote:
>> You're ignoring the API issue at hand.  Let me say it again for the 
>> cheap seats:  "search"  You search a list, and stick a pointer somewhere 
>> when found.  No hardware touched.  No allocations.  Real world.  There 
>> is an example of usage in the kernel today.
> 
> If it's called in that fashion then the caller should still check the
> device_for_each_child() return value to find out if it actually got a
> match.


Or in the case of scsi_sysfs.c, it simply <does something>.

Oh well, whatever.  This thing introduces endless build noise we won't 
kill for years, making it much harder to spot much more serious stuff.

	Jeff

Re: [PATCH] Illustration of warning explosion silliness

[Andrew Morton]

On Wed, 27 Sep 2006 21:36:28 -0700
Andrew Morton <akpm@osdl.org> wrote:

> device_for_each_child() 

All that being said, device_for_each_child() is rather broken by design. 
It walks a list of items applying a function to them and bales out on
first-error.

There's no way in which the caller can know which items have been operated
on, nor which items have yet to be operated on, nor which item experienced
the failure.  Any caller which is serious about error recovery presumably
won't use it, unless the callback function happens to be something which
makes no state changes.

Re: [PATCH] Illustration of warning explosion silliness

[Andrew Morton]

On Thu, 28 Sep 2006 00:42:24 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> This thing introduces endless build noise we won't 
> kill for years, making it much harder to spot much more serious stuff.

That's true, and it hurt.  But there are a lot of real bugs in there.

I thought of adding a stricter __must_check like

#ifdef CONFIG_MUST_CHECK_ANAL
#define __must_check_anal __must_check
#else
#define __must_check_anal
#endif

which we can still do, of course.

But it'd be better to fix the bugs...

Re: [PATCH] Illustration of warning explosion silliness

[Jeff Garzik]

Andrew Morton wrote:
> On Wed, 27 Sep 2006 21:36:28 -0700
> Andrew Morton <akpm@osdl.org> wrote:
> 
>> device_for_each_child() 
> 
> All that being said, device_for_each_child() is rather broken by design. 
> It walks a list of items applying a function to them and bales out on
> first-error.

Or, like scsi_sysfs.c, it stops when it meets the first match.  Which is 
a common thing to do.


> There's no way in which the caller can know which items have been operated
> on, nor which items have yet to be operated on, nor which item experienced
> the failure.  Any caller which is serious about error recovery presumably
> won't use it, unless the callback function happens to be something which
> makes no state changes.

A simple integer return error doesn't tell you all that information 
either.  The actor must obviously store that additional information 
somewhere, if it cares.

But whatever.  I give up.  I'm going back to working on the libata 
warnings each build spits out (iomap).

	Jeff

Re: [PATCH] Illustration of warning explosion silliness

[Andrew Morton]

On Thu, 28 Sep 2006 00:54:31 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Andrew Morton wrote:
> > On Wed, 27 Sep 2006 21:36:28 -0700
> > Andrew Morton <akpm@osdl.org> wrote:
> > 
> >> device_for_each_child() 
> > 
> > All that being said, device_for_each_child() is rather broken by design. 
> > It walks a list of items applying a function to them and bales out on
> > first-error.
> 
> Or, like scsi_sysfs.c, it stops when it meets the first match.  Which is 
> a common thing to do.

That code is flakey.  Trace through all the called functions, see all the
errors which get ignored.

> 
> > There's no way in which the caller can know which items have been operated
> > on, nor which items have yet to be operated on, nor which item experienced
> > the failure.  Any caller which is serious about error recovery presumably
> > won't use it, unless the callback function happens to be something which
> > makes no state changes.
> 
> A simple integer return error doesn't tell you all that information 
> either.  The actor must obviously store that additional information 
> somewhere, if it cares.

Yup.

> But whatever.  I give up.

That's the spirit ;)

> I'm going back to working on the libata 
> warnings each build spits out (iomap).

Thanks.

Re: [PATCH] Illustration of warning explosion silliness

[Jeff Garzik]

Andrew Morton wrote:
> There, I feel better now.  If you want to see the other warnings, set
> CONFIG_ENABLE_MUST_CHECK=n.

While Googling around for Hobson's Choice[1], I realized that we are 
presented with the utterly apropos Morton's Fork:

	http://en.wikipedia.org/wiki/Morton's_Fork

With CONFIG_ENABLE_MUST_CHECK warning explosion, we must choose between 
seeing warnings in our own code, but missing __must_check bugs, and 
seeing all the __must_check bugs but obscuring our own day-to-day devel 
problems.

In the future, I would hope that it would be reasonable to merge a 
feature like this along with the cleanups that avoid a warning explosion.

	Jeff


[1] http://en.wikipedia.org/wiki/Hobson's_choice