[Qemu-devel] [PATCH] vpc: Add missing error handling in alloc_block

[Qemu-devel] [PATCH] vpc: Add missing error handling in alloc_block

[Kevin Wolf]

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;
+    }
 
     // Write new footer (the old one will be overwritten)
     s->free_data_block_offset += s->block_size + s->bitmap_size;
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH] vpc: Add missing error handling in alloc_block

[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.

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Stefan

Re: [Qemu-devel] [PATCH] vpc: Add missing error handling in alloc_block

[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.

Kevin

Re: [Qemu-devel] [PATCH] vpc: Add missing error handling in alloc_block

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg