| Message ID | 1348072874-2096-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Under Review |
| Headers | show |
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
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
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
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
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
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.
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
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.
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
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"); + } } }
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(-)