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

[Kevin Wolf <kwolf@redhat.com>]

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