Re: [PATCH] Illustration of warning explosion silliness
From: Jeff Garzik
Date: Wed Sep 27 2006 - 21:49:10 EST
Andrew Morton wrote:
On Wed, 27 Sep 2006 20:58:30 -0400
Jeff Garzik <jeff@xxxxxxxxxx> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/