Am 28.02.2020 um 08:30 hat Chenqun (kuhn) geschrieben:
> >-----Original Message-----
> >From: Kevin Wolf [mailto:kw...@redhat.com]
> >Sent: Thursday, February 27, 2020 6:31 PM
> >To: Chenqun (kuhn) <kuhn.chen...@huawei.com>
> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
> >peter.mayd...@linaro.org; Zhanghailiang <zhang.zhanghaili...@huawei.com>;
> >Euler Robot <euler.ro...@huawei.com>; Ronnie Sahlberg
> ><ronniesahlb...@gmail.com>; Paolo Bonzini <pbonz...@redhat.com>; Peter
> >Lieven <p...@kamp.de>; Max Reitz <mre...@redhat.com>
> >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:kw...@redhat.com]
> >> >Sent: Wednesday, February 26, 2020 5:55 PM
> >> >To: Chenqun (kuhn) <kuhn.chen...@huawei.com>
> >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
> >> >peter.mayd...@linaro.org; Zhanghailiang
> >> ><zhang.zhanghaili...@huawei.com>; Euler Robot
> >> ><euler.ro...@huawei.com>; Ronnie Sahlberg
> ><ronniesahlb...@gmail.com>;
> >> >Paolo Bonzini <pbonz...@redhat.com>; Peter Lieven <p...@kamp.de>; Max
> >> >Reitz <mre...@redhat.com>
> >> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
> >> >in
> >> >iscsi_open()
> >> >
> >> >Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben:
> >> >> From: Chen Qun <kuhn.chen...@huawei.com>
> >> >>
> >> >> 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 <euler.ro...@huawei.com>
> >> >> Signed-off-by: Chen Qun <kuhn.chen...@huawei.com>
> >> >
> >> >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.
>
>
>
