Re: [Kernel-janitors] [PATCH] drivers/ide/ide-tape.c - handle kmalloc

[Eugene Teo <eugene.teo@eugeneteo.net>]

<quote sender="Randy.Dunlap">
> On Thu, 22 Jan 2004 12:08:39 +0800 Eugene Teo <eugene.teo@eugeneteo.net> wrote:
> 
> | I have not compile nor test this patch. stage has to be checked for
> | null. No trailing spaces too :) 
> | 
> | 2902: static idetape_stage_t *__idetape_kmalloc_stage (..........)
> | [...]
> | 2909:         if ((stage = (idetape_stage_t *) kmalloc (sizeof \
> | (idetape_stage_t),GFP_KERNEL)) = NULL)
> | 2910:                 return NULL;
> | 
> | diff -Naur -X /home/amnesia/w/dontdiff 2.6.2-rc1-orig/drivers/ide/ide-tape.c 2.6.2-rc1-fix/drivers/ide/ide-tape.c
> | --- 2.6.2-rc1-orig/drivers/ide/ide-tape.c	2004-01-22 02:19:18.000000000 +0800
> | +++ 2.6.2-rc1-fix/drivers/ide/ide-tape.c	2004-01-22 12:01:56.000000000 +0800
> | @@ -3619,6 +3619,8 @@
> |  	printk(KERN_INFO "ide-tape: %s: reading back %d frames from the drive's internal buffer\n", tape->name, frames);
> |  	for (i = 0; i < frames; i++) {
> |  		stage = __idetape_kmalloc_stage(tape, 0, 0);
> | +		if (!stage)
> | +			return;
> |  		if (!first)
> |  			first = stage;
> |  		aux = stage->aux;
> | 
> 
> so... what makes it OK for void function "idetape_onstream_read_back_buffer"
> to just return at this point?  It can't (or doesn't) return an error
> AFAICT, but the calling function expected certain work to be done...

this function is part of the write error recovery function that is
meant for the unrealiable pipelined mode. after recovering from the
internal buffer in an event of a write error, it will re-position the
tape.

Actually, I just spotted another thing that we need to fix. When we
are in the pipelined write mode, we can't inform the user about the
error codes (that is why it is a return void, not return error code),
until the next task. And the next possible place to inform the user,
is in the function "idetape_position_tape". If our read back buffer
fails because we are unable to allocate memory for a stage, then we
might not be able to position the tape (guess).

Sheese, idetape_discard_read_pipeline() should be tested to check
whether positioning can be done but it isn't for most of the calls
in ide-tape.c.

> I guess that it could panic(), although I hate adding panics.

I don't think it will panic, but it will be better if someone
can test this.

Eugene

> --
> ~Randy
> kernel-janitors project:  http://janitor.kernelnewbies.org/
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> http://lists.osdl.org/mailman/listinfo/kernel-janitors
> 

-- 
Eugene TEO  <eugeneteo at eugeneteo.net>  <http://www.anomalistic.org/>
1024D/14A0DDE5 print D851 4574 E357 469C D308  A01E 7321 A38A 14A0 DDE5
main(i) { putchar(182623909 >> (i-1) * 5&31|!!(i<7)<<6) && main(++i); }

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors