[Qemu-devel] [PATCH] pflash: Avoid warnings from coverity

[Qemu-devel] [PATCH] pflash: Avoid warnings from coverity

[Stefan Weil]

hw/pflash_cfi01.c:209:
check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).

hw/pflash_cfi02.c:144:
unterminated_default: The default case is not terminated by a 'break' statement.

hw/pflash_cfi02.c:238:
check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/pflash_cfi01.c |    6 ++++--
 hw/pflash_cfi02.c |    7 +++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 56ed33f..27ae4aa 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -205,8 +205,10 @@ static void pflash_update(pflash_t *pfl, int offset,
         /* round to sectors */
         offset = offset >> 9;
         offset_end = (offset_end + 511) >> 9;
-        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
-                   offset_end - offset);
+        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
+                       offset_end - offset) == -1) {
+            fprintf(stderr, "pflash: Error writing to flash storage\n");
+        }
     }
 }
 
diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
index 3e2002e..bca7d1b 100644
--- a/hw/pflash_cfi02.c
+++ b/hw/pflash_cfi02.c
@@ -147,6 +147,7 @@ static uint32_t pflash_read (pflash_t *pfl, target_phys_addr_t offset,
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
         pfl->wcycle = 0;
         pfl->cmd = 0;
+        /* Fall through */
     case 0x80:
         /* We accept reads during second unlock sequence... */
     case 0x00:
@@ -236,8 +237,10 @@ static void pflash_update(pflash_t *pfl, int offset,
         /* round to sectors */
         offset = offset >> 9;
         offset_end = (offset_end + 511) >> 9;
-        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
-                   offset_end - offset);
+        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
+                       offset_end - offset) == -1) {
+            fprintf(stderr, "pflash: Error writing to flash storage\n");
+        }
     }
 }
 
-- 
1.7.10

Re: [Qemu-devel] [PATCH] pflash: Avoid warnings from coverity

[Peter Maydell]

On 19 September 2012 17:41, Stefan Weil <sw@weilnetz.de> wrote:
> hw/pflash_cfi01.c:209:
> check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).
>
> hw/pflash_cfi02.c:144:
> unterminated_default: The default case is not terminated by a 'break' statement.
>
> hw/pflash_cfi02.c:238:
> check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/pflash_cfi01.c |    6 ++++--
>  hw/pflash_cfi02.c |    7 +++++--
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
> index 56ed33f..27ae4aa 100644
> --- a/hw/pflash_cfi01.c
> +++ b/hw/pflash_cfi01.c
> @@ -205,8 +205,10 @@ static void pflash_update(pflash_t *pfl, int offset,
>          /* round to sectors */
>          offset = offset >> 9;
>          offset_end = (offset_end + 511) >> 9;
> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
> -                   offset_end - offset);
> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
> +                       offset_end - offset) == -1) {
> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
> +        }

...shouldn't we be propagating the write error up to the guest?
It feels like this change is just silencing the coverity error
without actually fixing the underlying problem.

-- PMM

Re: [Qemu-devel] [PATCH] pflash: Avoid warnings from coverity

[Stefan Weil]

Am 19.09.2012 18:45, schrieb Peter Maydell:
> On 19 September 2012 17:41, Stefan Weil <sw@weilnetz.de> wrote:
>> hw/pflash_cfi01.c:209:
>> check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).
>>
>> hw/pflash_cfi02.c:144:
>> unterminated_default: The default case is not terminated by a 'break' statement.
>>
>> hw/pflash_cfi02.c:238:
>> check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>   hw/pflash_cfi01.c |    6 ++++--
>>   hw/pflash_cfi02.c |    7 +++++--
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
>> index 56ed33f..27ae4aa 100644
>> --- a/hw/pflash_cfi01.c
>> +++ b/hw/pflash_cfi01.c
>> @@ -205,8 +205,10 @@ static void pflash_update(pflash_t *pfl, int offset,
>>           /* round to sectors */
>>           offset = offset >> 9;
>>           offset_end = (offset_end + 511) >> 9;
>> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>> -                   offset_end - offset);
>> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>> +                       offset_end - offset) == -1) {
>> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
>> +        }
> ...shouldn't we be propagating the write error up to the guest?
> It feels like this change is just silencing the coverity error
> without actually fixing the underlying problem.
>
> -- PMM

As far as I know, there is no concept how a flash memory
can report a write error. Flash memory which cannot be
written is unusable.

hw/nand.c uses a similar handling for write errors,
but writes the error message to stdout (no good idea).

- Stefan

Re: [Qemu-devel] [PATCH] pflash: Avoid warnings from coverity

[Peter Maydell]

On 19 September 2012 21:51, Stefan Weil <sw@weilnetz.de> wrote:
> Am 19.09.2012 18:45, schrieb Peter Maydell:
>> ...shouldn't we be propagating the write error up to the guest?
>> It feels like this change is just silencing the coverity error
>> without actually fixing the underlying problem.

> As far as I know, there is no concept how a flash memory
> can report a write error. Flash memory which cannot be
> written is unusable.

Just looking at the callers of the pflash_update() function,
they have cases where they set bits in the status register
for errors, so there clearly is a mechanism for reporting
errors back to the guest.

-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH] pflash: Avoid warnings from coverity

[Stefan Hajnoczi]

On Wed, Sep 19, 2012 at 06:41:14PM +0200, Stefan Weil wrote:
> hw/pflash_cfi01.c:209:
> check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).
> 
> hw/pflash_cfi02.c:144:
> unterminated_default: The default case is not terminated by a 'break' statement.
> 
> hw/pflash_cfi02.c:238:
> check_return: Calling function "bdrv_write" without checking return value (as is done elsewhere 35 out of 37 times).
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/pflash_cfi01.c |    6 ++++--
>  hw/pflash_cfi02.c |    7 +++++--
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
> index 56ed33f..27ae4aa 100644
> --- a/hw/pflash_cfi01.c
> +++ b/hw/pflash_cfi01.c
> @@ -205,8 +205,10 @@ static void pflash_update(pflash_t *pfl, int offset,
>          /* round to sectors */
>          offset = offset >> 9;
>          offset_end = (offset_end + 511) >> 9;
> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
> -                   offset_end - offset);
> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
> +                       offset_end - offset) == -1) {

bdrv_write() returns -errno, not -1.

> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
> +        }

Please report the errno and possibly bdrv_get_device_name() to uniquely
identify this block device.

Peter's comments about reporting errors to the guest make sense to me.
I'm not sure how much work that involves, printing the error is a step
in the right direction but we shouldn't forget the TODO.

Stefan

Re: [Qemu-devel] [Qemu-trivial] [PATCH] pflash: Avoid warnings from coverity

[Peter Maydell]

On 22 September 2012 17:29, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Please report the errno and possibly bdrv_get_device_name() to uniquely
> identify this block device.
>
> Peter's comments about reporting errors to the guest make sense to me.
> I'm not sure how much work that involves, printing the error is a step
> in the right direction but we shouldn't forget the TODO.

I don't think printing errors to stderr at this layer is the right
thing to do at all. You can make a case for "print nothing and let
the guest handle it", and also for "log errors in the block layer"
(which would make all block device users consistent and mean we
only needed one bit of code for this rather than lots), but I don't
see a case for printing errors in the middle of the stack like this.

(I am also somewhat reluctant to add further random fprintfs without
instituting a proper controllable logging mechanism first.)

-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH] pflash: Avoid warnings from coverity

[Stefan Weil]

Am 22.09.2012 18:29, schrieb Stefan Hajnoczi:
> On Wed, Sep 19, 2012 at 06:41:14PM +0200, Stefan Weil wrote:
[snip]
>>           offset_end = (offset_end + 511) >> 9;
>> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>> -                   offset_end - offset);
>> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>> +                       offset_end - offset) == -1) {
> bdrv_write() returns -errno, not -1.

Thanks. It looks like we have more code which uses the wrong check
(and which I copied). So more patches are needed.

Should we also replace code which does bdrv_write() != 0 or !bdrv_write()
by bdrv_write() < 0 to get more uniform code (and the same for bdrv_read*),
even it is not strictly wrong?

Maybe Kevin as block maintainer should decide that.

>> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
>> +        }
> Please report the errno and possibly bdrv_get_device_name() to uniquely
> identify this block device.

That would be overkill here: writing flash memory is not used very
often (even on real hardware it is typically only used for firmware
updates). I expect that anyone who does a firmware update in a
QEMU guest will know the name of the flash image file.

Usually I replace the flash image file on the QEMU host when I want
to exchange the firmware (much easier than flashing in the guest).

Reporting errno might be more reasonable.Are there other values than
EIO (e.g. defective media) and ENOSPC (disk full) which could occur?

A common solution for all users of bdrv_write in the block layer
would be even better. VirtualBox for example stops the guest when
ENOSPC (disk full) occurs, so it's possible for users to fix that
and resume the emulation.

> Peter's comments about reporting errors to the guest make sense to me.
> I'm not sure how much work that involves, printing the error is a step
> in the right direction but we shouldn't forget the TODO.
>
> Stefan

There is no 1:1 mapping of block write errors on the host to
an error statusin the flash controller.

Currently the code in pflash_cfi01.c sets an error status which
can be (mis-)used for block write errors. I just prepared a
patch which does this.

In pflash_cfi02.c I did not see such error status handling,
therefore I'd stick to printing an error message there.

Regards

Stefan W.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] pflash: Avoid warnings from coverity

[Kevin Wolf]

Am 22.09.2012 20:53, schrieb Stefan Weil:
> Am 22.09.2012 18:29, schrieb Stefan Hajnoczi:
>> On Wed, Sep 19, 2012 at 06:41:14PM +0200, Stefan Weil wrote:
> [snip]
>>>           offset_end = (offset_end + 511) >> 9;
>>> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>>> -                   offset_end - offset);
>>> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>>> +                       offset_end - offset) == -1) {
>> bdrv_write() returns -errno, not -1.
> 
> Thanks. It looks like we have more code which uses the wrong check
> (and which I copied). So more patches are needed.
> 
> Should we also replace code which does bdrv_write() != 0 or !bdrv_write()
> by bdrv_write() < 0 to get more uniform code (and the same for bdrv_read*),
> even it is not strictly wrong?
> 
> Maybe Kevin as block maintainer should decide that.

Yes, I very much prefer ret < 0 checks for all block layer functions.

>>> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
>>> +        }
>> Please report the errno and possibly bdrv_get_device_name() to uniquely
>> identify this block device.
> 
> That would be overkill here: writing flash memory is not used very
> often (even on real hardware it is typically only used for firmware
> updates). I expect that anyone who does a firmware update in a
> QEMU guest will know the name of the flash image file.
> 
> Usually I replace the flash image file on the QEMU host when I want
> to exchange the firmware (much easier than flashing in the guest).
> 
> Reporting errno might be more reasonable.Are there other values than
> EIO (e.g. defective media) and ENOSPC (disk full) which could occur?

Basically anything that the OS can return. The block layer may
internally generate things like -EACCES for writing to read-only images,
or -ENOMEDIUM (not sure if it's possible for pflash).

> A common solution for all users of bdrv_write in the block layer
> would be even better. VirtualBox for example stops the guest when
> ENOSPC (disk full) occurs, so it's possible for users to fix that
> and resume the emulation.

virtio-blk/IDE/scsi-disk do that.

>> Peter's comments about reporting errors to the guest make sense to me.
>> I'm not sure how much work that involves, printing the error is a step
>> in the right direction but we shouldn't forget the TODO.

Shouldn't we avoid fprintfs that can be triggered by the guest?

Kevin

Re: [Qemu-devel] [Qemu-trivial] [PATCH] pflash: Avoid warnings from coverity

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 22.09.2012 20:53, schrieb Stefan Weil:
>> Am 22.09.2012 18:29, schrieb Stefan Hajnoczi:
>>> On Wed, Sep 19, 2012 at 06:41:14PM +0200, Stefan Weil wrote:
>> [snip]
>>>>           offset_end = (offset_end + 511) >> 9;
>>>> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>>>> -                   offset_end - offset);
>>>> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>>>> +                       offset_end - offset) == -1) {
>>> bdrv_write() returns -errno, not -1.
>> 
>> Thanks. It looks like we have more code which uses the wrong check
>> (and which I copied). So more patches are needed.
>> 
>> Should we also replace code which does bdrv_write() != 0 or !bdrv_write()
>> by bdrv_write() < 0 to get more uniform code (and the same for bdrv_read*),
>> even it is not strictly wrong?
>> 
>> Maybe Kevin as block maintainer should decide that.
>
> Yes, I very much prefer ret < 0 checks for all block layer functions.
>
>>>> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
>>>> +        }
>>> Please report the errno and possibly bdrv_get_device_name() to uniquely
>>> identify this block device.
>> 
>> That would be overkill here: writing flash memory is not used very
>> often (even on real hardware it is typically only used for firmware
>> updates). I expect that anyone who does a firmware update in a
>> QEMU guest will know the name of the flash image file.
>> 
>> Usually I replace the flash image file on the QEMU host when I want
>> to exchange the firmware (much easier than flashing in the guest).
>> 
>> Reporting errno might be more reasonable.Are there other values than
>> EIO (e.g. defective media) and ENOSPC (disk full) which could occur?
>
> Basically anything that the OS can return. The block layer may
> internally generate things like -EACCES for writing to read-only images,
> or -ENOMEDIUM (not sure if it's possible for pflash).
>
>> A common solution for all users of bdrv_write in the block layer
>> would be even better. VirtualBox for example stops the guest when
>> ENOSPC (disk full) occurs, so it's possible for users to fix that
>> and resume the emulation.
>
> virtio-blk/IDE/scsi-disk do that.

Doing it in the block layer for all devices would be cleaner
conceptually.  If I remember correctly, we did it in devices instead,
because that was much simpler.

>>> Peter's comments about reporting errors to the guest make sense to me.
>>> I'm not sure how much work that involves, printing the error is a step
>>> in the right direction but we shouldn't forget the TODO.
>
> Shouldn't we avoid fprintfs that can be triggered by the guest?

Yes, we should.

Besides, mumbling to stderr is no excuse for a device model to behave
incorrectly.  Adding a print is a step in the right direction only
insofar as it makes the brokenness of the device model more obvious.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] pflash: Avoid warnings from coverity

[Kevin Wolf]

Am 24.09.2012 10:41, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 22.09.2012 20:53, schrieb Stefan Weil:
>>> Am 22.09.2012 18:29, schrieb Stefan Hajnoczi:
>>>> On Wed, Sep 19, 2012 at 06:41:14PM +0200, Stefan Weil wrote:
>>> [snip]
>>>>>           offset_end = (offset_end + 511) >> 9;
>>>>> -        bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>>>>> -                   offset_end - offset);
>>>>> +        if (bdrv_write(pfl->bs, offset, pfl->storage + (offset << 9),
>>>>> +                       offset_end - offset) == -1) {
>>>> bdrv_write() returns -errno, not -1.
>>>
>>> Thanks. It looks like we have more code which uses the wrong check
>>> (and which I copied). So more patches are needed.
>>>
>>> Should we also replace code which does bdrv_write() != 0 or !bdrv_write()
>>> by bdrv_write() < 0 to get more uniform code (and the same for bdrv_read*),
>>> even it is not strictly wrong?
>>>
>>> Maybe Kevin as block maintainer should decide that.
>>
>> Yes, I very much prefer ret < 0 checks for all block layer functions.
>>
>>>>> +            fprintf(stderr, "pflash: Error writing to flash storage\n");
>>>>> +        }
>>>> Please report the errno and possibly bdrv_get_device_name() to uniquely
>>>> identify this block device.
>>>
>>> That would be overkill here: writing flash memory is not used very
>>> often (even on real hardware it is typically only used for firmware
>>> updates). I expect that anyone who does a firmware update in a
>>> QEMU guest will know the name of the flash image file.
>>>
>>> Usually I replace the flash image file on the QEMU host when I want
>>> to exchange the firmware (much easier than flashing in the guest).
>>>
>>> Reporting errno might be more reasonable.Are there other values than
>>> EIO (e.g. defective media) and ENOSPC (disk full) which could occur?
>>
>> Basically anything that the OS can return. The block layer may
>> internally generate things like -EACCES for writing to read-only images,
>> or -ENOMEDIUM (not sure if it's possible for pflash).
>>
>>> A common solution for all users of bdrv_write in the block layer
>>> would be even better. VirtualBox for example stops the guest when
>>> ENOSPC (disk full) occurs, so it's possible for users to fix that
>>> and resume the emulation.
>>
>> virtio-blk/IDE/scsi-disk do that.
> 
> Doing it in the block layer for all devices would be cleaner
> conceptually.  If I remember correctly, we did it in devices instead,
> because that was much simpler.

I believe today it wouldn't be too hard to implement the request
queueing in the block layer. However, we can't change it without
breaking migration, we'd need a VMState for the block layer.

Kevin