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

["Nicholas Piggin" <npiggin@gmail.com>]

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