Re: [PATCH] libiscsi: avoid unnecessary multiple NULL assignments

[Mike Christie]

On 03/19/2013 05:52 AM, Masatake YAMATO wrote:
> In iscsi_free_task, NULL is assigned to task->sc twice: before and
> after kfifo_in invocatoin. Allocating and freeing iscsi_task are guarded
> with session->lock, so multiple NULL assignments cause no trouble. But
> people reading the source code may be confused.
>
> The second NULL assignment comes from commit:
>
> 3e5c28ad0391389959ccae81c938c7533efb3490
>
> It seems that the line after kfifo_in invocation was introduced
> accidentally.
>
> Signed-off-by: Masatake YAMATO <yam...@redhat.com>
> Reviewed-by: Mike Christie <mich...@cs.wisc.edu>
> ---
> drivers/scsi/libiscsi.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 82c3fd4..7aacf3a 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -507,7 +507,6 @@ static void iscsi_free_task(struct iscsi_task *task)
> kfifo_in(&session->cmdpool.queue, (void*)&task, sizeof(void*));
>
> if (sc) {
> - task->sc = NULL;
> /* SCSI eh reuses commands to verify us */
> sc->SCp.ptr = NULL;
> /*
>

You are right. Thanks for the patch. Did you send this to the linux-scsi
list before, and did I ack on linux-scsi already? Did it just not get
picked up there by the scsi maintainer?

[Masatake YAMATO]

In iscsi_free_task, NULL is assigned to task->sc twice: before and
after kfifo_in invocatoin. Allocating and freeing iscsi_task are guarded
with session->lock, so multiple NULL assignments cause no trouble. But
people reading the source code may be confused.

The second NULL assignment comes from commit:

3e5c28ad0391389959ccae81c938c7533efb3490

It seems that the line after kfifo_in invocation was introduced
accidentally.

Signed-off-by: Masatake YAMATO <yam...@redhat.com>
Reviewed-by: Mike Christie <mich...@cs.wisc.edu>
---
drivers/scsi/libiscsi.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 82c3fd4..7aacf3a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -507,7 +507,6 @@ static void iscsi_free_task(struct iscsi_task *task)
kfifo_in(&session->cmdpool.queue, (void*)&task, sizeof(void*));

if (sc) {
- task->sc = NULL;
/* SCSI eh reuses commands to verify us */
sc->SCp.ptr = NULL;
/*

--
1.7.11.7

[Masatake YAMATO]

Yes. Again and again both lkml and linux-scsi.

> and did I ack on linux-scsi already?

No. What you wrote is "reviewed-by".

> Did it just not get
> picked up there by the scsi maintainer?

Yes.

Could you pick up this patch?

Masatake YAMATO

[Mike Christie]

On 03/21/2013 08:56 PM, Masatake YAMATO wrote:
>> Did it just not get
>> picked up there by the scsi maintainer?
>
> Yes.

Doh.

>
> Could you pick up this patch?
>

Yes. Sorry about that. I will send it with my next feature window update
since this is not a bug fix.

[Masatake YAMATO]

Thank you.

Masatake YAMATO

[Masatake YAMATO]

Could you cc me when you send it.
I'm sorry taking your time such small patch.
But I'd like to merge it to the offical tree because I took
long time to read the code.

Masatake YAMATO

[Mike Christie]

On 04/08/2013 03:05 AM, Masatake YAMATO wrote:
>> On 03/21/2013 08:56 PM, Masatake YAMATO wrote:
>>>> Did it just not get
>>>> picked up there by the scsi maintainer?
>>>
>>> Yes.
>>
>> Doh.
>>
>>>
>>> Could you pick up this patch?
>>>
>>
>> Yes. Sorry about that. I will send it with my next feature window update
>> since this is not a bug fix.
>
> Could you cc me when you send it.

I do and I will.

> I'm sorry taking your time such small patch.

It's not problem. I am about to send it with some other patch. Today or
tomorrow. Just running IO tests. Your patch is trivial but the other
patch required more testing.

> But I'd like to merge it to the offical tree because I took
> long time to read the code.
>

I understand. It is no problem. It is just a weird quirk where it just
got lost in James's tree.