All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes
@ 2010-10-07 20:25 Eduardo Habkost
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path Eduardo Habkost
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Eduardo Habkost @ 2010-10-07 20:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf

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(-)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path
  2010-10-07 20:25 [Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes Eduardo Habkost
@ 2010-10-07 20:25 ` Eduardo Habkost
  2010-10-08  9:36   ` Stefan Hajnoczi
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create() Eduardo Habkost
  2010-10-08 10:11 ` [Qemu-devel] Re: [PATCH 0/2] qcow2_create() error handling fixes Kevin Wolf
  2 siblings, 1 reply; 9+ messages in thread
From: Eduardo Habkost @ 2010-10-07 20:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create()
  2010-10-07 20:25 [Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes Eduardo Habkost
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path Eduardo Habkost
@ 2010-10-07 20:25 ` Eduardo Habkost
  2010-10-08  9:28   ` Stefan Hajnoczi
  2010-10-08 10:14   ` [Qemu-devel] " Kevin Wolf
  2010-10-08 10:11 ` [Qemu-devel] Re: [PATCH 0/2] qcow2_create() error handling fixes Kevin Wolf
  2 siblings, 2 replies; 9+ messages in thread
From: Eduardo Habkost @ 2010-10-07 20:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create()
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create() Eduardo Habkost
@ 2010-10-08  9:28   ` Stefan Hajnoczi
  2010-10-08 15:29     ` Eduardo Habkost
  2010-10-08 10:14   ` [Qemu-devel] " Kevin Wolf
  1 sibling, 1 reply; 9+ messages in thread
From: Stefan Hajnoczi @ 2010-10-08  9:28 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Kevin Wolf, qemu-devel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path Eduardo Habkost
@ 2010-10-08  9:36   ` Stefan Hajnoczi
  0 siblings, 0 replies; 9+ messages in thread
From: Stefan Hajnoczi @ 2010-10-08  9:36 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Kevin Wolf, qemu-devel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] Re: [PATCH 0/2] qcow2_create() error handling fixes
  2010-10-07 20:25 [Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes Eduardo Habkost
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path Eduardo Habkost
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create() Eduardo Habkost
@ 2010-10-08 10:11 ` Kevin Wolf
  2 siblings, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2010-10-08 10:11 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] Re: [PATCH 2/2] check for close() errors on qcow2_create()
  2010-10-07 20:25 ` [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create() Eduardo Habkost
  2010-10-08  9:28   ` Stefan Hajnoczi
@ 2010-10-08 10:14   ` Kevin Wolf
  2010-10-08 17:39     ` Eduardo Habkost
  1 sibling, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2010-10-08 10:14 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create()
  2010-10-08  9:28   ` Stefan Hajnoczi
@ 2010-10-08 15:29     ` Eduardo Habkost
  0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2010-10-08 15:29 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] Re: [PATCH 2/2] check for close() errors on qcow2_create()
  2010-10-08 10:14   ` [Qemu-devel] " Kevin Wolf
@ 2010-10-08 17:39     ` Eduardo Habkost
  0 siblings, 0 replies; 9+ messages in thread
From: Eduardo Habkost @ 2010-10-08 17:39 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-10-08 17:40 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-07 20:25 [Qemu-devel] [PATCH 0/2] qcow2_create() error handling fixes Eduardo Habkost
2010-10-07 20:25 ` [Qemu-devel] [PATCH 1/2] fix fd leak on one qcow2_create2() error path Eduardo Habkost
2010-10-08  9:36   ` Stefan Hajnoczi
2010-10-07 20:25 ` [Qemu-devel] [PATCH 2/2] check for close() errors on qcow2_create() Eduardo Habkost
2010-10-08  9:28   ` Stefan Hajnoczi
2010-10-08 15:29     ` Eduardo Habkost
2010-10-08 10:14   ` [Qemu-devel] " Kevin Wolf
2010-10-08 17:39     ` Eduardo Habkost
2010-10-08 10:11 ` [Qemu-devel] Re: [PATCH 0/2] qcow2_create() error handling fixes Kevin Wolf

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.