RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message- >From: Kevin Wolf [mailto:kw...@redhat.com] >Sent: Friday, February 28, 2020 6:55 PM >To: Chenqun (kuhn) >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >peter.mayd...@linaro.org; Zhanghailiang ; >Euler Robot ; Ronnie Sahlberg >; Paolo Bonzini ; Peter >Lieven ; Max Reitz >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) >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >> >peter.mayd...@linaro.org; Zhanghailiang >> >; Euler Robot >> >; Ronnie Sahlberg >; >> >Paolo Bonzini ; Peter Lieven ; Max >> >Reitz >> >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) >> >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >> >> >peter.mayd...@linaro.org; Zhanghailiang >> >> >; Euler Robot >> >> >; Ronnie Sahlberg >> >; >> >> >Paolo Bonzini ; Peter Lieven ; >> >> >Max Reitz >> >> >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 >> >> >> >> >> >> 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 >> >> >> Signed-off-by: Chen Qun >> >> > >> >> >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.
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)
> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
> >peter.mayd...@linaro.org; Zhanghailiang ;
> >Euler Robot ; Ronnie Sahlberg
> >; Paolo Bonzini ; Peter
> >Lieven ; Max Reitz
> >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)
> >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
> >> >peter.mayd...@linaro.org; Zhanghailiang
> >> >; Euler Robot
> >> >; Ronnie Sahlberg
> >;
> >> >Paolo Bonzini ; Peter Lieven ; Max
> >> >Reitz
> >> >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
> >> >>
> >> >> 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
> >> >> Signed-off-by: Chen Qun
> >> >
> >> >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.
>
>
>
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message-
>From: Kevin Wolf [mailto:kw...@redhat.com]
>Sent: Thursday, February 27, 2020 6:31 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>peter.mayd...@linaro.org; Zhanghailiang ;
>Euler Robot ; Ronnie Sahlberg
>; Paolo Bonzini ; Peter
>Lieven ; Max Reitz
>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)
>> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>> >peter.mayd...@linaro.org; Zhanghailiang
>> >; Euler Robot
>> >; Ronnie Sahlberg
>;
>> >Paolo Bonzini ; Peter Lieven ; Max
>> >Reitz
>> >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
>> >>
>> >> 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
>> >> Signed-off-by: Chen Qun
>> >
>> >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.
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message- >From: Kevin Wolf [mailto:kw...@redhat.com] >Sent: Thursday, February 27, 2020 6:31 PM >To: Chenqun (kuhn) >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >peter.mayd...@linaro.org; Zhanghailiang ; >Euler Robot ; Ronnie Sahlberg >; Paolo Bonzini ; Peter >Lieven ; Max Reitz >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) >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >> >peter.mayd...@linaro.org; Zhanghailiang >> >; Euler Robot >> >; Ronnie Sahlberg >; >> >Paolo Bonzini ; Peter Lieven ; Max >> >Reitz >> >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 >> >> >> >> 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 >> >> Signed-off-by: Chen Qun >> > >> >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.)
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) > >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; > >peter.mayd...@linaro.org; Zhanghailiang ; > >Euler Robot ; Ronnie Sahlberg > >; Paolo Bonzini ; Peter > >Lieven ; Max Reitz > >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 > >> > >> 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 > >> Signed-off-by: Chen Qun > > > >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
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message-
>From: Kevin Wolf [mailto:kw...@redhat.com]
>Sent: Wednesday, February 26, 2020 5:55 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>peter.mayd...@linaro.org; Zhanghailiang ;
>Euler Robot ; Ronnie Sahlberg
>; Paolo Bonzini ; Peter
>Lieven ; Max Reitz
>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
>>
>> 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
>> Signed-off-by: Chen Qun
>
>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
>> Cc: Paolo Bonzini
>> Cc: Peter Lieven
>> Cc: Kevin Wolf
>> Cc: Max Reitz
>> ---
>> 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
>>
>>
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
>
> 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
> Signed-off-by: Chen Qun
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
> Cc: Paolo Bonzini
> Cc: Peter Lieven
> Cc: Kevin Wolf
> Cc: Max Reitz
> ---
> 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
>
>
[PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
From: Chen Qun
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
Signed-off-by: Chen Qun
---
Cc: Ronnie Sahlberg
Cc: Paolo Bonzini
Cc: Peter Lieven
Cc: Kevin Wolf
Cc: Max Reitz
---
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
