Patchwork [2/6] cloop: Fix bdrv_open() error handling

login
register
mail settings
Submitter Kevin Wolf
Date Jan. 24, 2013, 11:02 a.m.
Message ID <1359025358-5725-3-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/215324/
State New
Headers show

Comments

Kevin Wolf - Jan. 24, 2013, 11:02 a.m.
Return -errno instead of -1 on errors. While touching the
code, fix a memory leak.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/cloop.c |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)
Markus Armbruster - Jan. 24, 2013, 12:48 p.m.
Kevin Wolf <kwolf@redhat.com> writes:

> Return -errno instead of -1 on errors. While touching the
> code, fix a memory leak.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/cloop.c |   27 +++++++++++++++++----------
>  1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/block/cloop.c b/block/cloop.c
> index 5a0d0d8..9b36063 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c
> @@ -57,27 +57,32 @@ static int cloop_open(BlockDriverState *bs, int flags)
>  {
>      BDRVCloopState *s = bs->opaque;
>      uint32_t offsets_size, max_compressed_block_size = 1, i;
> +    int ret;
>  
>      bs->read_only = 1;
>  
>      /* read header */
> -    if (bdrv_pread(bs->file, 128, &s->block_size, 4) < 4) {
> -        goto cloop_close;
> +    ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
> +    if (ret < 0) {
> +        return ret;
>      }
>      s->block_size = be32_to_cpu(s->block_size);
>  
> -    if (bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4) < 4) {
> -        goto cloop_close;
> +    ret = bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4);
> +    if (ret < 0) {
> +        return ret;
>      }
>      s->n_blocks = be32_to_cpu(s->n_blocks);
>  
>      /* read offsets */
>      offsets_size = s->n_blocks * sizeof(uint64_t);
>      s->offsets = g_malloc(offsets_size);
> -    if (bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size) <
> -            offsets_size) {
> -        goto cloop_close;
> +

Empty line visually detaches the /* read offsets */ comment from the
actual read.  Sure you want it?

> +    ret = bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size);
> +    if (ret < 0) {
> +        goto fail;
>      }
> +
>      for(i=0;i<s->n_blocks;i++) {
>          s->offsets[i] = be64_to_cpu(s->offsets[i]);
>          if (i > 0) {
> @@ -92,7 +97,8 @@ static int cloop_open(BlockDriverState *bs, int flags)
>      s->compressed_block = g_malloc(max_compressed_block_size + 1);
>      s->uncompressed_block = g_malloc(s->block_size);
>      if (inflateInit(&s->zstream) != Z_OK) {
> -        goto cloop_close;
> +        ret = -EINVAL;

inflateInit() can return a number of different errors.  But your change
doesn't make things worse, and that's good enough.

> +        goto fail;
>      }
>      s->current_block = s->n_blocks;
>  
> @@ -101,8 +107,9 @@ static int cloop_open(BlockDriverState *bs, int flags)
>      qemu_co_mutex_init(&s->lock);
>      return 0;
>  
> -cloop_close:
> -    return -1;
> +fail:
> +    g_free(s->offsets);

What about s->compressed_block and s->uncompressed_block?

> +    return ret;
>  }
>  
>  static inline int cloop_read_block(BlockDriverState *bs, int block_num)
Kevin Wolf - Jan. 24, 2013, 1:17 p.m.
Am 24.01.2013 13:48, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Return -errno instead of -1 on errors. While touching the
>> code, fix a memory leak.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/cloop.c |   27 +++++++++++++++++----------
>>  1 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/cloop.c b/block/cloop.c
>> index 5a0d0d8..9b36063 100644
>> --- a/block/cloop.c
>> +++ b/block/cloop.c
>> @@ -57,27 +57,32 @@ static int cloop_open(BlockDriverState *bs, int flags)
>>  {
>>      BDRVCloopState *s = bs->opaque;
>>      uint32_t offsets_size, max_compressed_block_size = 1, i;
>> +    int ret;
>>  
>>      bs->read_only = 1;
>>  
>>      /* read header */
>> -    if (bdrv_pread(bs->file, 128, &s->block_size, 4) < 4) {
>> -        goto cloop_close;
>> +    ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
>> +    if (ret < 0) {
>> +        return ret;
>>      }
>>      s->block_size = be32_to_cpu(s->block_size);
>>  
>> -    if (bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4) < 4) {
>> -        goto cloop_close;
>> +    ret = bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4);
>> +    if (ret < 0) {
>> +        return ret;
>>      }
>>      s->n_blocks = be32_to_cpu(s->n_blocks);
>>  
>>      /* read offsets */
>>      offsets_size = s->n_blocks * sizeof(uint64_t);
>>      s->offsets = g_malloc(offsets_size);
>> -    if (bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size) <
>> -            offsets_size) {
>> -        goto cloop_close;
>> +
> 
> Empty line visually detaches the /* read offsets */ comment from the
> actual read.  Sure you want it?

For comments like this, which organise the code in sections, I generally
don't assume that a comment is only valid until the next empty line, but
until the next comment comes.

I like the empty line here, but if you don't, I can remove it.

>> +    ret = bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size);
>> +    if (ret < 0) {
>> +        goto fail;
>>      }
>> +
>>      for(i=0;i<s->n_blocks;i++) {
>>          s->offsets[i] = be64_to_cpu(s->offsets[i]);
>>          if (i > 0) {
>> @@ -92,7 +97,8 @@ static int cloop_open(BlockDriverState *bs, int flags)
>>      s->compressed_block = g_malloc(max_compressed_block_size + 1);
>>      s->uncompressed_block = g_malloc(s->block_size);
>>      if (inflateInit(&s->zstream) != Z_OK) {
>> -        goto cloop_close;
>> +        ret = -EINVAL;
> 
> inflateInit() can return a number of different errors.  But your change
> doesn't make things worse, and that's good enough.

It doesn't use errno and I decided that I don't care enough about these
block drivers to write an error conversion function... As you said, it
doesn't make things worse, and it's also not the most likely error to
happen.

>> +        goto fail;
>>      }
>>      s->current_block = s->n_blocks;
>>  
>> @@ -101,8 +107,9 @@ static int cloop_open(BlockDriverState *bs, int flags)
>>      qemu_co_mutex_init(&s->lock);
>>      return 0;
>>  
>> -cloop_close:
>> -    return -1;
>> +fail:
>> +    g_free(s->offsets);
> 
> What about s->compressed_block and s->uncompressed_block?

Thanks, I missed them. Will fix.

Kevin

Patch

diff --git a/block/cloop.c b/block/cloop.c
index 5a0d0d8..9b36063 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -57,27 +57,32 @@  static int cloop_open(BlockDriverState *bs, int flags)
 {
     BDRVCloopState *s = bs->opaque;
     uint32_t offsets_size, max_compressed_block_size = 1, i;
+    int ret;
 
     bs->read_only = 1;
 
     /* read header */
-    if (bdrv_pread(bs->file, 128, &s->block_size, 4) < 4) {
-        goto cloop_close;
+    ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
+    if (ret < 0) {
+        return ret;
     }
     s->block_size = be32_to_cpu(s->block_size);
 
-    if (bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4) < 4) {
-        goto cloop_close;
+    ret = bdrv_pread(bs->file, 128 + 4, &s->n_blocks, 4);
+    if (ret < 0) {
+        return ret;
     }
     s->n_blocks = be32_to_cpu(s->n_blocks);
 
     /* read offsets */
     offsets_size = s->n_blocks * sizeof(uint64_t);
     s->offsets = g_malloc(offsets_size);
-    if (bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size) <
-            offsets_size) {
-        goto cloop_close;
+
+    ret = bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size);
+    if (ret < 0) {
+        goto fail;
     }
+
     for(i=0;i<s->n_blocks;i++) {
         s->offsets[i] = be64_to_cpu(s->offsets[i]);
         if (i > 0) {
@@ -92,7 +97,8 @@  static int cloop_open(BlockDriverState *bs, int flags)
     s->compressed_block = g_malloc(max_compressed_block_size + 1);
     s->uncompressed_block = g_malloc(s->block_size);
     if (inflateInit(&s->zstream) != Z_OK) {
-        goto cloop_close;
+        ret = -EINVAL;
+        goto fail;
     }
     s->current_block = s->n_blocks;
 
@@ -101,8 +107,9 @@  static int cloop_open(BlockDriverState *bs, int flags)
     qemu_co_mutex_init(&s->lock);
     return 0;
 
-cloop_close:
-    return -1;
+fail:
+    g_free(s->offsets);
+    return ret;
 }
 
 static inline int cloop_read_block(BlockDriverState *bs, int block_num)