Re: [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