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

[Markus Armbruster]

Kevin Wolf <address@hidden> 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.