[Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes

[Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes

[Eduardo Habkost]

From: Eduardo Habkost <ehabkost@raisama.net>

Hi,

Here are two small fixes on qcow2_create() error handling.

Eduardo Habkost (2):
  fix fd leak on a qcow2_create2() error path
  check for close() errors on qcow2_create()

 block/qcow2.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

[Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path

[Eduardo Habkost]

When getting an invalid cluster size, the open fd must be closed before
qcow2_create() returns an error.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block/qcow2.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ee3481b..c5fb28e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -918,7 +918,8 @@ static int qcow_create2(const char *filename, int64_t total_size,
             "%d and %dk\n",
             1 << MIN_CLUSTER_BITS,
             1 << (MAX_CLUSTER_BITS - 10));
-        return -EINVAL;
+        ret = -EINVAL;
+        goto exit_close;
     }
     s->cluster_size = 1 << s->cluster_bits;
 
@@ -1052,6 +1053,8 @@ static int qcow_create2(const char *filename, int64_t total_size,
 exit:
     qemu_free(s->refcount_table);
     qemu_free(s->refcount_block);
+
+exit_close:
     close(fd);
 
     /* Preallocate metadata */
-- 
1.6.5.5

Re: [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path

[Stefan Hajnoczi]

On Thu, Oct 7, 2010 at 9:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> When getting an invalid cluster size, the open fd must be closed before
> qcow2_create() returns an error.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block/qcow2.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)

Looks good.

Stefan

[Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create()

[Eduardo Habkost]

Errors when closing the file we just created should not be ignored. I/O errors
may happen and "qemu-img create" should fail in those cases.

If we are already exiting due to an error, we will still return the original
error number, though.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block/qcow2.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c5fb28e..d3a056b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -882,7 +882,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
     uint64_t old_ref_clusters;
     QCowCreateState s1, *s = &s1;
     QCowExtension ext_bf = {0, 0};
-    int ret;
+    int ret, cret;
 
     memset(s, 0, sizeof(*s));
 
@@ -1055,7 +1055,9 @@ exit:
     qemu_free(s->refcount_block);
 
 exit_close:
-    close(fd);
+    cret = close(fd);
+    if (ret == 0 && cret < 0)
+        ret = -errno;
 
     /* Preallocate metadata */
     if (ret == 0 && prealloc) {
-- 
1.6.5.5

Re: [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create()

[Stefan Hajnoczi]

On Thu, Oct 7, 2010 at 9:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Errors when closing the file we just created should not be ignored. I/O errors
> may happen and "qemu-img create" should fail in those cases.
>
> If we are already exiting due to an error, we will still return the original
> error number, though.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block/qcow2.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)

Sounds good.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index c5fb28e..d3a056b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -882,7 +882,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
>     uint64_t old_ref_clusters;
>     QCowCreateState s1, *s = &s1;
>     QCowExtension ext_bf = {0, 0};
> -    int ret;
> +    int ret, cret;
>
>     memset(s, 0, sizeof(*s));
>
> @@ -1055,7 +1055,9 @@ exit:
>     qemu_free(s->refcount_block);
>
>  exit_close:
> -    close(fd);
> +    cret = close(fd);
> +    if (ret == 0 && cret < 0)

if (close(fd) < 0 && ret == 0) {

Does the same without variable cret.

> +        ret = -errno;
>
>     /* Preallocate metadata */
>     if (ret == 0 && prealloc) {
> --
> 1.6.5.5
>
>
>

Re: [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create()

[Eduardo Habkost]

On Fri, Oct 08, 2010 at 10:28:37AM +0100, Stefan Hajnoczi wrote:
> >  exit_close:
> > -    close(fd);
> > +    cret = close(fd);
> > +    if (ret == 0 && cret < 0)
> 
> if (close(fd) < 0 && ret == 0) {
> 
> Does the same without variable cret.

Yes. I used the variable just for readability. I personally don't like
having side-effects inside a if condition.

-- 
Eduardo

[Qemu-devel] Re: [PATCH 2/2] check for close() errors on qcow2_create()

[Kevin Wolf]

Am 07.10.2010 22:25, schrieb Eduardo Habkost:
> Errors when closing the file we just created should not be ignored. I/O errors
> may happen and "qemu-img create" should fail in those cases.
> 
> If we are already exiting due to an error, we will still return the original
> error number, though.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  block/qcow2.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c5fb28e..d3a056b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -882,7 +882,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
>      uint64_t old_ref_clusters;
>      QCowCreateState s1, *s = &s1;
>      QCowExtension ext_bf = {0, 0};
> -    int ret;
> +    int ret, cret;
>  
>      memset(s, 0, sizeof(*s));
>  
> @@ -1055,7 +1055,9 @@ exit:
>      qemu_free(s->refcount_block);
>  
>  exit_close:
> -    close(fd);
> +    cret = close(fd);
> +    if (ret == 0 && cret < 0)
> +        ret = -errno;

Braces are missing here.

Kevin

[Qemu-devel] Re: [PATCH 2/2] check for close() errors on qcow2_create()

[Eduardo Habkost]

On Fri, Oct 08, 2010 at 12:14:07PM +0200, Kevin Wolf wrote:
> Am 07.10.2010 22:25, schrieb Eduardo Habkost:
> > Errors when closing the file we just created should not be ignored. I/O errors
> > may happen and "qemu-img create" should fail in those cases.
> > 
> > If we are already exiting due to an error, we will still return the original
> > error number, though.
[...]
> >  exit_close:
> > -    close(fd);
> > +    cret = close(fd);
> > +    if (ret == 0 && cret < 0)
> > +        ret = -errno;
> 
> Braces are missing here.

Updated patch below.

I won't resubmit the series as a new thread, as it depends on deciding
what to do about the qcow2_create() rewrite.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block/qcow2.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c5fb28e..e2e9a95 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -882,7 +882,7 @@ static int qcow_create2(const char *filename, int64_t total_size,
     uint64_t old_ref_clusters;
     QCowCreateState s1, *s = &s1;
     QCowExtension ext_bf = {0, 0};
-    int ret;
+    int ret, cret;
 
     memset(s, 0, sizeof(*s));
 
@@ -1055,7 +1055,10 @@ exit:
     qemu_free(s->refcount_block);
 
 exit_close:
-    close(fd);
+    cret = close(fd);
+    if (ret == 0 && cret < 0) {
+        ret = -errno;
+    }
 
     /* Preallocate metadata */
     if (ret == 0 && prealloc) {
-- 
1.6.5.5

[Qemu-devel] Re: [PATCH 0/2] qcow2_create() error handling fixes

[Kevin Wolf]

Am 07.10.2010 22:25, schrieb Eduardo Habkost:
> From: Eduardo Habkost <ehabkost@raisama.net>
> 
> Hi,
> 
> Here are two small fixes on qcow2_create() error handling.
> 
> Eduardo Habkost (2):
>   fix fd leak on a qcow2_create2() error path
>   check for close() errors on qcow2_create()

A while ago I sent a patch to completely rewrite qcow_create using qemu
block layer functions. I didn't submit it for inclusion yet because it
makes some assumptions in qemu-iotests invalid and the test cases need
to be fixed first.

In the new version, your first patch wouldn't be needed. However, for
the second one, I think we have a problem today:

    void bdrv_close(BlockDriverState *bs);

We need to convert bdrv_close to be able to return error values and to
pass them on up to the first caller (which is qemu-img in this case).

I'll have a look at fixing the test cases and bdrv_close. If I can't get
it fixed easily, I'll consider your patches.

Kevin