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

[Chenqun (kuhn)]

>-----Original Message-----
>From: Kevin Wolf [mailto:address@hidden]
>Sent: Thursday, February 27, 2020 6:31 PM
>To: Chenqun (kuhn) <address@hidden>
>Cc: address@hidden; address@hidden;
>address@hidden; Zhanghailiang <address@hidden>;
>Euler Robot <address@hidden>; Ronnie Sahlberg
><address@hidden>; Paolo Bonzini <address@hidden>; Peter
>Lieven <address@hidden>; Max Reitz <address@hidden>
>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:address@hidden]
>> >Sent: Wednesday, February 26, 2020 5:55 PM
>> >To: Chenqun (kuhn) <address@hidden>
>> >Cc: address@hidden; address@hidden;
>> >address@hidden; Zhanghailiang
>> ><address@hidden>; Euler Robot
>> ><address@hidden>; Ronnie Sahlberg
><address@hidden>;
>> >Paolo Bonzini <address@hidden>; Peter Lieven <address@hidden>; Max
>> >Reitz <address@hidden>
>> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
>> >in
>> >iscsi_open()
>> >
>> >Am 26.02.2020 um 09:46 hat address@hidden geschrieben:
>> >> From: Chen Qun <address@hidden>
>> >>
>> >> 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 <address@hidden>
>> >> Signed-off-by: Chen Qun <address@hidden>
>> >
>> >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.)