[v2,02/13] block/iscsi:Remove redundant statement in iscsi_open()

Commit Message

Chen Qun Feb. 26, 2020, 8:46 a.m. UTC

From: Chen Qun <[e-mail]>

Clang static code analyzer show warning:
  block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
        flags &= ~BDRV_O_RDWR;
        ^        ~~~~~~~~~~~~

Reported-by: Euler Robot <[e-mail]>
Signed-off-by: Chen Qun <[e-mail]>
---
Cc: Ronnie Sahlberg <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Peter Lieven <[e-mail]>
Cc: Kevin Wolf <[e-mail]>
Cc: Max Reitz <[e-mail]>
---
 block/iscsi.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Kevin Wolf Feb. 26, 2020, 9:54 a.m. UTC | #1

Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
> From: Chen Qun <[e-mail]>
> 
> Clang static code analyzer show warning:
>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>         flags &= ~BDRV_O_RDWR;
>         ^        ~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <[e-mail]>
> Signed-off-by: Chen Qun <[e-mail]>

Hmm, I'm not so sure about this one because if we remove the line, flags
will be inconsistent with bs->open_flags. It feels like setting a trap
for anyone who wants to add code using flags in the future.

Kevin

> Cc: Ronnie Sahlberg <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Peter Lieven <[e-mail]>
> Cc: Kevin Wolf <[e-mail]>
> Cc: Max Reitz <[e-mail]>
> ---
>  block/iscsi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 682abd8e09..ed88479ede 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1917,7 +1917,6 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto out;
>          }
> -        flags &= ~BDRV_O_RDWR;
>      }
>  
>      iscsi_readcapacity_sync(iscsilun, &local_err);
> -- 
> 2.23.0
> 
>

Chen Qun Feb. 27, 2020, 1:49 a.m. UTC | #2

>-----Original Message-----
>From: Kevin Wolf [mailto:[e-mail]]
>Sent: Wednesday, February 26, 2020 5:55 PM
>To: Chenqun (kuhn) <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Zhanghailiang <[e-mail]>;
>Euler Robot <[e-mail]>; Ronnie Sahlberg
><[e-mail]>; Paolo Bonzini <[e-mail]>; Peter
>Lieven <[e-mail]>; Max Reitz <[e-mail]>
>Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
>> From: Chen Qun <[e-mail]>
>>
>> Clang static code analyzer show warning:
>>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>>         flags &= ~BDRV_O_RDWR;
>>         ^        ~~~~~~~~~~~~
>>
>> Reported-by: Euler Robot <[e-mail]>
>> Signed-off-by: Chen Qun <[e-mail]>
>
>Hmm, I'm not so sure about this one because if we remove the line, flags will
>be inconsistent with bs->open_flags. It feels like setting a trap for anyone
>who wants to add code using flags in the future.
Hi Kevin,  
I find it exists since 8f3bf50d34037266.   :  )  
It's not a big deal,  just upset clang static code analyzer. 
As you said, it could be a trap for the future. 

It ’s okay, whether it exists or not.

Thanks.
>
>Kevin
>
>> Cc: Ronnie Sahlberg <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Peter Lieven <[e-mail]>
>> Cc: Kevin Wolf <[e-mail]>
>> Cc: Max Reitz <[e-mail]>
>> ---
>>  block/iscsi.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c index
>> 682abd8e09..ed88479ede 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1917,7 +1917,6 @@ static int iscsi_open(BlockDriverState *bs, QDict
>*options, int flags,
>>          if (ret < 0) {
>>              goto out;
>>          }
>> -        flags &= ~BDRV_O_RDWR;
>>      }
>>
>>      iscsi_readcapacity_sync(iscsilun, &local_err);
>> --
>> 2.23.0
>>
>>

Kevin Wolf Feb. 27, 2020, 10:30 a.m. UTC | #3

Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben:
> >-----Original Message-----
> >From: Kevin Wolf [mailto:[e-mail]]
> >Sent: Wednesday, February 26, 2020 5:55 PM
> >To: Chenqun (kuhn) <[e-mail]>
> >Cc: [e-mail]; [e-mail];
> >[e-mail]; Zhanghailiang <[e-mail]>;
> >Euler Robot <[e-mail]>; Ronnie Sahlberg
> ><[e-mail]>; Paolo Bonzini <[e-mail]>; Peter
> >Lieven <[e-mail]>; Max Reitz <[e-mail]>
> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
> >iscsi_open()
> >
> >Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
> >> From: Chen Qun <[e-mail]>
> >>
> >> Clang static code analyzer show warning:
> >>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
> >>         flags &= ~BDRV_O_RDWR;
> >>         ^        ~~~~~~~~~~~~
> >>
> >> Reported-by: Euler Robot <[e-mail]>
> >> Signed-off-by: Chen Qun <[e-mail]>
> >
> >Hmm, I'm not so sure about this one because if we remove the line, flags will
> >be inconsistent with bs->open_flags. It feels like setting a trap for anyone
> >who wants to add code using flags in the future.
> Hi Kevin,  
> I find it exists since 8f3bf50d34037266.   :  )  

Yes, it has existed from the start with auto-read-only.

> It's not a big deal,  just upset clang static code analyzer. 
> As you said, it could be a trap for the future. 

What's interesting is that we do have one user of the flags later in the
function, but it uses bs->open_flags instead:

    ret = iscsi_allocmap_init(iscsilun, bs->open_flags);

Maybe this should be using flags? (The value of the bits we're
interested in is the same, but when flags is passed as a parameter, I
would expect it to be used.)

Kevin

Chen Qun Feb. 27, 2020, 11:28 a.m. UTC | #4

>-----Original Message-----
>From: Kevin Wolf [mailto:[e-mail]]
>Sent: Thursday, February 27, 2020 6:31 PM
>To: Chenqun (kuhn) <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Zhanghailiang <[e-mail]>;
>Euler Robot <[e-mail]>; Ronnie Sahlberg
><[e-mail]>; Paolo Bonzini <[e-mail]>; Peter
>Lieven <[e-mail]>; Max Reitz <[e-mail]>
>Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben:
>> >-----Original Message-----
>> >From: Kevin Wolf [mailto:[e-mail]]
>> >Sent: Wednesday, February 26, 2020 5:55 PM
>> >To: Chenqun (kuhn) <[e-mail]>
>> >Cc: [e-mail]; [e-mail];
>> >[e-mail]; Zhanghailiang
>> ><[e-mail]>; Euler Robot
>> ><[e-mail]>; Ronnie Sahlberg
><[e-mail]>;
>> >Paolo Bonzini <[e-mail]>; Peter Lieven <[e-mail]>; Max
>> >Reitz <[e-mail]>
>> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
>> >in
>> >iscsi_open()
>> >
>> >Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
>> >> From: Chen Qun <[e-mail]>
>> >>
>> >> Clang static code analyzer show warning:
>> >>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>> >>         flags &= ~BDRV_O_RDWR;
>> >>         ^        ~~~~~~~~~~~~
>> >>
>> >> Reported-by: Euler Robot <[e-mail]>
>> >> Signed-off-by: Chen Qun <[e-mail]>
>> >
>> >Hmm, I'm not so sure about this one because if we remove the line,
>> >flags will be inconsistent with bs->open_flags. It feels like setting
>> >a trap for anyone who wants to add code using flags in the future.
>> Hi Kevin,
>> I find it exists since 8f3bf50d34037266.   :  )
>
>Yes, it has existed from the start with auto-read-only.
>
>> It's not a big deal,  just upset clang static code analyzer.
>> As you said, it could be a trap for the future.
>
>What's interesting is that we do have one user of the flags later in the function,
>but it uses bs->open_flags instead:
>
>    ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>
Good point,  I think this is exactly where the 'flags' are needed now.  
It should be right to keep it for the future, too.

Later version, I will modify it.

Thanks.
>
>Maybe this should be using flags? (The value of the bits we're interested in is
>the same, but when flags is passed as a parameter, I would expect it to be
>used.)

Chen Qun Feb. 28, 2020, 7:30 a.m. UTC | #5

>-----Original Message-----
>From: Kevin Wolf [mailto:[e-mail]]
>Sent: Thursday, February 27, 2020 6:31 PM
>To: Chenqun (kuhn) <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Zhanghailiang <[e-mail]>;
>Euler Robot <[e-mail]>; Ronnie Sahlberg
><[e-mail]>; Paolo Bonzini <[e-mail]>; Peter
>Lieven <[e-mail]>; Max Reitz <[e-mail]>
>Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben:
>> >-----Original Message-----
>> >From: Kevin Wolf [mailto:[e-mail]]
>> >Sent: Wednesday, February 26, 2020 5:55 PM
>> >To: Chenqun (kuhn) <[e-mail]>
>> >Cc: [e-mail]; [e-mail];
>> >[e-mail]; Zhanghailiang
>> ><[e-mail]>; Euler Robot
>> ><[e-mail]>; Ronnie Sahlberg
><[e-mail]>;
>> >Paolo Bonzini <[e-mail]>; Peter Lieven <[e-mail]>; Max
>> >Reitz <[e-mail]>
>> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
>> >in
>> >iscsi_open()
>> >
>> >Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
>> >> From: Chen Qun <[e-mail]>
>> >>
>> >> Clang static code analyzer show warning:
>> >>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>> >>         flags &= ~BDRV_O_RDWR;
>> >>         ^        ~~~~~~~~~~~~
>> >>
>> >> Reported-by: Euler Robot <[e-mail]>
>> >> Signed-off-by: Chen Qun <[e-mail]>
>> >
>> >Hmm, I'm not so sure about this one because if we remove the line,
>> >flags will be inconsistent with bs->open_flags. It feels like setting
>> >a trap for anyone who wants to add code using flags in the future.
>> Hi Kevin,
>> I find it exists since 8f3bf50d34037266.   :  )
>
>Yes, it has existed from the start with auto-read-only.
>
>> It's not a big deal,  just upset clang static code analyzer.
>> As you said, it could be a trap for the future.
>
>What's interesting is that we do have one user of the flags later in the function,
>but it uses bs->open_flags instead:
>
>    ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>
>Maybe this should be using flags? (The value of the bits we're interested in is
>the same, but when flags is passed as a parameter, I would expect it to be
>used.)
>
Hi Kevin,
I have a question: are 'flags' exactly the same as 'bs-> open_flags'? 
In the function bdrv_open_common() at block.c file,  the existence of statement( open_flags = bdrv_open_flags(bs, bs->open_flags); ) makes them a little different.
Will this place affect them inconsistently ?

Is it safer if we assign bs-> open_flags to flags?
Modify just like：
@@ -1917,7 +1917,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto out;
         }
-        flags &= ~BDRV_O_RDWR;
+        flags = bs->open_flags;
     }

     iscsi_readcapacity_sync(iscsilun, &local_err);
@@ -2002,7 +2002,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
             iscsilun->block_size;
         if (iscsilun->lbprz) {
-            ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
+            ret = iscsi_allocmap_init(iscsilun, flags);
         }
     }

Thanks.

Kevin Wolf Feb. 28, 2020, 10:55 a.m. UTC | #6

Am 28.02.2020 um 08:30 hat Chenqun (kuhn) geschrieben:
> >-----Original Message-----
> >From: Kevin Wolf [mailto:[e-mail]]
> >Sent: Thursday, February 27, 2020 6:31 PM
> >To: Chenqun (kuhn) <[e-mail]>
> >Cc: [e-mail]; [e-mail];
> >[e-mail]; Zhanghailiang <[e-mail]>;
> >Euler Robot <[e-mail]>; Ronnie Sahlberg
> ><[e-mail]>; Paolo Bonzini <[e-mail]>; Peter
> >Lieven <[e-mail]>; Max Reitz <[e-mail]>
> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
> >iscsi_open()
> >
> >Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben:
> >> >-----Original Message-----
> >> >From: Kevin Wolf [mailto:[e-mail]]
> >> >Sent: Wednesday, February 26, 2020 5:55 PM
> >> >To: Chenqun (kuhn) <[e-mail]>
> >> >Cc: [e-mail]; [e-mail];
> >> >[e-mail]; Zhanghailiang
> >> ><[e-mail]>; Euler Robot
> >> ><[e-mail]>; Ronnie Sahlberg
> ><[e-mail]>;
> >> >Paolo Bonzini <[e-mail]>; Peter Lieven <[e-mail]>; Max
> >> >Reitz <[e-mail]>
> >> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
> >> >in
> >> >iscsi_open()
> >> >
> >> >Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
> >> >> From: Chen Qun <[e-mail]>
> >> >>
> >> >> Clang static code analyzer show warning:
> >> >>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
> >> >>         flags &= ~BDRV_O_RDWR;
> >> >>         ^        ~~~~~~~~~~~~
> >> >>
> >> >> Reported-by: Euler Robot <[e-mail]>
> >> >> Signed-off-by: Chen Qun <[e-mail]>
> >> >
> >> >Hmm, I'm not so sure about this one because if we remove the line,
> >> >flags will be inconsistent with bs->open_flags. It feels like setting
> >> >a trap for anyone who wants to add code using flags in the future.
> >> Hi Kevin,
> >> I find it exists since 8f3bf50d34037266.   :  )
> >
> >Yes, it has existed from the start with auto-read-only.
> >
> >> It's not a big deal,  just upset clang static code analyzer.
> >> As you said, it could be a trap for the future.
> >
> >What's interesting is that we do have one user of the flags later in the function,
> >but it uses bs->open_flags instead:
> >
> >    ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
> >
> >Maybe this should be using flags? (The value of the bits we're interested in is
> >the same, but when flags is passed as a parameter, I would expect it to be
> >used.)
> >
> Hi Kevin,
> I have a question: are 'flags' exactly the same as 'bs-> open_flags'? 
> In the function bdrv_open_common() at block.c file,  the existence of statement( open_flags = bdrv_open_flags(bs, bs->open_flags); ) makes them a little different.
> Will this place affect them inconsistently ?

Not exactly the same, that's why I said "value of the bits we're
interested in is the same". bdrv_open_flags() basically just filters out
things that are handled by the generic block layer and none of the block
driver's business.

To be precise, iscsi_allocmap_init() only checks BDRV_O_NOCACHE, which
is the same in both.

> Is it safer if we assign bs-> open_flags to flags?

This would add back the flags that we consciously filtered out before,
so I would say no.

Kevin

> Modify just like：
> @@ -1917,7 +1917,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto out;
>          }
> -        flags &= ~BDRV_O_RDWR;
> +        flags = bs->open_flags;
>      }
> 
>      iscsi_readcapacity_sync(iscsilun, &local_err);
> @@ -2002,7 +2002,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
>              iscsilun->block_size;
>          if (iscsilun->lbprz) {
> -            ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
> +            ret = iscsi_allocmap_init(iscsilun, flags);
>          }
>      }
> 
> Thanks.
> 
> 
>

Chen Qun Feb. 29, 2020, 2:21 a.m. UTC | #7

>-----Original Message-----
>From: Kevin Wolf [mailto:[e-mail]]
>Sent: Friday, February 28, 2020 6:55 PM
>To: Chenqun (kuhn) <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Zhanghailiang <[e-mail]>;
>Euler Robot <[e-mail]>; Ronnie Sahlberg
><[e-mail]>; Paolo Bonzini <[e-mail]>; Peter
>Lieven <[e-mail]>; Max Reitz <[e-mail]>
>Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Am 28.02.2020 um 08:30 hat Chenqun (kuhn) geschrieben:
>> >-----Original Message-----
>> >From: Kevin Wolf [mailto:[e-mail]]
>> >Sent: Thursday, February 27, 2020 6:31 PM
>> >To: Chenqun (kuhn) <[e-mail]>
>> >Cc: [e-mail]; [e-mail];
>> >[e-mail]; Zhanghailiang
>> ><[e-mail]>; Euler Robot
>> ><[e-mail]>; Ronnie Sahlberg
><[e-mail]>;
>> >Paolo Bonzini <[e-mail]>; Peter Lieven <[e-mail]>; Max
>> >Reitz <[e-mail]>
>> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
>> >in
>> >iscsi_open()
>> >
>> >Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben:
>> >> >-----Original Message-----
>> >> >From: Kevin Wolf [mailto:[e-mail]]
>> >> >Sent: Wednesday, February 26, 2020 5:55 PM
>> >> >To: Chenqun (kuhn) <[e-mail]>
>> >> >Cc: [e-mail]; [e-mail];
>> >> >[e-mail]; Zhanghailiang
>> >> ><[e-mail]>; Euler Robot
>> >> ><[e-mail]>; Ronnie Sahlberg
>> ><[e-mail]>;
>> >> >Paolo Bonzini <[e-mail]>; Peter Lieven <[e-mail]>;
>> >> >Max Reitz <[e-mail]>
>> >> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant
>> >> >statement in
>> >> >iscsi_open()
>> >> >
>> >> >Am 26.02.2020 um 09:46 hat [e-mail] geschrieben:
>> >> >> From: Chen Qun <[e-mail]>
>> >> >>
>> >> >> Clang static code analyzer show warning:
>> >> >>   block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>> >> >>         flags &= ~BDRV_O_RDWR;
>> >> >>         ^        ~~~~~~~~~~~~
>> >> >>
>> >> >> Reported-by: Euler Robot <[e-mail]>
>> >> >> Signed-off-by: Chen Qun <[e-mail]>
>> >> >
>> >> >Hmm, I'm not so sure about this one because if we remove the line,
>> >> >flags will be inconsistent with bs->open_flags. It feels like
>> >> >setting a trap for anyone who wants to add code using flags in the
>future.
>> >> Hi Kevin,
>> >> I find it exists since 8f3bf50d34037266.   :  )
>> >
>> >Yes, it has existed from the start with auto-read-only.
>> >
>> >> It's not a big deal,  just upset clang static code analyzer.
>> >> As you said, it could be a trap for the future.
>> >
>> >What's interesting is that we do have one user of the flags later in
>> >the function, but it uses bs->open_flags instead:
>> >
>> >    ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>> >
>> >Maybe this should be using flags? (The value of the bits we're
>> >interested in is the same, but when flags is passed as a parameter, I
>> >would expect it to be
>> >used.)
>> >
>> Hi Kevin,
>> I have a question: are 'flags' exactly the same as 'bs-> open_flags'?
>> In the function bdrv_open_common() at block.c file,  the existence of
>statement( open_flags = bdrv_open_flags(bs, bs->open_flags); ) makes them
>a little different.
>> Will this place affect them inconsistently ?
>
>Not exactly the same, that's why I said "value of the bits we're interested in is
>the same". bdrv_open_flags() basically just filters out things that are handled
>by the generic block layer and none of the block driver's business.
>
>To be precise, iscsi_allocmap_init() only checks BDRV_O_NOCACHE, which is
>the same in both.
>
>> Is it safer if we assign bs-> open_flags to flags?
>
>This would add back the flags that we consciously filtered out before, so I
>would say no.
>
Well, I see, thank you very much for your detailed explanation!

Thanks.

Patch

diff --git a/block/iscsi.c b/block/iscsi.c
index 682abd8e09..ed88479ede 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1917,7 +1917,6 @@  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         if (ret < 0) {
             goto out;
         }
-        flags &= ~BDRV_O_RDWR;
     }
 
     iscsi_readcapacity_sync(iscsilun, &local_err);