[PATCH] q800: fix coverity warning CID 1412799

[PATCH] q800: fix coverity warning CID 1412799

[Laurent Vivier]

Check the return value of blk_write() and log an error if any

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/misc/mac_via.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..81343301b1 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -30,6 +30,7 @@
 #include "hw/qdev-properties.h"
 #include "sysemu/block-backend.h"
 #include "trace.h"
+#include "qemu/log.h"
 
 /*
  * VIAs: There are two in every machine,
@@ -381,8 +382,10 @@ static void via2_irq_request(void *opaque, int irq, int level)
 static void pram_update(MacVIAState *m)
 {
     if (m->blk) {
-        blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
-                   sizeof(m->mos6522_via1.PRAM), 0);
+        if (blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
+                       sizeof(m->mos6522_via1.PRAM), 0) < 0) {
+            qemu_log("pram_update: cannot write to file\n");
+        }
     }
 }
 
-- 
2.24.1

Re: [PATCH] q800: fix coverity warning CID 1412799

[Philippe Mathieu-Daudé]

On 2/10/20 2:22 PM, Laurent Vivier wrote:
> Check the return value of blk_write() and log an error if any
> 

Fixes: Coverity CID 1412799 (Error handling issues)

> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/misc/mac_via.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..81343301b1 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -30,6 +30,7 @@
>   #include "hw/qdev-properties.h"
>   #include "sysemu/block-backend.h"
>   #include "trace.h"
> +#include "qemu/log.h"
>   
>   /*
>    * VIAs: There are two in every machine,
> @@ -381,8 +382,10 @@ static void via2_irq_request(void *opaque, int irq, int level)
>   static void pram_update(MacVIAState *m)
>   {
>       if (m->blk) {
> -        blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
> -                   sizeof(m->mos6522_via1.PRAM), 0);
> +        if (blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
> +                       sizeof(m->mos6522_via1.PRAM), 0) < 0) {
> +            qemu_log("pram_update: cannot write to file\n");
> +        }
>       }
>   }
>   
> 

Re: [PATCH] q800: fix coverity warning CID 1412799

[Philippe Mathieu-Daudé]

Hi Laurent,

Cc'ing qemu-block@

On 2/10/20 3:56 PM, Philippe Mathieu-Daudé wrote:
> On 2/10/20 2:22 PM, Laurent Vivier wrote:
>> Check the return value of blk_write() and log an error if any
>>
> 
> Fixes: Coverity CID 1412799 (Error handling issues)
> 
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/misc/mac_via.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index b7d0012794..81343301b1 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -30,6 +30,7 @@
>>   #include "hw/qdev-properties.h"
>>   #include "sysemu/block-backend.h"
>>   #include "trace.h"
>> +#include "qemu/log.h"
>>   /*
>>    * VIAs: There are two in every machine,
>> @@ -381,8 +382,10 @@ static void via2_irq_request(void *opaque, int 
>> irq, int level)
>>   static void pram_update(MacVIAState *m)
>>   {
>>       if (m->blk) {
>> -        blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
>> -                   sizeof(m->mos6522_via1.PRAM), 0);
>> +        if (blk_pwrite(m->blk, 0, m->mos6522_via1.PRAM,
>> +                       sizeof(m->mos6522_via1.PRAM), 0) < 0) {
>> +            qemu_log("pram_update: cannot write to file\n");

I am not comfortable reviewing this patch, because it use a undocumented 
function. If I understand co-routine code enough, this eventually calls 
blk_co_pwritev_part(), which on success returns bdrv_co_pwritev_part(), 
which on success returns bdrv_aligned_pwritev(), which itself returns 0 
or errno (as negative value).

I don't understand how to treat the error, but apparently other devices 
do the same (only report some error and discarding the block not written).

So this can happens if your filesystem got full, the VM is running, the 
device can not sync the VIA PRAM but keeps running. You let the user the 
possibility to make some space on the filesystem so the next sync will 
succeed.

This does not seem critical, so I dare to add:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

But I'd rather have one of the block folks reviewing this pattern.

Regards,

Phil.

>> +        }
>>       }
>>   }
>>