* [Qemu-devel] [PATCH] block/vpc.c: Handle write failures in get_image_offset()
@ 2017-07-09 21:07 Peter Maydell
2017-07-13 12:32 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2017-07-09 21:07 UTC (permalink / raw)
To: qemu-devel; +Cc: patches, qemu-block, Max Reitz, Kevin Wolf
Coverity (CID 1355236) points out that get_image_offset() doesn't check that
it actually succeeded in writing the updated block bitmap to the file.
Check the error return from bdrv_pwrite_sync() and propagate an error
response back up to the function which calls get_image_offset() for
a write so that it can return the error to its caller.
get_sector_offset() is only used for reads, but we move it to the
same API for consistency.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The new get_image_offset() API is pretty clunky, but I couldn't
think of anything better -- we need to report one of 3 things:
* offset number (0..INT64_MAX)
* sector not allocated
* arbitrary errno
and they won't all fit into one return value.
I opted for "minimal change compared to current code".
---
block/vpc.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 4240ba9d1c..b93211df36 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -460,17 +460,23 @@ static int vpc_reopen_prepare(BDRVReopenState *state,
/*
* Returns the absolute byte offset of the given sector in the image file.
* If the sector is not allocated, -1 is returned instead.
+ * If an error occurred trying to write an updated block bitmap back to
+ * the file, -2 is returned, and the error value is written to *err.
+ * This can only happen for a write operation.
*
* The parameter write must be 1 if the offset will be used for a write
* operation (the block bitmaps is updated then), 0 otherwise.
+ * If write is true then err must not be NULL.
*/
static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,
- bool write)
+ bool write, int *err)
{
BDRVVPCState *s = bs->opaque;
uint64_t bitmap_offset, block_offset;
uint32_t pagetable_index, offset_in_block;
+ assert(!(write && err == NULL));
+
pagetable_index = offset / s->block_size;
offset_in_block = offset % s->block_size;
@@ -487,19 +493,25 @@ static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,
correctness. */
if (write && (s->last_bitmap_offset != bitmap_offset)) {
uint8_t bitmap[s->bitmap_size];
+ int r;
s->last_bitmap_offset = bitmap_offset;
memset(bitmap, 0xff, s->bitmap_size);
- bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
+ r = bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
+ if (r < 0) {
+ *err = r;
+ return -2;
+ }
}
return block_offset;
}
static inline int64_t get_sector_offset(BlockDriverState *bs,
- int64_t sector_num, bool write)
+ int64_t sector_num, bool write,
+ int *err)
{
- return get_image_offset(bs, sector_num * BDRV_SECTOR_SIZE, write);
+ return get_image_offset(bs, sector_num * BDRV_SECTOR_SIZE, write, err);
}
/*
@@ -567,7 +579,7 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t offset)
if (ret < 0)
goto fail;
- return get_image_offset(bs, offset, false);
+ return get_image_offset(bs, offset, false, NULL);
fail:
s->free_data_block_offset -= (s->block_size + s->bitmap_size);
@@ -607,7 +619,7 @@ vpc_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
qemu_iovec_init(&local_qiov, qiov->niov);
while (bytes > 0) {
- image_offset = get_image_offset(bs, offset, false);
+ image_offset = get_image_offset(bs, offset, false, NULL);
n_bytes = MIN(bytes, s->block_size - (offset % s->block_size));
if (image_offset == -1) {
@@ -656,7 +668,11 @@ vpc_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
qemu_iovec_init(&local_qiov, qiov->niov);
while (bytes > 0) {
- image_offset = get_image_offset(bs, offset, true);
+ image_offset = get_image_offset(bs, offset, true, &ret);
+ if (image_offset == -2) {
+ /* Failed to write block bitmap: can't proceed with write */
+ goto fail;
+ }
n_bytes = MIN(bytes, s->block_size - (offset % s->block_size));
if (image_offset == -1) {
@@ -705,7 +721,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
(sector_num << BDRV_SECTOR_BITS);
}
- offset = get_sector_offset(bs, sector_num, 0);
+ offset = get_sector_offset(bs, sector_num, false, NULL);
start = offset;
allocated = (offset != -1);
*pnum = 0;
@@ -728,7 +744,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs,
if (nb_sectors == 0) {
break;
}
- offset = get_sector_offset(bs, sector_num, 0);
+ offset = get_sector_offset(bs, sector_num, false, NULL);
} while (offset == -1);
return 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vpc.c: Handle write failures in get_image_offset()
2017-07-09 21:07 [Qemu-devel] [PATCH] block/vpc.c: Handle write failures in get_image_offset() Peter Maydell
@ 2017-07-13 12:32 ` Kevin Wolf
2017-07-13 12:36 ` Peter Maydell
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2017-07-13 12:32 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, patches, qemu-block, Max Reitz
Am 09.07.2017 um 23:07 hat Peter Maydell geschrieben:
> Coverity (CID 1355236) points out that get_image_offset() doesn't check that
> it actually succeeded in writing the updated block bitmap to the file.
> Check the error return from bdrv_pwrite_sync() and propagate an error
> response back up to the function which calls get_image_offset() for
> a write so that it can return the error to its caller.
>
> get_sector_offset() is only used for reads, but we move it to the
> same API for consistency.
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Thanks, applied to the block branch.
> The new get_image_offset() API is pretty clunky, but I couldn't
> think of anything better -- we need to report one of 3 things:
> * offset number (0..INT64_MAX)
> * sector not allocated
> * arbitrary errno
> and they won't all fit into one return value.
> I opted for "minimal change compared to current code".
Good enough for vpc. I guess I would have chosen to return the offset by
reference and use the actual return value for 1=allocated, 0=unallocated
and -errno on error.
get_sector_offset() has two useless parameters now instead of one, but
I'll just send a patch on top to remove them.
Kevin
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vpc.c: Handle write failures in get_image_offset()
2017-07-13 12:32 ` Kevin Wolf
@ 2017-07-13 12:36 ` Peter Maydell
0 siblings, 0 replies; 3+ messages in thread
From: Peter Maydell @ 2017-07-13 12:36 UTC (permalink / raw)
To: Kevin Wolf; +Cc: QEMU Developers, patches, Qemu-block, Max Reitz
On 13 July 2017 at 13:32, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 09.07.2017 um 23:07 hat Peter Maydell geschrieben:
>> Coverity (CID 1355236) points out that get_image_offset() doesn't check that
>> it actually succeeded in writing the updated block bitmap to the file.
>> Check the error return from bdrv_pwrite_sync() and propagate an error
>> response back up to the function which calls get_image_offset() for
>> a write so that it can return the error to its caller.
>>
>> get_sector_offset() is only used for reads, but we move it to the
>> same API for consistency.
> get_sector_offset() has two useless parameters now instead of one, but
> I'll just send a patch on top to remove them.
I thought about doing that, but I decided it was better to
keep it as it is. A get_sector_offset() that silently only
works for reads and causes disk corruption if you use it for
a write seemed like a bit of a bear trap to leave lying around.
thanks
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-07-13 12:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-09 21:07 [Qemu-devel] [PATCH] block/vpc.c: Handle write failures in get_image_offset() Peter Maydell
2017-07-13 12:32 ` Kevin Wolf
2017-07-13 12:36 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.