Jean Delvare wrote:
Hi Mike,
Sorry for the late answer, I have been traveling for the last 2 days.
Le mercredi 01 avril 2009, michaelc@xxxxxxxxxxx a écrit :
From: Jean Delvare <jdelvare@xxxxxxx>
Le lundi 30 mars 2009, Chris Wright a écrit :
q->queue could be ERR_PTR(-ENOMEM) which will break unwinding
on error. Make iscsi_pool_free more defensive.
Making the freeing of q->queue dependent on q->pool being set looks
really weird (although it is correct at the moment. But this seems
to be fixable in a much simpler way.
With the benefit that only the error case is slowed down. In both
cases we have a problem if q->queue contains an error value but it's
not -ENOMEM. Apparently this can't happen today, but it doesn't feel
right to assume this will always be true. Maybe it's the right time
to fix this as well.
Signed-off-by: Mike Christie <michaelc@xxxxxxxxxxx>
---
drivers/scsi/libiscsi.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index dfaa8ad..6896283 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1999,8 +1999,10 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
GFP_KERNEL, NULL);
- if (q->queue == ERR_PTR(-ENOMEM))
+ if (IS_ERR(q->queue)) {
This indeed solves the problem I had underlined before, but
introduces a new one. Right now the only error returned by kfifo_init
is -ENOMEM. This may however change in the future, and then the
above code would silently convert the other error code to -ENOMEM.
This might make it difficult to trace errors.
What do you mean by converting other errors codes to -ENOMEM?
I know this is all just theoretical, at the moment your code is
correct, but I don't much like relying on assumptions which are not
guaranteed to last. So I would rather cleanly transmit the error code
up to the caller, or change the calling convention of kfifo_init() to
What do you mean by cleanly transmitting the error code up the caller?
return NULL on error. The former will add a few lines of code, the
latter will result in faster code but is obviously more intrusive.
The latter might also be undesirable for some reason I am
overlooking; I'm really not familiar with kfifo.
Or you can consider I am too perfectionist and ignore me and go with
the current fix, that's also fine with me ;)
I think it is fine for now. We really do not care what the error is. If
it fails, it fails. If kfifo_init() were to return some new error code
for different errors we would have to evaluate the kfifo_init() change
as well as the iscsi code to made sure we could handle it or not, until
then it is safest to fail on all errors instead of possible limping
along and possibly causing corruption.
I am also going to remove iscsi's use of kfifos in the next feature window.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
