diff mbox series

[v2] hw/block: nvme: Fix a build error in nvme_get_feature()

Message ID 1612952597-62595-1-git-send-email-bmeng.cn@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] hw/block: nvme: Fix a build error in nvme_get_feature() | expand

Commit Message

Bin Meng Feb. 10, 2021, 10:23 a.m. UTC 
From: Bin Meng <bin.meng@windriver.com>

Current QEMU HEAD nvme.c does not compile:

  hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
         ^
  hw/block/nvme.c:3150:14: note: ‘result’ was declared here
     uint32_t result;
              ^

Explicitly initialize the result to fix it.

Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- update function name in the commit message

 hw/block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Klaus Jensen Feb. 10, 2021, 10:42 a.m. UTC | #1 
CC qemu-trivial.

On Feb 10 18:23, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Current QEMU HEAD nvme.c does not compile:
> 
>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>          ^
>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>      uint32_t result;
>               ^
> 
> Explicitly initialize the result to fix it.
> 
> Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - update function name in the commit message
> 
>  hw/block/nvme.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 5ce21b7..c122ac0 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3228,6 +3228,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>          result = ns->features.err_rec;
>          goto out;
>      case NVME_VOLATILE_WRITE_CACHE:
> +        result = 0;
>          for (i = 1; i <= n->num_namespaces; i++) {
>              ns = nvme_ns(n, i);
>              if (!ns) {
> -- 
> 2.7.4
> 
>
Minwoo Im Feb. 10, 2021, 10:47 a.m. UTC | #2 
On 21-02-10 18:23:17, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Current QEMU HEAD nvme.c does not compile:
> 
>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>          ^
>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>      uint32_t result;
>               ^
> 
> Explicitly initialize the result to fix it.
> 
> Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Bin,

Thanks for the fix!
Philippe Mathieu-Daudé Feb. 10, 2021, 11:12 a.m. UTC | #3 
Hi Bin,

On 2/10/21 11:23 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Current QEMU HEAD nvme.c does not compile:
> 
>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>          ^
>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>      uint32_t result;
>               ^

Why isn't this catched by our CI? What is your host OS? Fedora 33?

> 
> Explicitly initialize the result to fix it.
> 
> Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - update function name in the commit message
> 
>  hw/block/nvme.c | 1 +
>  1 file changed, 1 insertion(+)
Bin Meng Feb. 10, 2021, 11:15 a.m. UTC | #4 
Hi Philippe,

On Wed, Feb 10, 2021 at 7:12 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Bin,
>
> On 2/10/21 11:23 AM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Current QEMU HEAD nvme.c does not compile:
> >
> >   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> >          ^
> >   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
> >      uint32_t result;
> >               ^
>
> Why isn't this catched by our CI? What is your host OS? Fedora 33?
>

I am using GCC 5.4 on Ubuntu 16.04. Please see some initial analysis
from Peter about why newer version GCC does not report it.

Regards,
Bin
Philippe Mathieu-Daudé Feb. 10, 2021, 11:15 a.m. UTC | #5 
On 2/10/21 12:12 PM, Philippe Mathieu-Daudé wrote:
> Hi Bin,
> 
> On 2/10/21 11:23 AM, Bin Meng wrote:
>> From: Bin Meng <bin.meng@windriver.com>
>>
>> Current QEMU HEAD nvme.c does not compile:
>>
>>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>>          ^
>>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>>      uint32_t result;
>>               ^
> 
> Why isn't this catched by our CI? What is your host OS? Fedora 33?

Just noticed v1 and Peter's explanation:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03528.html

Can you amend "default GCC 5.4 on a Ubuntu 16.04 host" information
please?

> 
>>
>> Explicitly initialize the result to fix it.
>>
>> Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>
>> ---
>>
>> Changes in v2:
>> - update function name in the commit message
>>
>>  hw/block/nvme.c | 1 +
>>  1 file changed, 1 insertion(+)
Bin Meng Feb. 10, 2021, 11:17 a.m. UTC | #6 
Hi Philippe,

On Wed, Feb 10, 2021 at 7:15 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 2/10/21 12:12 PM, Philippe Mathieu-Daudé wrote:
> > Hi Bin,
> >
> > On 2/10/21 11:23 AM, Bin Meng wrote:
> >> From: Bin Meng <bin.meng@windriver.com>
> >>
> >> Current QEMU HEAD nvme.c does not compile:
> >>
> >>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> >>          ^
> >>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
> >>      uint32_t result;
> >>               ^
> >
> > Why isn't this catched by our CI? What is your host OS? Fedora 33?
>
> Just noticed v1 and Peter's explanation:
> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03528.html
>
> Can you amend "default GCC 5.4 on a Ubuntu 16.04 host" information
> please?
>

Sure.

Regards,
Bin
Daniel P. Berrangé Feb. 10, 2021, 11:22 a.m. UTC | #7 
On Wed, Feb 10, 2021 at 12:15:45PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/10/21 12:12 PM, Philippe Mathieu-Daudé wrote:
> > Hi Bin,
> > 
> > On 2/10/21 11:23 AM, Bin Meng wrote:
> >> From: Bin Meng <bin.meng@windriver.com>
> >>
> >> Current QEMU HEAD nvme.c does not compile:
> >>
> >>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> >>          ^
> >>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
> >>      uint32_t result;
> >>               ^
> > 
> > Why isn't this catched by our CI? What is your host OS? Fedora 33?
> 
> Just noticed v1 and Peter's explanation:
> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03528.html
> 
> Can you amend "default GCC 5.4 on a Ubuntu 16.04 host" information
> please?

Well Ubuntu 16.04 hasn't been considered a supported build target for
QEMU for a year now.

https://qemu.readthedocs.io/en/latest/system/build-platforms.html#linux-os-macos-freebsd-netbsd-openbsd

  "The project aims to support the most recent major version 
   at all times. Support for the previous major version will 
   be dropped 2 years after the new major version is released
   or when the vendor itself drops support, whichever comes 
   first."

IOW, we only aim for QEMU to be buildable on Ubuntu LTS 20.04 and 18.04
at this point in time.  16.04 is explicitly dropped and we will increasingly
introduce incompatibilities with it.

While this specific patch is simple, trying to keep QEMU git master
working on 16.04 is not a goal, so I'd really suggest upgrading to
a newer Ubuntu version at the soonest opportunity.

Regards,
Daniel
Thomas Huth Feb. 10, 2021, 11:23 a.m. UTC | #8 
On 10/02/2021 12.15, Bin Meng wrote:
> Hi Philippe,
> 
> On Wed, Feb 10, 2021 at 7:12 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> Hi Bin,
>>
>> On 2/10/21 11:23 AM, Bin Meng wrote:
>>> From: Bin Meng <bin.meng@windriver.com>
>>>
>>> Current QEMU HEAD nvme.c does not compile:
>>>
>>>    hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>           trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>>>           ^
>>>    hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>>>       uint32_t result;
>>>                ^
>>
>> Why isn't this catched by our CI? What is your host OS? Fedora 33?
>>
> 
> I am using GCC 5.4 on Ubuntu 16.04. Please see some initial analysis
> from Peter about why newer version GCC does not report it.

Please note that Ubuntu 16.04 is not a supported version by QEMU anymore, 
see https://qemu.readthedocs.io/en/latest/system/build-platforms.html and 
https://wiki.qemu.org/Supported_Build_Platforms for details.

  Thomas
Daniel P. Berrangé Feb. 10, 2021, 11:34 a.m. UTC | #9 
On Wed, Feb 10, 2021 at 11:22:19AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 10, 2021 at 12:15:45PM +0100, Philippe Mathieu-Daudé wrote:
> > On 2/10/21 12:12 PM, Philippe Mathieu-Daudé wrote:
> > > Hi Bin,
> > > 
> > > On 2/10/21 11:23 AM, Bin Meng wrote:
> > >> From: Bin Meng <bin.meng@windriver.com>
> > >>
> > >> Current QEMU HEAD nvme.c does not compile:
> > >>
> > >>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> > >>          ^
> > >>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
> > >>      uint32_t result;
> > >>               ^
> > > 
> > > Why isn't this catched by our CI? What is your host OS? Fedora 33?
> > 
> > Just noticed v1 and Peter's explanation:
> > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03528.html
> > 
> > Can you amend "default GCC 5.4 on a Ubuntu 16.04 host" information
> > please?
> 
> Well Ubuntu 16.04 hasn't been considered a supported build target for
> QEMU for a year now.
> 
> https://qemu.readthedocs.io/en/latest/system/build-platforms.html#linux-os-macos-freebsd-netbsd-openbsd
> 
>   "The project aims to support the most recent major version 
>    at all times. Support for the previous major version will 
>    be dropped 2 years after the new major version is released
>    or when the vendor itself drops support, whichever comes 
>    first."
> 
> IOW, we only aim for QEMU to be buildable on Ubuntu LTS 20.04 and 18.04
> at this point in time.  16.04 is explicitly dropped and we will increasingly
> introduce incompatibilities with it.
> 
> While this specific patch is simple, trying to keep QEMU git master
> working on 16.04 is not a goal, so I'd really suggest upgrading to
> a newer Ubuntu version at the soonest opportunity.

In particular after 6.0 QEMU is released, we'll be dropping RHEL-7
and then likely setting the  min required GCC to somewhere around
6.3 which will cut off Ubuntu 16.04 upfront.

Regards,
Daniel
Peter Maydell Feb. 10, 2021, 10:29 p.m. UTC | #10 
On Wed, 10 Feb 2021 at 10:23, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Current QEMU HEAD nvme.c does not compile:
>
>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>          ^
>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>      uint32_t result;
>               ^
>
> Explicitly initialize the result to fix it.
>
> Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>
> ---
>
> Changes in v2:
> - update function name in the commit message
>
>  hw/block/nvme.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 5ce21b7..c122ac0 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3228,6 +3228,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
>          result = ns->features.err_rec;
>          goto out;
>      case NVME_VOLATILE_WRITE_CACHE:
> +        result = 0;
>          for (i = 1; i <= n->num_namespaces; i++) {
>              ns = nvme_ns(n, i);
>              if (!ns) {
> --

Also spotted by Coverity: CID 1446371

-- PMM
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ce21b7..c122ac0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3228,6 +3228,7 @@  static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
         result = ns->features.err_rec;
         goto out;
     case NVME_VOLATILE_WRITE_CACHE:
+        result = 0;
         for (i = 1; i <= n->num_namespaces; i++) {
             ns = nvme_ns(n, i);
             if (!ns) {