>-----Original Message----- >From: Kevin Wolf [mailto:kw...@redhat.com] >Sent: Friday, February 28, 2020 6: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 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. > Well, I see, thank you very much for your detailed explanation!
Thanks.
