Re: [PATCH 7/7] libiscsi: fix iscsi pool error path

[Mike Christie <michaelc@cs.wisc.edu>]

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@cs.wisc.edu a écrit :
>> From: Jean Delvare <jdelvare@suse.de>
>>
>> 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@cs.wisc.edu>
>> ---
>>  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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html