Jean Delvare wrote:
> Memory freeing in iscsi_pool_free() looks wrong to me. Either q->pool
> can be NULL and this should be tested before dereferencing it, or it
> can't be NULL and it shouldn't be tested at all. As far as I can see,

I think two coding styles got mixed up. People that like to check before 
calling kfree and those that rely on kfree doing a null check for us. 
Also I think the person that wrote the code liked to have it where the 
free function just worked.

But right now it will not cause any harm will it? It is just extra 
checks and looks ugly. So this can go in the next feature window?


> the only case where q->pool is NULL is on early error in
> iscsi_pool_init(). One possible way to fix the bug is thus to not
> call iscsi_pool_free() in this case (nothing needs to be freed anyway)
> and then we can get rid of the q->pool check.

You can actually get rid of the q->pool check and still call 
iscsi_pool_free, right? kfree will check for null pointers for the 
caller. I think typically we are not supposed to check for null pointer 
and just let kfree do it.

Your patch is nice and it makes it consistent with other code. Thanks 
will queue for next window unless you tell me there is a possible bug 
like a null ptr or double free.


> 
> Signed-off-by: Jean Delvare <jdelv...@suse.de>
> Cc: Mike Christie <micha...@cs.wisc.edu>
> ---
> Another possible fix is to move the q->pool check one line up. Both
> are fine with me.
> 
> Note that this patch is untested, as I don't use iscsi myself.
> 
>  drivers/scsi/libiscsi.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> --- linux-2.6.29-rc5.orig/drivers/scsi/libiscsi.c     2009-01-29 
> 08:27:19.000000000 +0100
> +++ linux-2.6.29-rc5/drivers/scsi/libiscsi.c  2009-02-16 21:19:14.000000000 
> +0100
> @@ -1944,7 +1944,7 @@ iscsi_pool_init(struct iscsi_pool *q, in
>               num_arrays++;
>       q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
>       if (q->pool == NULL)
> -             goto enomem;
> +             return -ENOMEM;
>  
>       q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
>                             GFP_KERNEL, NULL);
> @@ -1979,8 +1979,7 @@ void iscsi_pool_free(struct iscsi_pool *
>  
>       for (i = 0; i < q->max; i++)
>               kfree(q->pool[i]);
> -     if (q->pool)
> -             kfree(q->pool);
> +     kfree(q->pool);
>       kfree(q->queue);
>  }
>  EXPORT_SYMBOL_GPL(iscsi_pool_free);
> 


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---

Reply via email to