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