Message ID | 1322044805-3687-1-git-send-email-kwolf@redhat.com |
---|---|
State | New |
Headers | show |
On Wed, Nov 23, 2011 at 10:40 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/vpc.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 75d7d4a..89a5ee2 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -362,8 +362,11 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num) > > // Initialize the block's bitmap > memset(bitmap, 0xff, s->bitmap_size); > - bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, > + ret = bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, > s->bitmap_size); > + if (ret < 0) { > + return ret; > + } I notice that s->pagetable[index] is left modified when the function fails. But this is a larger issue and could be addressed in a later patch which also looks at the other failure cases in this function. Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> Stefan
Am 23.11.2011 12:01, schrieb Stefan Hajnoczi: > On Wed, Nov 23, 2011 at 10:40 AM, Kevin Wolf <kwolf@redhat.com> wrote: >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block/vpc.c | 5 ++++- >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/block/vpc.c b/block/vpc.c >> index 75d7d4a..89a5ee2 100644 >> --- a/block/vpc.c >> +++ b/block/vpc.c >> @@ -362,8 +362,11 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num) >> >> // Initialize the block's bitmap >> memset(bitmap, 0xff, s->bitmap_size); >> - bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, >> + ret = bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, >> s->bitmap_size); >> + if (ret < 0) { >> + return ret; >> + } > > I notice that s->pagetable[index] is left modified when the function > fails. But this is a larger issue and could be addressed in a later > patch which also looks at the other failure cases in this function. Error handling in vpc needs some work anyway. For example, almost all places return -1 instead of the real error number. Probably even the order of operations is unsafe, didn't check that yet. Kevin
Am 23.11.2011 12:23, schrieb Kevin Wolf: > Am 23.11.2011 12:01, schrieb Stefan Hajnoczi: >> On Wed, Nov 23, 2011 at 10:40 AM, Kevin Wolf <kwolf@redhat.com> wrote: >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block/vpc.c | 5 ++++- >>> 1 files changed, 4 insertions(+), 1 deletions(-) >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 75d7d4a..89a5ee2 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -362,8 +362,11 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num) >>> >>> // Initialize the block's bitmap >>> memset(bitmap, 0xff, s->bitmap_size); >>> - bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, >>> + ret = bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, >>> s->bitmap_size); >>> + if (ret < 0) { >>> + return ret; >>> + } >> >> I notice that s->pagetable[index] is left modified when the function >> fails. But this is a larger issue and could be addressed in a later >> patch which also looks at the other failure cases in this function. > > Error handling in vpc needs some work anyway. For example, almost all > places return -1 instead of the real error number. Probably even the > order of operations is unsafe, didn't check that yet. As a note, as part of QEMU Test Day(s) I tested VHD images (cf. Wiki) - booting, installing software, etc. and haven't encountered any problem. Doesn't mean it's entirely safe though, obviously. Andreas
diff --git a/block/vpc.c b/block/vpc.c index 75d7d4a..89a5ee2 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -362,8 +362,11 @@ static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num) // Initialize the block's bitmap memset(bitmap, 0xff, s->bitmap_size); - bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, + ret = bdrv_pwrite_sync(bs->file, s->free_data_block_offset, bitmap, s->bitmap_size); + if (ret < 0) { + return ret; + } // Write new footer (the old one will be overwritten) s->free_data_block_offset += s->block_size + s->bitmap_size;
Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vpc.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)