| Message ID | 20200226084647.20636-3-kuhn.chenqun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | redundant code: Fix warnings reported by Clang static code analyzer | expand |
Am 26.02.2020 um 09:46 hat kuhn.chenqun@huawei.com geschrieben: > From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@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. Kevin > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Lieven <pl@kamp.de> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > --- > 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 > >
>-----Original Message----- >From: Kevin Wolf [mailto:kwolf@redhat.com] >Sent: Wednesday, February 26, 2020 5:55 PM >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>; >Euler Robot <euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Peter >Lieven <pl@kamp.de>; Max Reitz <mreitz@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.chenqun@huawei.com geschrieben: >> From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> >> Signed-off-by: Chen Qun <kuhn.chenqun@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. : ) 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 <ronniesahlberg@gmail.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Peter Lieven <pl@kamp.de> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: Max Reitz <mreitz@redhat.com> >> --- >> 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 >> >>
Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben: > >-----Original Message----- > >From: Kevin Wolf [mailto:kwolf@redhat.com] > >Sent: Wednesday, February 26, 2020 5:55 PM > >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> > >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; > >peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>; > >Euler Robot <euler.robot@huawei.com>; Ronnie Sahlberg > ><ronniesahlberg@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Peter > >Lieven <pl@kamp.de>; Max Reitz <mreitz@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.chenqun@huawei.com geschrieben: > >> From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> > >> Signed-off-by: Chen Qun <kuhn.chenqun@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.) Kevin
>-----Original Message----- >From: Kevin Wolf [mailto:kwolf@redhat.com] >Sent: Thursday, February 27, 2020 6:31 PM >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>; >Euler Robot <euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Peter >Lieven <pl@kamp.de>; Max Reitz <mreitz@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:kwolf@redhat.com] >> >Sent: Wednesday, February 26, 2020 5:55 PM >> >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >> >peter.maydell@linaro.org; Zhanghailiang >> ><zhang.zhanghailiang@huawei.com>; Euler Robot >> ><euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; >> >Paolo Bonzini <pbonzini@redhat.com>; Peter Lieven <pl@kamp.de>; Max >> >Reitz <mreitz@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.chenqun@huawei.com geschrieben: >> >> From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> >> >> Signed-off-by: Chen Qun <kuhn.chenqun@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); > 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.)
>-----Original Message----- >From: Kevin Wolf [mailto:kwolf@redhat.com] >Sent: Thursday, February 27, 2020 6:31 PM >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>; >Euler Robot <euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Peter >Lieven <pl@kamp.de>; Max Reitz <mreitz@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:kwolf@redhat.com] >> >Sent: Wednesday, February 26, 2020 5:55 PM >> >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >> >peter.maydell@linaro.org; Zhanghailiang >> ><zhang.zhanghailiang@huawei.com>; Euler Robot >> ><euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; >> >Paolo Bonzini <pbonzini@redhat.com>; Peter Lieven <pl@kamp.de>; Max >> >Reitz <mreitz@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.chenqun@huawei.com geschrieben: >> >> From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> >> >> Signed-off-by: Chen Qun <kuhn.chenqun@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 ? 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.
Am 28.02.2020 um 08:30 hat Chenqun (kuhn) geschrieben: > >-----Original Message----- > >From: Kevin Wolf [mailto:kwolf@redhat.com] > >Sent: Thursday, February 27, 2020 6:31 PM > >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> > >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; > >peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>; > >Euler Robot <euler.robot@huawei.com>; Ronnie Sahlberg > ><ronniesahlberg@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Peter > >Lieven <pl@kamp.de>; Max Reitz <mreitz@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:kwolf@redhat.com] > >> >Sent: Wednesday, February 26, 2020 5:55 PM > >> >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> > >> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; > >> >peter.maydell@linaro.org; Zhanghailiang > >> ><zhang.zhanghailiang@huawei.com>; Euler Robot > >> ><euler.robot@huawei.com>; Ronnie Sahlberg > ><ronniesahlberg@gmail.com>; > >> >Paolo Bonzini <pbonzini@redhat.com>; Peter Lieven <pl@kamp.de>; Max > >> >Reitz <mreitz@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.chenqun@huawei.com geschrieben: > >> >> From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> > >> >> Signed-off-by: Chen Qun <kuhn.chenqun@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. > > >
>-----Original Message----- >From: Kevin Wolf [mailto:kwolf@redhat.com] >Sent: Friday, February 28, 2020 6:55 PM >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>; >Euler Robot <euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; Paolo Bonzini <pbonzini@redhat.com>; Peter >Lieven <pl@kamp.de>; Max Reitz <mreitz@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:kwolf@redhat.com] >> >Sent: Thursday, February 27, 2020 6:31 PM >> >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >> >peter.maydell@linaro.org; Zhanghailiang >> ><zhang.zhanghailiang@huawei.com>; Euler Robot >> ><euler.robot@huawei.com>; Ronnie Sahlberg ><ronniesahlberg@gmail.com>; >> >Paolo Bonzini <pbonzini@redhat.com>; Peter Lieven <pl@kamp.de>; Max >> >Reitz <mreitz@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:kwolf@redhat.com] >> >> >Sent: Wednesday, February 26, 2020 5:55 PM >> >> >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >> >> >Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; >> >> >peter.maydell@linaro.org; Zhanghailiang >> >> ><zhang.zhanghailiang@huawei.com>; Euler Robot >> >> ><euler.robot@huawei.com>; Ronnie Sahlberg >> ><ronniesahlberg@gmail.com>; >> >> >Paolo Bonzini <pbonzini@redhat.com>; Peter Lieven <pl@kamp.de>; >> >> >Max Reitz <mreitz@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.chenqun@huawei.com geschrieben: >> >> >> From: Chen Qun <kuhn.chenqun@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.robot@huawei.com> >> >> >> Signed-off-by: Chen Qun <kuhn.chenqun@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.
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);