[PATCH] ppc/amigaone: Check blk_pwrite return value

[PATCH] ppc/amigaone: Check blk_pwrite return value

[BALATON Zoltan]

Coverity reported that return value of blk_pwrite() maybe should not
be ignored. We can't do much if this happens other than report an
error but let's do that to silence this report.

Resolves: Coverity CID 1593725
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/amigaone.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 483512125f..5d787c3059 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -108,8 +108,8 @@ static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
     uint8_t *p = memory_region_get_ram_ptr(&s->mr);
 
     p[addr] = val;
-    if (s->blk) {
-        blk_pwrite(s->blk, addr, 1, &val, 0);
+    if (s->blk && blk_pwrite(s->blk, addr, 1, &val, 0) < 0) {
+        error_report("%s: could not write %s", __func__, blk_name(s->blk));
     }
 }
 
@@ -151,15 +151,17 @@ static void nvram_realize(DeviceState *dev, Error **errp)
         *c = cpu_to_be32(CRC32_DEFAULT_ENV);
         /* Also copies terminating \0 as env is terminated by \0\0 */
         memcpy(p + 4, default_env, sizeof(default_env));
-        if (s->blk) {
-            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
+        if (s->blk &&
+            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0) < 0
+           ) {
+            error_report("%s: could not write %s", __func__, blk_name(s->blk));
         }
         return;
     }
     if (*c == 0) {
         *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
-        if (s->blk) {
-            blk_pwrite(s->blk, 0, 4, p, 0);
+        if (s->blk && blk_pwrite(s->blk, 0, 4, p, 0) < 0) {
+            error_report("%s: could not write %s", __func__, blk_name(s->blk));
         }
     }
     if (be32_to_cpu(*c) != crc) {
-- 
2.41.3

Re: [PATCH] ppc/amigaone: Check blk_pwrite return value

[Nicholas Piggin]

On Sat Mar 15, 2025 at 6:01 AM AEST, BALATON Zoltan wrote:
> Coverity reported that return value of blk_pwrite() maybe should not
> be ignored. We can't do much if this happens other than report an
> error but let's do that to silence this report.
>
> Resolves: Coverity CID 1593725
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  hw/ppc/amigaone.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index 483512125f..5d787c3059 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -108,8 +108,8 @@ static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
>      uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>  
>      p[addr] = val;
> -    if (s->blk) {
> -        blk_pwrite(s->blk, addr, 1, &val, 0);
> +    if (s->blk && blk_pwrite(s->blk, addr, 1, &val, 0) < 0) {
> +        error_report("%s: could not write %s", __func__, blk_name(s->blk));
>      }
>  }
>  
> @@ -151,15 +151,17 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>          *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>          /* Also copies terminating \0 as env is terminated by \0\0 */
>          memcpy(p + 4, default_env, sizeof(default_env));
> -        if (s->blk) {
> -            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
> +        if (s->blk &&
> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0) < 0
> +           ) {
> +            error_report("%s: could not write %s", __func__, blk_name(s->blk));
>          }
>          return;
>      }
>      if (*c == 0) {
>          *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
> -        if (s->blk) {
> -            blk_pwrite(s->blk, 0, 4, p, 0);
> +        if (s->blk && blk_pwrite(s->blk, 0, 4, p, 0) < 0) {
> +            error_report("%s: could not write %s", __func__, blk_name(s->blk));
>          }
>      }
>      if (be32_to_cpu(*c) != crc) {

Re: [PATCH] ppc/amigaone: Check blk_pwrite return value

[Cédric Le Goater]

On 3/14/25 21:01, BALATON Zoltan wrote:
> Coverity reported that return value of blk_pwrite() maybe should not
> be ignored. We can't do much if this happens other than report an
> error but let's do that to silence this report.
> 
> Resolves: Coverity CID 1593725
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/amigaone.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index 483512125f..5d787c3059 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -108,8 +108,8 @@ static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
>       uint8_t *p = memory_region_get_ram_ptr(&s->mr);

why is the nvram never read ?

>   
>       p[addr] = val;
> -    if (s->blk) {
> -        blk_pwrite(s->blk, addr, 1, &val, 0);
> +    if (s->blk && blk_pwrite(s->blk, addr, 1, &val, 0) < 0) {
> +        error_report("%s: could not write %s", __func__, blk_name(s->blk));

hmm, guest_error maybe ? since this is a runtime error.

>       }
>   }
>   
> @@ -151,15 +151,17 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>           *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>           /* Also copies terminating \0 as env is terminated by \0\0 */
>           memcpy(p + 4, default_env, sizeof(default_env));
> -        if (s->blk) {
> -            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
> +        if (s->blk &&
> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0) < 0
> +           ) {
> +            error_report("%s: could not write %s", __func__, blk_name(s->blk));

This should use the errp parameter.

>           }
>           return;
>       }
>       if (*c == 0) {
>           *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
> -        if (s->blk) {
> -            blk_pwrite(s->blk, 0, 4, p, 0);
> +        if (s->blk && blk_pwrite(s->blk, 0, 4, p, 0) < 0) {
> +            error_report("%s: could not write %s", __func__, blk_name(s->blk));

same here.

Thanks,

C.



>           }
>       }
>       if (be32_to_cpu(*c) != crc) {

Re: [PATCH] ppc/amigaone: Check blk_pwrite return value

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

On Mon, 17 Mar 2025, Cédric Le Goater wrote:
> On 3/14/25 21:01, BALATON Zoltan wrote:
>> Coverity reported that return value of blk_pwrite() maybe should not
>> be ignored. We can't do much if this happens other than report an
>> error but let's do that to silence this report.
>> 
>> Resolves: Coverity CID 1593725
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/amigaone.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>> index 483512125f..5d787c3059 100644
>> --- a/hw/ppc/amigaone.c
>> +++ b/hw/ppc/amigaone.c
>> @@ -108,8 +108,8 @@ static void nvram_write(void *opaque, hwaddr addr, 
>> uint64_t val,
>>       uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>
> why is the nvram never read ?

There's a comment about that. It's a rom_device which maps the memory 
region directly so does not go through the read callback. But I thin there 
must be a read callback and cannot be null so we have an empty one. 
Previously I had one that worked in case romd mode is turned off but Nick 
said having dead code is not wanted and better to mark it unreachable.

>>         p[addr] = val;
>> -    if (s->blk) {
>> -        blk_pwrite(s->blk, addr, 1, &val, 0);
>> +    if (s->blk && blk_pwrite(s->blk, addr, 1, &val, 0) < 0) {
>> +        error_report("%s: could not write %s", __func__, 
>> blk_name(s->blk));
>
> hmm, guest_error maybe ? since this is a runtime error.

It's not a guest error but some problem on the host with the backing file.

>>       }
>>   }
>>   @@ -151,15 +151,17 @@ static void nvram_realize(DeviceState *dev, Error 
>> **errp)
>>           *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>>           /* Also copies terminating \0 as env is terminated by \0\0 */
>>           memcpy(p + 4, default_env, sizeof(default_env));
>> -        if (s->blk) {
>> -            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 
>> 0);
>> +        if (s->blk &&
>> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0) 
>> < 0
>> +           ) {
>> +            error_report("%s: could not write %s", __func__, 
>> blk_name(s->blk));
>
> This should use the errp parameter.
>
>>           }
>>           return;
>>       }
>>       if (*c == 0) {
>>           *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>> -        if (s->blk) {
>> -            blk_pwrite(s->blk, 0, 4, p, 0);
>> +        if (s->blk && blk_pwrite(s->blk, 0, 4, p, 0) < 0) {
>> +            error_report("%s: could not write %s", __func__, 
>> blk_name(s->blk));
>
> same here.

It could but I think it's not needed. It still works without the backing 
file and the guest works, just may not save the NVRAM contents which is a 
problem on the host. So the error is reported but I'm not sure it should 
abort. In practice if there's some fatal error with the backing file the

blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
                      BLK_PERM_ALL, &error_fatal);

earlier will catch that so it won't even get here.

Regards,
BALATON Zoltan

Re: [PATCH] ppc/amigaone: Check blk_pwrite return value

[Nicholas Piggin]

On Mon Mar 17, 2025 at 11:13 PM AEST, BALATON Zoltan wrote:
> On Mon, 17 Mar 2025, Cédric Le Goater wrote:
>> On 3/14/25 21:01, BALATON Zoltan wrote:
>>> Coverity reported that return value of blk_pwrite() maybe should not
>>> be ignored. We can't do much if this happens other than report an
>>> error but let's do that to silence this report.
>>> 
>>> Resolves: Coverity CID 1593725
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ppc/amigaone.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>>> index 483512125f..5d787c3059 100644
>>> --- a/hw/ppc/amigaone.c
>>> +++ b/hw/ppc/amigaone.c
>>> @@ -108,8 +108,8 @@ static void nvram_write(void *opaque, hwaddr addr, 
>>> uint64_t val,
>>>       uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>>
>> why is the nvram never read ?
>
> There's a comment about that. It's a rom_device which maps the memory 
> region directly so does not go through the read callback. But I thin there 
> must be a read callback and cannot be null so we have an empty one. 
> Previously I had one that worked in case romd mode is turned off but Nick 
> said having dead code is not wanted and better to mark it unreachable.
>
>>>         p[addr] = val;
>>> -    if (s->blk) {
>>> -        blk_pwrite(s->blk, addr, 1, &val, 0);
>>> +    if (s->blk && blk_pwrite(s->blk, addr, 1, &val, 0) < 0) {
>>> +        error_report("%s: could not write %s", __func__, 
>>> blk_name(s->blk));
>>
>> hmm, guest_error maybe ? since this is a runtime error.
>
> It's not a guest error but some problem on the host with the backing file.
>
>>>       }
>>>   }
>>>   @@ -151,15 +151,17 @@ static void nvram_realize(DeviceState *dev, Error 
>>> **errp)
>>>           *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>>>           /* Also copies terminating \0 as env is terminated by \0\0 */
>>>           memcpy(p + 4, default_env, sizeof(default_env));
>>> -        if (s->blk) {
>>> -            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 
>>> 0);
>>> +        if (s->blk &&
>>> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0) 
>>> < 0
>>> +           ) {
>>> +            error_report("%s: could not write %s", __func__, 
>>> blk_name(s->blk));
>>
>> This should use the errp parameter.
>>
>>>           }
>>>           return;
>>>       }
>>>       if (*c == 0) {
>>>           *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>>> -        if (s->blk) {
>>> -            blk_pwrite(s->blk, 0, 4, p, 0);
>>> +        if (s->blk && blk_pwrite(s->blk, 0, 4, p, 0) < 0) {
>>> +            error_report("%s: could not write %s", __func__, 
>>> blk_name(s->blk));
>>
>> same here.
>
> It could but I think it's not needed. It still works without the backing 
> file and the guest works, just may not save the NVRAM contents which is a 
> problem on the host. So the error is reported but I'm not sure it should 
> abort. In practice if there's some fatal error with the backing file the
>
> blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>                       BLK_PERM_ALL, &error_fatal);
>
> earlier will catch that so it won't even get here.

I'll take it as is since it seems to be an improvement. Some other nvram
devices with backing files do the same thing with write failures.

Thanks,
Nick